[SPARK-21835][SQL] RewritePredicateSubquery should not produce unresolved query plans #19050

viirya · 2017-08-25T05:44:53Z

What changes were proposed in this pull request?

Correlated predicate subqueries are rewritten into Join by the rule RewritePredicateSubquery during optimization.

It is possibly that the two sides of the Join have conflicting attributes. The query plans produced by RewritePredicateSubquery become unresolved and break structural integrity.

We should check if there are conflicting attributes in the Join and de-duplicate them by adding a Project.

How was this patch tested?

Added tests.

SparkQA · 2017-08-25T07:04:48Z

Test build #81117 has finished for PR 19050 at commit edb6271.

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

viirya · 2017-08-25T07:06:32Z

retest this please.

SparkQA · 2017-08-25T09:39:51Z

Test build #81121 has finished for PR 19050 at commit edb6271.

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

hvanhovell · 2017-08-25T14:23:30Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

Shall we just move this into a separate file? The analyzer is way to big as it is.

hvanhovell · 2017-08-25T14:26:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/subquery.scala

Can you explain when this becomes a problem? LeftSemi/LeftAnti joins do output attributes from the right hand side of the join. Is the condition messed up?

duplicateResolved in logical Join doesn't consider if LeftSemi/LeftAnti or not.

But as LeftSemi/LeftAnti only do output from one side (left.output in logical.Join), should we revise duplicateResolved for logical.Join? Then we don't need to dedup here.

@viirya Just for my understanding, Can you please put an example query that depicts this problem ?

One example:

Seq(1 -> "a").toDF("i", "j").write.parquet(path.getCanonicalPath) sql(s"CREATE TABLE t1 USING parquet LOCATION '${path.toURI}'") val sqlText = """ |SELECT * FROM t1 |WHERE |NOT EXISTS (SELECT * FROM t1) """.stripMargin val ds = sql(sqlText) println(ds.queryExecution.analyzed) println(ds.queryExecution.optimizedPlan) Project [i#236, j#237] +- Filter NOT exists#235 [] : +- Project [i#236, j#237] : +- SubqueryAlias t1 : +- Relation[i#236,j#237] parquet +- SubqueryAlias t1 +- Relation[i#236,j#237] parquet 'Join LeftAnti :- Relation[i#236,j#237] parquet +- Relation[i#236,j#237] parquet

I think it is better to fix duplicateResolved.

We should add a project when outputs are duplicate. Could you check this for a correlated subquery?

Sure. Thanks for suggestion.

@viirya Thanks. I think, we should be de-duping the subquery output in case of correlated subquery. Thats why i was wondering about how we are ending up with duplicate attributes.
@hvanhovell I had a question. Do we anticipate to have future rewrites to convert left-semi/left-anti joins to other types join such as inner join ? Can this be a problem then ?

@hvanhovell Because predicate subqueries are rewritten into left semi/anti joins which don't have duplicate outputs. I think you mean correlated scalar subqueries which are rewritten into left outer joins, is it right?

@hvanhovell We may not be able to revise duplicateResolved.

Even LeftSemi do output from only left side of the join, we still need duplicateResolved as false if there are duplicate attributes between left and right sides.

Otherwise, if there is a condition, the condition will be pushdown to left side of the join, because all attribute references in the condition is belonging to one side. It changes the join results.

SparkQA · 2017-08-28T04:27:25Z

Test build #81173 has started for PR 19050 at commit bf07e2a.

SparkQA · 2017-08-28T05:08:17Z

Test build #81172 has finished for PR 19050 at commit 121ad5a.

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

SparkQA · 2017-08-28T20:12:21Z

Test build #81179 has finished for PR 19050 at commit 82b5bac.

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

viirya · 2017-08-28T23:46:22Z

ping @hvanhovell Does the current change look good for you?

viirya · 2017-08-28T23:46:45Z

also cc @cloud-fan for review. Thanks.

viirya · 2017-08-31T23:42:27Z

ping @cloud-fan @hvanhovell Can you have time to review this? Thanks.

viirya · 2017-09-04T03:55:18Z

ping @cloud-fan @hvanhovell This blocks the #18956 going forward, can you help review this change? Thanks.

viirya · 2017-09-05T23:18:36Z

@cloud-fan and @hvanhovell seems too busy these days. maybe @gatorsmile can also help review this. Thanks.

gatorsmile · 2017-09-05T23:51:19Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/subquery.scala

    }
  }

