andygrove commented on PR #5786:
URL:
https://github.com/apache/datafusion-comet/pull/5786#issuecomment-5782188124
Thanks for the rework. This is a much better shape than the metadata-time
validator, and moving the byte-identical half of #5602's split into
`match_struct_fields` gets the projection awareness for free, exactly as hoped.
I re-checked the pruned reads against this head and they hold, including the
array shape and the filter cases. Both of @sunchao's P2s look resolved to me,
and `cargo clippy --all-targets -- -D warnings` is clean, so the type-check
blocker is gone too.
CI still has not run here. `Comet CI` and `CodeQL` are both
`action_required` on `f5e1dc59`, so I ran the suite locally on the default
profile instead, Spark 4.1 with Scala 2.13. `duplicate Parquet field names -
multiple files and schema merge` is flaky. Five runs of
`CometNativeReaderSuite` against your head unmodified gave four failures and
one pass. The failures are at line 172, where `intercept[AnalysisException]`
catches `SparkException [FAILED_READ_FILE.NO_HINT]` instead. Two further runs
with that widened to `intercept[Exception]` showed both outcomes directly:
sometimes `AnalysisException [COLUMN_ALREADY_EXISTS]` as you expect, sometimes
the read failure from Spark's own reader. Comet is disabled for that block, so
this is Spark's parallel schema merge being order dependent. Merging
`struct<dup, other>` with `struct<dup, dup, other>` collapses the two `dup`
children when the clean schema is the base and keeps them when the
duplicate-bearing one is. I probed the single path
on its own and `spark.read.parquet(duplicatePath).schema` raises
`COLUMN_ALREADY_EXISTS` every time, so would pointing that assertion at the one
path give you the same coverage deterministically? The comment above the loop
rests on the same premise and is only reliable in that single-path form.
On `check_decoded_field_names`, I instrumented all six call sites and ran
the native parquet tests, the full `CometNativeReaderSuite`, and
`CometVariantTypeSuite`. Four of the six fire, but `schema_adapter.rs:1157` in
`wrap_direct_variant_column` and `schema_adapter.rs:1295` in the Variant branch
of `replace_with_spark_cast` never do. Is a Variant column whose physical
subtree carries duplicate names actually reachable? If it is, a test would be
worth having, and if it is not I would rather drop those two calls than carry
guard code nothing exercises.
Related to that, the new line on `is_pure_structural_narrowing` describes
this as a decoder-safety obligation, and I think that is the right framing.
What worries me is that the obligation is discharged in six places spread
across three functions. Whoever adds the next cast path to this file has to
know to add a seventh. Could the cast constructions and
`reject_on_non_empty_expr` go through one small helper here that does the
check, so the rule lives in one spot?
One cost question. `create` now folds `physical_file_schema` a second time
at line 912 and builds a `HashMap<String, Vec<usize>>` with a heap-allocated
`Vec` per column, then `retain` throws nearly all of it away. When
`needs_remap` is false, `adapted_physical_schema` is the same `Arc`, so line
925 repeats that same fold. Case-sensitive mode paid none of this before. On a
thousand-column table that is roughly two thousand allocations per file open,
on files that have no duplicates at all. Would a single `HashSet` pass work,
building the index map only after it sees the first collision? That is the same
per-open axis we were measuring last round.
A few smaller things. The message `Found duplicate Parquet field name
'{...}'` is now a literal in four places, `parquet_support.rs:480` and
`schema_adapter.rs:824`, `:1058` and `:1073`, and both the Rust and the Scala
tests match on it as a substring, so a shared constructor would stop those
drifting apart. I checked what reaches the user and it arrives as
`org.apache.comet.CometNativeException`, which seems right to me given Spark
has no error class to mirror here. And `id_duplicate_roots` at
`schema_adapter.rs:1029` is the only field on `SparkPhysicalExprAdapter`
without a doc comment, which is a shame because it is the one that most needs
it. Worth recording that it is keyed by folded logical name, that it is
populated only under field-id matching and only for byte-identical duplicate
physical names, and that it is checked ahead of the id-resolved skip so it
takes precedence.
On the docs, "In case-sensitive mode Spark instead silently picks one
sibling, so disable Comet for the query with an explicit read schema to use
that resolution" is hard to follow, and I am not sure we want to point people
at that resolution with much confidence. #5783 found last-wins for the nested
case, the root-level probe I ran gave first, and your own #5964 has mixed-type
roots returning anomalous values. Could the entry link #5964 alongside #5884 so
a reader knows the fallback is not reliably one sibling either?
Last, the description still describes the previous architecture. It says
validation happens when loading native-reader metadata including cache hits,
that the required schema is preserved through validation, and that field-ID
reads validate the entire file schema. None of that is in this head, and that
last one is the P2 this head fixed. The test section's 71 also predates the
fourteen new cases, since the suite is at 85 now. The body lands as the commit
message, so could you rewrite it against the current diff?
--
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]
