Skip to content

fix: preserve DPP filters during transition revert - #6137

Merged
sunchao merged 5 commits into
apache:mainfrom
sam-1112:fix-5486-aqe-dpp-transition-revert
Oct 4, 2026
Merged

sunchao merged 5 commits into
apache:mainfrom
sam-1112:fix-5486-aqe-dpp-transition-revert

Conversation

@sam-1112

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #5486.

Rationale for this change

With AQE and dynamic partition pruning enabled, the query-stage optimizer rewrites the DPP placeholder in the live CometNativeScanExec into an executable broadcast subquery.

RevertNativeForTransitionHeavyStages runs later as a post-columnar rule. When reverting a native V1 scan, it restored the frozen FileSourceScanExec from originalPlan. That plan still contained the original SubqueryAdaptiveBroadcastExec, which cannot be executed directly and caused:

  SubqueryAdaptiveBroadcastExec does not support the execute() code path

What changes are included in this PR?

  • Preserve the live partitionFilters and dataFilters when reverting a CometNativeScanExec to its original FileSourceScanExec.

  • Preserve the original scan's logical link.

  • Add a V1 Parquet regression test covering:

    • AQE enabled and disabled.
    • Transition reversion enabled and disabled.
    • Native project execution enabled and disabled.
  • Verify that all eight configurations return 400 rows.

  • When AQE and transition reversion are enabled, verify that:

    • The scan is reverted to FileSourceScanExec.
    • DPP retains an executable SubqueryBroadcastExec or CometSubqueryBroadcastExec.
    • No unexecutable SubqueryAdaptiveBroadcastExec remains.

How are these changes tested?

Built the native library:

  make core

Ran the complete transition-reversion suite on the default Spark 4.1.3 profile:

  ./mvnw test -Dtest=none -Dsuites="org.apache.comet.rules.RevertNativeForTransitionHeavyStagesSuite"

Result: 16 tests passed.

Ran the new regression test on Spark 3.5:

  ./mvnw test -Pspark-3.5 -Dtest=none \
    -Dsuites="org.apache.comet.rules.RevertNativeForTransitionHeavyStagesSuite AQE DPP remains
    executable when transition reversion restores a V1 scan" \
    -Dscalastyle.skip=true

Result: 1 test passed.

Ran the existing V1 DPP broadcast-join tests:

  ./mvnw test -Dtest=none \
    -Dsuites="org.apache.comet.exec.CometExecSuite AQE DPP: BHJ works with CometNativeScanExec" \
    -Dscalastyle.skip=true

Result: 2 tests passed.

The full Spark 4.1.3 suite invocation also completed the configured Spotless and Scalastyle checks successfully.

@github-actions github-actions Bot added the bug Something isn't working label Sep 23, 2026
@sam-1112
sam-1112 force-pushed the fix-5486-aqe-dpp-transition-revert branch from 3bfbc98 to 3660681 Compare September 23, 2026 10:46
@sam-1112
sam-1112 marked this pull request as ready for review September 24, 2026 03:54

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

  • Prior state and problem: Transition reversion restored a frozen FileSourceScanExec, reintroducing an unexecutable AQE DPP placeholder after the live scan’s filters had been rewritten.
  • Design approach: Restore the original scan with the live partitionFilters and dataFilters, preserving its logical link.
  • Correctness / compatibility analysis: Checked Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0. The named copy arguments, DPP rule ordering and subquery preparation contract support this approach. Spark 3.4’s existing AQE DPP fallback remains applicable.
  • Key design decisions: The scan-specific handling is localized to reversion and retains the original scan configuration. It introduces no broader abstraction or new per-row processing.
  • Implementation sketch: Copy the scan with current filters, restore its logical link, then use the existing child and transition handling. The regression test exercises eight AQE/reversion/project configurations and checks for executable DPP subqueries.
  • Behavioral changes worth calling out: Reverted V1 scans retain executable DPP expressions instead of restoring stale placeholders. Additional work consists of creating a scan-plan copy during reversion. No reproducible performance regression was identified.
  • Suggested improvements: None supported by reproducible P1/P2 evidence.

Reviewed the entire two-file diff at 4be5e0f1cf7d565e9c42adf8a249694783b8180f against e6e92f1a1a10e1bca2bcf32858113ab61b2ba685. The PR is not a draft. Read AGENTS.md, contributor guidance, the discussion snapshot and current comments. There were no existing reviews, comments or threads raising unresolved concerns.

Routed skills: review-comet-pr and review-comet-expression-pr.

Exact-head CI: Comet CI and CodeQL report action_required. Only the labeling check passed. There is no build or test verdict at this SHA.

Validation limits: make core and git diff --check passed. The focused RevertNativeForTransitionHeavyStagesSuite invocation could not start because the Maven wrapper failed to resolve repo.maven.apache.org. System Maven also rejected the repository’s maven.config. No JVM tests or Spark SQL suites ran locally, and the author’s reported test results were not independently reproduced.

No introduced P1/P2 issues found within this review.

…ansition-revert

# Conflicts:
#	spark/src/main/scala/org/apache/comet/rules/RevertNativeForTransitionHeavyStages.scala
@andygrove

Copy link
Copy Markdown
Member

This is a light fully automated review since there are so many PRs open.

The new CometNativeScanExec case in revertToSpark fixes the V1 Parquet path, but CometIcebergNativeScanExec still falls into the generic case _ => cometExec.originalPlan branch right below it (spark/src/main/scala/org/apache/comet/rules/RevertNativeForTransitionHeavyStages.scala:202-214), and it looks like it hits the same bug this PR closes. CometIcebergNativeScanExec.originalPlan is @transient and, per the comment in spark/src/main/scala/org/apache/spark/sql/comet/CometIcebergNativeScanExec.scala:96-105, is never touched when CometPlanAdaptiveDynamicPruningFilters rewrites the live scan's top-level runtimeFilters, so it keeps the pre-DPP SubqueryAdaptiveBroadcastExec placeholder. serializedPartitionData works around this by rebuilding effectiveOriginalPlan = originalPlan.copy(runtimeFilters = runtimeFilters) before reading it, but revertToSpark restores the stale originalPlan as-is. With AQE, dynamic partition pruning, spark.comet.exec.transitionRevert.enabled, and a native Iceberg scan (spark.comet.scan.icebergNative.enabled defaults to true) in the reverted stage, wouldn't this reproduce the same SubqueryAdaptiveBroadcastExec does not support the execute() failure, just for the wrapped BatchScanExec instead of FileSourceScanExec? Would it make sense to extend this case, or generalize it, to cover CometIcebergNativeScanExec too?

…ansition-revert

Co-authored-by: Cursor <cursoragent@cursor.com>
@andygrove andygrove added the run-all-spark-profiles Run the Comet test suites against every Spark profile on this pull request, ahead of the merge queue label Oct 3, 2026
@sunchao
sunchao added this pull request to the merge queue Oct 4, 2026
Merged via the queue into apache:main with commit e7760c0 Oct 4, 2026
83 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Iceberg bug Something isn't working run-all-spark-profiles Run the Comet test suites against every Spark profile on this pull request, ahead of the merge queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AQE + DPP + spark.comet.exec.transitionRevert.enabled fails with "SubqueryAdaptiveBroadcastExec does not support the execute() code path"

3 participants