[SPARK-26262][SQL] Runs SQLQueryTestSuite on mixed config sets: WHOLESTAGE_CODEGEN_ENABLED and CODEGEN_FACTORY_MODE #23213

maropu · 2018-12-04T04:35:32Z

What changes were proposed in this pull request?

For better test coverage, this pr proposed to use the 4 mixed config sets of WHOLESTAGE_CODEGEN_ENABLED and CODEGEN_FACTORY_MODE when running SQLQueryTestSuite:

WHOLESTAGE_CODEGEN_ENABLED=true, CODEGEN_FACTORY_MODE=CODEGEN_ONLY
WHOLESTAGE_CODEGEN_ENABLED=false, CODEGEN_FACTORY_MODE=CODEGEN_ONLY
WHOLESTAGE_CODEGEN_ENABLED=true, CODEGEN_FACTORY_MODE=NO_CODEGEN
WHOLESTAGE_CODEGEN_ENABLED=false, CODEGEN_FACTORY_MODE=NO_CODEGEN

This pr also moved some existing tests into ExplainSuite because explain output results are different between codegen and interpreter modes.

How was this patch tested?

Existing tests.

SparkQA · 2018-12-04T07:52:49Z

Test build #99646 has finished for PR 23213 at commit 2ced0ca.

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

maropu · 2018-12-04T11:02:08Z

cc: @cloud-fan @mgaido91

