[SPARK-25267][SQL][TEST] Disable ConvertToLocalRelation in the test cases of sql/core and sql/hive #22270

dilipbiswal · 2018-08-29T18:10:18Z

What changes were proposed in this pull request?

In SharedSparkSession and TestHive, we need to disable the rule ConvertToLocalRelation for better test case coverage.

How was this patch tested?

Identify the failures after excluding "ConvertToLocalRelation" rule.

SparkQA · 2018-08-29T19:00:49Z

Test build #95432 has finished for PR 22270 at commit bd57944.

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

dilipbiswal · 2018-08-29T20:36:31Z

retest this please

SparkQA · 2018-08-29T21:22:56Z

Test build #95435 has finished for PR 22270 at commit bd57944.

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

SparkQA · 2018-08-30T07:00:14Z

Test build #95457 has finished for PR 22270 at commit b83fa29.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-08-30T07:06:35Z

mllib/src/test/scala/org/apache/spark/ml/recommendation/ALSSuite.scala

 import com.github.fommil.netlib.BLAS.{getInstance => blas}
 import org.apache.commons.io.FileUtils
 import org.apache.commons.io.filefilter.TrueFileFilter
 import org.scalatest.BeforeAndAfterEach
-


Revert the blank lines?

viirya · 2018-08-30T08:09:25Z

mllib/src/test/scala/org/apache/spark/ml/recommendation/ALSSuite.scala

-    withClue("transform should fail when ids exceed integer range. ") {
-      val model = als.fit(df)
-      def testTransformIdExceedsIntRange[A : Encoder](dataFrame: DataFrame): Unit = {
+    withSQLConf(SQLConf.OPTIMIZER_EXCLUDED_RULES.key -> "") {


If we only want to disable ConvertToLocalRelation in sql/core and sql/hive, maybe we can set this sql conf at MLTest?

I don't see any usage of withSQLConf in ml tests. It looks a bit weird to see it here.

@viirya Thanks !! Actually currently i am just making changes to move forward with testing to identify the failures. I will open separate pr for code/test fix. So lets discuss the right way to fix the problem there ? I agree with your suggestion here :-)

viirya · 2018-08-30T08:10:09Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/test/TestHive.scala

@@ -59,7 +60,8 @@ object TestHive
        .set("spark.sql.warehouse.dir", TestHiveContext.makeWarehouseDir().toURI.getPath)
        // SPARK-8910
        .set("spark.ui.enabled", "false")
-        .set("spark.unsafe.exceptionOnMemoryLeak", "true")))
+        .set("spark.unsafe.exceptionOnMemoryLeak", "true")
+        .set(SQLConf.OPTIMIZER_EXCLUDED_RULES.key, ConvertToLocalRelation.ruleName)))


Can we add a comment for this?

@viirya Sure.. i will add.

SparkQA · 2018-08-30T10:59:13Z

Test build #95458 has finished for PR 22270 at commit 877ad96.

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

SparkQA · 2018-08-30T21:08:03Z

Test build #95484 has finished for PR 22270 at commit 7953ebd.

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

SparkQA · 2018-08-31T05:48:45Z

Test build #95516 has finished for PR 22270 at commit 53f4984.

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

SparkQA · 2018-08-31T12:14:45Z

Test build #95529 has finished for PR 22270 at commit 78cce41.

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

