Skip to content

API: Detect whether required fields nested within optionals can produce nulls - #14270

Merged
huaxingao merged 2 commits into
apache:mainfrom
nastra:nested-fields-with-nulls
Oct 10, 2025
Merged

API: Detect whether required fields nested within optionals can produce nulls#14270
huaxingao merged 2 commits into
apache:mainfrom
nastra:nested-fields-with-nulls

Conversation

@nastra

@nastra nastra commented Oct 7, 2025

Copy link
Copy Markdown
Contributor

This partially reverts some changes around the Accessor API that were introduced by #13804 and uses a Schema visitor to detect whether any of the parent fields of a nested required field are optional. This info is then used when IS_NULL / NOT_NULL is evaluated

@github-actions github-actions Bot added the API label Oct 7, 2025
@nastra
nastra force-pushed the nested-fields-with-nulls branch from cb947c6 to df5466b Compare October 7, 2025 09:50
@nastra

nastra commented Oct 7, 2025

Copy link
Copy Markdown
Contributor Author

@pvary @huaxingao @stevenzwu could you review this one please since you reviewed #13804 already?

@nastra
API: Detect whether required fields nested within optionals can produ…
ccb4cbd 
…ce nulls

This partially reverts some changes around the `Accessor` API that were introduced by apache#13804 and uses a Schema visitor
to detect whether any of the parent fields of a nested required field are optional.
This info is then used when IS_NULL / NOT_NULL is evaluated
@nastra
nastra force-pushed the nested-fields-with-nulls branch from df5466b to ccb4cbd Compare October 7, 2025 10:56
nastra
nastra commented Oct 7, 2025
View reviewed changes
Comment thread api/src/test/java/org/apache/iceberg/expressions/TestExpressionBinding.java
assertThat(pred.term().type()).isEqualTo(Types.fromPrimitiveString(typeName));
}

private static Stream<Arguments> nullCasesWithNestedStructs() {

@nastra nastra Oct 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code has been moved from TestBoundReference and slightly adjusted for better readability

nastra
nastra commented Oct 7, 2025
View reviewed changes
Comment thread api/src/test/java/org/apache/iceberg/expressions/TestInclusiveMetricsEvaluator.java
}

