Repository navigation
fix(spec): check optional ancestors when binding IS NULL on a nested required field - #3306
Open
LuciferYang wants to merge 1 commit into
Open
LuciferYang wants to merge 1 commit into
LuciferYang wants to merge 1 commit into
Conversation
…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.
JeonDaehong
approved these changes
Oct 15, 2026
JeonDaehong
left a comment
Contributor
There was a problem hiding this comment.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Predicate::bindrewritesIS NULLon a required field toAlwaysFalse(andIS NOT NULLtoAlwaysTrue) from the leaf field's ownrequiredflag, 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
personoptional andperson.agerequired,person.age IS NULLbound toAlwaysFalse, so a scan dropped the rows whosepersonis null instead of keeping them.IS NOT NULLhad the mirror bug: it bound toAlwaysTrueand 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 existingindex_parents, now computed once inbuild()and stored onSchema;validate_identifier_idstakes that shared map instead of recomputing its own. The twoUnaryarms inPredicate::binduse this check in place of the leaf-onlyrequiredflag.Tests
test_bind_is_null_required_leaf_under_optional_parent_is_keptand itsis_not_nullsibling: 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 toFalse/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.