Skip to content

Comments

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

Closed
Hisoka-X wants to merge 1 commit intoapache:masterfrom
Hisoka-X:SPARK-40850_Intrepetered_Queries_execute_Codegen
Closed

[SPARK-40850][SQL] Fix test case interpreted queries may execute Codegen#41467
Hisoka-X wants to merge 1 commit intoapache:masterfrom
Hisoka-X:SPARK-40850_Intrepetered_Queries_execute_Codegen

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:

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:

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
cloud-fan pushed a commit that referenced this pull request Aug 8, 2023
…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
…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
…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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants