xiangfu0 commented on code in PR #20060:
URL: https://github.com/apache/pinot/pull/20060#discussion_r4384708960
##########
pinot-plugins/pinot-input-format/pinot-parquet/src/test/java/org/apache/pinot/plugin/inputformat/parquet/ParquetNativeRecordExtractorTest.java:
##########
@@ -274,6 +320,33 @@ public void testInt96ExtractedAsRawLongEpochNanosWhenRaw()
{
epochMillis * 1_000_000L);
}
+ @Test
+ public void testInt96RangeAndNanosecondBoundaries() {
+ MessageType schema = MessageTypeParser.parseMessageType("message R {
required int96 col; }");
+ for (String text : new String[]{"1970-01-01T00:00:00Z",
"1969-12-31T23:59:59.999999999Z",
+ "1677-09-21T00:12:43.145224192Z", "2262-04-11T23:47:16.854775807Z",
Review Comment:
Nice touch exercising the exact Long.MIN/MAX nanosecond endpoints and the
just-over values here — that's precisely where the 128-bit carry logic could go
wrong. I independently fuzzed the carry math against BigInteger across many
millions of cases (including adversarial negative nanosOfDay) and it matches
exactly. Nit only: the magic 86_400L and 1_000_000_000L literals recur across
both convert methods; a couple of named constants would make the intent read a
bit more clearly.
##########
pinot-plugins/pinot-input-format/pinot-parquet/src/main/java/org/apache/pinot/plugin/inputformat/parquet/ParquetNativeRecordExtractor.java:
##########
@@ -149,6 +148,11 @@ private Object extractValue(Group from, int fieldIndex,
Type fieldType, int inde
return intValue;
case INT64:
long longValue = from.getLong(fieldIndex, index);
+ if (logicalTypeAnnotation instanceof
LogicalTypeAnnotation.IntLogicalTypeAnnotation integerType
+ && integerType.getBitWidth() == 64 && !integerType.isSigned() &&
longValue < 0) {
+ throw new IllegalArgumentException("Parquet UINT64 field " +
fieldType.getName() + " value "
+ + Long.toUnsignedString(longValue) + " exceeds the signed Long
range");
Review Comment:
Worth confirming the intended behavior: a single UINT64 value above Long.MAX
will now throw and fail the whole file/segment ingestion rather than the one
record. That's the right call per fail-fast (silently wrapping to a negative
was a correctness bug), and since this sits in the recursive extractValue it
correctly covers nested struct/list/map fields too — the added tests cover that
well. Just flagging it so the fail-the-whole-file semantics are a conscious
choice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
