Copilot commented on code in PR #3590:
URL: https://github.com/apache/parquet-java/pull/3590#discussion_r3315165327
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
@@ -826,7 +858,7 @@ public static Statistics toParquetStatistics(
formatStats.setMax(max);
}
- if (isMinMaxStatsSupported(stats.type()) || Arrays.equals(min, max)) {
+ if (isMinMaxStatsReadingSupported(createdBy, stats.type()) ||
Arrays.equals(min, max)) {
Review Comment:
`toParquetStatistics` is using `isMinMaxStatsReadingSupported(createdBy,
type)` to decide whether to *write* `min_value/max_value`. That couples stats
emission to the reader-side trust policy (`readInt96Stats` + `ValidInt96Stats`)
and can silently skip INT96 min/max output when `createdBy` is
unrecognized/unparseable or considered “not trusted” (e.g., custom builds or
prerelease versions), even though this writer can still generate correct INT96
stats. Stats emission should be controlled by write-side support
(`isMinMaxStatsWritingSupported` / column order), and the `createdBy`
validation should be applied only when *reading* stats back in
`fromParquetStatisticsInternal` / `fromParquetColumnIndex`.
##########
parquet-hadoop/src/test/java/org/apache/parquet/statistics/RandomValues.java:
##########
@@ -226,13 +227,21 @@ public Int96Generator(long seed) {
}
@Override
- public BigInteger nextValue() {
- return (minimum.add(randomInt96(range)));
+ public Binary nextValue() {
+ long timeOfDay = randomLong();
+ int julianDay = minimumJulianDay + randomPositiveInt(rangeJulianDay);
+
+ ByteBuffer.wrap(buffer)
+ .order(ByteOrder.LITTLE_ENDIAN)
+ .putLong(timeOfDay)
+ .putInt(julianDay);
Review Comment:
`Int96Generator.nextValue()` currently writes `timeOfDay` using
`randomLong()` (can be negative and outside the valid [0, nanos-per-day) range)
and `julianDay` derived from arbitrary random ints (can be negative /
overflow). Since this generator is now explicitly constructing INT96
*timestamp* encodings (little-endian nanos-of-day + Julian day), it should
constrain both fields to valid ranges to avoid generating invalid timestamps
and exercising undefined ordering behavior in INT96-specific comparators.
##########
parquet-hadoop/src/test/java/org/apache/parquet/statistics/RandomValues.java:
##########
@@ -226,13 +227,21 @@ public Int96Generator(long seed) {
}
@Override
- public BigInteger nextValue() {
- return (minimum.add(randomInt96(range)));
+ public Binary nextValue() {
+ long timeOfDay = randomLong();
+ int julianDay = minimumJulianDay + randomPositiveInt(rangeJulianDay);
+
+ ByteBuffer.wrap(buffer)
+ .order(ByteOrder.LITTLE_ENDIAN)
+ .putLong(timeOfDay)
+ .putInt(julianDay);
+
+ return Binary.fromReusedByteArray(buffer, 0, INT_96_LENGTH);
}
@Override
public Binary nextBinaryValue() {
- return FixedBinaryTestUtils.getFixedBinary(INT_96_LENGTH, nextValue());
+ return nextValue();
Review Comment:
`Int96Generator.nextValue()` returns `Binary.fromReusedByteArray(buffer,
...)` where `buffer` is the generator’s mutable scratch array. Any caller that
stores generated values (e.g., `wrapSorted` collects values into a list before
sorting) will end up with many `Binary` instances aliasing the same backing
array, so previously generated values get mutated on subsequent calls. Consider
returning a constant/copy-backed `Binary` for INT96 test data to keep generated
values stable.
--
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]