@Test
public void testIsNullInNestedStruct() {

@nastra nastra Oct 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added some additional evaluator tests that reproduce the original problem described in #13804

@nastra
nastra requested a review from rdblue October 7, 2025 10:59
pvary
pvary reviewed Oct 8, 2025
View reviewed changes
Comment thread api/src/main/java/org/apache/iceberg/expressions/UnboundPredicate.java Outdated
private Expression bindUnaryOperation(BoundTerm<T> boundTerm) {
private Expression bindUnaryOperation(StructType struct, BoundTerm<T> boundTerm) {
boolean allFieldsAreRequired =
TypeUtil.findParents(struct.asSchema(), boundTerm.ref().fieldId()).stream()

@pvary pvary Oct 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How often do we evaluate these expressions? For every manifest entry?
For a wide schema, what is the additional cost of calculating the parents every time?

@nastra nastra Oct 9, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we would evaluate these whenever we have an unbound predicate that we want to bind. I've updated this so that it's only evaluated when having a IS_NULL or NOT_NULL and when !boundTerm.producesNull() evaluates to true

@nastra
nastra requested review from huaxingao and stevenzwu October 8, 2025 13:39
@nastra
nastra force-pushed the nested-fields-with-nulls branch from 783db2f to fdba428 Compare October 8, 2025 13:42
huaxingao
huaxingao reviewed Oct 8, 2025
View reviewed changes
Comment thread api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java Outdated
huaxingao
huaxingao approved these changes Oct 8, 2025
View reviewed changes

@huaxingao huaxingao left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nice work!

stevenzwu
stevenzwu approved these changes Oct 9, 2025
View reviewed changes

@stevenzwu stevenzwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome work, @nastra. thanks for improving this before our next release.

Comment thread api/src/main/java/org/apache/iceberg/expressions/UnboundPredicate.java Outdated
Comment thread api/src/main/java/org/apache/iceberg/types/TypeUtil.java Outdated
@nastra
nastra force-pushed the nested-fields-with-nulls branch from fdba428 to ea2ce47 Compare October 9, 2025 06:34
@nastra
nastra force-pushed the nested-fields-with-nulls branch from ea2ce47 to c588bb1 Compare October 9, 2025 06:39
@huaxingao
huaxingao merged commit a473b1c into apache:main Oct 10, 2025
43 checks passed
@huaxingao

huaxingao commented Oct 10, 2025

Copy link
Copy Markdown
Contributor

Thanks @nastra for the PR! Thanks @stevenzwu @pvary for the review!

@huaxingao huaxingao added this to the Iceberg 1.10.1 milestone Oct 10, 2025
@nastra
nastra deleted the nested-fields-with-nulls branch October 15, 2025 15:46
huaxingao pushed a commit to huaxingao/iceberg that referenced this pull request Nov 6, 2025
@nastra @huaxingao
API: Detect whether required fields nested within optionals can produ…
f059c1c 
…ce nulls (apache#14270)

* API: Detect whether required fields nested within optionals can produce nulls

This partially reverts some changes around the `Accessor` API that were introduced by apache#13804 and uses a Schema visitor
to detect whether any of the parent fields of a nested required field are optional.
This info is then used when IS_NULL / NOT_NULL is evaluated

* only check parent fields on IS_NULL/NOT_NULL

(cherry picked from commit a473b1c)
huaxingao added a commit that referenced this pull request Nov 6, 2025
@huaxingao @nastra
API: Detect whether required fields nested within optionals can produ…
b5e921a 
…ce nulls (#14270) (#14512)

* API: Detect whether required fields nested within optionals can produce nulls

This partially reverts some changes around the `Accessor` API that were introduced by #13804 and uses a Schema visitor
to detect whether any of the parent fields of a nested required field are optional.
This info is then used when IS_NULL / NOT_NULL is evaluated

* only check parent fields on IS_NULL/NOT_NULL

(cherry picked from commit a473b1c)

Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>
thomaschow pushed a commit to thomaschow/iceberg that referenced this pull request Jan 19, 2026
@nastra @thomaschow
API: Detect whether required fields nested within optionals can produ…
dff6c5b 
…ce nulls (apache#14270)

* API: Detect whether required fields nested within optionals can produce nulls

This partially reverts some changes around the `Accessor` API that were introduced by apache#13804 and uses a Schema visitor
to detect whether any of the parent fields of a nested required field are optional.
This info is then used when IS_NULL / NOT_NULL is evaluated

* only check parent fields on IS_NULL/NOT_NULL
talatuyarer pushed a commit to talatuyarer/iceberg that referenced this pull request Apr 1, 2026
@nastra @talatuyarer
API: Detect whether required fields nested within optionals can produ…
d1b0d5d 
…ce nulls (apache#14270)

* API: Detect whether required fields nested within optionals can produce nulls

This partially reverts some changes around the `Accessor` API that were introduced by apache#13804 and uses a Schema visitor
to detect whether any of the parent fields of a nested required field are optional.
This info is then used when IS_NULL / NOT_NULL is evaluated

* only check parent fields on IS_NULL/NOT_NULL
@hsiang-c hsiang-c mentioned this pull request Aug 31, 2026
Open
@dominikandreasseitz dominikandreasseitz mentioned this pull request Oct 3, 2026
Closed
dominikandreasseitz added a commit to dominikandreasseitz/iceberg-python that referenced this pull request Oct 3, 2026
@dominikandreasseitz
Fix IsNull/NotNull binding ignoring ancestor nullability for required…
e77e3b9 
… nested fields

BoundIsNull/BoundNotNull folded to AlwaysFalse()/AlwaysTrue() at bind time
using only a field's own required flag. A required leaf field can still be
absent from a row if an optional ancestor struct containing it is null, so
plan_files() was silently dropping matching files once IsNull/NotNull hit a
required field under an optional parent.

Add Schema.is_field_required_in_path to walk the existing ancestor index and
check the full path, and move the fold from BoundIsNull/BoundNotNull.__new__
into IsNull.bind()/NotNull.bind(), where the schema is already in scope.
BoundReference and all other public signatures are unchanged.

This mirrors Java's fix for the same bug (apache/iceberg#14270), which
similarly reverted an earlier attempt to carry ancestor-nullability on the
Accessor/BoundTerm itself (apache/iceberg#13804) in favor of a schema-level
ancestor walk done only at bind time.
dominikandreasseitz added a commit to dominikandreasseitz/iceberg-python that referenced this pull request Oct 3, 2026
@dominikandreasseitz
Fix IsNull/NotNull binding ignoring ancestor nullability for required…
83bacb7 
… nested fields

BoundIsNull/BoundNotNull folded to AlwaysFalse()/AlwaysTrue() at bind time
using only a field's own required flag. A required leaf field can still be
absent from a row if an optional ancestor struct containing it is null, so
plan_files() was silently dropping matching files once IsNull/NotNull hit a
required field under an optional parent.

Add Schema.is_field_required_in_path to walk the existing ancestor index and
check the full path, and move the fold from BoundIsNull/BoundNotNull.__new__
into IsNull.bind()/NotNull.bind(), where the schema is already in scope.
BoundReference and all other public signatures are unchanged.

This mirrors Java's fix for the same bug (apache/iceberg#14270), which
similarly reverted an earlier attempt to carry ancestor-nullability on the
Accessor/BoundTerm itself (apache/iceberg#13804) in favor of a schema-level
ancestor walk done only at bind time.
This was referenced Oct 3, 2026
Open
Open
Open
@advancedxy advancedxy mentioned this pull request Oct 10, 2026
Open
dominikandreasseitz added a commit to dominikandreasseitz/iceberg-python that referenced this pull request Oct 13, 2026
@dominikandreasseitz
Fix IsNull/NotNull binding ignoring ancestor nullability for required…
3f26562 
… nested fields

BoundIsNull/BoundNotNull folded to AlwaysFalse()/AlwaysTrue() at bind time
using only a field's own required flag. A required leaf field can still be
absent from a row if an optional ancestor struct containing it is null, so
plan_files() was silently dropping matching files once IsNull/NotNull hit a
required field under an optional parent.

Add Schema.is_field_required_in_path to walk the existing ancestor index and
check the full path, and move the fold from BoundIsNull/BoundNotNull.__new__
into IsNull.bind()/NotNull.bind(), where the schema is already in scope.
BoundReference and all other public signatures are unchanged.

This mirrors Java's fix for the same bug (apache/iceberg#14270), which
similarly reverted an earlier attempt to carry ancestor-nullability on the
Accessor/BoundTerm itself (apache/iceberg#13804) in favor of a schema-level
ancestor walk done only at bind time.
dominikandreasseitz added a commit to dominikandreasseitz/iceberg-python that referenced this pull request Oct 16, 2026
@dominikandreasseitz
Fix IsNull/NotNull binding ignoring ancestor nullability for required…
b76908b 
… nested fields

BoundIsNull/BoundNotNull folded to AlwaysFalse()/AlwaysTrue() at bind time
using only a field's own required flag. A required leaf field can still be
absent from a row if an optional ancestor struct containing it is null, so
plan_files() was silently dropping matching files once IsNull/NotNull hit a
required field under an optional parent.

Add Schema.is_field_required_in_path to walk the existing ancestor index and
check the full path, and move the fold from BoundIsNull/BoundNotNull.__new__
into IsNull.bind()/NotNull.bind(), where the schema is already in scope.
BoundReference and all other public signatures are unchanged.

This mirrors Java's fix for the same bug (apache/iceberg#14270), which
similarly reverted an earlier attempt to carry ancestor-nullability on the
Accessor/BoundTerm itself (apache/iceberg#13804) in favor of a schema-level
ancestor walk done only at bind time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pvary pvary pvary left review comments
@stevenzwu stevenzwu stevenzwu approved these changes
@huaxingao huaxingao huaxingao approved these changes
@rdblue rdblue Awaiting requested review from rdblue

Assignees

No one assigned

Labels

API

Projects

None yet

Milestone

Iceberg 1.10.1

Development

Successfully merging this pull request may close these issues.

4 participants

@nastra @huaxingao @stevenzwu @pvary