Repository navigation
fix: reuse the Comet broadcast for a non-AQE DPP subquery whose build side is not a CometNativeExec - #7554
Draft
tlfua wants to merge 3 commits into
Draft
fix: reuse the Comet broadcast for a non-AQE DPP subquery whose build side is not a CometNativeExec#7554tlfua wants to merge 3 commits into
tlfua wants to merge 3 commits into
Conversation
… side is not a CometNativeExec Without AQE, CometExecRule.rewriteInSubqueryPlan only converted a DPP SubqueryBroadcastExec to CometSubqueryBroadcastExec when the broadcast child was a CometNativeExec. The join's own broadcast converts over CometUnionExec, CometCoalesceExec, CometTakeOrderedAndProjectExec and CometShuffleExchangeExec too, so ReuseExchangeAndSubquery couldn't match the two exchanges and the dimension was broadcast twice. Accept any columnar CometPlan, the same check the AQE path uses in CometPlanAdaptiveDynamicPruningFilters.isNativeBuildSide. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95bb3fbb-4ffd-407c-a7bb-e912bfe7b965
The join's inferred isnotnull filter on the key can't be pushed below the limit, so a native CometFilter is the top operator of that build side and CometTakeOrderedAndProjectExec never sits directly under the broadcast. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95bb3fbb-4ffd-407c-a7bb-e912bfe7b965
…roadcast exchange is disabled With spark.comet.exec.broadcastExchange.enabled=false the join's broadcast stays a Spark BroadcastExchangeExec, so converting the DPP subquery to a CometBroadcastExchangeExec breaks Spark's exchange reuse and broadcasts the dimension twice. Only convert the DPP subquery when the config is on, which matches the AQE path requiring a Comet join. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95bb3fbb-4ffd-407c-a7bb-e912bfe7b965
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #6815.
Rationale for this change
With AQE off,
CometExecRule.rewriteInSubqueryPlanonly converts a DPPSubqueryBroadcastExecto aCometSubqueryBroadcastExecwhen the broadcast's child is aCometNativeExec. The join's own broadcast converts to aCometBroadcastExchangeExecover more plans than that, becauseCometUnionExec,CometCoalesceExec,CometTakeOrderedAndProjectExecandCometShuffleExchangeExecare wrapped in a native placeholder while the join is converted. For those build sides the DPP subquery keeps a SparkBroadcastExchange,ReuseExchangeAndSubquerycan't match it to the join's exchange, and the dimension is broadcast twice.The AQE path already accepts any columnar
CometPlaninCometPlanAdaptiveDynamicPruningFilters.isNativeBuildSide(#6786), so this only changes the non-AQE path.What changes are included in this PR?
CometExecRule.rewriteInSubqueryPlannow requiresCometPlan && supportsColumnarinstead ofCometNativeExec, the same check asisNativeBuildSide.CometBroadcastExchangeExeconly needs a child that supportsexecuteColumnar(), so a child admitted by this check runs correctly. If the DPP side converts a plan the join side didn't, the only effect is that the two exchanges don't match and reuse is lost, as before.spark.comet.exec.broadcastExchange.enabled. With it off, the join's broadcast stays a SparkBroadcastExchangeExec, so converting the DPP subquery would leave two exchanges that can't match and broadcast the dimension twice. The relaxed check above would have made that happen for more build sides, and it already happened for aCometNativeExecbuild side before this PR. The AQE path gets the same result by requiring a Comet join (isCometJoin).CometPlanAdaptiveDynamicPruningFiltersthat pointed to the old non-AQE check, and a stale comment in an existing test that namedCometNativeExec.Build sides this change newly admits:
CometUnionExecCometCoalesceExecCometShuffleExchangeExecCometTakeOrderedAndProjectExecisnotnullfilter on a nullable key, and the optimizer can't push it below the limit, so a nativeCometFilteris the top operatorORDER BY ... LIMITcase in fork CI run https://github.com/tlfua/datafusion-comet/actions/runs/42359642929CometCollectLimitExecCollectLimitonly at the root of a queryCometLocalTableScanExec,CometEmptyRelationCometSparkToColumnarExec,CometScanExec,CometBatchScanExec,CometInMemoryTableScanExecHow are these changes tested?
Added three tests to
CometExecSuite, one for each tested shape in the table above. Each runs a non-AQE broadcast join with DPP in reuse-only mode. Each test asserts these things:CometBroadcastExchangeExec.CometNativeExec.CometSubqueryBroadcastExec.Added a fourth test that runs the
UNION ALLcase withspark.comet.exec.broadcastExchange.enabled=false. It asserts there is noCometBroadcastExchangeExec, the DPP subquery is a SparkSubqueryBroadcastExec, and it reuses the join's onlyBroadcastExchangeExec. The four tests share a helper that writes the tables.No TPC-DS approved plan has a Spark
BroadcastExchangeunder aSubqueryBroadcast, so no golden files change.Local checks:
./mvnw compile test-compile scalafix:scalafix -Psemanticdband./mvnw spotless:check: passed.Fork CI: https://github.com/tlfua/datafusion-comet/actions/runs/42395827505 (passed; the
[exec]job ran all four new tests). In an earlier run, anORDER BY ... LIMITcase showed theCometFilterdescribed in the table above, so I removed that case.Follow-up
If the config is on but the join itself stays on Spark, for example with
spark.comet.exec.broadcastHashJoin.enabled=false, the join's broadcast still converts to a Comet exchange and the non-AQE rule can't see the join to decide. That case predates this PR and isn't handled here.