You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "xndai (via GitHub)" <gi...@apache.org> on 2026/08/08 22:51:37 UTC
[PR] Parquet: Fix null counts for nested fields of a null struct [iceberg]
xndai opened a new pull request, #17560:
URL: https://github.com/apache/iceberg/pull/17560
When an optional struct is null, OptionWriter writes a null directly to every leaf column it contains, so the writers for those columns never see the value and cannot count it. OptionWriter dropped its own null count in that case, with a comment saying nested null stats were not used. They are used: the counts it returns become DataFile.nullValueCounts.
Float and double are the only types whose writers report metrics, and ParquetMetrics prefers writer metrics over footer statistics, so the correct footer count was never used. A float or double under a nullable struct was reported as having 0 nulls even when the struct was null for some rows.
The incorrect counting of nulls could affect query engines that relay on this stats for optimization. For example, they could simply skip the file with null_count == 0 for predicate `WHERE c.f_id IS NULL` and produces wrong result.
Add the nulls counted by an option writer to the metrics of the columns it wrote them to, at any depth. And add corresponding tests.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3786491593
##########
data/src/test/java/org/apache/iceberg/parquet/TestParquetMetrics.java:
##########
@@ -107,4 +123,96 @@ public int splitCount(InputFile inputFile) throws IOException {
public boolean supportsSmallRowGroups() {
return true;
}
+
+ @TestTemplate
+ public void testMetricsForNullStructWithFloatingAndGeoLeaves() throws IOException {
+ // float, double, geometry and geography are the only types whose writers report metrics, so
+ // they are the ones whose nested null counts could be dropped when a struct is null. Null
+ // counts are only tracked for optional fields.
+ StructType struct =
+ StructType.of(
+ optional(2, "optDouble", DoubleType.get()),
+ optional(3, "optFloat", FloatType.get()),
+ optional(4, "geom", GeometryType.crs84()),
+ optional(5, "optLong", LongType.get()));
+ Schema schema = new Schema(optional(1, "struct", struct));
+
+ Record inner = GenericRecord.create(struct);
+ inner.setField("optDouble", 1.5D);
+ inner.setField("optFloat", 2.5F);
+ inner.setField("geom", wkbPoint(30, 10));
+ inner.setField("optLong", 10L);
+ Record withStruct = GenericRecord.create(schema);
+ withStruct.setField("struct", inner);
+ Record nullStruct = GenericRecord.create(schema);
+ nullStruct.setField("struct", null);
+
+ Metrics metrics = getMetrics(schema, withStruct, nullStruct, nullStruct);
+
+ assertThat(metrics.recordCount()).isEqualTo(3L);
+ // each leaf has one value from the populated struct and two nulls from the null structs
+ assertCounts(2, 3L, 2L, 0L, metrics);
+ assertCounts(3, 3L, 2L, 0L, metrics);
+ assertCounts(4, 3L, 2L, metrics);
+ // a type without writer metrics was already correct via the footer; included as a control
+ assertCounts(5, 3L, 2L, metrics);
+
+ // the counts also reach a data file built from these metrics
+ DataFile dataFile =
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/file.parquet")
+ .withFileSizeInBytes(1024)
+ .withFormat(FileFormat.PARQUET)
+ .withMetrics(metrics)
+ .build();
+ assertThat(dataFile.nullValueCounts()).containsEntry(2, 2L).containsEntry(3, 2L);
+ }
+
+ @TestTemplate
+ public void testMetricsForRequiredNestedFieldInNullStruct() throws IOException {
Review Comment:
These tests duplicate a lot of what is tested in `TestParquetValueWriters`. I think the responsibility of this class is to validate the conversion of `FieldMetrics` to Parquet metrics, not to validate by type. I think one nested test is sufficient.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "xndai (via GitHub)" <gi...@apache.org>.
xndai commented on PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#issuecomment-5704510673
@rdblue @nssalian @RussellSpitzer can you help review this? This is still a correctness issue in the library and would like to get it addressed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3786396573
##########
data/src/test/java/org/apache/iceberg/parquet/TestParquetMetrics.java:
##########
@@ -107,4 +123,96 @@ public int splitCount(InputFile inputFile) throws IOException {
public boolean supportsSmallRowGroups() {
return true;
}
+
+ @TestTemplate
+ public void testMetricsForNullStructWithFloatingAndGeoLeaves() throws IOException {
Review Comment:
Let's separate this to have a test case per type.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3786472649
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetValueWriters.java:
##########
@@ -60,4 +68,144 @@ void geospatialValueSizeMetricsExcludeNulls() {
assertThat(metrics.nullValueCount()).isEqualTo(1);
assertThat(metrics.avgValueSizeInBytes()).isEqualTo(31);
}
+
+ @Test
+ void nullStructCountsNullsForOptionalNestedFields() {
+ // a null struct is also null for the optional fields it contains, but those columns are written
+ // by the struct's writer and never see the value, so the struct must count the nulls for them
+ Types.StructType struct =
+ Types.StructType.of(
+ optional(2, "d", Types.DoubleType.get()), optional(3, "f", Types.FloatType.get()));
+ Schema schema = new Schema(optional(1, "s", struct));
+
+ ParquetValueWriter<Record> writer = writerFor(schema);
+ Record inner = GenericRecord.create(struct);
+ inner.set(0, 2.0D);
+ inner.set(1, 1.0F);
+
+ writer.write(0, record(schema, inner));
+ writer.write(0, record(schema, null));
+ writer.write(0, record(schema, null));
+
+ Map<Integer, FieldMetrics<?>> metrics = metricsById(writer);
+ // both fields have one non-null value and two nulls from the null structs
+ assertThat(metrics.get(2).nullValueCount()).isEqualTo(2);
+ assertThat(metrics.get(2).valueCount()).isEqualTo(3);
+ assertThat(metrics.get(3).nullValueCount()).isEqualTo(2);
+ assertThat(metrics.get(3).valueCount()).isEqualTo(3);
+ }
+
+ @Test
+ void nullStructDoesNotCountNullsForRequiredNestedFields() {
Review Comment:
I think you can remove this because we want to track the null count for all fields, regardless of whether the leaf is required.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#issuecomment-5705682761
@xndai thanks for cleaning up the PR. This might need a rebase before going through.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3786478826
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetValueWriters.java:
##########
@@ -60,4 +68,144 @@ void geospatialValueSizeMetricsExcludeNulls() {
assertThat(metrics.nullValueCount()).isEqualTo(1);
assertThat(metrics.avgValueSizeInBytes()).isEqualTo(31);
}
+
+ @Test
+ void nullStructCountsNullsForOptionalNestedFields() {
+ // a null struct is also null for the optional fields it contains, but those columns are written
+ // by the struct's writer and never see the value, so the struct must count the nulls for them
+ Types.StructType struct =
+ Types.StructType.of(
+ optional(2, "d", Types.DoubleType.get()), optional(3, "f", Types.FloatType.get()));
+ Schema schema = new Schema(optional(1, "s", struct));
+
+ ParquetValueWriter<Record> writer = writerFor(schema);
+ Record inner = GenericRecord.create(struct);
+ inner.set(0, 2.0D);
+ inner.set(1, 1.0F);
+
+ writer.write(0, record(schema, inner));
+ writer.write(0, record(schema, null));
+ writer.write(0, record(schema, null));
+
+ Map<Integer, FieldMetrics<?>> metrics = metricsById(writer);
+ // both fields have one non-null value and two nulls from the null structs
+ assertThat(metrics.get(2).nullValueCount()).isEqualTo(2);
+ assertThat(metrics.get(2).valueCount()).isEqualTo(3);
+ assertThat(metrics.get(3).nullValueCount()).isEqualTo(2);
+ assertThat(metrics.get(3).valueCount()).isEqualTo(3);
+ }
+
+ @Test
+ void nullStructDoesNotCountNullsForRequiredNestedFields() {
+ // null counts are only tracked for optional fields, so a required leaf keeps its prior
+ // behavior: the null struct's nulls are not added to it
+ Types.StructType struct =
+ Types.StructType.of(
+ optional(2, "d", Types.DoubleType.get()), required(3, "f", Types.FloatType.get()));
+ Schema schema = new Schema(optional(1, "s", struct));
+
+ ParquetValueWriter<Record> writer = writerFor(schema);
+ Record inner = GenericRecord.create(struct);
+ inner.set(0, 2.0D);
+ inner.set(1, 1.0F);
+
+ writer.write(0, record(schema, inner));
+ writer.write(0, record(schema, null));
+ writer.write(0, record(schema, null));
+
+ Map<Integer, FieldMetrics<?>> metrics = metricsById(writer);
+ // the optional field is corrected
+ assertThat(metrics.get(2).nullValueCount()).isEqualTo(2);
+ assertThat(metrics.get(2).valueCount()).isEqualTo(3);
+ // the required field is left as the writer saw it: only the one populated value
+ assertThat(metrics.get(3).nullValueCount()).isEqualTo(0);
+ assertThat(metrics.get(3).valueCount()).isEqualTo(1);
+ }
+
+ @Test
+ void nullStructAddsToNullsCountedByNestedField() {
+ Types.StructType struct = Types.StructType.of(optional(2, "d", Types.DoubleType.get()));
+ Schema schema = new Schema(optional(1, "s", struct));
+
+ ParquetValueWriter<Record> writer = writerFor(schema);
+ Record present = GenericRecord.create(struct);
+ present.set(0, 2.0D);
Review Comment:
It is _okay_ to use the ordinal-based set method, but we prefer using `setField` by name.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "xndai (via GitHub)" <gi...@apache.org>.
xndai commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3787761557
##########
data/src/test/java/org/apache/iceberg/parquet/TestParquetMetrics.java:
##########
@@ -107,4 +123,96 @@ public int splitCount(InputFile inputFile) throws IOException {
public boolean supportsSmallRowGroups() {
return true;
}
+
+ @TestTemplate
+ public void testMetricsForNullStructWithFloatingAndGeoLeaves() throws IOException {
Review Comment:
Done.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#issuecomment-6243714827
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested fields of a null struct [iceberg]
Posted by "xndai (via GitHub)" <gi...@apache.org>.
xndai commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3753531633
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetValueWriters.java:
##########
@@ -60,4 +68,117 @@ void geospatialValueSizeMetricsExcludeNulls() {
assertThat(metrics.nullValueCount()).isEqualTo(1);
assertThat(metrics.avgValueSizeInBytes()).isEqualTo(31);
}
+
Review Comment:
Add a new test `testMetricsForNullStructWithFloatingAndGeoLeaves` in TestParquetMetrics, which covers the end to end writing parquet file and read back the `Metrics`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "xndai (via GitHub)" <gi...@apache.org>.
xndai commented on PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#issuecomment-5413558651
@rdblue can you please review again?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "xndai (via GitHub)" <gi...@apache.org>.
xndai commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3786602849
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueWriters.java:
##########
@@ -494,9 +515,31 @@ public Stream<FieldMetrics<?>> metrics() {
}
}
- // skipping updating null stats for non-primitive types since we don't use them today, to
- // avoid unnecessary work
- return writer.metrics();
+ // A null value here is also null for every descendant column, but those columns are written
+ // directly and never see it, so their writers cannot count it. Add it to the metrics of
+ // optional descendants; null counts are not tracked for required fields.
+ return writer
+ .metrics()
+ .map(
+ metrics ->
+ optionalFieldIds.contains(metrics.id()) ? addNullValues(metrics) : metrics);
Review Comment:
You were reviewing an old diff. I have removed this in the latest PR to align with our latest discussion. Please check again. Thanks.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "xndai (via GitHub)" <gi...@apache.org>.
xndai commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3787772042
##########
data/src/test/java/org/apache/iceberg/parquet/TestParquetMetrics.java:
##########
@@ -107,4 +123,96 @@ public int splitCount(InputFile inputFile) throws IOException {
public boolean supportsSmallRowGroups() {
return true;
}
+
+ @TestTemplate
+ public void testMetricsForNullStructWithFloatingAndGeoLeaves() throws IOException {
+ // float, double, geometry and geography are the only types whose writers report metrics, so
+ // they are the ones whose nested null counts could be dropped when a struct is null. Null
+ // counts are only tracked for optional fields.
+ StructType struct =
+ StructType.of(
+ optional(2, "optDouble", DoubleType.get()),
+ optional(3, "optFloat", FloatType.get()),
+ optional(4, "geom", GeometryType.crs84()),
+ optional(5, "optLong", LongType.get()));
+ Schema schema = new Schema(optional(1, "struct", struct));
+
+ Record inner = GenericRecord.create(struct);
+ inner.setField("optDouble", 1.5D);
+ inner.setField("optFloat", 2.5F);
+ inner.setField("geom", wkbPoint(30, 10));
+ inner.setField("optLong", 10L);
+ Record withStruct = GenericRecord.create(schema);
+ withStruct.setField("struct", inner);
+ Record nullStruct = GenericRecord.create(schema);
+ nullStruct.setField("struct", null);
+
+ Metrics metrics = getMetrics(schema, withStruct, nullStruct, nullStruct);
+
+ assertThat(metrics.recordCount()).isEqualTo(3L);
+ // each leaf has one value from the populated struct and two nulls from the null structs
+ assertCounts(2, 3L, 2L, 0L, metrics);
+ assertCounts(3, 3L, 2L, 0L, metrics);
+ assertCounts(4, 3L, 2L, metrics);
+ // a type without writer metrics was already correct via the footer; included as a control
+ assertCounts(5, 3L, 2L, metrics);
+
+ // the counts also reach a data file built from these metrics
+ DataFile dataFile =
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/file.parquet")
+ .withFileSizeInBytes(1024)
+ .withFormat(FileFormat.PARQUET)
+ .withMetrics(metrics)
+ .build();
+ assertThat(dataFile.nullValueCounts()).containsEntry(2, 2L).containsEntry(3, 2L);
+ }
+
+ @TestTemplate
+ public void testMetricsForRequiredNestedFieldInNullStruct() throws IOException {
Review Comment:
The test in `TestParquetMetrics.java` was added per feedback from @uros-b. That gives us a bit more end to end coverage and verify the stats reaches a `DataFile`. Now I only keep one general case. Tests for different types with optional and required are done in `TestParquetValueWriters`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "xndai (via GitHub)" <gi...@apache.org>.
xndai commented on PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#issuecomment-5282871210
I scoped down this fix to only apply to optional field, and not changing the required field behavior today. I believe the fix is still needed as for optional field current null_value_count is incorrect when parent struct is null. @rdblue @uros-b can you please review.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested fields of a null struct [iceberg]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3743403240
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetValueWriters.java:
##########
@@ -60,4 +68,117 @@ void geospatialValueSizeMetricsExcludeNulls() {
assertThat(metrics.nullValueCount()).isEqualTo(1);
assertThat(metrics.avgValueSizeInBytes()).isEqualTo(31);
}
+
Review Comment:
All four new tests assert only at the writer.metrics() boundary via a mocked ColumnWriteStore; none pins the corrected count at the user-visible end of the claim, that it reaches DataFile.nullValueCounts(). The seam is load-bearing: ParquetMetrics.primitive() (lines 242–248) prefers writer metrics and only falls through to the footer when absent, and ParquetMetrics.metrics() line 146 filters on nullValueCount() >= 0 before the value lands in the manifest. A future refactor of either could silently drop this fix with all four writer-level tests still green. There is an existing harness for exactly this (TestMetrics/TestParquetMetrics with assertCounts(fieldId, valueCount, nullValueCount, metrics)), but the existing NESTED_SCHEMA uses required(2, "nestedStructCol", ...), so a new test case with an optional struct containing a double leaf (or float, or geo) is needed. One added case in that harness closes the gap.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3786453831
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueWriters.java:
##########
@@ -56,12 +58,38 @@ private ParquetValueWriters() {}
public static <T> ParquetValueWriter<T> option(
Type type, int definitionLevel, ParquetValueWriter<T> writer) {
if (type.isRepetition(Type.Repetition.OPTIONAL)) {
- return new OptionWriter<>(definitionLevel, writer);
+ return new OptionWriter<>(definitionLevel, writer, optionalLeafIds(type));
}
return writer;
}
+ /**
+ * Collects the ids of optional primitive leaves within a type. A null written for an optional
+ * value is also a null for these fields, but not for required fields, whose null count is left as
+ * it was before (per the spec, null counts are only tracked for optional fields).
+ */
+ private static Set<Integer> optionalLeafIds(Type type) {
Review Comment:
As I noted below, I don't think that the behavior depends on whether the leaf type is optional. All fields should be updated if there is an optional parent that results in using the option writer.
A couple of other notes for future contributions though:
- This isn't the right place to analyze the leaf IDs. We write most type structure logic for Iceberg types so that it is reusable.
- Methods should not be written to process a tree structure recursively. The project uses visitors to standardize how types are visited so that we have consistent logic throughout. It also isolates the logic to traverse a type from the logic for what to do with each case.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "xndai (via GitHub)" <gi...@apache.org>.
xndai commented on PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#issuecomment-5707132146
> @xndai thanks for cleaning up the PR. This might need a rebase before going through.
Rebased. Thanks. There are some back and forth in the review thread. I tried to do it in a way so the history can be kept as much as possible.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#issuecomment-6391410964
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested fields of a null struct [iceberg]
Posted by "xndai (via GitHub)" <gi...@apache.org>.
xndai commented on PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#issuecomment-5228598494
Issue - https://github.com/apache/iceberg/issues/17561
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "xndai (via GitHub)" <gi...@apache.org>.
xndai commented on PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#issuecomment-5296623464
After more investigation and talking to @rdblue (https://github.com/apache/iceberg/pull/17413#discussion_r3779497865), I believe the original fix is correct, and we should record the null count even for required field. I reverted the last two commit, so the fix applies to both required and optional fields.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "xndai (via GitHub)" <gi...@apache.org>.
xndai commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3786605380
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueWriters.java:
##########
@@ -56,12 +58,38 @@ private ParquetValueWriters() {}
public static <T> ParquetValueWriter<T> option(
Type type, int definitionLevel, ParquetValueWriter<T> writer) {
if (type.isRepetition(Type.Repetition.OPTIONAL)) {
- return new OptionWriter<>(definitionLevel, writer);
+ return new OptionWriter<>(definitionLevel, writer, optionalLeafIds(type));
}
return writer;
}
+ /**
+ * Collects the ids of optional primitive leaves within a type. A null written for an optional
+ * value is also a null for these fields, but not for required fields, whose null count is left as
+ * it was before (per the spec, null counts are only tracked for optional fields).
+ */
+ private static Set<Integer> optionalLeafIds(Type type) {
Review Comment:
Same as above. And thanks for the general guides.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "xndai (via GitHub)" <gi...@apache.org>.
xndai commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3787762154
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetValueWriters.java:
##########
@@ -60,4 +68,144 @@ void geospatialValueSizeMetricsExcludeNulls() {
assertThat(metrics.nullValueCount()).isEqualTo(1);
assertThat(metrics.avgValueSizeInBytes()).isEqualTo(31);
}
+
+ @Test
+ void nullStructCountsNullsForOptionalNestedFields() {
+ // a null struct is also null for the optional fields it contains, but those columns are written
Review Comment:
Fixed the wording.
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetValueWriters.java:
##########
@@ -60,4 +68,144 @@ void geospatialValueSizeMetricsExcludeNulls() {
assertThat(metrics.nullValueCount()).isEqualTo(1);
assertThat(metrics.avgValueSizeInBytes()).isEqualTo(31);
}
+
+ @Test
+ void nullStructCountsNullsForOptionalNestedFields() {
+ // a null struct is also null for the optional fields it contains, but those columns are written
+ // by the struct's writer and never see the value, so the struct must count the nulls for them
+ Types.StructType struct =
+ Types.StructType.of(
+ optional(2, "d", Types.DoubleType.get()), optional(3, "f", Types.FloatType.get()));
Review Comment:
Done.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #17560: Parquet: Fix null counts for nested optional fields of a null struct
URL: https://github.com/apache/iceberg/pull/17560
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3786436321
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueWriters.java:
##########
@@ -494,9 +515,31 @@ public Stream<FieldMetrics<?>> metrics() {
}
}
- // skipping updating null stats for non-primitive types since we don't use them today, to
- // avoid unnecessary work
- return writer.metrics();
+ // A null value here is also null for every descendant column, but those columns are written
+ // directly and never see it, so their writers cannot count it. Add it to the metrics of
+ // optional descendants; null counts are not tracked for required fields.
+ return writer
+ .metrics()
+ .map(
+ metrics ->
+ optionalFieldIds.contains(metrics.id()) ? addNullValues(metrics) : metrics);
Review Comment:
I don't think this is correct. I think we want to update the null count regardless of whether the inner field is required or optional. The only case where we could omit the null count is when the field and all of its parents are required because it won't be accessed.
That means that the leaf's required/optional status doesn't matter. Any metrics that pass through this level should be updated.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3786468453
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetValueWriters.java:
##########
@@ -60,4 +68,144 @@ void geospatialValueSizeMetricsExcludeNulls() {
assertThat(metrics.nullValueCount()).isEqualTo(1);
assertThat(metrics.avgValueSizeInBytes()).isEqualTo(31);
}
+
+ @Test
+ void nullStructCountsNullsForOptionalNestedFields() {
+ // a null struct is also null for the optional fields it contains, but those columns are written
+ // by the struct's writer and never see the value, so the struct must count the nulls for them
+ Types.StructType struct =
+ Types.StructType.of(
+ optional(2, "d", Types.DoubleType.get()), optional(3, "f", Types.FloatType.get()));
Review Comment:
I think one of these fields should be required to also test that case.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3786464356
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetValueWriters.java:
##########
@@ -60,4 +68,144 @@ void geospatialValueSizeMetricsExcludeNulls() {
assertThat(metrics.nullValueCount()).isEqualTo(1);
assertThat(metrics.avgValueSizeInBytes()).isEqualTo(31);
}
+
+ @Test
+ void nullStructCountsNullsForOptionalNestedFields() {
+ // a null struct is also null for the optional fields it contains, but those columns are written
Review Comment:
This isn't true because it is a stronger statement. The fields themselves are not null, but the parent being null is counted in the stats.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Parquet: Fix null counts for nested optional fields of a null struct [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3786478826
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetValueWriters.java:
##########
@@ -60,4 +68,144 @@ void geospatialValueSizeMetricsExcludeNulls() {
assertThat(metrics.nullValueCount()).isEqualTo(1);
assertThat(metrics.avgValueSizeInBytes()).isEqualTo(31);
}
+
+ @Test
+ void nullStructCountsNullsForOptionalNestedFields() {
+ // a null struct is also null for the optional fields it contains, but those columns are written
+ // by the struct's writer and never see the value, so the struct must count the nulls for them
+ Types.StructType struct =
+ Types.StructType.of(
+ optional(2, "d", Types.DoubleType.get()), optional(3, "f", Types.FloatType.get()));
+ Schema schema = new Schema(optional(1, "s", struct));
+
+ ParquetValueWriter<Record> writer = writerFor(schema);
+ Record inner = GenericRecord.create(struct);
+ inner.set(0, 2.0D);
+ inner.set(1, 1.0F);
+
+ writer.write(0, record(schema, inner));
+ writer.write(0, record(schema, null));
+ writer.write(0, record(schema, null));
+
+ Map<Integer, FieldMetrics<?>> metrics = metricsById(writer);
+ // both fields have one non-null value and two nulls from the null structs
+ assertThat(metrics.get(2).nullValueCount()).isEqualTo(2);
+ assertThat(metrics.get(2).valueCount()).isEqualTo(3);
+ assertThat(metrics.get(3).nullValueCount()).isEqualTo(2);
+ assertThat(metrics.get(3).valueCount()).isEqualTo(3);
+ }
+
+ @Test
+ void nullStructDoesNotCountNullsForRequiredNestedFields() {
+ // null counts are only tracked for optional fields, so a required leaf keeps its prior
+ // behavior: the null struct's nulls are not added to it
+ Types.StructType struct =
+ Types.StructType.of(
+ optional(2, "d", Types.DoubleType.get()), required(3, "f", Types.FloatType.get()));
+ Schema schema = new Schema(optional(1, "s", struct));
+
+ ParquetValueWriter<Record> writer = writerFor(schema);
+ Record inner = GenericRecord.create(struct);
+ inner.set(0, 2.0D);
+ inner.set(1, 1.0F);
+
+ writer.write(0, record(schema, inner));
+ writer.write(0, record(schema, null));
+ writer.write(0, record(schema, null));
+
+ Map<Integer, FieldMetrics<?>> metrics = metricsById(writer);
+ // the optional field is corrected
+ assertThat(metrics.get(2).nullValueCount()).isEqualTo(2);
+ assertThat(metrics.get(2).valueCount()).isEqualTo(3);
+ // the required field is left as the writer saw it: only the one populated value
+ assertThat(metrics.get(3).nullValueCount()).isEqualTo(0);
+ assertThat(metrics.get(3).valueCount()).isEqualTo(1);
+ }
+
+ @Test
+ void nullStructAddsToNullsCountedByNestedField() {
+ Types.StructType struct = Types.StructType.of(optional(2, "d", Types.DoubleType.get()));
+ Schema schema = new Schema(optional(1, "s", struct));
+
+ ParquetValueWriter<Record> writer = writerFor(schema);
+ Record present = GenericRecord.create(struct);
+ present.set(0, 2.0D);
Review Comment:
It is _okay_ to use the ordinal-based `set` method because the schema is defined locally, but we prefer using `setField` by name.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org