[SPARK-27924][SQL] Support ANSI SQL Boolean-Predicate syntax by beliefer · Pull Request #25074 · apache/spark

beliefer · 2019-07-08T09:22:08Z

What changes were proposed in this pull request?

This PR aims to support ANSI SQL Boolean-Predicate syntax.

expression IS [NOT] TRUE
expression IS [NOT] FALSE
expression IS [NOT] UNKNOWN

There are some mainstream database support this syntax.

PostgreSQL: https://www.postgresql.org/docs/9.1/functions-comparison.html
Hive: https://issues.apache.org/jira/browse/HIVE-13583
Redshift: https://docs.aws.amazon.com/redshift/latest/dg/r_Boolean_type.html
Vertica: https://www.vertica.com/docs/9.2.x/HTML/Content/Authoring/SQLReferenceManual/LanguageElements/Predicates/Boolean-predicate.htm

For example:

spark-sql> select null is true, null is not true;
false	true

spark-sql> select false is true, false is not true;
false	true

spark-sql> select true is true, true is not true;
true	false

spark-sql> select null is false, null is not false;
false	true

spark-sql> select false is false, false is not false;
true	false

spark-sql> select true is false,  true is not false;
false	true

spark-sql> select null is unknown, null is not unknown;
true	false

spark-sql> select false is unknown, false is not unknown;
false	true

spark-sql> select true is unknown, true is not unknown;
false	true

Note: A null input is treated as the logical value "unknown".

How was this patch tested?

Pass the Jenkins with the newly added test cases.

SparkQA · 2019-07-08T12:31:46Z

Test build #107335 has finished for PR 25074 at commit b2fc258.

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

SparkQA · 2019-07-10T07:05:02Z

Test build #107430 has finished for PR 25074 at commit fa148db.

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

beliefer · 2019-07-10T07:57:23Z

Retest this please.

beliefer · 2019-07-10T08:40:35Z

@ueshin Could you take a look at this PR? Thanks a lot!

SparkQA · 2019-07-10T11:11:51Z

Test build #107449 has finished for PR 25074 at commit fa148db.

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

SparkQA · 2019-07-11T05:23:22Z

Test build #107500 has finished for PR 25074 at commit c22f12d.

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

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

dongjoon-hyun · 2019-07-16T04:53:25Z

Retest this please.

SparkQA · 2019-07-16T07:05:02Z

Test build #107720 has finished for PR 25074 at commit c22f12d.

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

beliefer · 2019-07-16T10:00:52Z

Retest this please.

SparkQA · 2019-07-16T10:13:27Z

Test build #107733 has finished for PR 25074 at commit c22f12d.

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

dongjoon-hyun · 2019-07-29T05:08:34Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala

+    def isFalse: Predicate = IsFalse(expr)
+    def isNotFalse: Predicate = IsNotFalse(expr)
+    def isUnknown: Predicate = IsUnknown(expr)
+    def isNotUnknown: Predicate = IsNotUnknown(expr)


Sorry for adding a comment for the late review rounds, but shall we avoid adding DSL since the whole picture is to support PostgreSQL feature parity? How do you think about that, @maropu ? Can we remove these DSL addition?

In that case, we should revert this and test("is true | false | unknown expressions").

I don't have a strong opnion on this though, +1 for your opnion. I don't think all the syntaxes need to be listed up here (It is enough to list up a basic part of them).

@dongjoon-hyun This is not only a PostgreSQL feature, but a ANSI SQL. I also want to confirm if I really want to delete this and test("is true | false | unknown expressions").

No, @beliefer . You are confused with Scala DSL and SQL.
This is sql/catalyst/dsl which providing Scala API. This is never ANSI SQL.
SQL Standard doesn't have def isNotUnknown. :)

No, @beliefer . You are confused with Scala DSL and SQL.
This is sql/catalyst/dsl which providing Scala API. This is never ANSI SQL.
SQL Standard doesn't have def isNotUnknown. :)

Thanks for your prompt. I am confused indeed and clear now.

dongjoon-hyun · 2019-07-29T05:21:21Z

For the others, this PR looks fine, @beliefer .
I think we can merge this if we can remove the DSL part and its test part.

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

dongjoon-hyun · 2019-07-30T00:43:44Z

Also, I slightly updated the PR description, @beliefer .

dongjoon-hyun

Please fix #25074 (comment) , @beliefer .

beliefer · 2019-07-30T04:29:24Z

Also, I slightly updated the PR description, @beliefer .

Thanks very much.

beliefer · 2019-07-30T04:35:16Z

Please fix #25074 (comment) , @beliefer .

I have reverted that code.

dongjoon-hyun · 2019-07-30T05:19:18Z

Thank you for updating again, @beliefer .

SparkQA · 2019-07-30T07:05:02Z

Test build #108368 has finished for PR 25074 at commit 4794b67.

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

dongjoon-hyun · 2019-07-30T07:19:29Z

Retest this please.

SparkQA · 2019-07-30T10:56:30Z

Test build #108373 has finished for PR 25074 at commit 4794b67.

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

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

SparkQA · 2019-07-31T06:59:14Z

Test build #108435 has finished for PR 25074 at commit ae126e7.

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

dongjoon-hyun

+1, LGTM. Thank you so much, @beliefer and @maropu !
Merged to master.

beliefer · 2019-07-31T07:13:27Z

@dongjoon-hyun @maropu Thanks for all your continuous review and help.

maropu · 2019-07-31T07:30:31Z

thanks for your hard work!

