[SPARK-24966][SQL] Implement precedence rules for set operations. #21941

dilipbiswal · 2018-08-01T05:49:57Z

What changes were proposed in this pull request?

Currently the set operations INTERSECT, UNION and EXCEPT are assigned the same precedence. This PR fixes the problem by giving INTERSECT higher precedence than UNION and EXCEPT. UNION and EXCEPT operators are evaluated in the order in which they appear in the query from left to right.

This results in change in behavior because of the change in order of evaluations of set operators in a query. The old behavior is still preserved under a newly added config parameter.

Query :

SELECT * FROM t1
UNION 
SELECT * FROM t2
EXCEPT
SELECT * FROM t3
INTERSECT
SELECT * FROM t4

Parsed plan before the change :

== Parsed Logical Plan ==
'Intersect false
:- 'Except false
:  :- 'Distinct
:  :  +- 'Union
:  :     :- 'Project [*]
:  :     :  +- 'UnresolvedRelation `t1`
:  :     +- 'Project [*]
:  :        +- 'UnresolvedRelation `t2`
:  +- 'Project [*]
:     +- 'UnresolvedRelation `t3`
+- 'Project [*]
   +- 'UnresolvedRelation `t4`

Parsed plan after the change :

== Parsed Logical Plan ==
'Except false
:- 'Distinct
:  +- 'Union
:     :- 'Project [*]
:     :  +- 'UnresolvedRelation `t1`
:     +- 'Project [*]
:        +- 'UnresolvedRelation `t2`
+- 'Intersect false
   :- 'Project [*]
   :  +- 'UnresolvedRelation `t3`
   +- 'Project [*]
      +- 'UnresolvedRelation `t4`

How was this patch tested?

Added tests in PlanParserSuite, SQLQueryTestSuite.

Please review http://spark.apache.org/contributing.html before opening a pull request.

holdensmagicalunicorn · 2018-08-01T05:50:01Z

@dilipbiswal, thanks! I am a bot who has found some folks who might be able to help with the review:@gatorsmile, @rxin and @hvanhovell

gatorsmile · 2018-08-01T06:07:26Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

When true, INTERSECT is given precedence over UNION and EXCEPT set operations as per

->

When true, INTERSECT is given the greater precedence over the other set operations (UNION, EXCEPT and MINUS) as per

gatorsmile · 2018-08-01T06:08:23Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala

also add withSQLConf(SQLConf.SETOPS_PRECEDENCE_ENFORCED.key -> "true") {

gatorsmile · 2018-08-01T06:09:46Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

also include MINUS

gatorsmile · 2018-08-01T06:11:14Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

let me think about the name of conf

@gatorsmile Sure.

@gatorsmile A couple of alternatives i had considered :-). FYI.
spark.sql.set.operators.precedence.enforced
spark.sql.set.operators.standard.compliance

spark.sql.legacy.setopsPrecedence.enabled

dilipbiswal · 2018-08-01T06:11:43Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

This is not related to the current PR. This addresses a comment from @HyukjinKwon in 21886

dilipbiswal · 2018-08-01T06:12:09Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala

This is not related to the current PR. This addresses a comment from @HyukjinKwon in 21886

dilipbiswal · 2018-08-01T06:12:15Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala

This is not related to the current PR. This addresses a comment from @HyukjinKwon in 21886

SparkQA · 2018-08-01T09:04:36Z

Test build #93871 has finished for PR 21941 at commit c0821b6.

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

SparkQA · 2018-08-01T10:43:35Z

Test build #93872 has finished for PR 21941 at commit 47cbc5a.

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

gatorsmile · 2018-08-01T20:23:19Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

let us mark it internal.

gatorsmile · 2018-08-01T20:24:03Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

SparkQA · 2018-08-02T03:16:58Z

Test build #93917 has finished for PR 21941 at commit e7d69db.

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

dilipbiswal · 2018-08-02T03:25:05Z

retest this please

SparkQA · 2018-08-02T05:32:28Z

Test build #93936 has finished for PR 21941 at commit e7d69db.

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

