adriangb commented on PR #24090:
URL: https://github.com/apache/datafusion/pull/24090#issuecomment-5194655874
I spent some time stress-testing this (mutation testing, randomised
differential testing against the arrow-rs reader, and benchmarks on wide
schemas) and found one correctness bug plus a few smaller things. I've opened
mbutrovich/datafusion#1 against your branch with fixes and tests — fold it in
however you like.
### Two cast targets on one column read too few leaves
`build_read_plan_with_cast_clipping` skips every cast access on a root it
has already clipped:
```rust
if whole_roots.contains(&root)
|| fallback_roots.contains(&root)
|| clipped_by_root.contains_key(&root) // <-- second cast on this
root: dropped
{ continue; }
```
If a projection consumes one column through two *different* narrowing casts,
only the first target's leaves reach the mask, and the second cast then
evaluates against a struct missing the children it names:
* overlapping targets → `cast_column` null-fills them → **silently wrong
results**
* disjoint targets → `validate_struct_compatibility` rejects it → the query
**fails** where it succeeds without pruning
I originally assumed this needed a custom adapter, but it's reachable from
plain SQL: `ProjectionExec` is merged into the scan via
`ParquetSource::try_pushdown_projection`, so a query-level `CAST(col AS
STRUCT<...>)` reaches the same analysis as an adapter-inserted one. Against a
table whose schema is inferred from the file (so no adapter cast is interposed):
```sql
-- file: s Struct<x Int64, y Utf8, pad Utf8>
SELECT CAST(s AS STRUCT<x BIGINT>), CAST(s AS STRUCT<x BIGINT, y VARCHAR>)
FROM t;
-- {x: 100} {x: 100, y: NULL} <- y should be 's1'
SELECT CAST(s AS STRUCT<x BIGINT>), CAST(s AS STRUCT<pad VARCHAR>) FROM t;
-- Error: Cannot cast struct with 1 fields to 1 fields because there is no
field name overlap
```
Both return correct results with `with_cast_collection` disabled, so they're
regressions rather than pre-existing behaviour. The fix in the linked PR
demotes the root to a full read when a second cast has a different target;
identical repeated targets (what the expression adapter actually produces)
still clip.
### Smaller things
* **A zero-overlap nested struct level predicts a type the reader never
emits.** `clip_type` can return `Struct([])` for a name-matched child whose own
children share no name with their target, while keeping that child in the
emitted type — but arrow-rs drops a struct child whose leaves are all masked
out (`complex.rs`, `if children.is_empty() { return Ok(None) }`). I couldn't
reach it through the default stack (`validate_struct_compatibility` and the
logical planner both reject the cast first), so it isn't live today. But the
module doc's safety argument rests on a *caller* invariant and the motivating
use case is a custom `PhysicalExprAdapter`, so I'd rather `clip_for_cast`
enforce it itself. Test added pinning the arrow-rs behaviour the argument
depends on.
* **`leaves_by_root[root]` is a panicking `BTreeMap` index**, on a path
where the same function already guards the "root with no parquet leaves" case
twenty lines earlier. Latent — I couldn't build a file that reaches it — but a
one-line fix.
### Performance
Three things weren't `O(projected_columns)`:
* `clip_type` matches struct fields with a linear `find` per physical child
— Θ(N·M) string comparisons per struct level, ~375k for a 1000→500 subfield
struct. Spark's `ParquetReadSupport.clipParquetGroupFields` uses a name→field
map for exactly this; the PR does the same above a small width.
* Widening the fast-path gate to `contains_struct` correctly fixes the
`List<Struct>` hole, but it also routes `Map`, `Dictionary<_, Struct>` and
every array-of-records column through `PushdownChecker`, whose
`Schema::index_of` is a linear name scan per column node — O(|exprs| ·
|schema|), paid once per file opened. Gating on the *projected* roots instead
keeps the `List<Struct>` fix and is O(projected).
* An O(G²) `field_with_name` re-lookup for `get_field` roots (also picks the
wrong field under duplicate root names).
New benchmark `parquet_wide_schema_read_plan` — 1000-column schemas and
1000-subfield structs over 32 small files, so per-file planning dominates.
Before → after, back to back, with two untouched-path controls so the numbers
mean something:
| case | change |
| --- | --- |
| `flat_wide/column_projection` (control) | −0.1% (p = 0.93) |
| `wide_struct/full_schema` (control) | +1.1% (p = 0.63) |
| `wide_struct/unrelated_column` — wide struct present but not projected |
**−13.6%** (p = 0.01) |
| `wide_struct/narrowed_schema` — 1000 → 500 subfield clip | **−7.2%** (p =
0.01) |
The feature holds up well at that width: 90.8 ms narrowed vs 166.2 ms full,
~45% less wall time.
### Coverage
`cargo-mutants` over the two files: 121 mutants, 82 viable, 16 survivors, of
which 8 were real gaps — including that `start + o` (rebasing clip offsets onto
absolute leaf indices) survives `start - o`, because every clip test casts to
the struct's *first* field where the two are identical; and that the whole
`get_field_accesses` branch of `build_read_plan_with_cast_clipping` is never
executed by any test. All eight now have tests, each verified by re-applying
the mutation.
I also added two seeded randomised differential harnesses — one checking
`clip_for_cast`'s predicted type and kept leaves against the real reader, one
comparing a clipped scan against `cast_column` over an unclipped one end to
end. ~63k and ~25k cases respectively, no further discrepancies. They're cheap
by default (~3.5 s / ~1.2 s) and widen by env var.
SLT coverage went from 110 to ~380 lines, moving most of the Rust-only
assertions into SQL: `SELECT *`, mixed whole-column and field access,
aggregation and filtering over a clipped column, a declared field order
differing from the file's, struct-in-struct, the two-level `ARRAY<STRUCT<...,
ARRAY<STRUCT<..>>>>` shape from the Comet issue, a `MAP` sibling, a scan mixing
narrow and wide files, and the zero-overlap rejection.
### One thing that isn't yours
While writing the SLT tests I hit a pre-existing soundness bug: with
`pushdown_filters = true`, a `get_field` predicate is silently dropped whenever
the file needs schema adaptation, so the query returns every row. It's not
caused by this PR (`row_filter.rs` is untouched), but every table this feature
targets carries exactly the cast that triggers it, so it's worth knowing about
before this lands. Filed separately as #24109.
---
_Generated by [Claude Code](https://claude.ai/code)_
--
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]
