[SPARK-20094][SQL] Preventing push down of IN subquery to Join operator #17428

wzhfy · 2017-03-25T07:36:17Z

What changes were proposed in this pull request?

TPCDS q45 fails becuase:
ReorderJoin collects all predicates and try to put them into join condition when creating ordered join. If a predicate with an IN subquery (ListQuery) is in a join condition instead of a filter condition, RewritePredicateSubquery.rewriteExistentialExpr would fail to convert the subquery to an ExistenceJoin, and thus result in error.

We should prevent push down of IN subquery to Join operator.

How was this patch tested?

Add a new test case in FilterPushdownSuite.

wzhfy · 2017-03-25T07:42:36Z

cc @nsyca @hvanhovell

SparkQA · 2017-03-25T09:45:41Z

Test build #75210 has finished for PR 17428 at commit bd91947.

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

SparkQA · 2017-03-25T10:06:04Z

Test build #75212 has finished for PR 17428 at commit 6af6b95.

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

wzhfy · 2017-03-27T00:39:53Z

also cc @cloud-fan

viirya · 2017-03-27T03:37:11Z

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

@@ -90,6 +90,7 @@ trait PredicateHelper {
   * Returns true iff `expr` could be evaluated as a condition within join.
   */
  protected def canEvaluateWithinJoin(expr: Expression): Boolean = expr match {
+    case l: ListQuery => false


Doesn't the following case e: SubqueryExpression prevent it?

add some comments to explain why ListQuery should always return false even the children is empty.

@viirya Now pulling up IN predicates is deferred to RewritePredicateSubquery. As a result, ListQuery's children can be empty before that rule and falls into case e: SubqueryExpression.

OK. The comment looks good.

SparkQA · 2017-03-27T09:37:48Z

Test build #75257 has finished for PR 17428 at commit 9f7dac5.

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

nsyca · 2017-03-27T14:34:43Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala

@@ -289,6 +289,9 @@ abstract class QueryPlan[PlanType <: QueryPlan[PlanType]] extends TreeNode[PlanT
      case m: Map[_, _] => m
      case d: DataType => d // Avoid unpacking Structs
      case seq: Traversable[_] => seq.map(recursiveTransform)
+      case ExistenceJoin(exists: Attribute) =>
+        // `exists` must be an Attribute in ExistenceJoin
+        ExistenceJoin(transformExpression(exists).asInstanceOf[Attribute])


Is there a particular reason we need to cast the return type of transformExpression(<instance_of_Attribute>) to type Attribute?

Because ExistenceJoin only accepts Attribute as its parameter type.

Can you give an example of transformExpression of an instance of Attribute not returning a value of type Attribute or its subtypes?

Why is this change necessary? This smells for two reasons:

QueryPlan shouldn't have know about such a detail. This means something is wrong with the abstraction.

Why do we want to change the attribute in an ExistenceJoin? It is produced by the join, and we really should not replace it.

Yes I feel the same. Fortunately, after modification as suggested by @nsyca , this change can also be removed. Because I move the test case to FilterPushdownSuite, we don't have to touch the RewritePredicateSubquery rule.

nsyca · 2017-03-27T15:09:33Z

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

+    case l: ListQuery =>
+      // Now pulling up IN predicates is deferred to `RewritePredicateSubquery`. As a result,
+      // `ListQuery`'s children can be empty before that rule and falls into the next case.
+      false


Suggested text in the comment:

[NOT] IN () expression cannot be evaluated as part of a Join operator. Currently the only way to evaluate a [NOT] IN subquery is to convert it to a LeftSemi or LeftAnti by RewritePredicateSubquery rule.

ok, thanks!

nsyca · 2017-03-27T15:15:05Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/JoinOptimizationSuite.scala

+      .join(w, ExistenceJoin(exists), Some("z.c".attr === "w.d".attr))
+      .where("x.a".attr > 1 || exists)
+      .select("x.a".attr, "x.b".attr, "x.c".attr, "z.a".attr, "z.b".attr, "z.c".attr)
+      .join(y, Inner, Some("y.d".attr === "z.a".attr))


This is a side question. It's not a review comment.

Is there any mechanism in this test suite that ensures that the plan Join( Join(x, z), y) is picked by the join reorder algorithm? Isn't a Join( Join(y, z), x) is also a viable plan if the join predicate (y.d = z.a) is estimated to be more selective than the predicate sets between x and z?

This test suite is for JoinReorder rule. This rule is not cost based and it's not related to estimation. It only searches for joinable tables and put cartesian product to the end of the plan.

My question was why Optimizer does not pick a plan of Join( Join(y, z), x).

I guess your test case depends on the fact that currently we don't have an optimization to push down table y below the join of (x, z).

Essentially, this is not a problem specifically to ReorderJoin as implied from the title of the PR. The problem is at the definition canEvaluateWithinJoin. I can demonstrate the same problem without the table y in this test case.

A slight modification of your test case as shown below also reveals the problem.

val queryPlan = x.join(z) .where(("x.b".attr === "z.b".attr) && ("x.a".attr > 1 || "z.c".attr.in(ListQuery(w.select("w.d".attr))))) val expectedPlan = x.join(z, Inner, Some("x.b".attr === "z.b".attr)) .join(w, ExistenceJoin(exists), Some("z.c".attr === "w.d".attr)) .where("x.a".attr > 1 || exists) .select("x.a".attr, "x.b".attr, "x.c".attr, "z.a".attr, "z.b".attr, "z.c".attr)

which has a call to canEvaluateWithinJoin from the rule PushPredicateThroughJoin.

May I suggest the following:

Change the title of the PR to "Preventing push down of IN subquery to Join operator"

Update the test case to its bare minimum by removing the join to table y

Change the title of the test case to reflect the new test

On the last point, removing the "reorder inner joins - " should do.

Yea, thanks for the suggestion. Also I will move the test case to FilterPushdownSuite since now it will not go into ReorderJoin rule.

SparkQA · 2017-03-27T18:36:58Z

Test build #75265 has finished for PR 17428 at commit c32599d.

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

hvanhovell · 2017-03-28T10:46:03Z

LGTM - pending jenkins.

SparkQA · 2017-03-28T11:27:58Z

Test build #75308 has finished for PR 17428 at commit bf7d202.

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

hvanhovell · 2017-03-28T11:42:41Z

Merging to master. Thanks!

SparkQA · 2017-03-28T12:48:12Z

Test build #75309 has finished for PR 17428 at commit 0cc6137.

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

viirya reviewed Mar 27, 2017

View reviewed changes

nsyca reviewed Mar 27, 2017

View reviewed changes

wzhfy added 4 commits March 28, 2017 15:56

don't put predicate with subquery into join condition in ReorderJoin

0be551a

specified as ListQuery

d3a174a

add comment

4739c0b

simplify test case and move it to FilterPushdownSuite

bf7d202

wzhfy force-pushed the noSubqueryInJoinCond branch from c32599d to bf7d202 Compare March 28, 2017 09:20

wzhfy changed the title ~~[SPARK-20094][SQL] Don't put predicate with IN subquery into join condition in ReorderJoin because it fails RewritePredicateSubquery.rewriteExistentialExpr~~ [SPARK-20094][SQL] Preventing push down of IN subquery to Join operator Mar 28, 2017

improve comment

0cc6137

asfgit closed this in 91559d2 Mar 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-20094][SQL] Preventing push down of IN subquery to Join operator #17428

[SPARK-20094][SQL] Preventing push down of IN subquery to Join operator #17428

wzhfy commented Mar 25, 2017 •

edited

Loading

wzhfy commented Mar 25, 2017

SparkQA commented Mar 25, 2017

SparkQA commented Mar 25, 2017

wzhfy commented Mar 27, 2017

viirya Mar 27, 2017

cloud-fan Mar 27, 2017

wzhfy Mar 27, 2017 •

edited

Loading

wzhfy Mar 27, 2017

viirya Mar 27, 2017

SparkQA commented Mar 27, 2017

nsyca Mar 27, 2017

wzhfy Mar 27, 2017

nsyca Mar 27, 2017

hvanhovell Mar 27, 2017

wzhfy Mar 28, 2017 •

edited

Loading

nsyca Mar 27, 2017

wzhfy Mar 27, 2017

nsyca Mar 27, 2017

wzhfy Mar 27, 2017

nsyca Mar 27, 2017

nsyca Mar 27, 2017

nsyca Mar 27, 2017

wzhfy Mar 28, 2017 •

edited

Loading

SparkQA commented Mar 27, 2017

hvanhovell commented Mar 28, 2017

SparkQA commented Mar 28, 2017

hvanhovell commented Mar 28, 2017

SparkQA commented Mar 28, 2017

[SPARK-20094][SQL] Preventing push down of IN subquery to Join operator #17428

[SPARK-20094][SQL] Preventing push down of IN subquery to Join operator #17428

Conversation

wzhfy commented Mar 25, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

wzhfy commented Mar 25, 2017

SparkQA commented Mar 25, 2017

SparkQA commented Mar 25, 2017

wzhfy commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzhfy Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 27, 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

wzhfy Mar 28, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzhfy Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Mar 27, 2017

hvanhovell commented Mar 28, 2017

SparkQA commented Mar 28, 2017

hvanhovell commented Mar 28, 2017

SparkQA commented Mar 28, 2017

wzhfy commented Mar 25, 2017 •

edited

Loading

wzhfy Mar 27, 2017 •

edited

Loading

wzhfy Mar 28, 2017 •

edited

Loading

wzhfy Mar 28, 2017 •

edited

Loading