dilipbiswal · 2018-08-02T06:48:13Z

@maropu Is this a transient failure ? Does not seem related to my change ?

maropu · 2018-08-02T06:59:27Z

no idea, but HiveClientSuites seems flaky: https://issues.apache.org/jira/browse/SPARK-23622 (the error message is different though...)

maropu · 2018-08-02T06:59:35Z

retest this please

SparkQA · 2018-08-02T09:19:25Z

Test build #93966 has finished for PR 21941 at commit e7d69db.

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

dilipbiswal · 2018-08-02T10:27:19Z

retest this please

SparkQA · 2018-08-02T12:28:08Z

Test build #93989 has finished for PR 21941 at commit e7d69db.

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

dilipbiswal · 2018-08-02T12:57:14Z

retest this please

SparkQA · 2018-08-02T16:07:29Z

Test build #94003 has finished for PR 21941 at commit e7d69db.

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

gatorsmile · 2018-08-02T16:30:37Z

retest this please

SparkQA · 2018-08-02T18:24:19Z

Test build #94028 has finished for PR 21941 at commit e7d69db.

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

dilipbiswal · 2018-08-02T18:46:54Z

@gatorsmile rebasing in a hope that the test result changes :-)

SparkQA · 2018-08-02T19:10:55Z

Test build #94046 has finished for PR 21941 at commit bc6df75.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2018-08-02T19:26:46Z

retest this please

SparkQA · 2018-08-02T23:45:06Z

Test build #94053 has finished for PR 21941 at commit bc6df75.

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

gatorsmile · 2018-08-03T05:04:15Z

LGTM

Thanks! Merged to master.

dilipbiswal · 2018-08-03T05:40:55Z

Thanks a lot @gatorsmile

gatorsmile reviewed Aug 1, 2018

View reviewed changes

dilipbiswal commented Aug 1, 2018

View reviewed changes

gatorsmile reviewed Aug 1, 2018

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated

Copy link

Member

gatorsmile Aug 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let us mark it internal.

gatorsmile reviewed Aug 1, 2018

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated

Copy link

Member

gatorsmile Aug 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and

dilipbiswal added 3 commits August 2, 2018 11:46

[SPARK-24966] Implement precedence rules for set operations.

26d34a2

Code review

f9b8bc5

Code review

bc6df75

dilipbiswal force-pushed the SPARK-24966 branch from e7d69db to bc6df75 Compare August 2, 2018 18:46

asfgit closed this in 73dd6cf Aug 3, 2018

[SPARK-24966][SQL] Implement precedence rules for set operations. #21941

[SPARK-24966][SQL] Implement precedence rules for set operations. #21941

Uh oh!

Conversation

dilipbiswal commented Aug 1, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

holdensmagicalunicorn commented Aug 1, 2018

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!

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!

SparkQA commented Aug 1, 2018

Uh oh!

SparkQA commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 2, 2018

Uh oh!

dilipbiswal commented Aug 2, 2018

Uh oh!

SparkQA commented Aug 2, 2018

Uh oh!

dilipbiswal commented Aug 2, 2018

Uh oh!

maropu commented Aug 2, 2018

Uh oh!

maropu commented Aug 2, 2018

Uh oh!

SparkQA commented Aug 2, 2018

Uh oh!

dilipbiswal commented Aug 2, 2018

Uh oh!

SparkQA commented Aug 2, 2018

Uh oh!

dilipbiswal commented Aug 2, 2018

Uh oh!

SparkQA commented Aug 2, 2018

Uh oh!

gatorsmile commented Aug 2, 2018

Uh oh!

SparkQA commented Aug 2, 2018

Uh oh!

dilipbiswal commented Aug 2, 2018

Uh oh!

SparkQA commented Aug 2, 2018

Uh oh!

gatorsmile commented Aug 2, 2018

Uh oh!

SparkQA commented Aug 2, 2018

Uh oh!

gatorsmile commented Aug 3, 2018

Uh oh!

dilipbiswal commented Aug 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects