ErikBPF commented on PR #5786: URL: https://github.com/apache/datafusion-comet/pull/5786#issuecomment-5777646079
Rebased onto `main` (`57ef3275d`) and reworked in response to the review. Head is now `f5e1dc59`. **Review points** - **P1 (does not compile against main)** — resolved. The fallible `fold_name`/`fold_names`/`fold_schema_names` signatures and `is_pure_structural_narrowing -> DataFusionResult<bool>` from main are used throughout and every call propagates `?`. `cargo clippy --locked --all-targets --workspace -- -D warnings` is clean. - **P2 (quadratic projection matching)** — the per-open projection reconstruction and the metadata whole-file validator are gone. Referenced columns are folded once per `rewrite`; physical names are folded once in `create`. The duplicate index is retained to the entries that actually collide. - **P2 (unrelated duplicates rejecting an unambiguous field-ID read)** — the field-ID path now exempts ids that resolve to a single physical field, so `a(id=1), a(id=2), b(id=3)` + `renamed_b(id=3)` reads natively. Only ids whose physical root name is byte-identically duplicated still reject; that is what prevents the silent row multiplication we reproduced. - **Variant cast paths** — both `wrap_direct_variant_column` and the Variant branch of `replace_with_spark_cast` now validate the decoded subtree, so the "every non-pruning cast path validates" invariant holds everywhere. - **Iceberg adapter test** — the third `SparkParquetOptions::new` argument is `allow_incompat`, not `case_sensitive`, so the test was running case-insensitive and passing without the fix. It now sets `case_sensitive = true`; it fails against `main`'s `schema_adapter.rs` and passes here. **Test matrix you listed** - duplicated struct in a single file — five nested shapes (two/three children, distinct sibling, array element, map value) at batch 1 and 4096, asserting a native scan and the duplicate error. - duplicated struct across multiple files / schema merging — new test: one clean file and one file whose `s` group is `dup,dup,other`, read as a two-path dataset with `mergeSchema` `true` and `false`, in both resolver modes. `mergeSchema` only affects inference here, and a schema inferred from a duplicate-bearing file is rejected by Spark (`COLUMN_ALREADY_EXISTS`) before Comet runs, so the matrix asserts that too. - `spark.read.schema(...)` combinations — bad in file only (unprojected and repeated reads, plus exact-name projection of the unique sibling), bad in both (native error), bad in schema only (Spark rejects at analysis; asserted with Comet disabled), bad in neither (distinct siblings and separate groups). - no fallback when the bad struct exists but is not read — asserted by requiring `CometNativeScanExec` in the plan alongside successful reads. **Compatibility note** (`scans.md`) now says what actually happens: Comet rejects referenced collisions, in case-sensitive mode Spark instead silently picks one sibling, and disabling Comet for the query with an explicit read schema is the escape hatch. **Validation** (28-thread host, `rustfmt` + `clippy -D warnings` clean): 201 Rust parquet tests, 7 Iceberg adapter tests, `make core`, `CometNativeReaderSuite` + `ParquetReadV1Suite` 153 passed / 0 failed, `ParquetEncryptionITCase` 18/18, BUILD SUCCESS. On ordering with #5654: the overlap is the referenced-column guard only, so I am fine landing after it and rebasing on top — happy to follow whatever order you prefer. -- 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]
