-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[GH-3609]: Add new sort order for int96 timestamps #3610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -385,7 +385,9 @@ public <T, E extends Exception> T convert(PrimitiveTypeNameConverter<T, E> conve | |
|
|
||
| @Override | ||
| PrimitiveComparator<?> comparator(LogicalTypeAnnotation logicalType, ColumnOrder columnOrder) { | ||
| return PrimitiveComparator.BINARY_AS_SIGNED_INTEGER_COMPARATOR; | ||
| return columnOrder != null && columnOrder.getColumnOrderName() == ColumnOrderName.INT96_TIMESTAMP_ORDER | ||
| ? PrimitiveComparator.BINARY_AS_INT96_TIMESTAMP_COMPARATOR | ||
| : PrimitiveComparator.BINARY_AS_SIGNED_INTEGER_COMPARATOR; | ||
| } | ||
| }, | ||
| FIXED_LEN_BYTE_ARRAY("getBinary", Binary.class) { | ||
|
|
@@ -578,9 +580,15 @@ public PrimitiveType( | |
| this.decimalMeta = decimalMeta; | ||
|
|
||
| if (columnOrder == null) { | ||
| columnOrder = primitive == PrimitiveTypeName.INT96 || originalType == OriginalType.INTERVAL | ||
| ? ColumnOrder.undefined() | ||
| : ColumnOrder.typeDefined(); | ||
| 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(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| } else if (originalType == OriginalType.INTERVAL) { | ||
| columnOrder = ColumnOrder.undefined(); | ||
| } else { | ||
| columnOrder = ColumnOrder.typeDefined(); | ||
| } | ||
| } else if (columnOrder.getColumnOrderName() == ColumnOrderName.IEEE_754_TOTAL_ORDER) { | ||
| Preconditions.checkArgument( | ||
| primitive == PrimitiveTypeName.FLOAT || primitive == PrimitiveTypeName.DOUBLE, | ||
|
|
@@ -629,10 +637,17 @@ public PrimitiveType( | |
| } | ||
|
|
||
| if (columnOrder == null) { | ||
| columnOrder = primitive == PrimitiveTypeName.INT96 | ||
| || logicalTypeAnnotation instanceof LogicalTypeAnnotation.IntervalLogicalTypeAnnotation | ||
| ? ColumnOrder.undefined() | ||
| : ColumnOrder.typeDefined(); | ||
| 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 = logicalTypeAnnotation == null | ||
| ? ColumnOrder.int96TimestampOrder() | ||
| : ColumnOrder.undefined(); | ||
| } else if (logicalTypeAnnotation instanceof LogicalTypeAnnotation.IntervalLogicalTypeAnnotation) { | ||
| columnOrder = ColumnOrder.undefined(); | ||
| } else { | ||
| columnOrder = ColumnOrder.typeDefined(); | ||
| } | ||
| } else if (columnOrder.getColumnOrderName() == ColumnOrderName.IEEE_754_TOTAL_ORDER) { | ||
| Preconditions.checkArgument( | ||
| primitive == PrimitiveTypeName.FLOAT | ||
|
|
@@ -651,9 +666,15 @@ public PrimitiveType( | |
| private ColumnOrder requireValidColumnOrder(ColumnOrder columnOrder) { | ||
| if (primitive == PrimitiveTypeName.INT96) { | ||
| Preconditions.checkArgument( | ||
| columnOrder.getColumnOrderName() == ColumnOrderName.UNDEFINED, | ||
| columnOrder.getColumnOrderName() == ColumnOrderName.UNDEFINED | ||
| || columnOrder.getColumnOrderName() == ColumnOrderName.INT96_TIMESTAMP_ORDER, | ||
| "The column order %s is not supported by INT96", | ||
| columnOrder); | ||
| } else { | ||
| Preconditions.checkArgument( | ||
| columnOrder.getColumnOrderName() != ColumnOrderName.INT96_TIMESTAMP_ORDER, | ||
| "The column order %s is only supported by INT96", | ||
| columnOrder); | ||
| } | ||
| if (getLogicalTypeAnnotation() != null) { | ||
| Preconditions.checkArgument( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,7 +93,16 @@ public void testContractNonStringTypes() { | |
| testTruncator( | ||
| Types.required(FIXED_LEN_BYTE_ARRAY).length(12).as(INTERVAL).named("test_fixed_interval"), false); | ||
| testTruncator(Types.required(BINARY).as(DECIMAL).precision(10).scale(2).named("test_binary_decimal"), false); | ||
| testTruncator(Types.required(INT96).named("test_int96"), false); | ||
|
|
||
| // 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( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is replacing the INT96 handling above, but this expansion deserves its own new test case because it is not running the standard |
||
| Types.required(INT96).named("test_int96")); | ||
| Binary int96Value = Binary.fromConstantByteArray( | ||
| new byte[] {0, 0, 0, 0, 0, 0, 0, 0, 1, 2, 3, 4}); | ||
| assertSame(int96Value, int96Truncator.truncateMin(int96Value, 4)); | ||
| assertSame(int96Value, int96Truncator.truncateMax(int96Value, 4)); | ||
| } | ||
|
|
||
| @Test | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
|
|
||
| import static org.apache.parquet.schema.PrimitiveComparator.BINARY_AS_FLOAT16_COMPARATOR; | ||
| import static org.apache.parquet.schema.PrimitiveComparator.BINARY_AS_FLOAT16_IEEE_754_TOTAL_ORDER_COMPARATOR; | ||
| import static org.apache.parquet.schema.PrimitiveComparator.BINARY_AS_INT96_TIMESTAMP_COMPARATOR; | ||
| import static org.apache.parquet.schema.PrimitiveComparator.BINARY_AS_SIGNED_INTEGER_COMPARATOR; | ||
| import static org.apache.parquet.schema.PrimitiveComparator.BOOLEAN_COMPARATOR; | ||
| import static org.apache.parquet.schema.PrimitiveComparator.DOUBLE_COMPARATOR; | ||
|
|
@@ -36,8 +37,12 @@ | |
|
|
||
| import java.math.BigInteger; | ||
| import java.nio.ByteBuffer; | ||
| import java.time.LocalDateTime; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import java.util.function.Function; | ||
| import org.apache.parquet.example.data.simple.NanoTime; | ||
| import org.apache.parquet.io.api.Binary; | ||
| import org.junit.Test; | ||
|
|
||
|
|
@@ -354,6 +359,60 @@ public void testBinaryAsSignedIntegerComparatorWithEquals() { | |
| } | ||
| } | ||
|
|
||
| private static Binary int96(int julianDay, long nanosOfDay) { | ||
| return new NanoTime(julianDay, nanosOfDay).toBinary(); | ||
| } | ||
|
|
||
| private static Binary timestampToInt96(String timestamp) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| LocalDateTime dt = LocalDateTime.parse(timestamp); | ||
| int julianDay = (int) (dt.toLocalDate().toEpochDay() + 2440588); | ||
| return new NanoTime(julianDay, dt.toLocalTime().toNanoOfDay()).toBinary(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testInt96TimestampComparator() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| Binary[] valuesInAscendingOrder = { | ||
| int96(Integer.MIN_VALUE, 0), // most negative julian day | ||
| int96(-1, 86_399_999_999_999L), // negative julian days sort before day 0 | ||
| int96(0, 0), // start of the julian period | ||
| int96(0, 86_399_999_999_999L), // same day, later time of day | ||
| timestampToInt96("1968-05-23T00:00:00.000000123"), // pre-epoch but positive julian day | ||
| timestampToInt96("2020-01-01T12:00:00"), | ||
| timestampToInt96("2020-02-01T11:00:00"), // later day even though earlier time of day | ||
| timestampToInt96("2020-02-01T11:00:00.000000001"), // nanos tie-break | ||
| int96(Integer.MAX_VALUE, 86_399_999_999_999L) | ||
| }; | ||
|
|
||
| // The same value in different Binary representations must compare identically; the offset | ||
| // 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()), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| b -> Binary.fromConstantByteArray(b.getBytes()), | ||
| b -> { | ||
| byte[] bytes = b.getBytes(); | ||
| byte[] padded = new byte[bytes.length + 20]; | ||
| Arrays.fill(padded, (byte) 0xAA); | ||
| System.arraycopy(bytes, 0, padded, 10, bytes.length); | ||
| return Binary.fromReusedByteArray(padded, 10, bytes.length); | ||
| }); | ||
|
|
||
| for (int i = 0; i < valuesInAscendingOrder.length; ++i) { | ||
| for (int j = 0; j < valuesInAscendingOrder.length; ++j) { | ||
| for (Function<Binary, Binary> fi : representations) { | ||
| for (Function<Binary, Binary> fj : representations) { | ||
| Binary bi = fi.apply(valuesInAscendingOrder[i]); | ||
| Binary bj = fj.apply(valuesInAscendingOrder[j]); | ||
| assertEquals( | ||
| "comparing value " + i + " to value " + j, | ||
| Integer.signum(Integer.compare(i, j)), | ||
| Integer.signum(BINARY_AS_INT96_TIMESTAMP_COMPARATOR.compare(bi, bj))); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testFloat16Comparator() { | ||
| Binary[] valuesInAscendingOrder = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,7 @@ | |
| import org.apache.parquet.format.GeometryType; | ||
| import org.apache.parquet.format.GeospatialStatistics; | ||
| import org.apache.parquet.format.IEEE754TotalOrder; | ||
| import org.apache.parquet.format.Int96TimestampOrder; | ||
| import org.apache.parquet.format.IntType; | ||
| import org.apache.parquet.format.KeyValue; | ||
| import org.apache.parquet.format.LogicalType; | ||
|
|
@@ -146,6 +147,7 @@ public class ParquetMetadataConverter { | |
|
|
||
| private static final TypeDefinedOrder TYPE_DEFINED_ORDER = new TypeDefinedOrder(); | ||
| private static final IEEE754TotalOrder IEEE_754_TOTAL_ORDER = new IEEE754TotalOrder(); | ||
| private static final Int96TimestampOrder INT96_TIMESTAMP_ORDER = new Int96TimestampOrder(); | ||
| public static final MetadataFilter NO_FILTER = new NoFilter(); | ||
| public static final MetadataFilter SKIP_ROW_GROUPS = new SkipMetadataFilter(); | ||
| public static final long MAX_STATS_SIZE = 4096; // limit stats to 4k | ||
|
|
@@ -290,6 +292,9 @@ private List<ColumnOrder> getColumnOrders(MessageType schema) { | |
| case IEEE_754_TOTAL_ORDER: | ||
| columnOrder.setIEEE_754_TOTAL_ORDER(IEEE_754_TOTAL_ORDER); | ||
| break; | ||
| case INT96_TIMESTAMP_ORDER: | ||
| columnOrder.setINT96_TIMESTAMP_ORDER(INT96_TIMESTAMP_ORDER); | ||
| break; | ||
| case UNDEFINED: | ||
| // Use TypeDefinedOrder if some types (e.g. INT96) have undefined column orders. | ||
| columnOrder.setTYPE_ORDER(TYPE_DEFINED_ORDER); | ||
|
|
@@ -911,8 +916,10 @@ private static byte[] tuncateMax(BinaryTruncator truncator, int truncateLength, | |
| } | ||
|
|
||
| private static boolean isMinMaxStatsSupported(PrimitiveType type) { | ||
| return type.columnOrder().getColumnOrderName() == ColumnOrderName.TYPE_DEFINED_ORDER | ||
| || type.columnOrder().getColumnOrderName() == ColumnOrderName.IEEE_754_TOTAL_ORDER; | ||
| ColumnOrderName name = type.columnOrder().getColumnOrderName(); | ||
| return name == ColumnOrderName.TYPE_DEFINED_ORDER | ||
| || name == ColumnOrderName.IEEE_754_TOTAL_ORDER | ||
| || name == ColumnOrderName.INT96_TIMESTAMP_ORDER; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is getting too complicated. Either use a switch or use |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -2057,7 +2064,17 @@ private void buildChildren( | |
| || schemaElement.converted_type == ConvertedType.INTERVAL)) { | ||
| columnOrder = org.apache.parquet.schema.ColumnOrder.undefined(); | ||
| } | ||
| // INT96_TIMESTAMP_ORDER is only valid for INT96 columns, ignore it anywhere else. | ||
| if (columnOrder.getColumnOrderName() == ColumnOrderName.INT96_TIMESTAMP_ORDER | ||
| && schemaElement.type != Type.INT96) { | ||
| columnOrder = org.apache.parquet.schema.ColumnOrder.undefined(); | ||
| } | ||
| primitiveBuilder.columnOrder(columnOrder); | ||
| } else if (schemaElement.type == Type.INT96) { | ||
| // 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()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There isn't a test in |
||
| } | ||
| childBuilder = primitiveBuilder; | ||
| } else { | ||
|
|
@@ -2110,6 +2127,9 @@ private static org.apache.parquet.schema.ColumnOrder fromParquetColumnOrder(Colu | |
| if (columnOrder.isSetIEEE_754_TOTAL_ORDER()) { | ||
| return org.apache.parquet.schema.ColumnOrder.ieee754TotalOrder(); | ||
| } | ||
| if (columnOrder.isSetINT96_TIMESTAMP_ORDER()) { | ||
| return org.apache.parquet.schema.ColumnOrder.int96TimestampOrder(); | ||
| } | ||
| // The column order is not yet supported by this API | ||
| return org.apache.parquet.schema.ColumnOrder.undefined(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1171,17 +1171,14 @@ public void testMissingValuesFromStats() { | |
|
|
||
| @Test | ||
| public void testSkippedV2Stats() { | ||
| // INTERVAL has an undefined column order, so its stats are skipped. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove this. It is unrelated and doesn't need to be part of this PR. |
||
| testSkippedV2Stats( | ||
| Types.optional(PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY) | ||
| .length(12) | ||
| .as(OriginalType.INTERVAL) | ||
| .named(""), | ||
| new BigInteger("12345678"), | ||
| new BigInteger("12345679")); | ||
| testSkippedV2Stats( | ||
| Types.optional(PrimitiveTypeName.INT96).named(""), | ||
| new BigInteger("-75687987"), | ||
| new BigInteger("45367657")); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are other tests that also need to be updated. For example, I think this should update all of the INT96 cases to use |
||
| } | ||
|
|
||
| private void testSkippedV2Stats(PrimitiveType type, Object min, Object max) { | ||
|
|
@@ -1383,8 +1380,7 @@ public void testColumnOrders() throws IOException { | |
| + " required binary key (UTF8);" // Key to be hacked to have unknown column order -> undefined | ||
| + " optional group list_col (LIST) {" | ||
| + " repeated group list {" | ||
| + " optional int96 array_element;" // INT96 element with type defined column order -> | ||
| // undefined | ||
| + " optional int96 array_element;" // plain INT96 element -> INT96_TIMESTAMP_ORDER | ||
| + " }" | ||
| + " }" | ||
| + " }" | ||
|
|
@@ -1398,9 +1394,10 @@ public void testColumnOrders() throws IOException { | |
|
|
||
| List<org.apache.parquet.format.ColumnOrder> columnOrders = formatMetadata.getColumn_orders(); | ||
| assertEquals(3, columnOrders.size()); | ||
| for (org.apache.parquet.format.ColumnOrder columnOrder : columnOrders) { | ||
| assertTrue(columnOrder.isSetTYPE_ORDER()); | ||
| } | ||
| // binary_col and key get TYPE_ORDER, the INT96 array_element gets INT96_TIMESTAMP_ORDER. | ||
| assertTrue(columnOrders.get(0).isSetTYPE_ORDER()); | ||
| assertTrue(columnOrders.get(1).isSetTYPE_ORDER()); | ||
| assertTrue(columnOrders.get(2).isSetINT96_TIMESTAMP_ORDER()); | ||
|
|
||
| // Simulate that thrift got a union type that is not in the generated code | ||
| // (when the file contains a not-yet-supported column order) | ||
|
|
@@ -1413,7 +1410,7 @@ public void testColumnOrders() throws IOException { | |
| assertEquals( | ||
| ColumnOrder.typeDefined(), columns.get(0).getPrimitiveType().columnOrder()); | ||
| assertEquals(ColumnOrder.undefined(), columns.get(1).getPrimitiveType().columnOrder()); | ||
| assertEquals(ColumnOrder.undefined(), columns.get(2).getPrimitiveType().columnOrder()); | ||
| assertEquals(ColumnOrder.int96TimestampOrder(), columns.get(2).getPrimitiveType().columnOrder()); | ||
| } | ||
|
|
||
| @Test | ||
|
|
@@ -1488,7 +1485,11 @@ public void testColumnIndexConversion() { | |
| assertNull( | ||
| "Should ignore unsupported types", | ||
| ParquetMetadataConverter.toParquetColumnIndex( | ||
| Types.required(PrimitiveTypeName.INT96).named("test_int96"), columnIndex)); | ||
| Types.required(PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY) | ||
| .length(12) | ||
| .as(OriginalType.INTERVAL) | ||
| .named("test_interval"), | ||
| columnIndex)); | ||
| assertNull( | ||
| "Should ignore unsupported types", | ||
| ParquetMetadataConverter.fromParquetColumnIndex( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.