fix(arrow): allow predicates on struct-nested leaf columns

[LuciferYang]

Which issue does this PR close?

• Closes Predicates on nested-leaf columns fail #2432.

What changes are included in this PR?

A predicate on a primitive leaf inside a struct (e.g. person.age > 25) fails while building the Arrow row filter with Leaf column ... isn't a root column in Parquet schema. PredicateConverter::bound_reference resolved the field to its Parquet leaf index and rejected it when that leaf's column root was a group, and project_column only handled top-level columns.

This resolves the referenced field to its Parquet column path (root to leaf) instead of a single leaf index, and has project_column walk that path through the projected RecordBatch, descending StructArray children by name to reach the primitive leaf. ProjectionMask::leaves preserves the Parquet nesting, so a predicate on nested.value sees nested as a StructArray holding the leaf.

While descending, project_column merges each struct's validity into its children with StructArray::flatten, so a leaf under a null parent struct reads as null. This follows the spec rule that a null parent struct implies its leaves are null, and it applies at every level, so a null outer struct in a doubly nested path is handled too.

Scope

Only top-level and struct-nested primitive leaves can carry a predicate: Schema::build_accessors builds no accessor for a list or map, nor for anything inside one, so Reference::bind rejects those predicates before the row filter is ever built. Every path segment before the leaf is therefore a struct. This matches the Java reader, which reads a struct-nested field normally.

Row-group and page-index pruning (RowGroupMetricsEvaluator / PageIndexEvaluator) resolve nested leaves by field id and never errored on this path; they are out of scope here and can be looked at separately.

Binding of IS NULL / IS NOT NULL on a required field does not yet check whether its ancestors are optional, so person.age IS NULL with person optional binds to AlwaysFalse. That is a separate, pre-existing gap, tracked in #3292.

Are these changes tested?

• predicate_visitor: unit coverage for the nested-leaf conversion (a group person { int32 age } Parquet schema, the shape that used to be rejected).
• row_filter: end-to-end reads of real Parquet files for a single-nested (person.age) and a double-nested (person.address.zip) struct leaf, so the projected RecordBatch shape comes from arrow-rs rather than a hand-built batch; a null parent struct over a required leaf (person.age) and an optional leaf (person.score), and a null outer struct in the doubly nested case, which fail on the pre-flatten code and pass after; a predicate over two leaves of the same struct (person.age > 25 AND person.score < 250), where the projected struct has more than one child so project_column must match children by name rather than by position; and a bind-time check that predicates on list-element and map-value fields are rejected before reaching the row filter. The read tests share an ids_kept_by_predicate helper that writes one batch and returns the ids a predicate keeps.

No public API signature changes.

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

[LuciferYang]

friendly ping @mbutrovich

[Copilot]

Pull request overview

This PR fixes Arrow row-filter predicate conversion so predicates can target primitive leaf fields nested inside structs (e.g. person.age > 25) when reading Parquet, addressing issue #2432.

Changes:

• Resolve bound references to a Parquet column path (root → leaf) instead of a single leaf index, allowing struct-nested leaves to be referenced safely.
• Update project_column to walk that path through the projected RecordBatch, descending StructArray children by field name until the primitive leaf is reached.
• Add regression coverage: unit test for nested-leaf conversion and end-to-end Parquet read tests for single- and double-nested struct leaf predicates, plus bind-time rejection checks for list/map interior references.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
crates/iceberg/src/arrow/reader/predicate_visitor.rs	Switch predicate reference resolution to Parquet column paths and traverse nested StructArray to reach primitive leaves; add a unit regression test.
crates/iceberg/src/arrow/reader/row_filter.rs	Add end-to-end Parquet regression tests for struct-nested leaf predicates and bind-time rejection tests for list/map interior fields.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[LuciferYang]

Fixed in bfb3a2e. The message now names the segment whose array was not a struct, and the nested-field lookup failure names the enclosing struct as well.

[LuciferYang]

Reworded in bfb3a2e. The comment now states that Schema::build_accessors builds accessors only for primitives and primitives nested in structs, and none for a list or map or anything inside one.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

[mbutrovich]

Thanks @LuciferYang for picking this up, and for the end-to-end tests that read real Parquet files instead of hand-built batches.

Binding has a related gap outside this diff. Predicate::bind rewrites IS NULL on a required field to AlwaysFalse and IS NOT NULL to AlwaysTrue without looking at the field's ancestors (predicate.rs). The spec says that "if a parent struct column is null it implies the leaf column is null" (spec), and Java only applies the rewrite when every ancestor is also required (UnboundPredicate.java). With person optional and person.age required, person.age IS NULL binds to False, so a scan over a file where one row has a null person returns no rows. This predates the PR, but it's the next wrong result a user hits once nested predicates work. Could you open an issue for it and link it here?

[mbutrovich]

Now that the index isn't used, what do you think about checking membership with contains? A discarded position reads as if the index matters, and ok_or builds the formatted error even when the column is found.

Suggested change

	// Confirm the leaf is among the projected columns.
	if !self.column_indices.contains(column_idx) {
	return Err(Error::new(
	ErrorKind::DataInvalid,
	format!(
	"Leaf column `{}` in predicates cannot be found in the required column indices.",
	reference.field().name
	),
	));
	}

