You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2026/07/29 12:13:06 UTC
[PR] API: Fix struct metrics in nested struct evaluator test [iceberg]
nastra opened a new pull request, #17412:
URL: https://github.com/apache/iceberg/pull/17412
`FILE_6` in `TestInclusiveMetricsEvaluator` carried value and null counts for the `required_address` and `optional_address` structs. Parquet produces metrics for leaf columns only in `ParquetMetrics.MetricsVisitor.struct` and returns just the concatenation of its children and never emits a `FieldMetrics` for the struct — so no Parquet file yields those entries. They made `notNull("optional_address")` appear prunable when in practice it is not
- Model: Claude Opus 4.6
- Platform/Tool: Cursor
- Human Oversight: [unknown - human to fill in]
- Prompt Summary: Align the nested struct test fixture with the metrics Parquet actually
produces, which excludes struct-level value and null counts.
--
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] API: Fix struct metrics in nested struct evaluator test [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #17412:
URL: https://github.com/apache/iceberg/pull/17412#discussion_r3677269688
##########
api/src/test/java/org/apache/iceberg/expressions/TestInclusiveMetricsEvaluator.java:
##########
Review Comment:
nit, not related to the change but I think the reason to prune the field 104 and 105 for `notNull` predicate is a bit different from "optional_address is optional".
Since both are nested within an optional struct, we cannot decide based on schema whether values are nullable or not. The decision is made based on the leaf field metrics (value count = 5 and null count = 5. so all values are null and evaluate to false)
Meant to comment on LINE966, my bad
--
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] API: Fix struct metrics in nested struct evaluator test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #17412:
URL: https://github.com/apache/iceberg/pull/17412#issuecomment-5150349799
I've opened https://github.com/apache/iceberg/issues/17463 and will go ahead and merge this one. Thanks everyone for the reviews
--
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] API: Fix struct metrics in nested struct evaluator test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #17412:
URL: https://github.com/apache/iceberg/pull/17412
--
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] API: Fix struct metrics in nested struct evaluator test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #17412:
URL: https://github.com/apache/iceberg/pull/17412#issuecomment-5143659925
> > Parquet produces metrics for leaf columns only in ParquetMetrics.MetricsVisitor.struct and returns just the concatenation of its children and never emits a FieldMetrics for the struct — so no Parquet file yields those entries.
>
> This is true for Parquet, but do we have anything in the spec which would prevent other file formats fill out these metrics? Shall we enforce this behaviour through the spec, or handle the struct metrics when they exist in the stats correctly?
That is a good point but we don't have anything in the spec or in the implementation that would prevent this. The spec and the read path don't forbid struct-level value/null counts. This PR matches Parquet (and v4 Java writer) reality and the motivation is to extend this test class and use the content stats evaluator and verify that the same results are produced, which isn't the case for this particular scenario because content stats don't keep stats for structs.
--
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] API: Fix struct metrics in nested struct evaluator test [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #17412:
URL: https://github.com/apache/iceberg/pull/17412#discussion_r3677269688
##########
api/src/test/java/org/apache/iceberg/expressions/TestInclusiveMetricsEvaluator.java:
##########
Review Comment:
nit, not related but I think required field in optional struct means it can be null, we can skip because all values are null (value count = 5 and null count = 5 for field 104)
##########
api/src/test/java/org/apache/iceberg/expressions/TestInclusiveMetricsEvaluator.java:
##########
Review Comment:
nit, not related to the change but I think required field in optional struct means it can be null, we can skip because all values are null (value count = 5 and null count = 5 for field 104)
--
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] API: Fix struct metrics in nested struct evaluator test [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #17412:
URL: https://github.com/apache/iceberg/pull/17412#issuecomment-5128819413
> Parquet produces metrics for leaf columns only in ParquetMetrics.MetricsVisitor.struct and returns just the concatenation of its children and never emits a FieldMetrics for the struct — so no Parquet file yields those entries.
This is true for Parquet, but do we have anything in the spec which would prevent other file formats fill out these metrics? Shall we enforce this behaviour through the spec, or handle the struct metrics when they exist in the stats correctly?
--
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] API: Fix struct metrics in nested struct evaluator test [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #17412:
URL: https://github.com/apache/iceberg/pull/17412#discussion_r3677269688
##########
api/src/test/java/org/apache/iceberg/expressions/TestInclusiveMetricsEvaluator.java:
##########
Review Comment:
nit, not related to the change but I think the reason to prune the field 104 and 105 is a bit different from "optional_address is optional".
Since both are nested within an optional struct, we cannot decide based on schema whether values are nullable or not. The decision is made based on the leaf field metrics (value count = 5 and null count = 5. so all values are null and evaluate to false)
--
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] API: Fix struct metrics in nested struct evaluator test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra closed pull request #17412: API: Fix struct metrics in nested struct evaluator test
URL: https://github.com/apache/iceberg/pull/17412
--
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