Skip to content

fix: explain ObjectHashAggregate fallback when Comet shuffle is disabled - #5746

Open
0lai0 wants to merge 1 commit into
apache:mainfrom
0lai0:fix-5500-objecthash-fallback-reason
Open

fix: explain ObjectHashAggregate fallback when Comet shuffle is disabled#5746
0lai0 wants to merge 1 commit into
apache:mainfrom
0lai0:fix-5500-objecthash-fallback-reason

Conversation

@0lai0

@0lai0 0lai0 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Explain ObjectHashAggregate fallback when Comet shuffle is disabled

1 participant