adibmbrk commented on PR #5607: URL: https://github.com/apache/datafusion-comet/pull/5607#issuecomment-5567012443
Thanks for the detailed review, @sunchao — addressed both rounds: 1. Compile break (P1) — Fixed. `describe()` now captures its result inside the `withSQLConf` block into a `var` and returns it after, so it compiles regardless of whether `withSQLConf` returns `Unit` (3.4/3.5) or the block's value. Verified with a clean `test-compile` on both the default (Spark 4.1) and Spark 3.5 profiles. 2. Default route regression (P2) — Reverted. `CometLength`, `CometBitLength`, and `CometOctetLength` no longer mix in `CodegenDispatchFallback`; `BinaryType` input falls straight back to Spark again, same as before the original PR. Docs and SQL fixtures reverted to match (`Hybrid` → `Native`, `expect_fallback` assertions restored). 3. Timed-plan capture — Fixed. The benchmark now registers a `QueryExecutionListener` around `df.noop()` and reports the actual write command's `executedPlan`, instead of the pre-`noop` SELECT plan. 4. Additional benchmark coverage — Added a native downstream-consumer case (`sum(length(...))`) and a combined-projection case (all three roots in one query), each across the three width shapes. Re-ran the full benchmark (21 scenarios × 3 arms, 2M rows) after the fix. Results: both Comet arms (`Spark fallback` / `codegen dispatch`) now report identical plans and near-identical timings at every shape, confirming the dispatcher no longer affects these three roots on binary input. Separately, the numbers also show Comet trailing plain Spark at 1 KB width regardless of arm (~0.6–0.7x) — that's a pre-existing `CometColumnarToRow` transition cost unrelated to the dispatcher, not something this PR introduces or claims to fix. -- 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]
