[SPARK-14502][SQL] Add optimization for Binary Comparison Simplification by dongjoon-hyun · Pull Request #12267 · apache/spark

dongjoon-hyun · 2016-04-08T22:54:18Z

What changes were proposed in this pull request?

We can simplifies binary comparisons with semantically-equal operands:

Replace '<=>' with 'true' literal.
Replace '=', '<=', and '>=' with 'true' literal if both operands are non-nullable.
Replace '<' and '>' with 'false' literal if both operands are non-nullable.

For example, the following example plan

scala> sql("SELECT * FROM (SELECT explode(array(1,2,3)) a) T WHERE a BETWEEN a AND a+7").explain()
...
:  +- Filter ((a#59 >= a#59) && (a#59 <= (a#59 + 7)))
...

will be optimized into the following.

:  +- Filter (a#47 <= (a#47 + 7))

How was this patch tested?

Pass the Jenkins tests including new BinaryComparisonSimplificationSuite.

SparkQA · 2016-04-09T00:21:42Z

Test build #55404 has finished for PR 12267 at commit 69fadfe.

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

dongjoon-hyun · 2016-04-09T00:48:15Z

Hi, @rxin .
Could you review this PR please?

rxin · 2016-04-10T04:44:01Z

cc @cloud-fan

cloud-fan · 2016-04-11T01:23:16Z

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

I think this case doesn't need !a.nullable && !b.nullable

Right. I'll fix that.

Oh. @cloud-fan . When I looked at the EqualNullSafe one more time, I realized that that a EqualNullSafe b is null-safe but both a and b are not. The following is the code snippet of EqualNullSafe.

case class EqualNullSafe(left: Expression, right: Expression) extends BinaryComparison { ... override def nullable: Boolean = false ... override def eval(input: InternalRow): Any = { val input1 = left.eval(input) val input2 = right.eval(input) if (input1 == null && input2 == null) { true

In this case, I think we should keep !a.nullable && !b.nullable because we need to evaluate a and b indeed. How do you think about that?

if a semanticEquals b, does it mean a and b should be both null or both not null?

Oh, actually the semanticEquals just used deterministic and canonicalized string comparison.

def semanticEquals(other: Expression): Boolean = deterministic && other.deterministic && canonicalized == other.canonicalized

The javadoc of semanticEquals says: returns true when two expressions will always compute the same result. So I think they should be both null or both not null if they are semanticEquals.

Correct. I missed that. Thank you so much. I'll fix that.

dongjoon-hyun · 2016-04-11T05:33:47Z

Thank you for deep review, @cloud-fan . :)
I changed PR according to the one comment first.
For the others, I asked some questions to understand more correctly.

cloud-fan · 2016-04-11T06:02:21Z

...org/apache/spark/sql/catalyst/optimizer/NonNullableBinaryComparisonSimplificationSuite.scala

the e is not used.

it's a overkill to use loop here for only 2 cases.

dongjoon-hyun · 2016-04-11T06:27:12Z

@cloud-fan . Thank you so much for improving this PR.
Overall, this PR starts to include one Non-Null case (EqualNullSafe) now.
May I change the classname from NonNullableBinaryComparisonSimplification to BinaryComparisonSimplification, too?
(I thought it's a little bit too long.)

cloud-fan · 2016-04-11T06:38:45Z

BinaryComparisonSimplification LGTM

dongjoon-hyun · 2016-04-11T07:00:27Z

Up to now, I updated the followings.

Extends EqualNullSafe case to handle non-nullable operands.
Adds a testcase Nullable Simplication Primitive.
Changes the class name from NonNullableBinaryComparisonSimplification to BinaryComparisonSimplification.
Fix test coverage by using for correctly and adding more testcases in Preserve nullable or non-deterministic exprs in general.
Fix test coverage by replacing '||' to '&&' in testcase Expression Normalization.
Simplify test relations

If I missed something, please let me know.

SparkQA · 2016-04-11T07:00:43Z

Test build #55505 has finished for PR 12267 at commit 7ea4e29.

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

SparkQA · 2016-04-11T07:11:34Z

Test build #55507 has finished for PR 12267 at commit 86380de.

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

SparkQA · 2016-04-11T08:02:59Z

Test build #55511 has finished for PR 12267 at commit 4e9096d.

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

…n Simplification

dongjoon-hyun · 2016-04-11T08:21:42Z

The failure is irrelevant to this PR. So, I rebased to the master in order to trigger Jenkins again.

[info] - event ordering *** FAILED *** (1 minute)
[info]   The code passed to failAfter did not complete within 1 minute. (ContinuousQueryListenerSuite.scala:150)

cloud-fan · 2016-04-11T08:51:58Z

...test/scala/org/apache/spark/sql/catalyst/optimizer/BinaryComparisonSimplificationSuite.scala

+  val nonNullableRelation = LocalRelation('a.int.withNullability(false))
+
+  test("Preserve nullable or non-deterministic exprs in general") {
+    for (e <- Seq('a === 'a, 'a <= 'a, 'a >= 'a, 'a < 'a, 'a > 'a, Rand(0) === Rand(0))) {


We should not test non-deterministic expressions on nullable relation, as nullable relation stops this optimization for all kind of expressions.

My bad. It smells a bug. I will split the testcase.

…ALSE Filter tests.

SparkQA · 2016-04-11T09:53:29Z

Test build #55517 has finished for PR 12267 at commit bdc309c.

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

SparkQA · 2016-04-11T10:50:09Z

Test build #55520 has finished for PR 12267 at commit 9ca13be.

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

cloud-fan · 2016-04-11T11:48:55Z

LGTM, cc @davies

davies · 2016-04-11T16:52:02Z

Merging this into master, thanks!

dongjoon-hyun · 2016-04-11T16:54:11Z

Thank you, @davies , @cloud-fan , and @rxin !

cloud-fan reviewed Apr 11, 2016
View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-14502][SQL] Add optimization for Non-Nullable Binary Comparison Simplification~~ [SPARK-14502][SQL] Add optimization for Binary Comparison Simplification Apr 11, 2016

dongjoon-hyun added 4 commits April 11, 2016 01:18

[SPARK-14502][SQL] Add optimization for Non-Nullable Binary Compariso…

ef41e26

…n Simplification

Replace '||' with '&&'

68b58b6

Simplify test by removing useless data.

71bf40f

Extend EqualNullSafe case to handle non-nullable exprs.

bdc309c

cloud-fan reviewed Apr 11, 2016
View reviewed changes

Add non-deterministic test on non-nullable relation and remove TRUE/F…

9ca13be

…ALSE Filter tests.

asfgit closed this in 5de2619 Apr 11, 2016

dongjoon-hyun deleted the SPARK-14502 branch May 12, 2016 00:58

Conversation

dongjoon-hyun commented Apr 8, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Apr 9, 2016

Uh oh!

dongjoon-hyun commented Apr 9, 2016

Uh oh!

rxin commented Apr 10, 2016

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!

dongjoon-hyun commented Apr 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Apr 11, 2016

Uh oh!

cloud-fan commented Apr 11, 2016

Uh oh!

dongjoon-hyun commented Apr 11, 2016

Uh oh!

SparkQA commented Apr 11, 2016

Uh oh!

SparkQA commented Apr 11, 2016

Uh oh!

SparkQA commented Apr 11, 2016

Uh oh!

dongjoon-hyun commented Apr 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 11, 2016

Uh oh!

SparkQA commented Apr 11, 2016

Uh oh!

cloud-fan commented Apr 11, 2016

Uh oh!

davies commented Apr 11, 2016

Uh oh!

dongjoon-hyun commented Apr 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants