comphead commented on PR #6004:
URL:
https://github.com/apache/datafusion-comet/pull/6004#issuecomment-5723145094
@andygrove on the operator gate — it is intentionally out of this PR, and it
should be filed separately so it does not get lost. I'd rather not bring it
back here: this PR is now just "check the schema, fall back", and the operator
gate is a different mechanism with a different blast radius.
Your `CometArrayExpressionSuite` finding is a good argument for keeping them
apart, and thank you for running it — that suite was not in what I ran locally.
Confirmed: the gate fired before the serde, so the recorded reason became
`Native operators do not support v.map value: struct with duplicate field names
(x)` and the expected `Unsupported data type MapType` was never recorded. Right
answer, wrong reason, and the test rightly cared. That suite is green on this
head.
Worth recording what I found while checking whether this PR could also cover
#5783, because it changes what a fix there should aim for. I compared four
readers on the file Spark's writer produces for `named_struct('dup', id, 'dup',
id + 100)`:
| File written by | chunk `num_values` / row group `num_rows` | Spark |
DuckDB | DataFusion 55 | Comet |
| --- | --- | --- | --- | --- | --- |
| Spark | 6 / 3 | 3 rows, `0, 100, 1` | 3 rows, same | 6 rows (`count(*)`
says 3) | 6 rows |
| DataFusion | 3 / 3 | 3 rows, correct | 3 rows, correct | 3 rows, correct |
**3 rows, correct** |
| DuckDB (renames to `dup_1`) | 3 / 3 | 3 rows, correct | 3 rows, correct |
3 rows, correct | **3 rows, correct** |
Two things follow.
**Comet reads a duplicate-named struct correctly when the file is well
formed.** On the DataFusion-written file — same duplicate `dup`/`dup` struct,
same `spark.read.schema("s struct<dup: bigint>")` — Spark and Comet both return
`[0], [1], [2]` with `CometNativeScanExec` in the plan. DuckDB refuses to
construct the shape at all (`Binder Error: Duplicate struct entry name "dup"`).
**The file Spark writes is internally inconsistent**: the row group declares
3 rows while each of the two same-path `s.dup` leaf chunks declares 6 values.
parquet-mr cannot read it (`ParquetEncodingException: [s, dup] -(0)-> 2`).
Spark's own 3 rows are `0, 100, 1`, the two children interleaved — the right
row count over the wrong values. So there is no correct answer to match on that
file, and #5783's premise that Spark is right and Comet is wrong does not hold.
Practical consequence for #5786: rejecting duplicate Parquet field *names*
before decoding would make Comet refuse the DataFusion-written file, which it
currently reads correctly and in agreement with Spark. If a guard is wanted
there, keying it on the `num_values` vs `row_group_num_rows` inconsistency
rather than on duplicate names would avoid that regression. cc @dwsmith1983
Separately, DataFusion answers `count(*)` from row group metadata and the
scan from `num_values`, so on the malformed file it returns 3 and 6 in the same
session. Filed upstream as apache/datafusion#25433.
--
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]
