Repository navigation
#95: Add support for Parquet TIMESTAMP(NANOS, ...) values - #96
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30baeffe4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Not ready to approve
The user guide type-mapping table introduces a likely-invalid logical type label (timestamp_nanos) that should be corrected to accurately reflect Parquet’s TIMESTAMP logical type with NANOS unit.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds end-to-end support for reading Parquet INT64 columns annotated with the modern TIMESTAMP(NANOS, ...) logical type, materializing them as java.sql.Timestamp with full nanosecond precision (including pre-epoch values), and updates release/docs/tooling accordingly.
Changes:
- Added a dedicated
ParquetTimestampNanosConverterand factory dispatch for modern timestamp logical types (MILLIS/MICROS/NANOS). - Implemented
DateTimeHelper.getTimestampFromNanos(...)with correct handling for negative epoch values and added unit/integration tests. - Updated documentation, release notes, dependencies, and GitHub workflows as part of the 2.1.0 release.
File summaries
| File | Description |
|---|---|
| src/main/java/com/exasol/parquetio/reader/converter/ParquetConverterFactory.java | Detects modern timestamp logical type annotations and dispatches by TimeUnit including NANOS. |
| src/main/java/com/exasol/parquetio/reader/converter/ParquetTimestampNanosConverter.java | New primitive converter that maps INT64 nanos-since-epoch to Timestamp. |
| src/main/java/com/exasol/parquetio/reader/converter/DateTimeHelper.java | Adds nanos-to-Timestamp conversion utility with negative-value handling. |
| src/test/java/com/exasol/parquetio/reader/RowParquetReaderPrimitiveTypesTest.java | Adds reader-level tests for modern timestamp millis/micros and nanos (including pre-epoch). |
| src/test/java/com/exasol/parquetio/reader/converter/DateTimeHelperTest.java | Adds unit tests validating nanos conversion behavior. |
| src/test/java/com/exasol/parquetio/reader/converter/ParquetConverterFactoryTest.java | Adds tests ensuring the factory selects the correct converter for modern timestamp units. |
| doc/system_requirements.md | Adds a requirement documenting nanosecond timestamp reading behavior. |
| doc/design.md | Adds design section describing nanosecond timestamp conversion behavior. |
| doc/user_guide.md | Extends the type mapping table to include nanosecond timestamp support. |
| doc/changes/changes_2.1.0.md | Adds 2.1.0 release notes entry for the feature and dependency updates. |
| doc/changes/changelog.md | Links the new 2.1.0 changelog page. |
| pom.xml | Bumps project version/parent version and updates Parquet + plugin versions. |
| pk_generated_parent.pom | Updates plugin versions/configuration and adds SBOM-related plugins. |
| dependencies.md | Regenerates dependency/license documentation to reflect updated plugins. |
| .github/zizmor.yml | Updates zizmor ignore rationale comment. |
| .github/workflows/ci-build.yml | Updates checkout/cache action versions, Java matrix (11/21), and reformats the artifact verification script. |
| .github/workflows/release.yml | Updates checkout action version and Java matrix (11/21). |
| .github/workflows/dependencies_update.yml | Updates checkout action version and Java matrix (11/21). |
| .github/workflows/dependencies_check.yml | Updates checkout action version, Java matrix (11/21), and python-toolbox action version. |
| .github/workflows/broken_links_checker.yml | Updates checkout action version. |
Review details
Suppressed comments (1)
src/test/java/com/exasol/parquetio/reader/RowParquetReaderPrimitiveTypesTest.java:110
- This test also uses the modern timestamp logical type annotation (via getModernTimestampSchema), so its name would be clearer if it followed the same "ModernTimestamp" naming pattern as the other modern timestamp tests.
@Test
void testReadsInt64TimestampNanosBeforeEpochAsTimestampValue() throws IOException {
final MessageType schema = getModernTimestampSchema(TimeUnit.NANOS);
try (ParquetWriter<Group> writer = getParquetWriter(schema, false)) {
final SimpleGroup parquetRow = new SimpleGroup(schema);
- Files reviewed: 20/20 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
@codex review |
There was a problem hiding this comment.
🟡 Not ready to approve
The newly added local-epoch timestamp helpers currently use long multiplications that can overflow and silently yield incorrect timestamps for large values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/main/java/com/exasol/parquetio/reader/converter/DateTimeHelper.java:102
getLocalTimestampFromMillismultipliesmillis * 1_000_000, which can overflowlong(e.g., for timestamps beyond ~2262-04-11) and silently produce wrong results. UsingfloorDiv/floorModwithplusSeconds/plusNanosavoids the overflow while keeping correct behavior for negative values.
static Timestamp getLocalTimestampFromMillis(final long millis) {
return getLocalTimestampFromNanos(millis * NANOS_PER_MILLIS);
}
src/main/java/com/exasol/parquetio/reader/converter/DateTimeHelper.java:53
getLocalTimestampFromMicrosmultipliesmicroseconds * 1000, which can overflowlongfor large Parquet timestamp values and silently yield incorrect timestamps. Consider splitting into seconds + fractional part usingMath.floorDiv/Math.floorModto avoid overflow and handle negatives correctly.
This issue also appears on line 100 of the same file.
static Timestamp getLocalTimestampFromMicros(final long microseconds) {
return getLocalTimestampFromNanos(microseconds * NANOS_PER_MICROS);
}
- Files reviewed: 23/23 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15368a381f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88a8a61bee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f24449ad59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8c9171754
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Closes #95