sunchao commented on PR #5262:
URL:
https://github.com/apache/datafusion-comet/pull/5262#issuecomment-5547149795
I found **four correctness issues still present** at `f536ca7f`, reviewed
against `a089669d`. These overlap existing discussions; fresh tests confirm
they remain.
1. **[P1] Array functions silently change results on supported Spark
versions.**
[The signed-zero tests are
skipped](https://github.com/apache/datafusion-comet/blob/f536ca7f9bcd49b1012f2017cdcf67555b38410d/spark/src/test/resources/sql-tests/expressions/array/array_distinct.sql#L139),
but `array_distinct` and `array_union` remain enabled by default. Spark 4.1.3
returns `[0.0, -0.0, 1.0]` for the distinct example; DataFusion 55 returns
`[0.0, 1.0]`. I reproduced the difference for both functions. Add compatible
evaluation or type/version-aware fallback, then restore the tests. The comment
claiming literal inputs are normalized is also incorrect for Spark 4.1.
2. **[P2] Nested Parquet pruning bypasses Spark’s ambiguity check.**
[schema_adapter.rs:132](https://github.com/apache/datafusion-comet/blob/f536ca7f9bcd49b1012f2017cdcf67555b38410d/native/core/src/parquet/schema_adapter.rs#L132)
accepts an exact `id` match within `struct<id:bigint,ID:bigint>` when case
sensitivity is disabled. The retained DataFusion cast returns `id`; Spark and
Comet’s existing converter reject the ambiguity. Reproduced through the actual
adapter for both structs and lists of structs. Require uniqueness under Spark’s
configured name resolver before retaining the cast.
3. **[P2] Iceberg partition-source augmentation creates invalid schemas.**
[CometIcebergNativeScan.scala:1092](https://github.com/apache/datafusion-comet/blob/f536ca7f9bcd49b1012f2017cdcf67555b38410d/spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala#L1092)
calls a helper that checks only top-level IDs. A partition source already
present as `s.region` gets appended again, causing a duplicate-ID exception.
Dropping and re-adding `category` can instead produce conflicting same-name
fields with different IDs. Both failures reproduced using the exact Scala
helper and Iceberg 1.11. Resolve IDs recursively and preserve nesting; exclude
partition sources that are omitted from the serialized representation.
4. **[P2] Mixed live/dropped v1 partitions fail the new task validation.**
[planner.rs:4540](https://github.com/apache/datafusion-comet/blob/f536ca7f9bcd49b1012f2017cdcf67555b38410d/native/core/src/execution/planner.rs#L4540)
rejects a serialized `[identity(region), void(category)]` spec containing only
the surviving region value. I passed the exact Scala-emitted protobufs into the
native parser and reproduced “partition has 1 fields but partition spec has 2
fields.” Preserving a null placeholder makes the control pass. Keep partition
specs and values consistent through evolution.
For **performance**, the new unconditional Iceberg helper allocated
approximately **79 KB per file for a 256-column schema**, even with no required
fields. Checking only required IDs avoids this work. I also reproduced an **8
MiB allocation for two visible array values** because DataFusion normalizes the
entire backing buffer before slicing; slicing first reduced the largest
allocation to 416 bytes. These are component allocation measurements, not
query-speed results.
For **design and abstraction**, reusing DataFusion’s cast and pruning
machinery is sensible, and `spark_sqrt` is a simple, appropriate compatibility
implementation. The main simplification is to derive Iceberg’s effective spec,
values, and required schema fields together. I found no additional actionable
issue in the aggregate or shuffle API migrations.
**Validation:** 213 existing native tests passed: 103 shuffle, 61 aggregate,
46 schema-adapter, and 3 square-root tests. The added Parquet regression failed
as expected. Core tests excluded optional HDFS support; full Comet/Spark
integration and throughput benchmarks were not run. GitHub reports no checks
for the current head. Nothing was posted, and the checkout is clean.
--
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]
