sunchao commented on PR #5654:
URL:
https://github.com/apache/datafusion-comet/pull/5654#issuecomment-5536054089
Reviewed head `58e67fee` against base `55ae4f20`. The PR is focused, but I
found **one new regression, an incomplete validation fix, and avoidable
batch-processing overhead**.
1. **[P2] Placeholder collisions can suppress column defaults.**
[schema_adapter.rs:177](https://github.com/apache/datafusion-comet/blob/58e67fee104aaf6ddd4119b96969d44d45bdbcf0/native/core/src/parquet/schema_adapter.rs#L177)
reserves exact names, while missing-column detection uses case-folded names. A
generated `__comet_unmatched_field_id_1` therefore collides with requested
`__COMET_UNMATCHED_FIELD_ID_1`. In my reproduction, the base returns the
configured default `7`; this PR returns NULL.
Reserve names using the existing folded schema names. That change passed
the reproduction. Constructing the reservation set only when shielding needs it
would also avoid extra hashing on ordinary reads without field IDs.
2. **[P2] Duplicate-ID validation still depends on whether a cast occurs.**
The new guard in
[cast_column.rs:292](https://github.com/apache/datafusion-comet/blob/58e67fee104aaf6ddd4119b96969d44d45bdbcf0/native/core/src/parquet/cast_column.rs#L292)
misses identical physical/requested schemas. Such reads can omit the cast
entirely or return before the guard.
A real native Parquet scan of identical `s<x: long id=1, y: long id=1>`
schemas returned `[42, 43]`; Spark 4.1.3’s schema-clipping check rejected the
duplicate ID. **This also occurs on the base, so it is an incomplete fix, not a
new regression.** Validation needs to cover reads that require no conversion.
3. **Avoid allocating a vector for every unique ID on every struct
conversion.**
[parquet_support.rs:270](https://github.com/apache/datafusion-comet/blob/58e67fee104aaf6ddd4119b96969d44d45bdbcf0/native/core/src/parquet/parquet_support.rs#L270)
changes the index to `HashMap<i32, Vec<usize>>`. An allocation probe using the
old and new construction loops measured **8 → 264 allocations for 256 unique
IDs**.
A compact unique/duplicate entry would preserve the behavior. Collect
matching field names only when reporting an ambiguity. The new
`contains_field_id_metadata` predicate also depends on immutable expression
state and can be computed once.
The strongest design improvement is to **resolve and validate requested
fields once per file schema, then reuse the mapping across batches**. That
addresses the validation bypasses and repeated lookup work together.
Metadata-only relabeling remains safe when the resolved mapping is positional.
A small mapping object is a useful abstraction here.
Validation: **100 native Parquet tests passed**, with default HDFS features
disabled. Additional head/base probes confirmed both correctness cases.
Performance evidence measures component allocations, not overall scan speed. CI
snapshot: **57 passed, 7 running, 7 skipped**. Nothing was posted to GitHub.
--
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]
