xiangfu0 commented on code in PR #20060:
URL: https://github.com/apache/pinot/pull/20060#discussion_r4384414031
##########
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
Review Comment:
Behavior note (not blocking): rejecting UINT64 > Long.MAX now fails
ingestion of the whole file instead of silently wrapping to a negative long.
That's the right call — a wrapped value is corrupt data — but worth a line in
the PR description so operators know previously-"working" files with such
values will now error rather than ingest garbage.
##########
pinot-plugins/pinot-input-format/pinot-parquet/src/main/java/org/apache/pinot/plugin/inputformat/parquet/ParquetUtils.java:
##########
@@ -159,14 +159,37 @@ 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 * 86_400L + Math.floorDiv(nanosOfDay,
1_000_000_000L);
+ Timestamp timestamp = new Timestamp(epochSeconds * 1_000L);
+ timestamp.setNanos((int) Math.floorMod(nanosOfDay, 1_000_000_000L));
+ 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;
+ long carry = Long.compareUnsigned(nanos, product) < 0 ? 1 : 0;
+ long high = Math.multiplyHigh(days, NANOS_PER_DAY) + (nanosOfDay >> 63) +
carry;
+ if (high != (nanos >> 63)) {
+ throw new IllegalArgumentException("INT96 timestamp " +
convertInt96ToTimestamp(int96Bytes).toInstant()
+ + " is outside the signed epoch-nanosecond interval and cannot be
returned as raw epoch nanoseconds");
+ }
+ return nanos;
}
}
Review Comment:
The 128-bit carry logic is correct, but subtle: a one-line note on why
`Long.compareUnsigned(nanos, product) < 0` is the right carry test (it detects
unsigned low-word overflow, with the sign of `nanosOfDay` folded in via
`nanosOfDay >> 63`) would help the next reader. Optional nit.
--
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]