cloud-fan · 2020-02-21T13:55:32Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

      case SqlBaseParser.NULL =>
        IsNull(e)
+      case SqlBaseParser.TRUE => ctx.NOT match {
+        case null => IsTrue(e)


isn't it simply EqualNullSafe(e, Literal(true))? Why do we create a bunch of new expressions?

@cloud-fan It looks more reasonable.
But there exists a different the input expression must be of a boolean type. The input of EqualNullSafe is any DataType.
Do I need to adjust? cc @dongjoon-hyun @maropu

EqualNullSafe requires the left type to be the same as right type, so EqualNullSafe(e, Literal(true)) requires e to be boolean.

IsNull is a problem so I think that change makes sense.

Ah, I missed that approach and the @cloud-fan one sounds more reasonable. If @dongjoon-hyun is not against the idea, can you make a PR for that? @beliefer

@cloud-fan @maropu Let's wait @dongjoon-hyun , then I will make a PR.

Sorry for being late. +1 for that approach.

Thanks for the check, @dongjoon-hyun ! Plz go ahead, @beliefer

### What changes were proposed in this pull request? This PR follows #25074 and improves the implement. ### Why are the changes needed? Improve code. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Exists UT Closes #27699 from beliefer/improve-boolean-test. Authored-by: beliefer <beliefer@163.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

### What changes were proposed in this pull request? This PR follows #25074 and improves the implement. ### Why are the changes needed? Improve code. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Exists UT Closes #27699 from beliefer/improve-boolean-test. Authored-by: beliefer <beliefer@163.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 1515d45) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

### What changes were proposed in this pull request? This PR follows apache#25074 and improves the implement. ### Why are the changes needed? Improve code. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Exists UT Closes apache#27699 from beliefer/improve-boolean-test. Authored-by: beliefer <beliefer@163.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

beliefer added 14 commits July 5, 2019 18:47

implement boolean test.

4a9ac98

Add UT.

3963455

support boolean test.

00cc822

support boolean test.

5e72437

update code.

349d865

update code.

4ef1c12

update code.

3013cee

update UT.

6629b33

update UT.

a0d2d07

update code.

05d36f9

update ut.

bd05be2

update ut.

97e7812

update ut.

782b75e

update ut.

b2fc258

dongjoon-hyun added the SQL label Jul 8, 2019

Supplement Boolean test.

fa148db

Enhance UT.

c22f12d

maropu reviewed Jul 11, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/booleanExpressions.scala Outdated Show resolved Hide resolved

maropu reviewed Jul 11, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/booleanExpressions.scala Outdated Show resolved Hide resolved

dongjoon-hyun changed the title ~~[SPARK-27924]Support ANSI SQL Boolean-Predicate syntax~~ [SPARK-27924][SQL] Support ANSI SQL Boolean-Predicate syntax Jul 16, 2019

dongjoon-hyun reviewed Jul 29, 2019

View reviewed changes

maropu reviewed Jul 29, 2019

View reviewed changes

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

maropu reviewed Jul 29, 2019

View reviewed changes

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

dongjoon-hyun requested changes Jul 30, 2019

View reviewed changes

Optimize code.

4794b67

beliefer closed this Jul 30, 2019

beliefer reopened this Jul 30, 2019

maropu reviewed Jul 30, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala Outdated Show resolved Hide resolved

Fix ambiguous variable.

ae126e7

dongjoon-hyun approved these changes Jul 31, 2019

View reviewed changes

dongjoon-hyun closed this in d03ec65 Jul 31, 2019

beliefer mentioned this pull request Aug 6, 2019

[SPARK-27924][SQL][TEST][FOLLOW-UP] Enable Boolean-Predicate syntax tests #25366

Closed

cloud-fan reviewed Feb 21, 2020

View reviewed changes

beliefer mentioned this pull request Feb 26, 2020

[SPARK-27924][SQL][FOLLOW-UP] Improve ANSI SQL Boolean-Predicate #27699

Closed

Conversation

beliefer commented Jul 8, 2019 • edited by dongjoon-hyun 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 Jul 8, 2019

Uh oh!

SparkQA commented Jul 10, 2019

Uh oh!

beliefer commented Jul 10, 2019

Uh oh!

beliefer commented Jul 10, 2019

Uh oh!

SparkQA commented Jul 10, 2019

Uh oh!

SparkQA commented Jul 11, 2019

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun commented Jul 16, 2019

Uh oh!

SparkQA commented Jul 16, 2019

Uh oh!

beliefer commented Jul 16, 2019

Uh oh!

SparkQA commented Jul 16, 2019

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 Jul 30, 2019 • 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!

dongjoon-hyun commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun commented Jul 30, 2019

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

beliefer commented Jul 30, 2019

Uh oh!

beliefer commented Jul 30, 2019

Uh oh!

dongjoon-hyun commented Jul 30, 2019

Uh oh!

SparkQA commented Jul 30, 2019

Uh oh!

dongjoon-hyun commented Jul 30, 2019

Uh oh!

SparkQA commented Jul 30, 2019

Uh oh!

Uh oh!

SparkQA commented Jul 31, 2019

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

beliefer commented Jul 31, 2019

Uh oh!

maropu commented Jul 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beliefer Feb 21, 2020 • 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!

beliefer commented Jul 8, 2019 •

edited by dongjoon-hyun

Loading

dongjoon-hyun Jul 30, 2019 •

edited

Loading

dongjoon-hyun commented Jul 29, 2019 •

edited

Loading

beliefer Feb 21, 2020 •

edited

Loading

maropu Feb 21, 2020 •

edited

Loading