GH-3679: Allow TimestampType to annotate FLBA(12) - #3680
Conversation
stevomitric
left a comment
stevomitric
left a comment
There was a problem hiding this comment.
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.
divjotarora
left a comment
divjotarora
left a comment
There was a problem hiding this comment.
Should the
converted_typebe 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.
| if (value == null) { | ||
| return BINARY_NULL; | ||
| } | ||
| byte[] littleEndian = value.getBytesUnsafe(); |
There was a problem hiding this comment.
consider refactoring to toBigEndian method. I think there might already be some byte utils someplace.
There was a problem hiding this comment.
Added a general reverse() function to BytesUtils, this can be used for bidirectional LE <--> BE swaps, but let me know if you'd prefer toBigEndian specifically.
emkornfield
left a comment
emkornfield
left a comment
There was a problem hiding this comment.
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
left a comment
divjotarora
left a comment
There was a problem hiding this comment.
Should the
converted_typebe 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.
| if (value == null) { | ||
| return BINARY_NULL; | ||
| } | ||
| byte[] littleEndian = value.getBytesUnsafe(); |
There was a problem hiding this comment.
Added a general reverse() function to BytesUtils, this can be used for bidirectional LE <--> BE swaps, but let me know if you'd prefer toBigEndian specifically.
3 participants
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
TimestampTypelogical type annotation onFIXED_LEN_BYTE_ARRAYphysical type values withtype_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