[SPARK-13995][SQL] Extract correct IsNotNull constraints for Expression by viirya · Pull Request #11809 · apache/spark

viirya · 2016-03-18T02:57:16Z

What changes were proposed in this pull request?

JIRA: https://issues.apache.org/jira/browse/SPARK-13995

We infer relative IsNotNull constraints from logical plan's expressions in constructIsNotNullConstraints now. However, we don't consider the case of (nested) Cast.

For example:

val tr = LocalRelation('a.int, 'b.long)
val plan = tr.where('a.attr === 'b.attr).analyze

Then, the plan's constraints will have IsNotNull(Cast(resolveColumn(tr, "a"), LongType)), instead of IsNotNull(resolveColumn(tr, "a")). This PR fixes it.

Besides, as IsNotNull constraints are most useful for Attribute, we should do recursing through any Expression that is null intolerant and construct IsNotNull constraints for all Attributes under these Expressions.

For example, consider the following constraints:

val df = Seq((1,2,3)).toDF("a", "b", "c")
df.where("a + b = c").queryExecution.analyzed.constraints

The inferred isnotnull constraints should be isnotnull(a), isnotnull(b), isnotnull(c), instead of isnotnull(a + c) and isnotnull(c).

How was this patch tested?

Test is added into ConstraintPropagationSuite.

gatorsmile · 2016-03-18T03:19:12Z

Yeah, I also hit this issue when fixing this PR: #11765

You are so fast! Actually, they are related. My original plan is to merge the previous one and then revisit this issue. If this is merged before the above PR, I need to redo the work. Anyway, thank you for fixing this issue!

viirya · 2016-03-18T03:49:23Z

@gatorsmile Thanks for providing the info! I found this issue before when dealing with another PR. But I has no time to submit it separately as new PR until today.

SparkQA · 2016-03-18T05:05:13Z

Test build #53495 has finished for PR 11809 at commit b4e6033.

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

viirya · 2016-03-19T07:33:44Z

cc @marmbrus @sameeragarwal

sameeragarwal · 2016-03-21T17:59:08Z