maropu · 2018-08-31T12:35:40Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala

      s"""
      for (int $i = 0; $i < $arr.numElements(); $i ++) {
        if ($arr.isNullAt($i)) {
-          ${ev.isNull} = true;
+          ${setIsNullCode}


nit: ${setIsNullCode} -> $setIsNullCode
btw, you found some bugs when excluding the ConvertToLocalRelation rule in tests, right?

Please open separate JIRAs and PRs for the codegen fixes.

@gatorsmile OK.. Sean.

maropu · 2018-08-31T12:35:48Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala

        } else if (${ctx.genEqual(right.dataType, value, getValue)}) {
-          ${ev.isNull} = false;
+          ${unsetIsNullCode}


@maropu will change

maropu · 2018-08-31T12:41:39Z

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

@@ -1730,9 +1730,8 @@ class DataFrameSuite extends QueryTest with SharedSQLContext {

  test("SPARK-9083: sort with non-deterministic expressions") {
    import org.apache.spark.util.random.XORShiftRandom


Can we move this import into the top? (this is not related to this pr though)

@maropu will do.

maropu · 2018-08-31T12:47:11Z

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

@@ -85,12 +85,12 @@ class DataFrameFunctionsSuite extends QueryTest with SharedSQLContext {
    }

    val df5 = Seq((Seq("a", null), Seq(1, 2))).toDF("k", "v")
-    intercept[RuntimeException] {
+    intercept[Exception] {
      df5.select(map_from_arrays($"k", $"v")).collect


What's the concrete exception this query throws?

@maropu I will double check and get back. But i think it was SparkException.

@maropu We get a SparkException here which in turn wraps a RuntimeException. When we have ConvertToLocalRelation active, we get a RuntimeException from driver. But when we disable it, the error is raised from the executor with a SparkException as the top level exception. Please let me know if my understanding is not correct.

maropu · 2018-08-31T13:24:36Z

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala

@@ -1464,12 +1465,14 @@ case class ArrayContains(left: Expression, right: Expression)
    nullSafeCodeGen(ctx, ev, (arr, value) => {
      val i = ctx.freshName("i")
      val getValue = CodeGenerator.getValue(arr, right.dataType, i)
+      val setIsNullCode = if (nullable) s"${ev.isNull} = true;" else ""
+      val unsetIsNullCode = if (nullable) s"${ev.isNull} = false;" else ""
      s"""
      for (int $i = 0; $i < $arr.numElements(); $i ++) {
        if ($arr.isNullAt($i)) {


(This is also not related to this pr though....) when left.dataType.asInstanceOf[ArrayType].containsNull = false, I think we don't need this condition if ($arr.isNullAt($i)) {?

@maropu You are right !! I will try to optimize this in the other pr i am going to open. please check if you like it.

many thanks! ya, plz ping me to review ;)

gatorsmile · 2018-08-31T18:00:11Z

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

@@ -85,12 +85,12 @@ class DataFrameFunctionsSuite extends QueryTest with SharedSQLContext {
    }

    val df5 = Seq((Seq("a", null), Seq(1, 2))).toDF("k", "v")
-    intercept[RuntimeException] {
+    intercept[Exception] {


Let us capture the exception and compare the error messages.

@gatorsmile Thanks.. I am checking the message now.

cloud-fan · 2018-09-01T00:07:55Z

Thanks for looking into this!

I think we have to clean up our test framework later (after 2.4). We should identify the test cases that are actually testing the expressions, and run it with/without enabling the local relation optimization, to test both codegen and interpreted code paths.

Since the current test suites are a little messy, this will be a lot of work, to reorganize them. I'm looking forward to seeing us accomplish it in Spark 3.0!

dilipbiswal · 2018-09-01T00:26:28Z

@cloud-fan Thank you Wenchen. Do we want to fix the two codegen compile errors in 2.4 ? One is in ArrayContains and the other is in ArraySort. I will work on re-organizing the suites to test both codegen and non codegen path for spark 3.0.

ueshin · 2018-09-02T01:02:59Z

@dilipbiswal Let's fix the two functions in 2.4. Could you open a pr to fix the two functions? Thanks!

dilipbiswal · 2018-09-02T02:35:13Z

@ueshin Thank you.. will do.

dilipbiswal · 2018-09-02T06:28:51Z

@ueshin Opened (#22314) and (#22315). Thank you.

HyukjinKwon · 2018-09-03T02:17:31Z

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

@@ -85,12 +85,12 @@ class DataFrameFunctionsSuite extends QueryTest with SharedSQLContext {
    }

    val df5 = Seq((Seq("a", null), Seq(1, 2))).toDF("k", "v")
-    intercept[RuntimeException] {
+    intercept[Exception] {


Shall we also catch specific exception per https://github.com/databricks/scala-style-guide#testing-intercepting

@HyukjinKwon We get a SparkException here which in turn wraps a RuntimeException. When we have ConvertToLocalRelation active, we get a RuntimeException from driver. But when we disable it, the error is raised from the executor with a SparkException as the top level exception. Thats the reason i changed it to intercept Exception so that this test can run both when the rule is active vs when its not.

…/core and sql/hive

gatorsmile · 2018-09-05T22:27:07Z

@dilipbiswal Any update on this PR?

dilipbiswal · 2018-09-05T23:04:30Z

@gatorsmile Yeah.. i will push the changes tonight for you to take a look.

gatorsmile · 2018-09-05T23:40:55Z

Thank you!

SparkQA · 2018-09-06T07:05:02Z

Test build #95735 has finished for PR 22270 at commit 507f89c.

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

dilipbiswal · 2018-09-06T07:06:22Z

retest this please

SparkQA · 2018-09-06T11:03:16Z

Test build #95742 has finished for PR 22270 at commit 507f89c.

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

dilipbiswal · 2018-09-07T05:50:21Z

cc @gatorsmile

gatorsmile · 2018-09-07T06:34:25Z

LGTM Thanks! Merged to master/2.4

…ases of sql/core and sql/hive ## What changes were proposed in this pull request? In SharedSparkSession and TestHive, we need to disable the rule ConvertToLocalRelation for better test case coverage. ## How was this patch tested? Identify the failures after excluding "ConvertToLocalRelation" rule. Closes #22270 from dilipbiswal/SPARK-25267-final. Authored-by: Dilip Biswal <dbiswal@us.ibm.com> Signed-off-by: gatorsmile <gatorsmile@gmail.com> (cherry picked from commit 6d7bc5a) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

dilipbiswal · 2018-09-07T06:43:21Z

Thanks a lot @gatorsmile

cloud-fan · 2018-09-07T07:28:46Z

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

    val seed = 33
-    val df = (1 to 100).map(Tuple1.apply).toDF("i")
+    val df = (1 to 100).map(Tuple1.apply).toDF("i").repartition(1)


Sorry I didn't follow this thread closely. Why do we need these repartition(1) changes?

@cloud-fan I was just trying get this test case to pass when ConvertToLocalRelation is enabled as well as disabled. So when this rule is active, i saw that all the calls to random.nextXXX happens in one thread. When this rule is disabled, the random values get evaluated under the project operator and gets called from multiple threads. Thats why i am repartitioning the data frame to enforce single threaded execution. Is this not the right thing to do ? Please let me know ..

BTW, do we still test the local relation conversion, which might be more common to users as well?

@HyukjinKwon We are leaving this optimization on for MLTest as of now. Should we open it up for TestHive and keep it disabled it for SharedSparkSession ? cc @gatorsmile

I agree with this change It's okay. Was wondering if we actually make the coverage lower for local relation specifically, or if some other tests should be added additionally.

HyukjinKwon · 2018-09-10T01:19:12Z

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

@@ -85,14 +85,16 @@ class DataFrameFunctionsSuite extends QueryTest with SharedSQLContext {
    }

    val df5 = Seq((Seq("a", null), Seq(1, 2))).toDF("k", "v")
-    intercept[RuntimeException] {
+    val msg1 = intercept[Exception] {


re: #22270 (comment)

Didn't we disable the local relation test? Why don't we catch explicit SparkExection?

@HyukjinKwon Yeah... we could have caught SparkException here. My intention was to have this test case pass both when location relation optimization is on and off. Thats why i changed it a a generic exception along with verifying the error text.

viirya reviewed Aug 30, 2018

View reviewed changes

dilipbiswal force-pushed the SPARK-25267-final branch from 877ad96 to 7953ebd Compare August 30, 2018 18:17

maropu reviewed Aug 31, 2018

View reviewed changes

gatorsmile reviewed Aug 31, 2018

View reviewed changes

HyukjinKwon reviewed Sep 3, 2018

View reviewed changes

[SPARK-25267] Disable ConvertToLocalRelation in the test cases of sql…

4619453

…/core and sql/hive

dilipbiswal added 2 commits September 5, 2018 20:15

Fix to test with non deterministic expressions

ca319f4

Fix to DataFrameFunctionsSuite

507f89c

dilipbiswal force-pushed the SPARK-25267-final branch from 78cce41 to 507f89c Compare September 6, 2018 03:18

asfgit closed this in 6d7bc5a Sep 7, 2018

cloud-fan reviewed Sep 7, 2018

View reviewed changes

HyukjinKwon reviewed Sep 10, 2018

View reviewed changes

		@@ -1730,9 +1730,8 @@ class DataFrameSuite extends QueryTest with SharedSQLContext {

		test("SPARK-9083: sort with non-deterministic expressions") {
		import org.apache.spark.util.random.XORShiftRandom

[SPARK-25267][SQL][TEST] Disable ConvertToLocalRelation in the test cases of sql/core and sql/hive #22270

[SPARK-25267][SQL][TEST] Disable ConvertToLocalRelation in the test cases of sql/core and sql/hive #22270

Conversation

dilipbiswal commented Aug 29, 2018

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Aug 29, 2018

dilipbiswal commented Aug 29, 2018

SparkQA commented Aug 29, 2018

SparkQA commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilipbiswal Aug 30, 2018 • edited Loading

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

SparkQA commented Aug 30, 2018

SparkQA commented Aug 30, 2018

SparkQA commented Aug 31, 2018

SparkQA commented Aug 31, 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

dilipbiswal Sep 6, 2018 • edited Loading

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

cloud-fan commented Sep 1, 2018

dilipbiswal commented Sep 1, 2018 • edited Loading

ueshin commented Sep 2, 2018

dilipbiswal commented Sep 2, 2018

dilipbiswal commented Sep 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatorsmile commented Sep 5, 2018

dilipbiswal commented Sep 5, 2018

gatorsmile commented Sep 5, 2018

SparkQA commented Sep 6, 2018

dilipbiswal commented Sep 6, 2018

SparkQA commented Sep 6, 2018

dilipbiswal commented Sep 7, 2018

gatorsmile commented Sep 7, 2018

dilipbiswal commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon Sep 10, 2018 • edited Loading

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

dilipbiswal Aug 30, 2018 •

edited

Loading

dilipbiswal Sep 6, 2018 •

edited

Loading

dilipbiswal commented Sep 1, 2018 •

edited

Loading

HyukjinKwon Sep 10, 2018 •

edited

Loading