andygrove opened a new pull request, #6082: URL: https://github.com/apache/datafusion-comet/pull/6082
## Which issue does this PR close? Closes #6081. Heads up: I used an LLM to help draft this. The design is mine, but the code and prose have been shaped with LLM assistance, so review with that in mind. ## Rationale for this change `CometScanRule` and `CometExecRule` are registered as two independent rules, adjacently, in both the columnar path and the query-stage-prep path. They are really one pass split in two, held together by registration order rather than by anything in the code. Nothing ever runs between them: both registrations happen back to back inside one `apply`, so no other extension can interleave. And neither is useful on its own. `CometExecRule.transform` seeds its native chain only from the nodes `CometScanRule` produces, so operator conversion over unconverted Spark scans converts nothing; scan conversion is not independently useful either, since the V1 `CometScanExec` path is gated on `spark.comet.exec.enabled`. The split leaks into anything that needs the whole conversion rather than half of it. It is what forces #5394 to widen both rules' `_apply` to package visibility and thread a `forPreview` flag through `CometExecRule`, so one rule can reach into the other and rebuild the conversion for a report. I plan to rebase #5394 onto this and drop that machinery. ## What changes are included in this PR? - New `CometRule`, which applies `CometScanRule` then `CometExecRule`. - `CometSparkSessionExtensions` registers one `ColumnarRule` and one query-stage-prep rule instead of two of each. `CometScanColumnar` and `CometExecColumnar` collapse into `CometColumnar`; nothing referenced either by name. - Rule-order docstring updated. Both rules keep their own classes, files and tests. This only changes how they are sequenced. `spark.comet.explain.transformations` output is unaffected, since `CometRule` delegates to each inner rule's `apply` and so each logs under its own `ruleName`. ## How are these changes tested? Existing coverage, plus one new test in `CometExecRuleSuite` pinning the invariant that motivates the change: applying `CometExecRule` without scan conversion converts nothing. The load-bearing evidence is plan stability. `CometTPCDSV1_4_PlanStabilitySuite` (97) and `CometTPCDSV2_7_PlanStabilitySuite` (32) both pass, so every golden plan is byte-identical to before. Also green locally: `CometScanRuleSuite`, `CometExecRuleSuite`, `RevertNativeForTransitionHeavyStagesSuite`, `CometScanSchemeFallbackSuite` (66), `CometExecSuite` (146), and `test-compile` on the default profile and `-Pspark-3.4`. This touches rule registration, so it wants the broader suites before merge — I have applied `run-all-spark-profiles` and `run-spark-4.1-tests`. -- 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]
