sunchao commented on PR #5394:
URL:
https://github.com/apache/datafusion-comet/pull/5394#issuecomment-5798537512
Reviewed `a80ae3fce` with **five agents**. I found **three reproducible
reporting issues** to address before merging.
1. **P2 — `explain.format=fallback` removes the coverage report.**
[CometRule.scala:154](https://github.com/apache/datafusion-comet/blob/a80ae3fce500cd180dd87eecb3edeff2b1dd7671/spark/src/main/scala/org/apache/comet/rules/CometRule.scala#L154)
uses a formatter that honors the existing explain-format setting. A supported
Parquet projection reports **2/2 operators, 100%** with `verbose`, but logs
only `[Comet plan-only]` with `fallback`. Render the plan and coverage
explicitly, independent of that setting.
2. **P2 — Initially empty AQE queries can receive no report.**
[CometRule.scala:90](https://github.com/apache/datafusion-comet/blob/a80ae3fce500cd180dd87eecb3edeff2b1dd7671/spark/src/main/scala/org/apache/comet/rules/CometRule.scala#L90)
treats `maxRows == 0` as evidence of adaptive re-planning. However, `SELECT id
FROM range(0) DISTRIBUTE BY id` satisfies this during initial planning. It
emits **zero reports with AQE enabled**, versus one with AQE disabled. The rule
also marks the plan as reported, preventing later recovery. Distinguish initial
preparation from an actual re-plan.
3. **P3 — Repeated subqueries rebuild previews without an execution ID.**
[CometRule.scala:95](https://github.com/apache/datafusion-comet/blob/a80ae3fce500cd180dd87eecb3edeff2b1dd7671/spark/src/main/scala/org/apache/comet/rules/CometRule.scala#L95)
bypasses canonical deduplication when the ID is absent. The PR’s repeated
nested-subquery query emits **seven reports, only three distinct**, through
`queryExecution.toRdd.count()` with AQE off; AQE on emits five. `collect()`
emits three. This repeats preview construction and identical warnings.
Deduplication needs query-scoped ownership that also works without an execution
ID.
The overall design is sound: composing conversion in `CometRule` and sharing
post-columnar rules keeps eligibility aligned with normal planning. The
subquery handling reflects real Spark preparation boundaries. Reporting
ownership remains the fragile part. I found no additional substantiated
result-correctness, native-lifetime, or default-off performance defect.
**Validation:** Current CI has 23 successful checks and 14 skipped. Its
execution group passed 999 tests, including all 62 `CometExecRuleSuite` tests.
Spark SQL and Iceberg SQL matrices were skipped. [CI
run](https://github.com/apache/datafusion-comet/actions/runs/35676248431)
Local probes used Spark 4.1.3/JDK 17 with freshly compiled current Scala
changes over cached dependencies and native library. Disk constraints prevented
a clean full build. These are targeted integration reproductions; no
whole-query performance claim is made.
--
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]
