-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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.
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
[SPARK-44236][SQL] Disable WholeStageCodegen when set spark.sql.codegen.factoryMode
to NO_CODEGEN
#41779
Conversation
…gen.factoryMode` to NO_CODEGEN
cc @MaxGekk |
@@ -47,8 +47,7 @@ trait CodegenInterpretedPlanTest extends PlanTest { | |||
super.test(testName + " (codegen path)", testTags: _*)( | |||
withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenMode) { testFun })(pos) | |||
super.test(testName + " (interpreted path)", testTags: _*)( | |||
withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> interpretedMode) { | |||
withSQLConf(SQLConf.WHOLESTAGE_CODEGEN_ENABLED.key -> "false") { testFun }})(pos) | |||
withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> interpretedMode) {{ testFun }})(pos) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't get why you reverted #41467 , and why do you need double curly brackets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't get why you reverted #41467
If when we support set spark.sql.codegen.factoryMode
to NO_CODEGEN
then disable WholeStageCodegen in this PR(just like set WHOLESTAGE_CODEGEN_ENABLED to false). The change seem unnecessary. This idea come from I collect other codegen config mistakes. The NO_CODEGEN
behavior not right at now.
why do you need double curly brackets.
My mistake, I will fix it. Thanks
@@ -942,7 +942,8 @@ case class CollapseCodegenStages( | |||
} | |||
|
|||
def apply(plan: SparkPlan): SparkPlan = { | |||
if (conf.wholeStageEnabled) { | |||
if (conf.wholeStageEnabled && CodegenObjectFactoryMode.withName(conf.codegenFactoryMode) | |||
!= CodegenObjectFactoryMode.NO_CODEGEN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @cloud-fan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cloud-fan Could you look at this, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @cloud-fan Can you take a look this, thanks.
this config is mostly for testing and experimenting, so better to make it easy to use (no need to set whole stage codegen conf if we don't want codegen) |
…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>
Thanks @MaxGekk @cloud-fan |
…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>
…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>
What changes were proposed in this pull request?
After #41467 , we fix the
CodegenInterpretedPlanTest
will execute codeGen even setspark.sql.codegen.factoryMode
toNO_CODEGEN
. Before this PR,spark.sql.codegen.factoryMode
can't disable WholeStageCodegen, many test case want to disable codegen by setspark.sql.codegen.factoryMode
toNO_CODEGEN
, but it not work for WholeStageCodegen. So this PR change thespark.sql.codegen.factoryMode
behavior, when setNO_CODEGEN
, we will disableWholeStageCodegen
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.