xiangfu0 commented on code in PR #20060:
URL: https://github.com/apache/pinot/pull/20060#discussion_r4385261260
##########
pinot-plugins/pinot-input-format/pinot-parquet/src/main/java/org/apache/pinot/plugin/inputformat/parquet/ParquetNativeRecordReader.java:
##########
@@ -122,9 +122,10 @@ public GenericRow next(GenericRow reuse)
} catch (Exception e) {
throw new RecordFetchException("Failed to read next Parquet native
record", e);
}
+ // The physical row is consumed even if extraction fails and the caller
skips it.
+ _currentPageIdx++;
Review Comment:
Nice fix — moving the increment ahead of `extract()` is what keeps
`hasNext()` honest when a record is rejected and skipped. Worth noting it
correctly stays *after* the `read()` try/catch, so a genuine fetch failure
doesn't wrongly count a row as consumed.
##########
pinot-plugins/pinot-input-format/pinot-parquet/src/main/java/org/apache/pinot/plugin/inputformat/parquet/ParquetUtils.java:
##########
@@ -159,14 +161,38 @@ public static Timestamp convertLongToTimestamp(long
value, LogicalTypeAnnotation
}
}
- /// Converts a 12-byte INT96 timestamp to `Long` epoch nanos. INT96 stores
the timestamp split as bytes
- /// 0..7 (nanos within the day, long, little-endian) and bytes 8..11 (Julian
day number, int, little-endian).
- /// Nanos is INT96's natural unit since this physical encoding carries
nanosecond precision; pair with
- /// [TimestampUtils#fromNanosSinceEpoch] for a [Timestamp].
+ /// Converts a 12-byte INT96 timestamp to [Timestamp] without narrowing it
to signed epoch nanoseconds.
+ /// INT96 stores bytes 0..7 as nanoseconds within the day and bytes 8..11 as
the Julian day, little-endian.
+ /// This also preserves instants outside the approximately 1677–2262 range
of signed epoch nanoseconds.
+ public static Timestamp convertInt96ToTimestamp(byte[] int96Bytes) {
+ ByteBuffer buffer =
ByteBuffer.wrap(int96Bytes).order(ByteOrder.LITTLE_ENDIAN);
+ long days = buffer.getInt(8) - JULIAN_DAY_NUMBER_FOR_UNIX_EPOCH;
+ long nanosOfDay = buffer.getLong(0);
+ long epochSeconds = days * SECONDS_PER_DAY + Math.floorDiv(nanosOfDay,
NANOS_PER_SECOND);
+ Timestamp timestamp = new Timestamp(epochSeconds * 1_000L);
+ timestamp.setNanos((int) Math.floorMod(nanosOfDay, NANOS_PER_SECOND));
+ return timestamp;
+ }
+
+ /// Converts a 12-byte INT96 timestamp to signed `Long` epoch nanoseconds.
+ ///
+ /// @throws IllegalArgumentException if the exact value does not fit in a
signed `long`; logical extraction
+ /// should use [#convertInt96ToTimestamp(byte\[\])] instead
public static long convertInt96ToEpochNanos(byte[] int96Bytes) {
- ByteBuffer buf =
ByteBuffer.wrap(int96Bytes).order(ByteOrder.LITTLE_ENDIAN);
- long days = buf.getInt(8) - JULIAN_DAY_NUMBER_FOR_UNIX_EPOCH;
- long nanosOfDay = buf.getLong(0);
- return days * NANOS_PER_DAY + nanosOfDay;
+ ByteBuffer buffer =
ByteBuffer.wrap(int96Bytes).order(ByteOrder.LITTLE_ENDIAN);
+ long days = buffer.getInt(8) - JULIAN_DAY_NUMBER_FOR_UNIX_EPOCH;
+ long nanosOfDay = buffer.getLong(0);
+ // Evaluate the complete sum in 128 bits. At Long.MIN_VALUE, the day
product alone underflows, but adding
+ // the positive day fraction brings the sum back into range, so
multiplyExact followed by addExact is wrong.
+ long product = days * NANOS_PER_DAY;
+ long nanos = product + nanosOfDay;
+ // Unsigned low-word overflow produces a carry; nanosOfDay >> 63
sign-extends the addend into the high word.
+ long carry = Long.compareUnsigned(nanos, product) < 0 ? 1 : 0;
Review Comment:
Verified this carry logic against a BigInteger oracle over 20M random cases
(incl. Long.MIN/MAX nanosOfDay) — no mismatches or false throws. The
reconstruction of the high word is solid.
##########
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) {
Review Comment:
`longValue < 0` is exactly the set of UINT64 values above Long.MAX, so the
guard is precise. Good that the message uses `toUnsignedString` so the logged
value is the real one.
--
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]
