[GH-3609]: Add new sort order for int96 timestamps#3610
Conversation
| /** | ||
| * Chronological order for INT96 timestamps: values are compared by the Julian day (the last 4 | ||
| * bytes, as a little-endian signed int32), then by the nanoseconds within the day (the first 8 | ||
| * bytes, as a little-endian signed int64). Only supported for the INT96 physical type. |
There was a problem hiding this comment.
I don't think that this Javadoc is the right place for documenting the format and how to compare. This is part of the spec so the spec needs to be clear and this needs to state what the order means.
There was a problem hiding this comment.
Sorry if my comment wasn't very clear. I can see how it would be misread. I think this should state "Chronological order of INT96 timestamps" and not cover serialization.
|
Please resolve the conflicts. |
@wgtmac Sorry for the delay, I was on holiday last week. Conflicts are resolved now |
| * | ||
| * Two-level comparison, matching the INT96 timestamp sort order: | ||
| * 1. Compare the last 4 bytes (Julian day) as a signed little-endian int32. | ||
| * 2. If equal, compare the first 8 bytes (nanos) as a signed little-endian int64. |
There was a problem hiding this comment.
The correctness of this depends on the two fields so I think we have to be more careful since the caller may pass in something unexpected.
I think that we need to validate that the nanosecond value is positive and less than the number of nanoseconds per day. If either of these is violated, then this sort order is no longer correct.
| if (primitive == PrimitiveTypeName.INT96) { | ||
| // A plain INT96 is the legacy timestamp encoding; default it to the chronological order. | ||
| // An annotated INT96 carries other semantics, so leave its order undefined. | ||
| columnOrder = originalType == null ? ColumnOrder.int96TimestampOrder() : ColumnOrder.undefined(); |
There was a problem hiding this comment.
I don't think that there are any original types that can be applied to INT96. We stopped adding them years ago. I think it would be safe to just return ColumnOrder.int96TimestampOrder() without the check. The comment would be "INT96 is only used for (deprecated) timestamps".
| // INT96 has a fixed 12-byte width and a chronological comparator (so it is excluded from the | ||
| // variable-length checks above, like FLOAT16). Its truncator is a no-op: verify it returns the | ||
| // value unchanged regardless of the requested length. | ||
| BinaryTruncator int96Truncator = BinaryTruncator.getTruncator( |
There was a problem hiding this comment.
This is replacing the INT96 handling above, but this expansion deserves its own new test case because it is not running the standard testTrunctator logic. This should be in a new testInt96.
| // variant guards against absolute reads not being relative to the value's start | ||
| List<Function<Binary, Binary>> representations = List.of( | ||
| b -> b, | ||
| b -> Binary.fromReusedByteArray(b.getBytes()), |
There was a problem hiding this comment.
There's no functional difference between reused and constant here. It is just a way to inform Parquet that the value may be modified (and should be copied) or not.
I also don't think there is much value in these cases more generally. This is testing that Binary#toByteBuffer does the right thing and that is not the responsibility of this test to validate.
| return new NanoTime(julianDay, nanosOfDay).toBinary(); | ||
| } | ||
|
|
||
| private static Binary timestampToInt96(String timestamp) { |
There was a problem hiding this comment.
I don't see the value of complicating the tests with string parsing. I think you can test what you need to with Julian day and nanos. No need to make your tests depend on correct conversion from epoch to Julian.
| } | ||
|
|
||
| @Test | ||
| public void testInt96TimestampComparator() { |
There was a problem hiding this comment.
This also needs to cover cases like negative nanos and nanos >= 86_400_000_000_000. Use a separate test case if the intent is to fail.
| ColumnOrderName name = type.columnOrder().getColumnOrderName(); | ||
| return name == ColumnOrderName.TYPE_DEFINED_ORDER | ||
| || name == ColumnOrderName.IEEE_754_TOTAL_ORDER | ||
| || name == ColumnOrderName.INT96_TIMESTAMP_ORDER; |
There was a problem hiding this comment.
This is getting too complicated. Either use a switch or use name != UNDEFINED.
|
|
||
| @Test | ||
| public void testSkippedV2Stats() { | ||
| // INTERVAL has an undefined column order, so its stats are skipped. |
There was a problem hiding this comment.
Please remove this. It is unrelated and doesn't need to be part of this PR.
| testSkippedV2Stats( | ||
| Types.optional(PrimitiveTypeName.INT96).named(""), | ||
| new BigInteger("-75687987"), | ||
| new BigInteger("45367657")); |
There was a problem hiding this comment.
There are other tests that also need to be updated. For example, testV2StatsEqualMinMax tests cases where the min and max are equal and stats are preserved. This also uses BigInteger to pass the values, which is suspicious.
I think this should update all of the INT96 cases to use NanoTime. Looking at the code path, I don't think that NanoTime is plumbed to work since I don't see a Statistics implementation for it. Maybe this should actually pass a Binary produced by NanoTime instead.
| // A footer without column orders predates INT96_TIMESTAMP_ORDER, so an INT96 column here | ||
| // must not inherit the (chronological) construction-time default: its stats, if any, were | ||
| // written under the legacy order and must be ignored. | ||
| primitiveBuilder.columnOrder(org.apache.parquet.schema.ColumnOrder.undefined()); |
There was a problem hiding this comment.
There isn't a test in TestParquetMetadataConverter that validates Parquet metadata for an INT96 without a column order. There is a test that a new schema will produce a column order, but not one to verify that a schema without one will be read correctly.
| @Rule | ||
| public TemporaryFolder tmp = new TemporaryFolder(); | ||
|
|
||
| private static Binary int96(int julianDay, long nanosOfDay) { |
There was a problem hiding this comment.
Please don't add new serializations. Use NanoTime#toBinary() and Binary#toByteBuffer().
| private static ParquetMetadata readFooter(File file, Configuration conf) throws IOException { | ||
| Path path = new Path(file.getAbsolutePath()); | ||
| try (ParquetFileReader reader = ParquetFileReader.open( | ||
| HadoopInputFile.fromPath(path, conf), |
There was a problem hiding this comment.
I recommend using the InputFile and OutputFile paths rather than having a dependency on Hadoop. You should use LocalInputFile and LocalOutputFile.
| } | ||
|
|
||
| /** Rewrites the footer of src into a new file, keeping all data pages byte-identical. */ | ||
| private File rewriteFooter(File src, FileMetaData footer, String name) |
There was a problem hiding this comment.
This method seems misnamed. It doesn't match the comment.
What about copyWithNewFooter?
| } | ||
|
|
||
| private File writeFile() throws IOException { | ||
| File file = new File(tmp.getRoot(), "int96.parquet"); |
There was a problem hiding this comment.
This should use tmp.newFile("int96.parquet") instead of constructing one manually.
| * min/max statistics in place. This simulates a legacy writer that emitted INT96 stats under | ||
| * TYPE_ORDER before INT96_TIMESTAMP_ORDER existed; such stats must be ignored by the reader. | ||
| */ | ||
| private static void downgradeInt96ToTypeOrder(FileMetaData footer) { |
There was a problem hiding this comment.
Can you add a couple more cases? I'd like to see UNDEFINED, TYPE_DEFINED_ORDER, and unset cases.
| assertTrue(rawFooter.getColumn_orders().get(1).isSetTYPE_ORDER()); | ||
|
|
||
| for (RowGroup rowGroup : rawFooter.getRow_groups()) { | ||
| for (ColumnChunk chunk : rowGroup.getColumns()) { |
There was a problem hiding this comment.
Use get(0) like above. There's no need to search.
| FileMetaData rawFooter = readRawFooter(file); | ||
| // schema[0] is the message root; column_orders are indexed by leaf order: ts=0, id=1 | ||
| assertTrue(rawFooter.getColumn_orders().get(0).isSetINT96_TIMESTAMP_ORDER()); | ||
| assertTrue(rawFooter.getColumn_orders().get(1).isSetTYPE_ORDER()); |
| } | ||
| } | ||
|
|
||
| // Column index should be present for both columns. |
There was a problem hiding this comment.
But only validated for INT96.
| assertNotNull(columnIndex); | ||
| assertArrayEquals(EXPECTED_MIN.getBytes(), toArray(columnIndex.getMinValues().get(0))); | ||
| assertArrayEquals(EXPECTED_MAX.getBytes(), | ||
| toArray(columnIndex.getMaxValues().get(columnIndex.getMaxValues().size() - 1))); |
There was a problem hiding this comment.
There is only one page, right?
|
I think there are a few things to be fixed, but overall I trust that this works. |
3 participants
Rationale for this change
See parquet-format proposal for rationale: apache/parquet-format#584
What changes are included in this PR?
Add new
INT96_TIMESTAMP_ORDERsort order and related parsing.Are these changes tested?
Yes, new unit tests.
Are there any user-facing changes?
Two new flags and a Thrift change.
Closes #3609