comphead commented on PR #5786:
URL:
https://github.com/apache/datafusion-comet/pull/5786#issuecomment-5721458036
## Review: reject duplicate Parquet field names before decoding
Reviewed the full diff plus `schema_adapter.rs`, `parquet_support.rs`,
`name_fold.rs`, parquet-rs 59.2 and datafusion-datasource-parquet 55.0/55.1.
Grouped by severity, then simplification and test dedup.
### Blockers
**1. Does not compile against current `main`: `fold_name` and
`is_pure_structural_narrowing` are now fallible.**
The head here is 60 commits behind. On `main`, `name_fold.rs:144` is
`fold_name(..) -> DataFusionResult<String>` and `schema_adapter.rs:110` is
`is_pure_structural_narrowing(..) -> DataFusionResult<bool>` (#5845, merged).
Both are called infallibly in `validate_field_names`, `validate_field_type`,
and the `selected` builder.
The `?` is not the hard part. `find(|candidate| fold_name(a, cs) ==
fold_name(b, cs))` and `map_or_else(|| .., |source| if
is_pure_structural_narrowing(..) { .. })` are infallible-closure positions, so
this needs restructuring — which is the same restructuring finding 2 asks for.
**2. Name matching is O(physical x projected) with two `String` allocations
per comparison, against this module's explicit bulk-fold contract.**
In `validate_field_names`, `fold_name(field.name(), case_sensitive)` is
recomputed inside the inner `find` for every candidate, though it is
loop-invariant. `fold_name` allocates even in case-sensitive mode
(`name.to_string()`), and in case-insensitive mode with non-ASCII names it
takes an `RwLock` read and can cross into the JVM per call. The same nested
`find` repeats in the top-level `selected` builder. This runs per file open, at
every nesting level.
The neighbouring code deliberately does the opposite:
- `schema_adapter.rs:125` — *"Fold the source field names once (O(sources),
not O(targets x sources)), matching this file's bulk-fold convention."*
- `parquet_support.rs::match_struct_fields` — folds `from` + `to` in one
`fold_names` call, `split_at`s, then builds `folded_to_indices: HashMap<&str,
Vec<usize>>` and uses `indices.len() > 1` as the ambiguity check.
- `name_fold.rs` module docs — keeping the policy in one module *"is what
stops those copies from drifting apart, which is the drift that produced #5495
in the first place."*
This adds a fourth copy of the resolution policy, with a raw-name `HashSet`
for dedup instead.
Direction: reuse `match_struct_fields`, or at minimum fold each group's
names once with `fold_names` and build the `folded -> Vec<index>` map.
`indices.len() > 1` becomes the duplicate check, the `HashSet` goes away, and
the `?` from finding 1 lands naturally.
### Major
**3. Field-ID reads validate the entire file, turning working queries into
errors.**
`with_required_schema` drops the projection whenever `options.use_field_id`
is set — not only when the requested schema actually carries IDs. Reading
column `b` by ID from a file whose unrelated column `a` is duplicated now
fails, though no `a` leaf is ever decoded. The `renamed_b` / `parquet.field.id
= 3` block in `CometNativeReaderSuite` asserts exactly this new failure.
`schema_adapter.rs` already has `schema_has_field_ids()` and
`remap_physical_schema`'s `id_to_phys_names`. Gate on those and resolve the
selected roots by ID, or at minimum use `!use_field_id ||
!schema_has_field_ids(required)`.
**4. No evidence the repro still fails on current `main`. Please re-verify
per shape.**
The published head predates DF 55.1 (#5865) and #5845, and the follow-up
numbers in the thread are for an unpublished branch. Two things make a fresh
check worth the time:
- DataFusion's cast-clipping already pins duplicate handling:
`nested_schema_pruning.rs:611 clip_keeps_duplicate_physical_field_names` shows
`struct<a, pad, a>` with target `struct<a>` keeping **both** leaves and
emitting `struct<a, a>`. Present identically in 55.0 and 55.1. So the
multiplication is not produced by the clipping path, and it would help the PR
to name the path that does produce it.
- The `missing`-field case looks like it may be a new failure rather than a
fixed one. For `s struct<other: bigint, missing: bigint>` over file
`struct<dup, dup, other>`: the target names no duplicated field,
`is_pure_structural_narrowing` is false so nothing is clipped, and the opener
hands the reader the *physical* schema (`opener/mod.rs:959`, `with_schema` is
only ever given `physical_file_schema`), i.e. 3 distinct leaves to 3 children.
If that reads correctly on unpatched `main`, this PR converts a working read
into a hard error, and the `"Missing fields require Comet's cast, which decodes
the complete physical struct"` assertion locks the regression in.
Running all five parameterized shapes plus the `missing` case against
unpatched `main` and stating which actually fail would settle both.
**5. Rejection pre-empts #5654's last-wins resolution.**
Already raised by @andygrove, so just the compatibility framing: Spark's
`ParquetReadSupport` builds the map with `.toMap`, so last-wins is the
Spark-compatible answer and the issue accepts an error only as a
strictly-better-than-wrong-results fallback. Worth stating in the PR body which
shapes this claims permanently and which are a placeholder until #5654.
### Simplification
**6. Stray `ponytail:` marker** in the `// Validate cache hits too` comment.
No other occurrence in the tree; reads like a leftover internal tag. Make it
`TODO(#5884)` or drop it.
**7. Use the hint-aware physical schema instead of the `schema_hints` escape
hatch.**
`parquet_to_arrow_schema(descr, None)` deliberately discards `ARROW:schema`,
which then forces pruning off for every file that carries one.
`ArrowReaderMetadata::try_new(Arc::clone(&metadata),
ArrowReaderOptions::new())` (parquet 59.2, `arrow_reader/mod.rs:942`) returns
`.schema()` with the hint applied — the schema the decoder will actually use.
Feed that to `is_pure_structural_narrowing` and the `schema_hints` flag, its
conservative branch, and the dictionary regression test all disappear. Worth
doing: `ARROW:schema` is on essentially all pyarrow output, so the conservative
branch is the common path, not the rare one.
**8. Walk Arrow `Fields` rather than the raw Parquet `Type` tree.**
The LIST arm re-implements parquet-rs's 2-level/3-level heuristic
(`complex.rs:596`) while omitting its `!repeated_field.is_list()` and
`!has_single_repeated_child()` guards. It is conservative today (the omissions
only route more shapes to full validation), but it is a hand-copy of an
internal rule that will drift. `physical_schema` is already computed and
`parquet_to_arrow_schema` preserves duplicate sibling names — the `ArrowWriter`
test in this PR relies on that. Walking two Arrow trees removes both layout
heuristics, the per-list-field `format!("{}_tuple", ..)` allocation, and three
unit tests.
**9. Dead and no-op branches in `validate_field_type`.**
- `LargeList` / `FixedSizeList` are unreachable. Spark `ArrayType` maps to
`DataType::List` (`execution/serde.rs:131`) and `parquet_to_arrow_schema` only
emits `List`.
- The `Map` arm never prunes anything: `is_pure_structural_narrowing`
returns `false` for any `Map` (`schema_adapter.rs:175`), so `selected` always
carries the physical Map type and the arm pays an O(n x m) match to reach the
same answer as full validation. Drop it and treat Map as validate-in-full.
**10. When `schema_hints` holds, the whole `selected` construction is wasted
work.** Every matched root gets its physical type back, so the result is just
"physical types, restricted to selected roots". Build that directly by
filtering `physical.fields()` against a set of folded required names in one
pass. That also avoids paying `parquet_to_arrow_schema` twice on the Variant
path, where `with_spark_arrow_schema` -> `spark_enum_schema` calls it again.
**11. The error carries no file path.** On a scan over many files the user
cannot tell which one is bad. `object_meta.location` is in scope — the `Failed
to fetch metadata for file {}` error two blocks up already formats it. Consider
a typed error alongside the existing
`SparkError::duplicate_field_case_insensitive` / `DuplicateFieldByFieldId` so
the JVM sees a Spark-shaped message rather than a generic
`ParquetError::General`.
**12. `projection: Option<(SchemaRef, SparkParquetOptions)>` is cloned in
`create_reader` and again in `get_metadata`.** `SparkParquetOptions` owns a
`String timezone`. `Arc<(SchemaRef, SparkParquetOptions)>` removes both clones.
**13. `case_sensitive` is used asymmetrically in `validate_field_names`:**
the projection match folds, the dedup `HashSet` inserts raw names. So
`struct<a, A>` under `caseSensitive=false` is not reported here (it is caught
later by `match_struct_fields`). Defensible, since the PR targets
byte-identical names — but the doc sentence *"The check applies in both
case-sensitivity modes"* reads as though collisions are caught in both, and the
parameter name invites the same misreading. Folding into the set as part of
finding 2 resolves it; otherwise one comment.
**14. Two spellings of one type:** `parquet::errors::Result<()>` on
`validate_field_names` vs `ParquetResult<()>` on `validate_field_type`;
`Type::GroupType { fields, .. }` in one vs `schema.is_group()` in the other.
### Test dedup
- `duplicate_root_names_are_rejected` is the final third of
`projected_fields_skip_unselected_roots` — same schema string, same assertion.
Merge.
- `duplicate_names_in_list_element_are_rejected` and
`duplicate_names_in_map_key_value_are_rejected` are already covered by the
`assert!(validate_field_names(&schema, None, cs).is_err())` inside
`projected_fields_skip_unselected_nested_duplicates`'s loop, for both LIST and
MAP. Keep the group-name assertion in one and drop the other two.
- `duplicate_names_in_map_key_value_are_rejected` also tests an unreachable
shape: a `key_value` with three children fails in `parquet_to_arrow_schema`
first (*"Child of map field must have two children, found 3"*, parquet 59.2
`complex.rs:410`), which in the projected path runs before the validator.
- The Map row of `projected_fields_skip_unselected_nested_duplicates`
asserts pruning the real pipeline never performs (finding 9) — it hand-builds a
`selected` that `get_metadata` cannot produce. Either drop the row or drive it
through `get_metadata` so it tests the actual decision.
- Scala: the parameterized `"distinct sibling"` case and the `unpruned` half
of `duplicate Parquet field names outside a nested projection remain readable`
assert the same full-subtree rejection. Merge.
- Scala `duplicate Parquet field names - unprojected fields and repeated
reads` runs 2 case modes x 2 repeats x 3 queries = 12 executions for one
behavior. Keep the second read only if the metadata-cache hit is the point, and
say so in a comment; drop the rest.
- Scala `duplicate Parquet field names - distinct siblings and repeated
names in separate groups` uses `dup`/`Dup` under `caseSensitive=true`, i.e.
distinct names, so it exercises the same no-collision path as the Rust
`repeated_names_in_separate_groups_are_valid`. Pick one.
- On moving these to `.sql` fixtures: not possible. Every case needs a file
written via `named_struct('dup', .., 'dup', ..)` or `writeDirect` and then read
back with an explicit `spark.read.schema(...)` that differs from the file
schema, which the SQL fixture runner cannot express. They belong in Scala.
### Assessments
**Scope.** Right layer — the footer is the only place that sees the
duplicate, and #5783 is not reachable from any JVM planning rule. Slightly too
broad in two places: the whole-file walk under `use_field_id` (finding 3) and
the whole-subtree walk for every `ARROW:schema` file (finding 7). Both widen
the rejection past what the decoder can actually break on.
**Spark compatibility.** Spark resolves duplicates last-wins via `.toMap`,
so erroring is a deliberate divergence. Acceptable as an interim per #5783, but
it needs the shape-by-shape split with #5654 agreed before merge, and the docs
paragraph should say that reads Spark handles will now fail rather than only
that ambiguous reads are rejected.
**Tests.** The end-to-end Scala coverage of the reported shapes is the right
mechanism and the case-sensitivity matrix is good. The gap is the "would this
fail if the fix were reverted, for the right reason?" check on the
`missing`-field and field-ID cases (findings 3 and 4). Rust unit tests calling
`validate_field_names` directly are fine for the walker, but three of them
assert behavior `get_metadata` cannot produce.
**Performance.** Per file open, on the metadata path, including cache hits:
one full `parquet_to_arrow_schema` over the whole file schema (two on the
Variant path), plus O(physical x projected) folded-name comparisons with two
`String` allocations each, plus a `format!` per list field, plus two
`SparkParquetOptions` clones. Small per file, but it scales with schema width x
file count and every piece of it is avoidable. Findings 2, 7, 10 and 12 remove
essentially all of it; no benchmark needed if they are applied, one wide-schema
open-rate measurement if they are not.
--
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]
