Skip to content

fix(expressions): bound ancestor nullability in IsNull/NotNull binding - #4068

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

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

Conversation

@dominikandreasseitz

Copy link
Copy Markdown

Closes #4067

Description

Fixes IsNull/NotNull binding silently dropping rows/files for a required field nested under an optional ancestor struct.
Related iceberg java/rust PRs:

Bug:

  • BoundIsNull/BoundNotNull.__new__ folded to AlwaysFalse()/AlwaysTrue() using only a field's own required flag.
  • A required leaf can still be absent if an optional ancestor struct containing it is null
  • plan_files() silently dropped matching files once IsNull/NotNull hit a required field under an optional parent.

Example:

# schema: s is optional, its only child x is required
# s = None for the one row -> s.x is absent too

files = table.scan(row_filter=IsNull("s.x")).plan_files()

len(files)  # 0  <-- empty, should contain the file with the s = None row

New behavior:

  • IsNull/NotNull on a required field nested under an optional ancestor now correctly matches rows/files instead of silently pruning them.
  • No public API changes.

Change

  • pyiceberg/schema.py — add Schema.is_field_required_in_path(field_id), which walks the existing ancestor index to check the field and every ancestor is required. Mirrors Java's TypeUtil.ancestorFields/allAncestorFieldsAreRequired.
  • pyiceberg/expressions/__init__.py — remove the leaf-only fold from BoundIsNull/BoundNotNull.__new__; do the fold in IsNull.bind()/NotNull.bind() instead, matching where Java does it (UnboundPredicate.bindUnaryOperation) — no change to BoundReference.
  • tests/test_schema.py, tests/expressions/test_expressions.py — coverage for the fix.

… 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 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.

IsNull/NotNull binding ignores ancestor nullability for required nested fields

1 participant