jayzhan211 commented on PR #25013:
URL: https://github.com/apache/datafusion/pull/25013#issuecomment-5636482935
@peterxcli , here is a suggestion:
**requirement paths ending at a `Struct` bypass the List/Map pushdown
policy**
`projection_read_plan.rs:440-446` short-circuits on `Struct` before looking
at what the struct
contains, so the policy guard doesn't hold for anything nested below a
selected sub-struct:
```rust
resolve_struct_field_type(&data_type, path).is_some_and(|leaf| {
matches!(leaf, DataType::Struct(_)) // <-- accepts the whole subtree
unseen
|| !leaf.is_nested()
|| self.is_nested_type_supported(leaf)
})
```
Repro (dropped into `projection_read_plan::test`, UDF requires
`["selected"]` on
`s: Struct<selected: Struct<arr: List<Int32>>>`, `allow_list_columns =
false`):
```
PROBE prevents_pushdown (list nested in selected struct, allow_lists=false)
= false
PROBE baseline whole-struct prevents_pushdown = true
```
The same list data reached via `get_field(s,'selected')` or a whole-column
reference to `s` is
blocked. `allow_list_columns` comes from `supports_list_predicates`, i.e.
the allowlist of
verified predicates (`array_has`/`array_has_all`/`array_has_any`, `IS [NOT]
NULL`) — a UDF
requirement isn't on it, so declaring `required_input_fields` becomes a
general escape hatch for
getting list/map columns into the row filter. That's precisely what the
comment two lines above
says must not happen:
```rust
// Declaring dependencies cannot bypass the List/Map pushdown policy.
```
Map is affected identically, since the subtree is never inspected.
Suggested fix — check every leaf of the selected subtree instead of
accepting `Struct` wholesale:
```diff
+ /// Whether every leaf below `data_type` is acceptable under the current
+ /// nested-pushdown policy. A selected sub-struct must not smuggle a
List
+ /// or Map past the policy that a direct reference to it would hit.
+ fn subtree_is_pushable(&self, data_type: &DataType) -> bool {
+ match data_type {
+ DataType::Struct(fields) => fields
+ .iter()
+ .all(|field| self.subtree_is_pushable(field.data_type())),
+ other => !other.is_nested() ||
self.is_nested_type_supported(other),
+ }
+ }
```
```diff
data_type.is_some_and(|data_type| {
requirement.field_paths.iter().all(|path| {
- resolve_struct_field_type(&data_type,
path).is_some_and(|leaf| {
- matches!(leaf, DataType::Struct(_))
- || !leaf.is_nested()
- || self.is_nested_type_supported(leaf)
- })
+ resolve_struct_field_type(&data_type, path)
+ .is_some_and(|leaf|
self.subtree_is_pushable(leaf))
})
})
```
Existing `udf_input_requirements_respect_nested_pushdown_policy` cases still
pass under this
(`Struct<value: Int32>` is all-primitive). Worth extending that test's type
matrix with a
`Struct` wrapping each of the List/Map variants so the nested shape is
pinned too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
