[SPARK-22219][SQL] Refactor code to get a value for "spark.sql.codegen.comments" #19449

kiszk · 2017-10-06T17:59:42Z

What changes were proposed in this pull request?

This PR refactors code to get a value for "spark.sql.codegen.comments" by avoiding SparkEnv.get.conf. This PR uses SQLConf.get.codegenComments since SQLConf.get always returns an instance of SQLConf.

How was this patch tested?

Added test case to DebuggingSuite

SparkQA · 2017-10-06T20:43:00Z

Test build #82520 has finished for PR 19449 at commit be4220d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tejasapatil · 2017-10-06T20:52:08Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

@@ -929,7 +929,7 @@ class CodegenContext {
    // be extremely expensive in certain cases, such as deeply-nested expressions which operate over
    // inputs with wide schemas. For more details on the performance issues that motivated this
    // flat, see SPARK-15680.
-    if (SparkEnv.get != null && SparkEnv.get.conf.getBoolean("spark.sql.codegen.comments", false)) {


note to other reviewers: for historical context why this was done, see https://github.com/apache/spark/pull/13421/files#r65268674

SQLConf is based on the active spark session. The active spark session is not always correctly set. Thus, we do not encourage the community to use SQLConf.

See the PR: #18568

There are places in codebase which do SQLConf.get (esp. CodeGenerator.scala which is being modified in this PR). Are you suggesting to change that everywhere like #18568 ? I think it will be good thing to do.

Thank you for your comments. Is it better to pass SQLConf to the constructor of CodegenContext?

I am afraid it might require a lot of code changes if we add SQLConf to CodegenContext.

@tejasapatil Maybe just do nothing now. We need to fix the bugs in active session management first.

~~Good to hear that~~
~~@tejasapatil Could you please let us know when it is available as a PR?~~
I misunderstood @gatorsmile 's comment. I understood there is no activity to fix active session management.

So far, I do not have a bandwidth to fix it. If anybody is interested in this, please feel free to start it. This requires a design doc at first.

tejasapatil · 2017-10-06T20:53:11Z

LGTM. There are already multiple places in codegen where SQLConf.get is being used so this will make things consistent

viirya · 2017-10-07T08:31:32Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

@@ -929,7 +929,7 @@ class CodegenContext {
    // be extremely expensive in certain cases, such as deeply-nested expressions which operate over
    // inputs with wide schemas. For more details on the performance issues that motivated this
    // flat, see SPARK-15680.


We should move these comments to the SQLConf description.

kiszk · 2018-05-18T02:37:20Z

When #21299 will be merged, I think that we can revisit this PR.

SparkQA · 2018-05-18T03:01:05Z

Test build #90760 has finished for PR 19449 at commit be4220d.

This patch fails PySpark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-07-16T03:25:49Z

Hm, shall we leave this closed then?

kiszk · 2018-07-16T07:11:16Z

Sorry for leaving this for a while. I will update it using StaticSQLConf soon.

SparkQA · 2018-07-16T20:22:25Z

Test build #93118 has finished for PR 19449 at commit a198901.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2018-07-16T21:41:57Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

@@ -751,6 +751,12 @@ object SQLConf {
    .booleanConf
    .createWithDefault(true)

+  val MAX_CASES_BRANCHES = buildConf("spark.sql.codegen.maxCaseBranches")


Oh..., I will remove this

SparkQA · 2018-07-17T04:15:23Z

Test build #93148 has finished for PR 19449 at commit 8680026.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-07-17T06:20:34Z

retest this please

SparkQA · 2018-07-17T07:05:01Z

Test build #93156 has finished for PR 19449 at commit 8680026.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-07-17T07:25:18Z

retest this please

SparkQA · 2018-07-17T11:23:14Z

Test build #93159 has finished for PR 19449 at commit 8680026.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-07-22T05:53:23Z

cc @gatorsmile

kiszk · 2018-07-30T02:28:14Z

cc @gatorsmile

HyukjinKwon · 2018-07-30T04:42:39Z

Seems okay to me.

srowen · 2018-07-31T18:30:05Z

@kiszk looks OK except there's an old comment about moving the comments to the new member you've extracted.

SparkQA · 2018-07-31T21:17:39Z

Test build #93841 has finished for PR 19449 at commit afe889d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-07-31T23:36:43Z

Test build #4225 has finished for PR 19449 at commit afe889d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-08-01T00:31:16Z

retest this please

SparkQA · 2018-08-01T04:30:29Z

Test build #93852 has finished for PR 19449 at commit afe889d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2018-08-01T05:41:23Z

sql/core/src/test/scala/org/apache/spark/sql/internal/ExecutorSideSQLConfSuite.scala

+      assert(res.length == 2)
+      assert(res.forall{ case (_, code) =>
+        code.contains("* Codegend pipeline") && code.contains("// input[")})
+    }


combine these two?

Seq(true, false).foreach { flag => ... if (flag) { ... } else { ... } }

SparkQA · 2018-08-01T20:54:20Z

Test build #93895 has finished for PR 19449 at commit 253bc19.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2018-08-01T21:06:21Z

LGTM pending Jenkins.

SparkQA · 2018-08-02T01:06:03Z

Test build #4229 has finished for PR 19449 at commit 253bc19.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-08-02T01:39:15Z

retest this please

SparkQA · 2018-08-02T03:39:35Z

Test build #93923 has finished for PR 19449 at commit 253bc19.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2018-08-02T03:48:14Z

retest this please

gatorsmile · 2018-08-02T05:16:03Z

ok to test

SparkQA · 2018-08-02T07:05:02Z

Test build #93953 has finished for PR 19449 at commit 253bc19.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-08-02T08:52:39Z

retest this please

SparkQA · 2018-08-02T12:26:32Z

Test build #93982 has finished for PR 19449 at commit 253bc19.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-08-02T14:17:04Z

retest this please

SparkQA · 2018-08-02T17:38:01Z

Test build #94014 has finished for PR 19449 at commit 253bc19.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-08-02T18:51:51Z

retest this please

srowen · 2018-08-02T19:04:04Z

I think we have a problem in the kafka tests right now that will cause this to fail. Seems to be the cause several times now

SparkQA · 2018-08-02T23:01:47Z

Test build #94049 has finished for PR 19449 at commit 253bc19.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2018-08-02T23:19:42Z

Merged to master

tejasapatil reviewed Oct 6, 2017

View reviewed changes

viirya reviewed Oct 7, 2017

View reviewed changes

This was referenced Oct 9, 2017

[SPARK-22215][SQL] Add configuration to set the threshold for generated class #19447

Closed

[SPARK-21751][SQL] CodeGeneraor.splitExpressions counts code size more precisely #18966

Closed

kiszk mentioned this pull request Nov 10, 2017

[SPARK-21720][SQL] Fix 64KB JVM bytecode limit problem with AND or OR #18972

Closed

kiszk mentioned this pull request Dec 13, 2017

[SPARK-18016][SQL] Code Generation: Constant Pool Limit - reduce entries for mutable state #19811

Closed

mgaido91 mentioned this pull request Dec 20, 2017

[SPARK-22036][SQL] Decimal multiplication with high precision/scale often returns NULL #20023

Closed

kiszk mentioned this pull request Feb 13, 2018

[SPARK-23032][SQL][FOLLOW-UP]Add codegenStageId in comment #20419

Closed

Initial commit

9c26e2e

use StaticSQLConf

a198901

kiszk force-pushed the SPARK-22219 branch from be4220d to a198901 Compare July 16, 2018 16:27

gatorsmile reviewed Jul 16, 2018

View reviewed changes

address review comment

8680026

address review comments

afe889d

srowen approved these changes Jul 31, 2018

View reviewed changes

gatorsmile reviewed Aug 1, 2018

View reviewed changes

address review comment

253bc19

asfgit closed this in bbdcc3b Aug 2, 2018

[SPARK-22219][SQL] Refactor code to get a value for "spark.sql.codegen.comments" #19449

[SPARK-22219][SQL] Refactor code to get a value for "spark.sql.codegen.comments" #19449

Conversation

kiszk commented Oct 6, 2017

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatorsmile Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

kiszk Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejasapatil commented Oct 6, 2017

Choose a reason for hiding this comment

kiszk commented May 18, 2018

SparkQA commented May 18, 2018

HyukjinKwon commented Jul 16, 2018

kiszk commented Jul 16, 2018

SparkQA commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 17, 2018

kiszk commented Jul 17, 2018

SparkQA commented Jul 17, 2018

kiszk commented Jul 17, 2018

SparkQA commented Jul 17, 2018

kiszk commented Jul 22, 2018

kiszk commented Jul 30, 2018

HyukjinKwon commented Jul 30, 2018

srowen commented Jul 31, 2018

SparkQA commented Jul 31, 2018

SparkQA commented Jul 31, 2018

kiszk commented Aug 1, 2018

SparkQA commented Aug 1, 2018

Choose a reason for hiding this comment

SparkQA commented Aug 1, 2018

gatorsmile commented Aug 1, 2018

SparkQA commented Aug 2, 2018

HyukjinKwon commented Aug 2, 2018

SparkQA commented Aug 2, 2018

gatorsmile commented Aug 2, 2018

gatorsmile commented Aug 2, 2018

SparkQA commented Aug 2, 2018

HyukjinKwon commented Aug 2, 2018

SparkQA commented Aug 2, 2018

HyukjinKwon commented Aug 2, 2018

SparkQA commented Aug 2, 2018

kiszk commented Aug 2, 2018

srowen commented Aug 2, 2018

SparkQA commented Aug 2, 2018

srowen commented Aug 2, 2018

gatorsmile Oct 8, 2017 •

edited

Loading

kiszk Oct 8, 2017 •

edited

Loading