+  def dedupJoin(plan: LogicalPlan): LogicalPlan = {


-> private def

gatorsmile · 2017-09-05T23:51:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/subquery.scala


+  def dedupJoin(plan: LogicalPlan): LogicalPlan = {
+    plan transform {
+      case j @ Join(left, right, joinType, joinCond) =>


All join types?

Be safe, this only makes sense for LeftAnti and LeftSemi

gatorsmile · 2017-09-05T23:51:57Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/subquery.scala

          val (newCond, inputPlan) = rewriteExistentialExpr(Seq(predicate), p)
          Project(p.output, Filter(newCond.get, inputPlan))
      }
+      dedupJoin(rewritten)


add a comment above this line to explain it.

After rethinking it, we can be more conservative. Instead of doing a dedup at the end, we should do it when we convert it to the Join.

Fair point. Will follow it.

gatorsmile · 2017-09-05T23:52:22Z

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

+            |WHERE
+            |NOT EXISTS (SELECT * FROM t1)
+          """.stripMargin
+        val ds = sql(sqlText)


useless ds?

Yes, missing this. I'll remove it.

gatorsmile · 2017-09-06T04:05:23Z

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

+        val optimizedPlan = sql(sqlText).queryExecution.optimizedPlan
+        val join = optimizedPlan.collect {
+          case j: Join => j
+        }.head.asInstanceOf[Join]


val join = optimizedPlan.collectFirst { case j: Join => j }.get

gatorsmile · 2017-09-06T04:05:43Z

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

+        val joinNodes = optimizedPlan.collect {
+          case j: Join => j
+        }.map(_.asInstanceOf[Join])
+        joinNodes.map(j => assert(j.duplicateResolved))


val joinNodes = optimizedPlan.collect { case j: Join => j } joinNodes.foreach(j => assert(j.duplicateResolved))

gatorsmile · 2017-09-06T04:17:05Z

LGTM pending Jenkins

viirya · 2017-09-06T04:18:50Z

Thanks @gatorsmile

SparkQA · 2017-09-06T05:40:45Z

Test build #81439 has finished for PR 19050 at commit c1325fb.

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

SparkQA · 2017-09-06T06:56:12Z

Test build #81440 has finished for PR 19050 at commit 8550828.

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

gatorsmile · 2017-09-06T14:42:42Z

Thanks! Merging to master

…duce unresolved query plans ## What changes were proposed in this pull request? This is a follow-up of apache#19050 to deal with `ExistenceJoin` case. ## How was this patch tested? Added test. Author: Liang-Chi Hsieh <[email protected]> Closes apache#19151 from viirya/SPARK-21835-followup.

viirya mentioned this pull request Aug 25, 2017

[SPARK-21726][SQL] Check for structural integrity of the plan in Optimzer in test mode. #18956

Closed

hvanhovell reviewed Aug 25, 2017

View reviewed changes

viirya force-pushed the SPARK-21835 branch 2 times, most recently from 121ad5a to bf07e2a Compare August 28, 2017 04:22

Deduplicate join output for correlated predicate subquery.

82b5bac

viirya force-pushed the SPARK-21835 branch from bf07e2a to 82b5bac Compare August 28, 2017 08:59

viirya changed the title ~~[SPARK-21835][SQL][WIP] RewritePredicateSubquery should not produce unresolved query plans~~ [SPARK-21835][SQL] RewritePredicateSubquery should not produce unresolved query plans Aug 28, 2017

gatorsmile reviewed Sep 5, 2017

View reviewed changes

Address comments.

c1325fb

gatorsmile reviewed Sep 6, 2017

View reviewed changes

Address comment.

8550828

asfgit closed this in f2e22ae Sep 6, 2017

viirya mentioned this pull request Sep 7, 2017

[SPARK-21835][SQL][FOLLOW-UP] RewritePredicateSubquery should not produce unresolved query plans #19151

Closed

viirya deleted the SPARK-21835 branch December 27, 2023 18:34

[SPARK-21835][SQL] RewritePredicateSubquery should not produce unresolved query plans #19050

[SPARK-21835][SQL] RewritePredicateSubquery should not produce unresolved query plans #19050

Uh oh!

Conversation

viirya commented Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Aug 25, 2017

Uh oh!

viirya commented Aug 25, 2017

Uh oh!

SparkQA commented Aug 25, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dilipbiswal Aug 26, 2017 • 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!

SparkQA commented Aug 28, 2017

Uh oh!

SparkQA commented Aug 28, 2017

Uh oh!

SparkQA commented Aug 28, 2017

Uh oh!

viirya commented Aug 28, 2017

Uh oh!

viirya commented Aug 28, 2017

Uh oh!

viirya commented Aug 31, 2017

Uh oh!

viirya commented Sep 4, 2017

Uh oh!

viirya commented Sep 5, 2017

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!

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!

gatorsmile commented Sep 6, 2017

viirya commented Aug 25, 2017 •

edited

Loading

dilipbiswal Aug 26, 2017 •

edited

Loading