You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "MaxGekk (via GitHub)" <gi...@apache.org> on 2026/09/24 13:27:28 UTC
[PR] [SPARK-59764][SQL][TESTS] Check generated code of benchmark queries with AQE disabled [spark]
MaxGekk opened a new pull request, #59017:
URL: https://github.com/apache/spark/pull/59017
### What changes were proposed in this pull request?
`BenchmarkQueryTest.checkGeneratedCode` is meant to check that every whole-stage codegen subtree of a benchmark query compiles, and that its largest generated method is within HotSpot's `HugeMethodLimit` (8000 bytes). It walks `sql(query).queryExecution.executedPlan` without running the query. Since adaptive query execution became the default (SPARK-33679, 3.2.0), that plan is an `AdaptiveSparkPlanExec`, whose `WholeStageCodegenExec` subtrees are created only when the query runs. So the walk finds none, and `TPCDSQuerySuite` (with `TPCDSQueryWithStatsSuite` and `TPCDSQueryANSISuite`), `TPCHQuerySuite` and `SSBQuerySuite` pass without compiling or checking anything.
This PR:
1. Mixes `DisableAdaptiveExecutionSuite` into `BenchmarkQueryTest`, as `LogicalPlanTagInSparkPlanSuite` already does, so the plans these suites check have their whole-stage codegen subtrees.
2. Asserts in `checkGeneratedCode` that at least one `WholeStageCodegenExec` subtree was found, so the check cannot become vacuous again unnoticed.
3. Removes the `spark.sql.readSideCharPadding=false` override from `TPCDSQuerySuite`. Its comment says it keeps the generated code under 8000 bytes, but it has guarded a check that did not run. With the check running, the suites pass with Spark's default padding on. The `modified-q3` exclusion (SPARK-29128) stays: its stage is 12214 bytes and really is past the limit.
### Why are the changes needed?
The compile check and the method size check in these suites have not run since 3.2.0, so a codegen regression in the TPC-DS, TPC-H or SSB queries would not be caught.
### Does this PR introduce _any_ user-facing change?
No. Test-only change.
### How was this patch tested?
By running the affected suites:
```
build/sbt "sql/testOnly org.apache.spark.sql.TPCDSQuerySuite org.apache.spark.sql.TPCDSQueryWithStatsSuite org.apache.spark.sql.TPCDSQueryANSISuite org.apache.spark.sql.TPCHQuerySuite org.apache.spark.sql.SSBQuerySuite org.apache.spark.sql.execution.LogicalPlanTagInSparkPlanSuite"
```
All six suites passed, 635 tests.
As a negative check, with only the new assertion and without `DisableAdaptiveExecutionSuite`, all 22 tests of `TPCHQuerySuite` fail with `no WholeStageCodegenExec subtree found to check in the plan`, which is the bug this PR fixes.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5.5)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59764][SQL][TESTS] Check generated code of benchmark queries with AQE disabled [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #59017:
URL: https://github.com/apache/spark/pull/59017#issuecomment-5826924462
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/73054c78f8b8acfac3074e3d3e29ee5482970378
- merged into branch-4.x https://github.com/apache/spark/commit/2c92cdb165a872ad86530fc28389a1121389765c
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59764][SQL][TESTS] Check generated code of benchmark queries with AQE disabled [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #59017:
URL: https://github.com/apache/spark/pull/59017#issuecomment-5826904819
Merging to master/4.x. Thank you, @dongjoon-hyun for review.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59764][SQL][TESTS] Check generated code of benchmark queries with AQE disabled [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #59017: [SPARK-59764][SQL][TESTS] Check generated code of benchmark queries with AQE disabled
URL: https://github.com/apache/spark/pull/59017
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59764][SQL][TESTS] Check generated code of benchmark queries with AQE disabled [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #59017:
URL: https://github.com/apache/spark/pull/59017#issuecomment-5816272793
A related limitation, which this PR does not change and which I filed separately as SPARK-59765: with AQE off, `BroadcastHashJoinExec` runs the broadcast while generating code. Over the empty TPC-DS tables the relation is `EmptyHashedRelation`, so the join generates a stub, and the rest of its stage isn't generated either. On master, with `WholeStageCodegenSizeBenchmark`'s setup (SF100 stats, AQE off), 324 of the 2201 stages of the TPC-DS v1.4 and v2.7 queries contain that stub, in 127 of the 129 queries.
So both `checkGeneratedCode` here and `WholeStageCodegenSizeBenchmark` (SPARK-57915) measure less code than the queries compile over real data. The JIRA proposes generating each stage's code from a copy in which every broadcast exchange reads one row of non-null default values, and asserting that no stub is left.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org