Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-40850][SQL] Fix test case interpreted queries may execute Codegen #41467

Closed
wants to merge 1 commit into from
Closed

[SPARK-40850][SQL] Fix test case interpreted queries may execute Codegen #41467

wants to merge 1 commit into from

Conversation

Hisoka-X
Copy link
Member

@Hisoka-X Hisoka-X commented Jun 5, 2023

What changes were proposed in this pull request?

Fix CodegenInterpretedPlanTest always will execute Codegen even set spark.sql.codegen.factoryMode to NO_CODEGEN. We should set spark.sql.codegen.wholeStage to false too.

Why are the changes needed?

Fix test case

Does this PR introduce any user-facing change?

No

How was this patch tested?

Add breakpoint on WholeStageCodegenExec::doConsume. Even set spark.sql.codegen.factoryMode to NO_CODEGEN, the WholeStageCodegenExec::doConsume also will be executed. When set spark.sql.codegen.wholeStage to false, the logic will skip.

@github-actions github-actions bot added the SQL label Jun 5, 2023
@Hisoka-X
Copy link
Member Author

Hisoka-X commented Jun 5, 2023

cc @HyukjinKwon @MaxGekk

@Hisoka-X
Copy link
Member Author

cc @holdenk

@MaxGekk
Copy link
Member

MaxGekk commented Jun 28, 2023

@Hisoka-X How about other places, like:

spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala

Line 81 in f26bdb7

withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenMode.toString) {

Could you re-check them too, please ... or open an JIRA if you don't have time.

@MaxGekk
Copy link
Member

MaxGekk commented Jun 28, 2023

+1, LGTM. Merging to master.
Thank you, @Hisoka-X.

@MaxGekk MaxGekk closed this in d14a6ec Jun 28, 2023
@Hisoka-X
Copy link
Member Author

Thanks @MaxGekk

@Hisoka-X
Copy link
Member Author

@Hisoka-X How about other places, like:

spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala

Line 81 in f26bdb7

withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenMode.toString) {

Could you re-check them too, please ... or open an JIRA if you don't have time.

Ok, I will do it.

@Hisoka-X Hisoka-X deleted the SPARK-40850_Intrepetered_Queries_execute_Codegen branch June 29, 2023 01:11
@Hisoka-X Hisoka-X mentioned this pull request Jun 29, 2023
Closed
cloud-fan pushed a commit that referenced this pull request Aug 8, 2023
@Hisoka-X @cloud-fan
[SPARK-44236][SQL] Disable WholeStageCodegen when set `spark.sql.code…
74fa07c 
…gen.factoryMode` to NO_CODEGEN

### What changes were proposed in this pull request?
After #41467 , we fix the `CodegenInterpretedPlanTest ` will execute codeGen even set `spark.sql.codegen.factoryMode` to `NO_CODEGEN`. Before this PR, `spark.sql.codegen.factoryMode` can't disable WholeStageCodegen, many test case want to disable codegen by set  `spark.sql.codegen.factoryMode` to `NO_CODEGEN`, but it not work for WholeStageCodegen. So this PR change the `spark.sql.codegen.factoryMode` behavior, when set `NO_CODEGEN`, we will disable `WholeStageCodegen` too.

### Why are the changes needed?
Fix the `spark.sql.codegen.factoryMode` config behavior.

### Does this PR introduce _any_ user-facing change?
Yes, the config logic changed.

### How was this patch tested?
add new test.

Closes #41779 from Hisoka-X/SPARK-44236_wholecodegen_disable.

Authored-by: Jia Fan <fanjiaeminem@qq.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
cloud-fan pushed a commit that referenced this pull request Aug 8, 2023
@Hisoka-X @cloud-fan
[SPARK-44236][SQL] Disable WholeStageCodegen when set `spark.sql.code…
0e25219 
…gen.factoryMode` to NO_CODEGEN

### What changes were proposed in this pull request?
After #41467 , we fix the `CodegenInterpretedPlanTest ` will execute codeGen even set `spark.sql.codegen.factoryMode` to `NO_CODEGEN`. Before this PR, `spark.sql.codegen.factoryMode` can't disable WholeStageCodegen, many test case want to disable codegen by set  `spark.sql.codegen.factoryMode` to `NO_CODEGEN`, but it not work for WholeStageCodegen. So this PR change the `spark.sql.codegen.factoryMode` behavior, when set `NO_CODEGEN`, we will disable `WholeStageCodegen` too.

### Why are the changes needed?
Fix the `spark.sql.codegen.factoryMode` config behavior.

### Does this PR introduce _any_ user-facing change?
Yes, the config logic changed.

### How was this patch tested?
add new test.

Closes #41779 from Hisoka-X/SPARK-44236_wholecodegen_disable.

Authored-by: Jia Fan <fanjiaeminem@qq.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
(cherry picked from commit 74fa07c)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
valentinp17 pushed a commit to valentinp17/spark that referenced this pull request Aug 24, 2023
@Hisoka-X
[SPARK-44236][SQL] Disable WholeStageCodegen when set `spark.sql.code…
6e2f9d6 
…gen.factoryMode` to NO_CODEGEN

### What changes were proposed in this pull request?
After apache#41467 , we fix the `CodegenInterpretedPlanTest ` will execute codeGen even set `spark.sql.codegen.factoryMode` to `NO_CODEGEN`. Before this PR, `spark.sql.codegen.factoryMode` can't disable WholeStageCodegen, many test case want to disable codegen by set  `spark.sql.codegen.factoryMode` to `NO_CODEGEN`, but it not work for WholeStageCodegen. So this PR change the `spark.sql.codegen.factoryMode` behavior, when set `NO_CODEGEN`, we will disable `WholeStageCodegen` too.

### Why are the changes needed?
Fix the `spark.sql.codegen.factoryMode` config behavior.

### Does this PR introduce _any_ user-facing change?
Yes, the config logic changed.

### How was this patch tested?
add new test.

Closes apache#41779 from Hisoka-X/SPARK-44236_wholecodegen_disable.

Authored-by: Jia Fan <fanjiaeminem@qq.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
ragnarok56 pushed a commit to ragnarok56/spark that referenced this pull request Mar 2, 2024
@Hisoka-X @ragnarok56
[SPARK-44236][SQL] Disable WholeStageCodegen when set `spark.sql.code…
ab3de81 
…gen.factoryMode` to NO_CODEGEN

### What changes were proposed in this pull request?
After apache#41467 , we fix the `CodegenInterpretedPlanTest ` will execute codeGen even set `spark.sql.codegen.factoryMode` to `NO_CODEGEN`. Before this PR, `spark.sql.codegen.factoryMode` can't disable WholeStageCodegen, many test case want to disable codegen by set  `spark.sql.codegen.factoryMode` to `NO_CODEGEN`, but it not work for WholeStageCodegen. So this PR change the `spark.sql.codegen.factoryMode` behavior, when set `NO_CODEGEN`, we will disable `WholeStageCodegen` too.

### Why are the changes needed?
Fix the `spark.sql.codegen.factoryMode` config behavior.

### Does this PR introduce _any_ user-facing change?
Yes, the config logic changed.

### How was this patch tested?
add new test.

Closes apache#41779 from Hisoka-X/SPARK-44236_wholecodegen_disable.

Authored-by: Jia Fan <fanjiaeminem@qq.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxGekk MaxGekk MaxGekk approved these changes

Assignees
No one assigned
Labels
SQL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Hisoka-X @MaxGekk