0lai0 opened a new pull request, #5746: URL: https://github.com/apache/datafusion-comet/pull/5746
## Which issue does this PR close? Closes #5500. ## Rationale for this change `CometObjectHashAggregateExec` deliberately declines when Comet shuffle is disabled, because converting would split the aggregate across a Comet partial and a Spark final. That decline was a bare `return None` inside `convert`, with no `withFallbackReason` call, so the reason never reached the user. Under `spark.comet.explain.fallback.strict.enabled=true`, planning throws: ``` java.lang.IllegalStateException: Comet did not convert ObjectHashAggregate but recorded no fallback reason on the operator or any of its expressions. ``` Under the production default the generic catch-all stands in for the real cause, which is worse because it is wrong. Comet supports this operator fine, the user just turned shuffle off: ``` FALLBACK_REASONS = Set(ObjectHashAggregate is not supported) ``` The check simply lived in the wrong place. `CometExecRule.isOperatorEnabled` turns an `Unsupported(Some(reason))` from `getSupportLevel` into a `withFallbackReason` call centrally, so a check placed there is explained for free. `CometCollectLimitExec` and `CometTakeOrderedAndProjectExec` gate on the same predicate and already do it that way. ## What changes are included in this PR? - Move the shuffle guard from `convert` into `getSupportLevel`, returning `Unsupported(Some(...))`. `convert` becomes the same `doConvert` call `CometHashAggregateExec.convert` already is. - Remove the duplicate copy of the predicate from `CometExecRule.canAggregateBeConverted`. The line above it already calls `isOperatorEnabled`, so the block was dead. - Document the prerequisite on the `ObjectHashAggregateExec` row of `operators.md`. - Add three regression tests. The guard sits after the two test-knob checks so a test that disables partial or final aggregates still sees its own reason. The message names no config key, because `isCometShuffleEnabled` is a conjunction of `spark.comet.shuffle.enabled`, the shuffle manager, and the Celeborn check, and naming one would misdirect when another is the cause. That is also why this PR does not delegate to `CometShuffleExchangeExec.isCometShuffleEnabledReason`, which computes its Celeborn branch from `op.outputPartitioning.numPartitions` rather than the hardcoded `1` that `isCometShuffleEnabled` uses, and so could return no reason while the predicate is still false. ### Not visible in the diff **The Final aggregate now carries the reason too.** `getSupportLevel` runs for every stage, so the Final node gets it where previously it recorded nothing at all. I confirmed this with a probe rather than by reasoning about it. The message is worded stage-neutrally for that reason. Eligibility is unchanged: `hasFallbackReason` is consulted only for shuffle exchanges and `CometNativeScan`, never for aggregates. **`operators.md` rows 74 and 75 are still blank.** `CollectLimitExec` and `TakeOrderedAndProjectExec` gate on the same predicate and do not mention it, so the three rows now disagree. Changing user-facing text for two already-merged operators is out of scope here, but it is worth picking up separately. ## How are these changes tested? Three new tests in `CometExecRuleSuite`, all of which fail on unfixed code: 1. The partial `ObjectHashAggregateExec` records the shuffle reason, not the generic message, and the reason survives into `ExtendedExplainInfo`. Run with strict mode both enabled and disabled as the issue asks, with a fresh plan per iteration because fallback reasons accumulate on tags. 2. `getSupportLevel` returns `Unsupported` with the reason when shuffle is off and `Compatible` when it is on. Before the fix it returned `Compatible(None,None)`. 3. With shuffle enabled the query still converts, pinning the issue's "without changing aggregate execution eligibility" requirement. -- 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]
