Skip to content

fix(spec): check optional ancestors when binding IS NULL on a nested required field - #3306

Open
LuciferYang wants to merge 1 commit into
apache:mainfrom
LuciferYang:fix/3292-isnull-nested-ancestor-required
Open

LuciferYang wants to merge 1 commit into
apache:mainfrom
LuciferYang:fix/3292-isnull-nested-ancestor-required

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

What

Predicate::bind rewrites IS NULL on a required field to AlwaysFalse (and IS NOT NULL to AlwaysTrue) from the leaf field's own required flag, without checking whether an ancestor struct is optional. The spec says a null parent struct implies its leaves are null, so the shortcut is only valid when the field and every ancestor are required.

With person optional and person.age required, person.age IS NULL bound to AlwaysFalse, so a scan dropped the rows whose person is null instead of keeping them. IS NOT NULL had the mirror bug: it bound to AlwaysTrue and kept those rows.

Closes #3292.

How

Added Schema::is_field_and_ancestors_required(field_id), which walks the parent chain and returns true only when the leaf and every ancestor struct is required. The parent chain comes from the existing index_parents, now computed once in build() and stored on Schema; validate_identifier_ids takes that shared map instead of recomputing its own. The two Unary arms in Predicate::bind use this check in place of the leaf-only required flag.

Tests

  • test_bind_is_null_required_leaf_under_optional_parent_is_kept and its is_not_null sibling: a required leaf under an optional parent keeps the predicate instead of folding to a constant.
  • test_bind_null_predicates_on_leaf_under_required_parent_short_circuit: a fully required path still folds to False / True.
  • test_bind_is_null_required_leaf_under_optional_grandparent_is_kept: an optional struct two levels up is caught by the ancestor walk.
  • test_is_field_and_ancestors_required: helper coverage for top-level fields and an unknown id.

End-to-end reads of a null parent through the Arrow row filter depend on #2906 (nested-leaf predicate pushdown); this PR fixes the binding layer, which is reachable and testable on its own.

…required field

Predicate::bind short-circuited IS NULL to AlwaysFalse (and IS NOT NULL to
AlwaysTrue) from the leaf's own `required` flag alone. When an ancestor struct is
optional a null parent makes the leaf null, so the shortcut returned wrong rows.
Only short-circuit when the field and all ancestors are required.
Copilot AI balanced review requested due to automatic review settings September 30, 2026 11:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@JeonDaehong JeonDaehong 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.

Checked this end to end with #2906 and #3453: an equality delete s.f = 20 on a required f under an optional s also removed the row whose s is null, because the delete's keep predicate s.f IS NULL OR s.f != 20 lost its IS NULL at bind time. With this PR that row is kept, as the spec says. cargo test -p iceberg --lib passes on current main with this merged. LGTM.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IS NULL / IS NOT NULL on a required nested field is rewritten without checking optional ancestors

3 participants