[LuciferYang]

Done in d61f4fb. Switched to if !self.column_indices.contains(column_idx), so the discarded position is gone and the error is only built when the leaf is actually missing.

[mbutrovich]

What happens when the parent struct is null? column_by_name returns the child array as stored, without the struct's validity. I wrote a file with id and person: optional struct<age: required int, score: optional int>, three rows with ids 1, 2, 3, where row 2 has a null person. At the head commit, person.age < 1000 keeps ids [1, 2, 3] and person.age != 30 keeps [2, 3]. The spec says that "if a parent struct column is null it implies the leaf column is null" (spec), so I'd expect [1, 3] and [3]. The optional person.score gives the right answer for the same predicates. If I'm reading the results right, arrow-rs carries the parent's nulls into an optional child but leaves a required child non-null under a null parent.

StructArray::flatten in arrow-array 58.4 ANDs the struct's validity into each child. With the change below, both predicates keep the expected ids and the tests in this PR still pass. Each level merges its own nulls into its children, so it also covers a null person over address in the doubly nested case.

Suggested change

	current = struct_array
	.column_by_name(part)
	.ok_or_else(|| {
	ArrowError::SchemaError(format!(
	"Predicate column nested field `{part}` not found in struct `{current_name}`."
	))
	})?
	.clone();
	// `flatten` ANDs the struct's validity into each child, so a leaf under a null
	// struct reads as null.
	let (fields, mut columns) = struct_array.flatten();
	let (idx, _) = fields.find(part).ok_or_else(|| {
	ArrowError::SchemaError(format!(
	"Predicate column nested field `{part}` not found in struct `{current_name}`."
	))
	})?;
	current = columns.swap_remove(idx);

[LuciferYang]

Done in d61f4fb. project_column now descends with StructArray::flatten, which ANDs the struct's validity into each child, so a leaf under a null parent struct reads as null. Your two cases come out as expected now: person.age < 1000 keeps [1, 3] and person.age != 30 keeps [3]. Because each level merges its own nulls, the null outer person over address in the doubly nested case is covered too, and there are tests for both.

[mbutrovich]

Could we add a test with an optional parent struct that has null rows? It would cover a required leaf and an optional leaf under the null struct, with predicates that the stored child value satisfies (< and !=), and a null outer struct in the doubly nested case. Both tests here use required structs, so they don't reach the null-parent path. They also repeat the same write-then-read scaffolding, so a helper that writes one batch and returns the ids a predicate keeps would keep the new cases short.

[LuciferYang]

Added. test_predicate_on_leaf_under_null_parent_struct writes an optional parent struct with a null row and asserts on a required leaf (person.age, with < and !=) and an optional leaf (person.score, with <); test_predicate_on_leaf_under_null_outer_struct_doubly_nested covers the null outer struct in the doubly nested case. Both go through a new ids_kept_by_predicate helper that writes one batch and returns the ids a predicate keeps, and the two existing nested-leaf read tests use it now too.

[LuciferYang]

Thanks for the review, @mbutrovich. I opened #3292 for the IS NULL / IS NOT NULL binding gap on required nested fields under optional ancestors, and linked this PR from it.

The three inline points are addressed in d61f4fb, with a follow-up test in c8f1a81: parent-struct nulls now propagate to nested leaves via StructArray::flatten, the projection membership check uses contains, and there are null-parent tests for a required and an optional leaf plus the doubly nested null outer struct, behind a shared write-then-read helper. I also merged the latest main into the branch.

[mbutrovich]

Thanks @LuciferYang for the updates and for opening #3292.

[mbutrovich]

Could we add a read test where one predicate references two leaves of the same struct, such as person.age > 25 AND person.score < 250? Each test in this PR references one leaf per struct, so ProjectionMask::leaves projects a struct with a single child, and this lookup always lands on index 0. I changed columns.swap_remove(idx) to columns.swap_remove(0) and every test in the PR still passes. With that change, a six-row file with person: optional struct<age: required int, score: optional int> returns ids [1, 3, 4, 6] for the predicate above, where the head commit returns the correct [1, 6]. A test like this would check that the child is found by name when the projected struct has more than one child.

[LuciferYang]

Added in 2439ce2: test_predicate_on_two_leaves_of_same_struct reads a file with person: optional struct<age: required int, score: optional int> and the predicate person.age > 25 AND person.score < 250, so the struct is projected with both children. It keeps [1, 4, 6]. I confirmed it fails the way you described: swapping swap_remove(idx) for swap_remove(0) makes the score comparison read age, the test returns [1, 3, 4, 6], and the single-leaf tests still pass. Good catch.

[JeonDaehong]

Tried this together with nested equality deletes (#3453): the delete now applies end to end, including rows whose parent struct is null. LGTM after a rebase. Two notes:

• On current main, test_read_parquet_with_name_mapping_maps_nested_fields (added in #3384) fails with this PR: it expects s.a > 15 on a name-mapped file to be rejected. With this PR the read succeeds and returns ids 2 and 3 (s.a = 20, 30), which is correct, so that assertion just needs to change after rebasing.
• An equality delete on a required leaf under an optional struct still removes the rows where the struct is null, because s.f IS NULL binds to false (#3292). With #3306 merged as well, those rows are kept.