+
+ private def collectCasts(e: Expression): Option[Attribute] = {
+ if (e.isInstanceOf[Cast]) {
+ collectCasts(e.children(0))


e.child for better readability?

sameeragarwal · 2016-03-21T18:05:26Z

Thanks for fixing this! Just few non-critical suggestions.

marmbrus · 2016-03-21T18:23:43Z

This fix seems okay, but I feel like we are just adding one-offs instead of taking a step back and thinking about how to generally infer null-intollerance from an expression. For example, after this PR we still aren't doing great in this case:

scala>valdf=Seq((1,2,3)).toDF("a", "b", "c")
scala> df.where("a + b = c").queryExecution.analyzed.constraints
res2: org.apache.spark.sql.catalyst.expressions.ExpressionSet=Set(((a#4+ b#5) = c#6), isnotnull((a#4+ b#5)), isnotnull(c#6))

Given that it seems most useful to infer IsNotNull for Attributes, it seems that we just fix constructIsNotNullConstraints to recurse through any Expression that is null intolerant (i.e. any null input results in null output). Then it can just return a Seq[Attribute].

We could even consider making NullIntolerant into a trait so we could easily mark all the valid expressions as such. Which would make the logic in QueryPlan really simple to reason about.

sameeragarwal · 2016-03-21T18:31:16Z

+1 completely agree with @marmbrus

gatorsmile · 2016-03-21T19:40:03Z

Yeah, also agree with @marmbrus .

This is a general issue. When fixing isNotNull Constraints, we also have to fix the related reasoning parts for using these Constraints. If possible, we really need to have a fundamental way to do it easily. Otherwise, we might fix many small bugs again and again.

viirya · 2016-03-22T01:01:07Z

@marmbrus Great suggestion. Thanks! I will update this according to your suggestion.

Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/patterns.scala

SparkQA · 2016-03-22T12:08:21Z

Test build #53766 has finished for PR 11809 at commit c819572.

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

SparkQA · 2016-03-22T12:49:32Z

Test build #53765 has finished for PR 11809 at commit 5fa1760.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

gatorsmile · 2016-03-22T15:18:46Z

+
+ private def scanNullIntolerantExpr(expr: Expression): Set[Expression] = expr match {
+ case a: Attribute => Set(IsNotNull(a))
+ case IsNotNull(e) =>


Make it more general. Here, you just cover a single case

SparkQA · 2016-03-22T15:47:17Z

Test build #53771 has finished for PR 11809 at commit 3373396.

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

SparkQA · 2016-03-23T08:22:21Z

Test build #53903 has finished for PR 11809 at commit c8fb736.

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

viirya · 2016-03-23T10:28:56Z

retest this please.

SparkQA · 2016-03-23T11:43:58Z

Test build #53927 has finished for PR 11809 at commit 81c46c7.

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

viirya · 2016-03-23T11:52:30Z

retest this please.

SparkQA · 2016-03-23T12:02:31Z

Test build #53917 has finished for PR 11809 at commit 81c46c7.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-03-23T16:07:31Z

Test build #53929 has finished for PR 11809 at commit 81c46c7.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2016-03-24T02:42:47Z

A not related flaky test...

viirya · 2016-03-24T03:56:03Z

retest this please.

SparkQA · 2016-03-24T05:32:38Z

Test build #53999 has finished for PR 11809 at commit 81c46c7.

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

SparkQA · 2016-03-24T07:45:05Z

Test build #54012 has finished for PR 11809 at commit 8e8dd72.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- abstract class Attribute extends LeafExpression with NamedExpression with NullIntolerant

viirya · 2016-03-24T07:51:59Z

retest this please.

SparkQA · 2016-03-24T09:04:09Z

Test build #54022 has finished for PR 11809 at commit 8e8dd72.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- abstract class Attribute extends LeafExpression with NamedExpression with NullIntolerant

viirya · 2016-03-24T09:07:46Z

retest this please.

viirya · 2016-03-24T09:16:02Z

Any hint about why org.apache.spark.sql.hive.execution.SQLQuerySuite continues to be failed across different PRs. Error message: org.apache.spark.sql.AnalysisException: Table not found: src.

Tests are passed locally.

SparkQA · 2016-03-24T10:33:15Z

Test build #54026 has finished for PR 11809 at commit 8e8dd72.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- abstract class Attribute extends LeafExpression with NamedExpression with NullIntolerant

viirya · 2016-03-24T15:10:28Z

retest this please.

SparkQA · 2016-03-24T16:40:16Z

Test build #54046 has finished for PR 11809 at commit 8e8dd72.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- abstract class Attribute extends LeafExpression with NamedExpression with NullIntolerant

viirya · 2016-03-24T23:32:54Z

@marmbrus @sameeragarwal @gatorsmile This is ready for review now. Thanks!

Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/ConstraintPropagationSuite.scala

viirya · 2016-03-31T14:53:25Z

@marmbrus Can you take a look this? Thanks.

SparkQA · 2016-03-31T16:23:10Z

Test build #54635 has finished for PR 11809 at commit ec84c5f.

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

sameeragarwal · 2016-03-31T18:21:48Z

LGTM, this approach is pretty neat. Thanks!

viirya · 2016-04-01T06:31:44Z

@sameeragarwal Thanks for reviewing. Waiting for @marmbrus checking this.

marmbrus · 2016-04-01T20:00:12Z

Thanks, merging to master!

viirya added 2 commits March 17, 2016 14:02

init import.
c2f9b05

Add test.
b4e6033

sameeragarwal reviewed Mar 21, 2016
View reviewed changes

viirya added 3 commits March 22, 2016 01:04

Address comments.
d6bb43b

Add nullIntolerant as a method to Expression.
3d08625

Address comments.
5fa1760

viirya changed the title ~~[SPARK-13995][SQL] Constraints should take care of Cast~~[SPARK-13995][SQL] Extract correct IsNotNull constraints for ExpressionMar 22, 2016

Merge remote-tracking branch 'upstream/master' into constraint-cast
c819572
Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/patterns.scala

Fix test.
3373396

gatorsmile reviewed Mar 22, 2016
View reviewed changes

gatorsmile mentioned this pull request Mar 22, 2016
[SPARK-13891] [SQL] [TEST] Issue Exceptions when Hitting the Max Iteration Limit in Optimizer and Analyzer #11714
Closed

viirya mentioned this pull request Mar 24, 2016
[SPARK-13996][SQL] Add more not null attributes for Filter codegen #11810
Closed

viirya added 2 commits March 24, 2016 06:02

Merge remote-tracking branch 'upstream/master' into constraint-cast
6f7751d

Consider IsNotNull(IsNotNull(expr)) case. Add a test for it too.
8e8dd72

Merge remote-tracking branch 'upstream/master' into constraint-cast
ec84c5f
Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/ConstraintPropagationSuite.scala

asfgit closed this in df68bebApr 1, 2016

JoshRosen mentioned this pull request Jun 1, 2019
[SPARK-27915][SQL][WIP] Update logical Filter's output nullability based on IsNotNull conditions #24765
Closed

viirya deleted the constraint-cast branch December 27, 2023 18:33

Uh oh!

Conversation

viirya commented Mar 18, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

gatorsmile commented Mar 18, 2016

Uh oh!

viirya commented Mar 18, 2016

Uh oh!

SparkQA commented Mar 18, 2016

Uh oh!

viirya commented Mar 19, 2016

Uh oh!

sameeragarwalMar 21, 2016

Choose a reason for hiding this comment

Uh oh!

sameeragarwal commented Mar 21, 2016

Uh oh!

marmbrus commented Mar 21, 2016

Uh oh!

sameeragarwal commented Mar 21, 2016

Uh oh!

gatorsmile commented Mar 21, 2016

Uh oh!

viirya commented Mar 22, 2016

Uh oh!

SparkQA commented Mar 22, 2016

Uh oh!

SparkQA commented Mar 22, 2016

Uh oh!

gatorsmileMar 22, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 22, 2016

Uh oh!

SparkQA commented Mar 23, 2016

Uh oh!

viirya commented Mar 23, 2016

Uh oh!

SparkQA commented Mar 23, 2016

Uh oh!

viirya commented Mar 23, 2016

Uh oh!

SparkQA commented Mar 23, 2016

Uh oh!

SparkQA commented Mar 23, 2016

Uh oh!

viirya commented Mar 24, 2016

Uh oh!

viirya commented Mar 24, 2016

Uh oh!

SparkQA commented Mar 24, 2016

Uh oh!

SparkQA commented Mar 24, 2016

Uh oh!

viirya commented Mar 24, 2016

Uh oh!

SparkQA commented Mar 24, 2016

Uh oh!

viirya commented Mar 24, 2016

Uh oh!

viirya commented Mar 24, 2016

Uh oh!

SparkQA commented Mar 24, 2016

Uh oh!

viirya commented Mar 24, 2016

Uh oh!

SparkQA commented Mar 24, 2016

Uh oh!

viirya commented Mar 24, 2016

Uh oh!

viirya commented Mar 31, 2016

Uh oh!

SparkQA commented Mar 31, 2016

Uh oh!

sameeragarwal commented Mar 31, 2016

Uh oh!

viirya commented Apr 1, 2016

Uh oh!