[SPARK-27279][SQL] Reuse subquery should compare child plan of `SubqueryExec` #24214

adrian-wang · 2019-03-26T03:27:03Z

What changes were proposed in this pull request?

For now, ReuseSubquery in Spark compares two subqueries at SubqueryExec level, which invalidates the ReuseSubquery rule. This pull request fixes this, and add a configuration key for subquery reuse exclusively.

How was this patch tested?

add a unit test.

maropu · 2019-03-26T04:48:18Z

sql/core/src/main/scala/org/apache/spark/sql/execution/subquery.scala

      return plan
    }
    // Build a hash map using schema of subqueries to avoid O(N*N) sameResult calls.
    val subqueries = mutable.HashMap[StructType, ArrayBuffer[SubqueryExec]]()
    plan transformAllExpressions {
      case sub: ExecSubqueryExpression =>
        val sameSchema = subqueries.getOrElseUpdate(sub.plan.schema, ArrayBuffer[SubqueryExec]())
-        val sameResult = sameSchema.find(_.sameResult(sub.plan))
+        val sameResult = sameSchema.find(_.child.sameResult(sub.plan.child))


This change fixes the bug you described in the PR description? Sorry, but I'm not clearly sure about what's the problem here, so could you show us an example query to reproduce the bug? Also, could you make the title more precise...?

Thanks for the review, you can see the example in the added test.

I am not sure either about this fix. I remember this problem which I tried to fix in 36fa664. As you can see, the PR didn't actually do this fix as per @cloud-fan 's comment. So I am pinging him here for reviewing this too. Anyway, when I debugged it, the problem was that being the same object according to equals, the tree was not updated, so I see no reason for the current fix.

shall we just overwrite SubqueryExec.doCanonicalize to strip it? It seems the SubqueryExec.name is stopping us from match semantically same subqueries.

I have updated code as @cloud-fan suggests, thanks for all your reviews!

SparkQA · 2019-03-26T05:36:16Z

Test build #103941 has finished for PR 24214 at commit 5fa78c8.

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

adrian-wang · 2019-03-26T06:34:45Z

retest this please.

SparkQA · 2019-03-26T07:05:02Z

Test build #103952 has finished for PR 24214 at commit 5fa78c8.

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

adrian-wang · 2019-03-26T07:32:33Z

retest this please.

SparkQA · 2019-03-26T11:57:52Z

Test build #103961 has finished for PR 24214 at commit 5fa78c8.

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

maropu · 2019-03-26T22:37:50Z

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

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

+  val SUBQUERY_REUSE_ENABLED = buildConf("spark.sql.subquery.reuse")


Why we need this config in this pr?

It's indeed a separated thing, but I'm ok to include it in this PR, as it's obviously wrong to use exchangeReuseEnabled in ReuseSubquery

I created a PR separately to fix this problem before, #23998

10110346 · 2019-03-27T05:51:40Z

sql/core/src/main/scala/org/apache/spark/sql/execution/basicPhysicalOperators.scala

@@ -674,6 +674,8 @@ case class SubqueryExec(name: String, child: SparkPlan) extends UnaryExecNode {

  override def outputOrdering: Seq[SortOrder] = child.outputOrdering

+  override def doCanonicalize(): SparkPlan = child.canonicalized


#18169 fixed this problem like this before, but I don't know why was it reverted.

@gatorsmile may you please share if you remember why it was reverted? Thanks.

10110346 · 2019-03-27T05:52:07Z

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

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

+  val SUBQUERY_REUSE_ENABLED = buildConf("spark.sql.subquery.reuse")


I created a PR separately to fix this problem before, #23998

SparkQA · 2019-03-27T07:05:01Z

Test build #104002 has finished for PR 24214 at commit 40dc3f9.

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

SparkQA · 2019-03-27T07:05:02Z

Test build #104001 has finished for PR 24214 at commit 7177ed7.

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

cloud-fan · 2019-03-27T07:41:17Z

Hi @adrian-wang can you do a rebase? I just merged #23998

SparkQA · 2019-03-27T12:13:11Z

Test build #104006 has finished for PR 24214 at commit 6a69e05.

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

adrian-wang · 2019-03-27T15:10:26Z

@cloud-fan , rebase done and test passed.

gatorsmile

LGTM

Thanks! Merged to master.

maropu reviewed Mar 26, 2019

View reviewed changes

maropu changed the title ~~[SPARK-27279] [SQL] reuse subquery correctly~~ [SPARK-27279][SQL] Reuse subquery correctly Mar 26, 2019

adrian-wang changed the title ~~[SPARK-27279][SQL] Reuse subquery correctly~~ [SPARK-27279][SQL] Reuse subquery should compare child plan of SubqueryExec Mar 26, 2019

maropu reviewed Mar 26, 2019

View reviewed changes

10110346 reviewed Mar 27, 2019

View reviewed changes

reuse subquery correctly

6a69e05

adrian-wang force-pushed the reuse branch from 40dc3f9 to 6a69e05 Compare March 27, 2019 07:59

gatorsmile reviewed Mar 27, 2019

View reviewed changes

gatorsmile closed this in f1fe805 Mar 27, 2019

		@@ -674,6 +674,8 @@ case class SubqueryExec(name: String, child: SparkPlan) extends UnaryExecNode {

		override def outputOrdering: Seq[SortOrder] = child.outputOrdering

		override def doCanonicalize(): SparkPlan = child.canonicalized

[SPARK-27279][SQL] Reuse subquery should compare child plan of SubqueryExec #24214

[SPARK-27279][SQL] Reuse subquery should compare child plan of SubqueryExec #24214

Uh oh!

Conversation

adrian-wang commented Mar 26, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

maropu Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 26, 2019

Uh oh!

adrian-wang commented Mar 26, 2019

Uh oh!

SparkQA commented Mar 26, 2019

Uh oh!

adrian-wang commented Mar 26, 2019

Uh oh!

SparkQA commented Mar 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 27, 2019

Uh oh!

SparkQA commented Mar 27, 2019

Uh oh!

cloud-fan commented Mar 27, 2019

Uh oh!

SparkQA commented Mar 27, 2019

Uh oh!

adrian-wang commented Mar 27, 2019

Uh oh!

gatorsmile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[SPARK-27279][SQL] Reuse subquery should compare child plan of `SubqueryExec` #24214

[SPARK-27279][SQL] Reuse subquery should compare child plan of `SubqueryExec` #24214

maropu Mar 26, 2019 •

edited

Loading