Skip to content

GH-3679: Allow TimestampType to annotate FLBA(12) - #3680

Open
divjotarora wants to merge 6 commits into
apache:masterfrom
divjotarora:flba12-ts
Open

divjotarora wants to merge 6 commits into
apache:masterfrom
divjotarora:flba12-ts

Conversation

@divjotarora

Copy link
Copy Markdown
Contributor

Rationale for this change

See parquet-format issue apache/parquet-format#600 and the linked proposal document for rationale

What changes are included in this PR?

This PR implements support for the TimestampType logical type annotation on FIXED_LEN_BYTE_ARRAY physical type values with type_length=12 (96 bits).

Are these changes tested?

Yes, several test files are updated along with the source.

Are there any user-facing changes?

Users will be able to declare schemas with this physical/logical type combination. No API changes.

Closes #3679

@stevomitric stevomitric left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the converted_type be suppressed for the FLBA(12) carrier, the same way NANOS already returns empty? Those legacy types are defined as INT64-only, so emitting them here gives a contradictory footer.

Comment thread parquet-column/src/main/java/org/apache/parquet/schema/Types.java Outdated
Comment thread parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveComparator.java Outdated

@divjotarora divjotarora left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the converted_type be suppressed for the FLBA(12) carrier, the same way NANOS already returns empty? Those legacy types are defined as INT64-only, so emitting them here gives a contradictory footer.

Thanks for catching @stevomitric, the existing tests didn't catch this because they were focusing on using FLBA(12) with NANOS and it doesn't have a converted type already. The code is now fixed and there are more tests to validate FLBA(12) with MILLIS and MICROS as well.

Comment thread parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveComparator.java Outdated
Comment thread parquet-column/src/main/java/org/apache/parquet/schema/Types.java Outdated
Comment thread parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveStringifier.java Outdated
Comment thread parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveStringifier.java Outdated
Comment thread parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveStringifier.java Outdated

@emkornfield emkornfield left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that I think we want to at least look at is if we build an FLBA(12) will common wrappers (e.g. avro) correctly write the correct bytes to the field or will they fail?

@divjotarora divjotarora left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the converted_type be suppressed for the FLBA(12) carrier, the same way NANOS already returns empty? Those legacy types are defined as INT64-only, so emitting them here gives a contradictory footer.

Thanks for catching @stevomitric, the existing tests didn't catch this because they were focusing on using FLBA(12) with NANOS and it doesn't have a converted type already. The code is now fixed and there are more tests to validate FLBA(12) with MILLIS and MICROS as well.

Comment thread parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveStringifier.java Outdated
Comment thread parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveStringifier.java Outdated
Comment thread parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveStringifier.java Outdated
@divjotarora
divjotarora requested a review from emkornfield July 31, 2026 11:36

@Fokko Fokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@divjotarora Can you fix the merge conflicts?

@divjotarora

Copy link
Copy Markdown
Contributor Author

@divjotarora Can you fix the merge conflicts?

Updated the PR, should be clean now @Fokko

@Fokko

Fokko commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

@divjotarora We need to move to JUnit5:

[ERROR] Banned imports detected in TEST code:
[ERROR] 
[ERROR] Reason: Use JUnit 5 (org.junit.jupiter.*) and AssertJ instead of JUnit 4
[ERROR] 	in file:///home/runner/work/parquet-java/parquet-java/parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestInputFormatFileStatusReuse.java
[ERROR] 		static org.junit.Assert.assertEquals 	(Line: 20, Matched by: org.junit.**)
[ERROR] 		static org.junit.Assert.assertSame   	(Line: 21, Matched by: org.junit.**)
[ERROR] 		static org.junit.Assert.assertTrue   	(Line: 22, Matched by: org.junit.**)
[ERROR] 		org.junit.Rule                       	(Line: 50, Matched by: org.junit.**)
[ERROR] 		org.junit.Test                       	(Line: 51, Matched by: org.junit.**)
[ERROR] 		org.junit.rules.TemporaryFolder      	(Line: 52, Matched by: org.junit.**)
[ERROR] 
[ERROR] Analysis of 257 files took less than 1 second
[ERROR] -> [Help 1]

@divjotarora

Copy link
Copy Markdown
Contributor Author

@divjotarora We need to move to JUnit5:

@Fokko The TestInputFormatFileStatusReuse.java file is not modified by this PR. It seems spotless checks are failing due to #3674 being merged. I've opened #3813 to fix this on the master branch, PTAL

@divjotarora

Copy link
Copy Markdown
Contributor Author

@Fokko I've rebased this PR on top of the merged #3813, should be good to go now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support extended precision for nanosecond timestamps

4 participants