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.
| }; | ||
|
|
||
| // Converts a count of time units since epoch, held as a BigInteger (the 96-bit FLBA(12) carrier), | ||
| // into an Instant. The 96-bit count does not fit in a long, but the whole-second count does, so |
There was a problem hiding this comment.
doesn't this depend on TimeUnit?
| return units.toString(); | ||
| } | ||
| } catch (NumberFormatException e) { | ||
| return BINARY_INVALID; |
There was a problem hiding this comment.
what cases does this throw?
| 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.
| .length(12) | ||
| .as(LogicalTypeAnnotation.timestampType(true, TimeUnit.NANOS)) | ||
| .named("test_fixed_timestamp")); | ||
| Binary value = Binary.fromConstantByteArray(new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 1, 2, 3, 4}); |
There was a problem hiding this comment.
why are only the last 4 bytes non-zero?
| return Binary.fromConstantByteArray(array); | ||
| } | ||
|
|
||
| // Encodes a unit count as the extended-precision timestamp carrier: a 12-byte signed |
There was a problem hiding this comment.
don't we need tests for out of range values?
| // two's-complement little-endian value (sign-extended, so negatives fill the high bytes with | ||
| // 0xFF). | ||
| private Binary flba12(long units) { | ||
| byte[] le = new byte[12]; |
There was a problem hiding this comment.
nit: might be clearer if we can use BigInteger to byte array and flip the bytes?
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?
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