andygrove commented on PR #5394: URL: https://github.com/apache/datafusion-comet/pull/5394#issuecomment-5764162418
Rebased onto #6082, which composes `CometScanRule` and `CometExecRule` into a single `CometRule`. That PR should go in first; until it does, the diff here includes it. The reason for splitting it out is that most of the awkwardness in this PR came from the two rules being registered separately. With one rule there is a single place that holds the whole conversion, so plan-only mode gets a single short-circuit and a single call to build the plan it reports on: - `CometScanRule` is back to its version on main. It no longer knows about plan-only mode at all, and its `_apply` is private again. - `CometExecRule` loses the `forPreview` parameter, the plan-only branch in `_apply`, and the `queryStagePrep` constructor parameter. It is now purely operator conversion, and this PR touches 4 lines of it rather than 287. - The reporting state, the reporting decision and the preview machinery live on `CometRule`. `buildPreview` calls `convert(...)` rather than reaching into both rules' private `_apply`s. One behavioural note. The plan-only check used to sit inside `CometExecRule`'s exec-enabled branch, so it never engaged with `spark.comet.exec.enabled=false`. `CometRule.planOnlyApplies` keeps that scoping, along with the `isCometLoaded` and streaming guards, so a plan the conversion rules would have left alone is not diverted into a report and Comet's columnar shuffle still applies with exec disabled. Also fixed here: `IcebergWriteStrategy` is registered with `injectPlannerStrategy`, so it runs ahead of both conversion rules and was not covered by the short-circuit — an Iceberg V2 write was still being offloaded under plan-only mode. Neither `IcebergCommitExec` nor `IcebergWriteExec` extends `CometPlan`, so the existing "no Comet operators" assertions could not see it. It now declines in plan-only mode, with a test in `CometIcebergWriteDetectionSuite` and a note in the user guide about the coverage that costs. Local runs: `CometExecRuleSuite` 62, plus `CometIcebergWriteDetectionSuite`, `CometScanRuleSuite`, `RevertNativeForTransitionHeavyStagesSuite` and `CometScanSchemeFallbackSuite` at 81 between them. `test-compile` clean on the default profile and `-Pspark-3.4`. -- 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]
