[SPARK-17733][SQL] InferFiltersFromConstraints rule never terminates for query #15319

jiangxb1987 · 2016-10-01T05:03:13Z

What changes were proposed in this pull request?

The function QueryPlan.inferAdditionalConstraints and UnaryNode.getAliasedConstraints can produce a non-converging set of constraints for recursive functions. For instance, if we have two constraints of the form(where a is an alias):
a = b, a = f(b, c)
Applying both these rules in the next iteration would infer:
f(b, c) = f(f(b, c), c)
This process repeated, the iteration won't converge and the set of constraints will grow larger and larger until OOM.

~~To fix this problem, we collect alias from expressions and skip infer constraints if we are to transform an Expression to another which contains it.~~
To fix this problem, we apply additional check in inferAdditionalConstraints, when it's possible to generate recursive constraints, we skip generate that.

How was this patch tested?

Add new testcase in SQLQuerySuite/InferFiltersFromConstraintsSuite.

…verging set of constraints

rxin · 2016-10-01T05:04:53Z

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

@@ -2678,4 +2678,45 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext {
      }
    }
  }
+
+  test("SPARK-17733 InferFiltersFromConstraints rule never terminates for query") {


can we construct a unit test rather than an end-to-end test here?

Yes - Perhaps we could add new testcases in InferFiltersFromConstraintsSuite.

Given that you already have a unit test for cases like these, how about we remove this now? This test was randomly generated to catch issues like this and in its current form, it isn't very obvious how this query has anything to do with InferFiltersFromConstraints.

SparkQA · 2016-10-01T05:21:48Z

Test build #66205 has finished for PR 15319 at commit ebba446.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

hvanhovell · 2016-10-01T05:31:33Z

retest this please

SparkQA · 2016-10-01T07:40:40Z

Test build #66206 has finished for PR 15319 at commit ebba446.

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

jiangxb1987 · 2016-10-01T08:33:50Z

...rc/test/scala/org/apache/spark/sql/catalyst/optimizer/InferFiltersFromConstraintsSuite.scala

+      .analyze
+    val correctAnswer = t1.where(IsNotNull('a) && 'a === Coalesce(Seq('a, 'b))
+      && IsNotNull('b) && 'b === Coalesce(Seq('a, 'b))
+      && IsNotNull(Coalesce(Seq('a, 'b))) && 'a === 'b)


These predicates are infered from t.a = t2.a, t.d = t2.a, t.int_col = t2.a, which in line with our expectation.

SparkQA · 2016-10-01T10:28:59Z

Test build #66212 has finished for PR 15319 at commit 7d9e2b0.

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

SparkQA · 2016-10-01T10:39:54Z

Test build #66213 has finished for PR 15319 at commit 3b93209.

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

jiangxb1987 · 2016-10-01T17:12:34Z

After more thoughts, I discovered various ways to create non-converge constraint set(in the following cases, a represents Alias(f(b, c), "a")):
Condition 1. a = b will infer f(b, c) = f(f(b, c), c);
Condition 2. a = d, b = d will infer a = b and therefore deducts f(b, c) = f(f(b, c), c);
Condition 3. a = d, d = e, e = f, ..., x = b will deduct f(b, c) = f(f(b, c), c) too after certain iterations;
Condition 4. a = d, b = d will also infer f(d, c) = f(f(d, c), c) and so on.

For the Cond. 1/2/3, we can avoid to create non-converge constraint set by checking whether constraint contains a.child(e.g. f(b, c)), but for the Cond. 4, I haven't figure out a work around.

A new approach will be avoid replace an Alias into constraint, it will ensure that QueryPlan.inferAdditionalConstraints and UnaryNode.getAliasedConstraints won't both apply and therefore no non-converge constraint set will be created. For this approach, we may miss some constraints that could have been infered, but I think it won't be super harmful because these constraints are commonly complex that rarely filters more data.

jiangxb1987 · 2016-10-01T17:15:06Z

...rc/test/scala/org/apache/spark/sql/catalyst/optimizer/InferFiltersFromConstraintsSuite.scala

-    val batches = Batch("InferFilters", FixedPoint(5), InferFiltersFromConstraints) ::
-      Batch("PredicatePushdown", FixedPoint(5), PushPredicateThroughJoin) ::
-      Batch("CombineFilters", FixedPoint(5), CombineFilters) :: Nil
+    val batches =


Previous batches will not apply InferFiltersFromConstraints after PushPredicateThroughJoin.

SparkQA · 2016-10-01T18:51:21Z

Test build #66223 has finished for PR 15319 at commit 5b25fce.

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

SparkQA · 2016-10-01T19:11:27Z

Test build #66224 has finished for PR 15319 at commit e5912f8.

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

SparkQA · 2016-10-02T03:51:36Z

Test build #66234 has finished for PR 15319 at commit 9639c71.

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

hvanhovell · 2016-10-05T02:01:51Z

cc @sameeragarwal

jiangxb1987 · 2016-10-08T11:12:27Z

@sameeragarwal Could you review this PR please?

lins05 · 2016-10-08T15:03:11Z

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

+    // because then both `QueryPlan.inferAdditionalConstraints` and
+    // `UnaryNode.getAliasedConstraints` applies and may produce a non-converging set of
+    // constraints.
+    // For more details, infer https://issues.apache.org/jira/browse/SPARK-17733


typo "infer" -> "refer" (to)?

lins05 · 2016-10-08T15:04:39Z

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

+    // `UnaryNode.getAliasedConstraints` applies and may produce a non-converging set of
+    // constraints.
+    // For more details, infer https://issues.apache.org/jira/browse/SPARK-17733
+    val aliasMap = AttributeMap((expressions ++ children.flatMap(_.expressions)).collect {


Why not using AttributeSet?

Yes, AttributeSet is a better choice here.

SparkQA · 2016-10-09T08:23:17Z

Test build #66597 has finished for PR 15319 at commit 1558d4c.

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

jiangxb1987 · 2016-10-10T06:30:52Z

retest this please.

SparkQA · 2016-10-10T08:33:37Z

Test build #66631 has finished for PR 15319 at commit 1558d4c.

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

jiangxb1987 · 2016-10-10T10:34:51Z

Not sure why these testcases are failing, they passed in my local envirement.

SparkQA · 2016-10-10T13:00:41Z

Test build #3308 has finished for PR 15319 at commit 1558d4c.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

jiangxb1987 · 2016-10-10T15:44:43Z

retest this please.

SparkQA · 2016-10-10T18:07:37Z

Test build #66658 has finished for PR 15319 at commit 1558d4c.

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

SparkQA · 2016-10-10T20:48:35Z

Test build #3319 has finished for PR 15319 at commit 1558d4c.

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

jiangxb1987 · 2016-10-15T05:38:27Z

retest this please.

SparkQA · 2016-10-15T07:54:13Z

Test build #67007 has finished for PR 15319 at commit 1558d4c.

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

SparkQA · 2016-10-15T19:05:26Z

Test build #67020 has finished for PR 15319 at commit 388443d.

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

jiangxb1987 · 2016-10-17T01:57:31Z

ping @sameeragarwal

jiangxb1987 · 2016-10-18T03:12:30Z

This PR is ready for review, would anyone look at it?

sameeragarwal · 2016-10-18T06:56:48Z

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

    var inferredConstraints = Set.empty[Expression]
    constraints.foreach {
      case eq @ EqualTo(l: Attribute, r: Attribute) =>
        inferredConstraints ++= (constraints - eq).map(_ transform {
-          case a: Attribute if a.semanticEquals(l) => r
+          case a: Attribute if a.semanticEquals(l) && !aliasSet.contains(r) => r


@jiangxb1987 isn't this a fairly restrictive way to solve this problem? You are essentially not inferring any additional constraints from those that contain aliases. For e.g., if we have a subquery SELECT a AS a1, b AS b1 WHERE a1 = 1 AND a1 = b1, this change would never allow us to infer a filter/constraint on b = 1. Can we identify and just disallow recursive constraints?

Thank you a lot for your advice! Perhaps We should generate the following two sets:

Set of Alias which have references to other expressions, for instance, Alias(f(b, c), "a"), or Alias(a, "a1");

Generate sets of equivalence classes out of EqualTo operators in constraints, e.g., when we have a = b and c = b and e = f, then the sets would be ((a, b, c), (e, f)).
Here, for any expressions to be used to infer new constraints, we should check that either it's not in our AliasSet, or its reference doesn't contain any expressions in the corresponding equivalence classes set.

I'll update this check rule ASAP. Thank you for helping!

SparkQA · 2016-10-19T14:30:51Z

Test build #67195 has finished for PR 15319 at commit 909d2cd.

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

jiangxb1987 · 2016-10-19T15:17:27Z

@sameeragarwal I've updated the rule, please review when you have time! Thank you!

sameeragarwal · 2016-10-21T05:39:30Z

Thanks @jiangxb1987, this equivalence class approach looks pretty solid. I'll take a closer look tomorrow!

sameeragarwal

Thanks again @jiangxb1987, I just left some comments around structure and styling but overall this approach looks good to me.

sameeragarwal · 2016-10-24T08:51:09Z

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

+    // Don't apply transform on constraints if the replacement will cause an recursive deduction,
+    // when that happens a non-converging set of constraints will be created and finally throw
+    // OOM Exception.
+    // For more details, refer to https://issues.apache.org/jira/browse/SPARK-17733


This comment doesn't seem to give a lot of context about the underlying issue. How about we just add a top-level comment for this method summarizing the issue and remove this? Perhaps, something along the lines of the following:

/** * Infers an additional set of constraints from a given set of equality constraints. * For e.g., if an operator has constraints of the form (`a = 5`, `a = b`), this returns an * additional constraint of the form `b = 5`. * * [SPARK-17733] We explicitly prevent producing recursive constraints of the form `a = f(a, b)` * as they are often useless and can lead to a non-converging set of constraints. */ private def inferAdditionalConstraints(constraints: Set[Expression]): Set[Expression]

sameeragarwal · 2016-10-24T08:52:10Z

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

+    // when that happens a non-converging set of constraints will be created and finally throw
+    // OOM Exception.
+    // For more details, refer to https://issues.apache.org/jira/browse/SPARK-17733
+    val aliasMap = AttributeMap((expressions ++ children.flatMap(_.expressions)).collect {


Since aliasMap is referenced at a number of places, let's just make this a private lazy val and move it outside of this method in QueryPlan.

sameeragarwal · 2016-10-24T08:57:44Z

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

+   */
+  private def generateEqualExpressionSets(
+      constraints: Set[Expression],
+      aliasMap: AttributeMap[Expression]): Seq[Set[Expression]] = {


we can now remove aliasMap from here

sameeragarwal · 2016-10-24T09:06:41Z

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

+      case a: Alias => (a.toAttribute, a.child)
+    })
+
+    val equalExprSets = generateEqualExpressionSets(constraints, aliasMap)


nit: How about calling them val constraintClasses?

sameeragarwal · 2016-10-24T09:08:28Z

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

+   * expression sets: (Set(a, b, c), Set(e, f)). This will be used to search all expressions equal
+   * to an selected attribute.
+   */
+  private def generateEqualExpressionSets(


How about generateEquivalentConstraintClasses or generateEquivalentConstraintSet?

sameeragarwal · 2016-10-24T09:24:39Z

...rc/test/scala/org/apache/spark/sql/catalyst/optimizer/InferFiltersFromConstraintsSuite.scala

+      .join(t2, Inner, Some("t.a".attr === "t2.a".attr && "t.d".attr === "t2.a".attr))
+      .analyze
+    val currectAnswer = t1.where(IsNotNull('a) && IsNotNull('b)
+        && 'a <=> 'a && 'b <=> 'b &&'a === 'b)


nit: 2 spaces

sameeragarwal · 2016-10-24T09:25:02Z

...rc/test/scala/org/apache/spark/sql/catalyst/optimizer/InferFiltersFromConstraintsSuite.scala

+      .join(t2, Inner, Some("t.a".attr === "t2.a".attr && "t.int_col".attr === "t2.a".attr))
+      .analyze
+    val currectAnswer = t1.where(IsNotNull('a) && IsNotNull(Coalesce(Seq('a, 'b)))
+        &&'a === Coalesce(Seq('a, 'b)))


nit: 2 spaces

sameeragarwal · 2016-10-24T09:25:15Z

...rc/test/scala/org/apache/spark/sql/catalyst/optimizer/InferFiltersFromConstraintsSuite.scala

+      && Coalesce(Seq('b, 'b)) <=> Coalesce(Seq('b, 'b)) && 'b <=> 'b)
+      .select('a, 'b.as('d), Coalesce(Seq('a, 'b)).as('int_col)).as("t")
+      .join(t2.where(IsNotNull('a) && IsNotNull(Coalesce(Seq('a, 'a)))
+      && 'a === Coalesce(Seq('a, 'a)) && 'a <=> Coalesce(Seq('a, 'a)) && 'a <=> 'a


nit: 2 spaces

sameeragarwal · 2016-10-24T09:25:19Z

...rc/test/scala/org/apache/spark/sql/catalyst/optimizer/InferFiltersFromConstraintsSuite.scala

+      .select('a, 'b.as('d), Coalesce(Seq('a, 'b)).as('int_col)).as("t")
+      .join(t2.where(IsNotNull('a) && IsNotNull(Coalesce(Seq('a, 'a)))
+      && 'a === Coalesce(Seq('a, 'a)) && 'a <=> Coalesce(Seq('a, 'a)) && 'a <=> 'a
+      && Coalesce(Seq('a, 'a)) <=> Coalesce(Seq('a, 'a))), Inner,


nit: 2 spaces

sameeragarwal · 2016-10-24T09:29:33Z

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

@@ -2678,4 +2678,45 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext {
      }
    }
  }
+
+  test("SPARK-17733 InferFiltersFromConstraints rule never terminates for query") {


Given that you already have a unit test for cases like these, how about we remove this now? This test was randomly generated to catch issues like this and in its current form, it isn't very obvious how this query has anything to do with InferFiltersFromConstraints.

jiangxb1987 · 2016-10-25T07:12:19Z

Thank you @sameeragarwal ! I've updated these codes following your advice, please have a look at them when you have time.

SparkQA · 2016-10-25T09:27:48Z

Test build #67495 has finished for PR 15319 at commit 905eaa1.

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

SparkQA · 2016-10-25T09:30:20Z

Test build #67497 has finished for PR 15319 at commit 45308d5.

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

sameeragarwal · 2016-10-26T12:02:08Z

LGTM, Thanks!

hvanhovell · 2016-10-26T15:09:04Z

Merging to master/2.0! Thanks!

…for query ## What changes were proposed in this pull request? The function `QueryPlan.inferAdditionalConstraints` and `UnaryNode.getAliasedConstraints` can produce a non-converging set of constraints for recursive functions. For instance, if we have two constraints of the form(where a is an alias): `a = b, a = f(b, c)` Applying both these rules in the next iteration would infer: `f(b, c) = f(f(b, c), c)` This process repeated, the iteration won't converge and the set of constraints will grow larger and larger until OOM. ~~To fix this problem, we collect alias from expressions and skip infer constraints if we are to transform an `Expression` to another which contains it.~~ To fix this problem, we apply additional check in `inferAdditionalConstraints`, when it's possible to generate recursive constraints, we skip generate that. ## How was this patch tested? Add new testcase in `SQLQuerySuite`/`InferFiltersFromConstraintsSuite`. Author: jiangxingbo <jiangxb1987@gmail.com> Closes #15319 from jiangxb1987/constraints. (cherry picked from commit 3c02357) Signed-off-by: Herman van Hovell <hvanhovell@databricks.com>

…for query ## What changes were proposed in this pull request? The function `QueryPlan.inferAdditionalConstraints` and `UnaryNode.getAliasedConstraints` can produce a non-converging set of constraints for recursive functions. For instance, if we have two constraints of the form(where a is an alias): `a = b, a = f(b, c)` Applying both these rules in the next iteration would infer: `f(b, c) = f(f(b, c), c)` This process repeated, the iteration won't converge and the set of constraints will grow larger and larger until OOM. ~~To fix this problem, we collect alias from expressions and skip infer constraints if we are to transform an `Expression` to another which contains it.~~ To fix this problem, we apply additional check in `inferAdditionalConstraints`, when it's possible to generate recursive constraints, we skip generate that. ## How was this patch tested? Add new testcase in `SQLQuerySuite`/`InferFiltersFromConstraintsSuite`. Author: jiangxingbo <jiangxb1987@gmail.com> Closes apache#15319 from jiangxb1987/constraints.

## What changes were proposed in this pull request? Improve QueryPlanConstraints framework, make it robust and simple. In apache#15319, constraints for expressions like `a = f(b, c)` is resolved. However, for expressions like ```scala a = f(b, c) && c = g(a, b) ``` The current QueryPlanConstraints framework will produce non-converging constraints. Essentially, the problem is caused by having both the name and child of aliases in the same constraint set. We infer constraints, and push down constraints as predicates in filters, later on these predicates are propagated as constraints, etc.. Simply using the alias names only can resolve these problems. The size of constraints is reduced without losing any information. We can always get these inferred constraints on child of aliases when pushing down filters. Also, the EqualNullSafe between name and child in propagating alias is meaningless ```scala allConstraints += EqualNullSafe(e, a.toAttribute) ``` It just produces redundant constraints. ## How was this patch tested? Unit test Author: Wang Gengliang <ltnwgl@gmail.com> Closes apache#19201 from gengliangwang/QueryPlanConstraints.

jiangxb1987 added 2 commits October 1, 2016 12:36

modify function inferAdditionalConstraints to avoid producing non-con…

b77a4d6

…verging set of constraints

remove commented code.

ebba446

rxin reviewed Oct 1, 2016

View reviewed changes

jiangxb1987 added 2 commits October 1, 2016 16:29

add new testcase.

7d9e2b0

remove unused code.

3b93209

jiangxb1987 commented Oct 1, 2016

View reviewed changes

another approach and more testcases.

5b25fce

jiangxb1987 commented Oct 1, 2016

View reviewed changes

simplify infered filters.

e5912f8

revert testcase change.

9639c71

lins05 reviewed Oct 8, 2016

View reviewed changes

change alias collection structure.

1558d4c

bugfix

388443d

sameeragarwal suggested changes Oct 18, 2016

View reviewed changes

jiangxb1987 added 2 commits October 19, 2016 20:08

update rule for recursive constraints.

52ef1d8

add testcase

909d2cd

sameeragarwal suggested changes Oct 24, 2016

View reviewed changes

improve structure and styling.

905eaa1

remove unused import.

45308d5

sameeragarwal mentioned this pull request Oct 26, 2016

[SPARK-18063][SQL] Failed to infer constraints over multiple aliases #15597

Closed

asfgit closed this in 3c02357 Oct 26, 2016

jiangxb1987 deleted the constraints branch October 27, 2016 02:03

gengliangwang mentioned this pull request Sep 12, 2017

[SPARK-21979][SQL]Improve QueryPlanConstraints framework #19201

Closed

[SPARK-17733][SQL] InferFiltersFromConstraints rule never terminates for query #15319

[SPARK-17733][SQL] InferFiltersFromConstraints rule never terminates for query #15319

Conversation

jiangxb1987 commented Oct 1, 2016 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

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 Oct 1, 2016

hvanhovell commented Oct 1, 2016

SparkQA commented Oct 1, 2016

Choose a reason for hiding this comment

SparkQA commented Oct 1, 2016

SparkQA commented Oct 1, 2016

jiangxb1987 commented Oct 1, 2016

Choose a reason for hiding this comment

SparkQA commented Oct 1, 2016

SparkQA commented Oct 1, 2016

SparkQA commented Oct 2, 2016

hvanhovell commented Oct 5, 2016

jiangxb1987 commented Oct 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Oct 9, 2016

jiangxb1987 commented Oct 10, 2016

SparkQA commented Oct 10, 2016

jiangxb1987 commented Oct 10, 2016

SparkQA commented Oct 10, 2016

jiangxb1987 commented Oct 10, 2016

SparkQA commented Oct 10, 2016

SparkQA commented Oct 10, 2016

jiangxb1987 commented Oct 15, 2016

SparkQA commented Oct 15, 2016

SparkQA commented Oct 15, 2016

jiangxb1987 commented Oct 17, 2016

jiangxb1987 commented Oct 18, 2016

sameeragarwal Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

jiangxb1987 Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Oct 19, 2016

jiangxb1987 commented Oct 19, 2016

sameeragarwal commented Oct 21, 2016

sameeragarwal left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangxb1987 commented Oct 25, 2016

SparkQA commented Oct 25, 2016

SparkQA commented Oct 25, 2016

sameeragarwal commented Oct 26, 2016

hvanhovell commented Oct 26, 2016 • edited Loading

jiangxb1987 commented Oct 1, 2016 •

edited

Loading

sameeragarwal Oct 18, 2016 •

edited

Loading

jiangxb1987 Oct 18, 2016 •

edited

Loading

hvanhovell commented Oct 26, 2016 •

edited

Loading