nssalian commented on code in PR #17557:
URL: https://github.com/apache/iceberg/pull/17557#discussion_r3972708677
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestVariantMetrics.java:
##########
@@ -466,6 +473,122 @@ public void testShreddedStringBoundsAcrossRowGroups()
throws IOException {
.isEqualTo(Variants.of(supplementary));
}
+ @Test
+ public void testMissingNullCountAcrossRowGroups() throws IOException {
+ // A variant column chunk may omit null_count in its footer statistics,
which Parquet reports
+ // as -1. When one row group is missing the count and another has it, the
total must be
+ // reported as unknown rather than summing the -1 into a lower count.
+ ParquetMetadata footer =
+ footer(Variant.of(EMPTY, Variants.of(1)), null, null); // 1 value, 2
nulls
+
+ // build a two row group footer: the first as written, the second with
null_count removed
+ // from the variant sub columns
+ BlockMetaData withCount = footer.getBlocks().get(0);
+ BlockMetaData withoutCount =
dropVariantNullCounts(footer.getBlocks().get(0));
+ ParquetMetadata twoRowGroups =
+ new ParquetMetadata(footer.getFileMetaData(),
Lists.newArrayList(withCount, withoutCount));
+
+ Metrics metrics =
+ ParquetUtil.footerMetrics(twoRowGroups, Stream.empty(),
MetricsConfig.getDefault());
+
+ // the variant column (id 2) null count is unknown because one row group
did not report it
+ assertThat(metrics.nullValueCounts()).doesNotContainKey(2);
+ assertThat(metrics.valueCounts()).containsEntry(2, 6L);
+ }
+
+ @Test
+ public void testShreddedNullVariantsWithMissingNullCount() throws
IOException {
+ // For a shredded column where the values are either shredded into
typed_value or are null
+ // variants, the value column holds only null variants. That count comes
from the value count,
+ // not the footer null count, so shredded bounds are still trusted even
when a row group omits
+ // null_count.
+ VariantValue value = Variants.of(1234);
+ ParquetMetadata footer =
+ footer(
+ (id, name) -> ParquetVariantUtil.toParquetSchema(value),
+ Variant.of(EMPTY, value),
+ Variant.of(EMPTY, Variants.ofNull()));
+
+ BlockMetaData withCount = footer.getBlocks().get(0);
+ BlockMetaData withoutCount =
dropVariantNullCounts(footer.getBlocks().get(0));
+ ParquetMetadata twoRowGroups =
+ new ParquetMetadata(footer.getFileMetaData(),
Lists.newArrayList(withCount, withoutCount));
+
+ Metrics metrics =
+ ParquetUtil.footerMetrics(twoRowGroups, Stream.empty(),
MetricsConfig.getDefault());
+
+ // the shredded bounds survive: the all-null value column does not depend
on footer null counts
+ assertThat(metrics.lowerBounds().get(2))
+ .extracting(b -> Variant.from(b).value().asObject().get(ROOT_FIELD))
+ .isEqualTo(value);
+ assertThat(metrics.upperBounds().get(2))
+ .extracting(b -> Variant.from(b).value().asObject().get(ROOT_FIELD))
+ .isEqualTo(value);
+ }
+
+ /** Rebuilds a row group with null_count removed from the variant column's
statistics. */
+ private static BlockMetaData dropVariantNullCounts(BlockMetaData block) {
+ BlockMetaData result = new BlockMetaData();
+ result.setRowCount(block.getRowCount());
+ result.setTotalByteSize(block.getTotalByteSize());
+
+ for (ColumnChunkMetaData column : block.getColumns()) {
+ Statistics<?> stats = column.getStatistics();
+ if (column.getPath().toDotString().startsWith("var")) {
+ Statistics.Builder builder =
Statistics.getBuilderForReading(column.getPrimitiveType());
+ if (stats.hasNonNullValue()) {
+ builder.withMin(stats.getMinBytes()).withMax(stats.getMaxBytes());
+ }
+
+ stats = builder.build(); // built without withNumNulls, so getNumNulls
returns -1
+ }
+
+ result.addColumn(
+ ColumnChunkMetaData.get(
+ column.getPath(),
+ column.getPrimitiveType(),
+ column.getCodec(),
+ column.getEncodingStats(),
+ column.getEncodings(),
+ stats,
+ column.getFirstDataPageOffset(),
+ column.getDictionaryPageOffset(),
+ column.getValueCount(),
+ column.getTotalSize(),
+ column.getTotalUncompressedSize()));
+ }
+
+ return result;
+ }
+
+ private ParquetMetadata footer(Variant... variants) throws IOException {
+ return footer(null, variants);
+ }
+
+ private ParquetMetadata footer(VariantShreddingFunction shredding,
Variant... variants)
Review Comment:
Some de-duping here: footer() repeats the same Parquet.write(out)...build()
+ write-loop as writeParquetWithMetricsConfig and
writeParquetWithRowGroupSize. The only difference is the builder option and
return. A writeVariants() helper could be useful here.
--
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]