cloud-fan · 2018-12-04T11:32:12Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+    }
+  }
+
+  test("optimized plan should show the rewritten aggregate expression") {


can we move them to ExplainSuite?

all the tests?

all the explain related tests.

Please update the PR description too.

+1 for @viirya 's comment. We need to update the title and description of PR and JIRA.

updated! Thanks, guys!

mgaido91 · 2018-12-04T11:32:44Z

just a question, why didn't we introduce something like what was done in SPARK-24562? I see that these are configs which are valid for all queries, so using what was done in SPARK-24562 is not a good idea, but something similar (eg a file with all the config sets to use)?

cloud-fan · 2018-12-04T11:33:16Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

-    val codegenConfigSets = Array(CODEGEN_ONLY, NO_CODEGEN).map {
-      case codegenFactoryMode =>
-        Array(SQLConf.CODEGEN_FACTORY_MODE.key -> codegenFactoryMode.toString)
+    val codegenConfigSets = Array(("false", "NO_CODEGEN"), ("true", "CODEGEN_ONLY")).map {


shall we test all the combinations? e.g. wholeStage=on, codegen=off

will this increase too much test time?

I will check the time later, too.

maropu · 2018-12-04T11:49:36Z

yea, its similar, but I personally think its orthogonal to SPARK-24562. This pr only targets a default config set for codegen-only and interpreter mode tests.

mgaido91 · 2018-12-04T11:54:06Z

I personally think its orthogonal to SPARK-24562.

yes I agree. I am just asking if it makes sense to create a framework like that. Now it is only about codegen, but in the future we may want to add more configs. What do you think?

cloud-fan · 2018-12-04T12:11:09Z

We should create such a framework when we need to have per-file config settings for testing.

SparkQA · 2018-12-04T15:08:38Z

Test build #99661 has finished for PR 23213 at commit 3ef5e3e.

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

SparkQA · 2018-12-04T15:19:00Z

Test build #99663 has finished for PR 23213 at commit 57eec69.

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

SparkQA · 2018-12-04T15:20:30Z

Test build #99662 has finished for PR 23213 at commit 0305a05.

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

dongjoon-hyun · 2018-12-04T19:23:29Z

sql/core/src/test/scala/org/apache/spark/sql/ExplainSuite.scala

+      // plan should show the rewritten aggregate expression.
+      val df = sql("SELECT k, every(v), some(v), any(v) FROM test_agg GROUP BY k")
+      checkKeywordsExistsInExplain(df,
+        "Aggregate [k#x], [k#x, min(v#x) AS every(v)#x, max(v#x) AS some(v)#x, " +


Since extended=false in line 33, the test suite only compares with Physical Plan. Maybe, did you change line 33 in your codebase?

The other two failures fail with the same reason.

yea, you're right... I forgot to set true at extended in explain...

SparkQA · 2018-12-05T05:40:40Z

Test build #99692 has finished for PR 23213 at commit 808af50.

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

cloud-fan · 2018-12-05T07:04:16Z

do you know how long SQLQueryTestSuite takes? We are making it longer by 4 times here, so better to know the overhead.

maropu · 2018-12-05T07:13:29Z

I'm looking into that now ;) Just give me more time to check.

maropu · 2018-12-05T07:28:45Z

yea, it seemed it was longer by ~4 times;

23:25:43.880 WARN org.apache.spark.sql.SQLQueryTestSuite: 
=== Codegen/Interpreter Time Metrics ===
Total time: 602.64531157 seconds

Configs                                                                       Run Time                             

spark.sql.codegen.wholeStage=true,spark.sql.codegen.factoryMode=NO_CODEGEN    156414789416                                   
spark.sql.codegen.wholeStage=false,spark.sql.codegen.factoryMode=CODEGEN_ONLY 138343055840                                   
spark.sql.codegen.wholeStage=true,spark.sql.codegen.factoryMode=CODEGEN_ONLY  171905020550                                   
spark.sql.codegen.wholeStage=false,spark.sql.codegen.factoryMode=NO_CODEGEN   135982445764

7a69e0b

cloud-fan · 2018-12-05T08:18:07Z

that's a lot of time...

Can we think more about the combination of codegen and wholeStage? When we turn on whole stage codegen but turn off codegen, what will happen?

maropu · 2018-12-05T10:11:37Z

Sorry, my bad; it was longer than the current master by ~2 times. That's because the current master has already run two config set patterns (wholeStage=true,factoryMode=CODEGEN_ONLY and wholeStage=true,factoryMode=NO_CODEGEN) in SQLQueryTestSuite. The second test run (wholeStage=true,factoryMode=NO_CODEGEN) was introduced in my previous pr (#22512).

IMHO two config set patterns below could cover most code paths in Spark?

wholeStage=true, factoryMode=CODEGEN_ONLY
wholeStage=false, factoryMode=NO_CODEGEN

In this case, there is little change in the test time;

// the current master
=== Codegen/Interpreter Time Metrics ===
Total time: 358.584989321 seconds

Configs                                                                      Run Time                             
spark.sql.codegen.wholeStage=true,spark.sql.codegen.factoryMode=NO_CODEGEN   165961038511                                   
spark.sql.codegen.wholeStage=true,spark.sql.codegen.factoryMode=CODEGEN_ONLY 192623950810  

// with this pr
=== Codegen/Interpreter Time Metrics ===
Total time: 345.468455247 seconds

Configs                                                                      Run Time                             
spark.sql.codegen.wholeStage=false,spark.sql.codegen.factoryMode=NO_CODEGEN  148895478870    
spark.sql.codegen.wholeStage=true,spark.sql.codegen.factoryMode=CODEGEN_ONLY 196572976377

WDYT?

mgaido91 · 2018-12-05T10:23:01Z

Yes, I am wondering too: which is the difference between:
spark.sql.codegen.wholeStage=false,spark.sql.codegen.factoryMode=NO_CODEGEN and spark.sql.codegen.wholeStage=true,spark.sql.codegen.factoryMode=NO_CODEGEN?

cloud-fan · 2018-12-05T11:14:43Z

how about wholeStage=false, factoryMode=CODE_ONLY? I think it's different from wholeStage=false, factoryMode=NO_CODEGEN.

maropu · 2018-12-05T13:57:36Z

yea, I think they're not totally the same..., but I'm not sure that the test run (wholeStage=false, factoryMode=CODE_ONLY) is worth the time cost.

cloud-fan · 2018-12-05T14:34:58Z

But whole stage codegen will not test GenerateUnsafeProject, GenerateMutableProject, etc., right?

cloud-fan · 2018-12-05T14:36:33Z

If we look at test coverage, wholeStage=false, factoryMode=CODE_ONLY will go through code paths that wholeStageCodegen doesn't cover. Or did I miss something?

SparkQA · 2018-12-06T05:55:01Z

Test build #99750 has finished for PR 23213 at commit a9c108f.

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

maropu · 2018-12-06T06:09:02Z

retest this please

SparkQA · 2018-12-06T08:05:02Z

Test build #99756 has finished for PR 23213 at commit a9c108f.

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

dongjoon-hyun · 2018-12-06T08:23:05Z

Retest this please.

mgaido91 · 2018-12-06T10:26:41Z

@maropu I'd say so, but I am still not sure what (if there is one) is the difference between wholeStage=false,sactoryMode=NO_CODEGEN and wholeStage=true,factoryMode=NO_CODEGEN. wholeStage=true,factoryMode=NO_CODEGEN doesn't make much sense IMHO. Could you please check what that runs?

SparkQA · 2018-12-06T12:01:25Z

Test build #99762 has finished for PR 23213 at commit a9c108f.

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

cloud-fan · 2018-12-06T13:30:34Z

these 3 combinations LGTM.

SparkQA · 2018-12-10T23:21:16Z

Test build #99933 has finished for PR 23213 at commit a9c108f.

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

cloud-fan · 2018-12-11T02:17:39Z

retest this please

SparkQA · 2018-12-11T04:05:40Z

Test build #99942 has finished for PR 23213 at commit a9c108f.

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

dongjoon-hyun · 2018-12-11T04:31:41Z

Retest this please.

HyukjinKwon · 2018-12-11T06:32:15Z

Ah, I had the same question as #23213 (comment). It would be good to update PR description :-).

cloud-fan · 2018-12-11T06:39:01Z

when wholeStageCogen is on, there is no way to avoid codegen, so codegenFactoryMode doesn't make difference.

viirya · 2018-12-11T07:51:20Z

I think wholeStageCodegen doesn't disallow using those objects in interpreted mode. The objects can be in interpreted mode if it rolls back from codegen in case of compilation error.

SparkQA · 2018-12-11T08:05:02Z

Test build #99945 has finished for PR 23213 at commit a9c108f.

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

HyukjinKwon · 2018-12-11T08:11:45Z

retest this please

cloud-fan · 2018-12-11T08:24:01Z

I think wholeStageCodegen doesn't disallow using those objects in interpreted mode. The objects can be in interpreted mode if it rolls back from codegen in case of compilation error.

But the test coverage is the same, right? If wholeStageCodege falls back, we are testing the same thing as turning off wholeStageCodegen

viirya · 2018-12-11T08:33:10Z

Isn't it possibly that wholeStageCodege doesn't falls back but codegen.factoryMode falls back, and vice verse? The falling back of factoryMode is happened at individual codegen generator.

cloud-fan · 2018-12-11T08:39:23Z

The definition of "fallback" is different in wholeStageCodegen. It's effectively turning off wholeStageCodegen. Depending on the factoryMode, it might further fallbacks to interpreted execution, or normal codegen.

mgaido91 · 2018-12-11T09:50:28Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+    // note: this is not a robust way to split queries using semicolon, but works for now.
+    val queries = code.mkString("\n").split("(?<=[^\\\\]);").map(_.trim).filter(_ != "").toSeq
+
+    // When we are regenerating the golden files, we don't need to set any config as they


I am not sure about this. Imagine a case which is failing without setting a config for instance: I think we should pick one config set (any is fine) instead.

Imagine a case which is failing without setting a config for instance

We will check the result for all different configs, and test will fail, right?

We will check the result for all different configs

For all different configs specified there doesn't mean without setting that config at all. For instance, let's imagine that we add a config for throwing an exception when a decimal overflows (with this behavior being the default): in the current tests, we can just add the config value to "do not throw an exception" for all the config sets which are present there. After this patch, that wouldn't work.

Currently this test framework requires all the cases return the same result(without config and with any combination of configs). Did I miss something?

I think only with any combination of configs is true, not without config.

Do you know which test case leverages this feature? I'm a little surprised that we allow tests without configs have different behavior.

no, I think there is none as of now (this feature is not widely adopted...), but I think it is a case which can happen in the future (as in the example above, which may really be a case if we go on with the creating of a SQL strict mode)

If we do need to test the SQL strict mode, I think we need to adopt SPARK-24562 then.

I mean in general, in the current condition, there is no way you can successfully run the tests if the default value of a config produces (for any reason) an output different from some other values and we want to test only the non-default values. It may not be a problem but I think anyway this is a limitation which we can easily avoid by adding here the SETs for the first config set if any. So I see no reason why not to do that, which is safer IMHO.

I think it's easier to reason about the test if we require it to always produce the same result without config and with any combination of configs.

If no tests depend on it, I'd like to not bother about it and keep it as it was. We already clear configs with generating result: https://github.com/apache/spark/pull/23213/files#diff-432455394ca50800d5de508861984ca5L157

mgaido91 · 2018-12-11T09:51:21Z

Thanks for the kind answer @cloud-fan. Then yes, I am +1 for the 3 configs suggested in #23213 (comment).

SparkQA · 2018-12-11T11:58:16Z

Test build #99959 has finished for PR 23213 at commit a9c108f.

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

SparkQA · 2018-12-11T11:58:50Z

Test build #99957 has finished for PR 23213 at commit a9c108f.

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

kiszk · 2018-12-19T18:04:30Z

LGTM with the 3 configs in #23213 (comment).

kiszk · 2018-12-19T18:04:37Z

retest this please

SparkQA · 2018-12-19T22:03:19Z

Test build #100313 has finished for PR 23213 at commit a9c108f.

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

cloud-fan · 2018-12-20T02:42:18Z

thanks, merging to master!

…STAGE_CODEGEN_ENABLED and CODEGEN_FACTORY_MODE ## What changes were proposed in this pull request? For better test coverage, this pr proposed to use the 4 mixed config sets of `WHOLESTAGE_CODEGEN_ENABLED` and `CODEGEN_FACTORY_MODE` when running `SQLQueryTestSuite`: 1. WHOLESTAGE_CODEGEN_ENABLED=true, CODEGEN_FACTORY_MODE=CODEGEN_ONLY 2. WHOLESTAGE_CODEGEN_ENABLED=false, CODEGEN_FACTORY_MODE=CODEGEN_ONLY 3. WHOLESTAGE_CODEGEN_ENABLED=true, CODEGEN_FACTORY_MODE=NO_CODEGEN 4. WHOLESTAGE_CODEGEN_ENABLED=false, CODEGEN_FACTORY_MODE=NO_CODEGEN This pr also moved some existing tests into `ExplainSuite` because explain output results are different between codegen and interpreter modes. ## How was this patch tested? Existing tests. Closes apache#23213 from maropu/InterpreterModeTest. Authored-by: Takeshi Yamamuro <yamamuro@apache.org> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan reviewed Dec 4, 2018

View reviewed changes

maropu force-pushed the InterpreterModeTest branch from 3ef5e3e to 0305a05 Compare December 4, 2018 12:12

cloud-fan approved these changes Dec 4, 2018

View reviewed changes

dongjoon-hyun reviewed Dec 4, 2018

View reviewed changes

maropu changed the title ~~[SPARK-26262][SQL] Run SQLQueryTestSuite with WHOLESTAGE_CODEGEN_ENABLED=false~~ [SPARK-26262][SQL] Runs SQLQueryTestSuite on mixed config sets: WHOLESTAGE_CODEGEN_ENABLED and CODEGEN_FACTORY_MODE Dec 5, 2018

maropu added 4 commits December 5, 2018 13:38

Fix

7c611fd

Fix

e2a38c7

Update comment

7502793

Fix

ec857b6

mgaido91 reviewed Dec 11, 2018

View reviewed changes

asfgit closed this in 61c443a Dec 20, 2018

[SPARK-26262][SQL] Runs SQLQueryTestSuite on mixed config sets: WHOLESTAGE_CODEGEN_ENABLED and CODEGEN_FACTORY_MODE #23213

[SPARK-26262][SQL] Runs SQLQueryTestSuite on mixed config sets: WHOLESTAGE_CODEGEN_ENABLED and CODEGEN_FACTORY_MODE #23213

Conversation

maropu commented Dec 4, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Dec 4, 2018

maropu commented Dec 4, 2018

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

Choose a reason for hiding this comment

mgaido91 commented Dec 4, 2018

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

maropu commented Dec 4, 2018

mgaido91 commented Dec 4, 2018

cloud-fan commented Dec 4, 2018

SparkQA commented Dec 4, 2018

SparkQA commented Dec 4, 2018

SparkQA commented Dec 4, 2018

Choose a reason for hiding this comment

dongjoon-hyun Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

maropu Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Dec 5, 2018

cloud-fan commented Dec 5, 2018

maropu commented Dec 5, 2018

maropu commented Dec 5, 2018 • edited Loading

cloud-fan commented Dec 5, 2018

maropu commented Dec 5, 2018 • edited Loading

mgaido91 commented Dec 5, 2018

cloud-fan commented Dec 5, 2018

maropu commented Dec 5, 2018

cloud-fan commented Dec 5, 2018

cloud-fan commented Dec 5, 2018

SparkQA commented Dec 6, 2018

maropu commented Dec 6, 2018

SparkQA commented Dec 6, 2018

dongjoon-hyun commented Dec 6, 2018

mgaido91 commented Dec 6, 2018

SparkQA commented Dec 6, 2018

cloud-fan commented Dec 6, 2018

SparkQA commented Dec 10, 2018

cloud-fan commented Dec 11, 2018

SparkQA commented Dec 11, 2018

dongjoon-hyun commented Dec 11, 2018

HyukjinKwon commented Dec 11, 2018 • edited Loading

cloud-fan commented Dec 11, 2018

viirya commented Dec 11, 2018

SparkQA commented Dec 11, 2018

HyukjinKwon commented Dec 11, 2018

cloud-fan commented Dec 11, 2018

viirya commented Dec 11, 2018

cloud-fan commented Dec 11, 2018

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

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

mgaido91 commented Dec 11, 2018

SparkQA commented Dec 11, 2018

SparkQA commented Dec 11, 2018

kiszk commented Dec 19, 2018

kiszk commented Dec 19, 2018

SparkQA commented Dec 19, 2018

cloud-fan commented Dec 20, 2018

maropu commented Dec 4, 2018 •

edited

Loading

dongjoon-hyun Dec 4, 2018 •

edited

Loading

maropu Dec 5, 2018 •

edited

Loading

maropu commented Dec 5, 2018 •

edited

Loading

maropu commented Dec 5, 2018 •

edited

Loading

HyukjinKwon commented Dec 11, 2018 •

edited

Loading