[SPARK-21274][SQL] Implement EXCEPT ALL clause. #21857

dilipbiswal · 2018-07-24T07:41:27Z

What changes were proposed in this pull request?

Implements EXCEPT ALL clause through query rewrites using existing operators in Spark. In this PR, an internal UDTF (replicate_rows) is added to aid in preserving duplicate rows. Please refer to Link for the design.

Note This proposed UDTF is kept as a internal function that is purely used to aid with this particular rewrite to give us flexibility to change to a more generalized UDTF in future.

Input Query

SELECT c1 FROM ut1 EXCEPT ALL SELECT c1 FROM ut2

Rewritten Query

SELECT c1
    FROM (
     SELECT replicate_rows(sum_val, c1)
       FROM (
         SELECT c1, sum_val
           FROM (
             SELECT c1, sum(vcol) AS sum_val
               FROM (
                 SELECT 1L as vcol, c1 FROM ut1
                 UNION ALL
                 SELECT -1L as vcol, c1 FROM ut2
              ) AS union_all
            GROUP BY union_all.c1
          )
        WHERE sum_val > 0
       )
   )

How was this patch tested?

Added test cases in SQLQueryTestSuite, DataFrameSuite and SetOperationSuite

SparkQA · 2018-07-24T07:51:35Z

Test build #93488 has finished for PR 21857 at commit 5cf8c4c.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ReplicateRows(children: Seq[Expression]) extends Generator with CodegenFallback
abstract class ExceptBase(left: LogicalPlan, right: LogicalPlan) extends SetOperation(left, right)
case class Except(left: LogicalPlan, right: LogicalPlan) extends ExceptBase(left, right)
case class ExceptAll(left: LogicalPlan, right: LogicalPlan) extends ExceptBase(left, right)

HyukjinKwon · 2018-07-24T09:05:01Z

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

If it's for an internal purpose, you can just remove this though.

@HyukjinKwon OK..

SparkQA · 2018-07-24T11:11:44Z

Test build #93490 has finished for PR 21857 at commit 988d3b4.

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

kiszk · 2018-07-24T12:56:27Z

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

nit: i -> _

kiszk · 2018-07-24T13:17:32Z

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

nit: a internal -> an internal

maropu

I left some comments.

maropu · 2018-07-24T13:38:37Z

sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala

IIUC the feature freeze will come soon, so I'm not sure this will appear in v2.4. cc: @gatorsmile @rxin

We are still targeting it to 2.4. If we are unable to make it, we can change it.

maropu · 2018-07-24T13:44:16Z

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

We need a logical node for ExceptAll? As another option, we can add a flag as a field value of Except (e.g., case class Except(left: LogicalPlan, right: LogicalPlan, all: Boolean)? I think Except and ExceptAll is not different for the analyzer.

@maropu Right. So this way , most of the pattern matching happens on the the Base class where things are common. I went back and forth on this as well.. If there is a consensus i will change it.

ok cc: @gatorsmile @cloud-fan

@maropu Some details to aid the decision making. I remember now.. This way, i had to change less number of files. I just looked at the usage of Except to double check.

Let us avoid adding a new logical plan node. : )

maropu · 2018-07-24T13:44:50Z

sql/core/src/test/resources/sql-tests/inputs/except-all.sql

I feel it's ok to move these tests to except.sql.

@maropu I thought we like to keep these sql files relatively small and not contain too many sqls.

maropu · 2018-07-24T13:46:42Z

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

nit: need indents

@maropu will check and fix.

maropu · 2018-07-24T13:50:17Z

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

We don't support codegen?

@maropu I would like to take this in a follow up. I think we have codegen disabled for generators in general. So we will not be able to take advantage of it ?

ah, ok. It sounds ok to me.

maropu · 2018-07-24T13:51:49Z

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

We need tests for the RewriteExceptAll rule (you can check RewriteDistinctAggregatesSuite as a reference).

Ah.. ok... Thanks a lot.

@maropu I have added a unit test to check the plan. Please look at it when you get a chance.

maropu · 2018-07-24T13:52:42Z

sql/core/src/test/resources/sql-tests/inputs/except-all.sql

super nit: need spaces like (0), (1), (2), (3), ....

@maropu Will do.

SparkQA · 2018-07-24T15:34:29Z

Test build #93495 has finished for PR 21857 at commit a6fc341.

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

gatorsmile · 2018-07-24T19:46:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

then, we do not need these changes.

@gatorsmile Thats right Sean. We will not need changes here. However may i request you to please command-B on Except class ? We may need to change the pattern matching in other places, right ? Just wanted to make sure you are okay with it before i went ahead and made the changes.

I am fine about that. Please make a change and avoid introducing a new LogicalPlan node.

@gatorsmile @maropu OK

@gatorsmile @maropu I have removed ExceptAll operator.

SparkQA · 2018-07-24T19:49:11Z

Test build #93505 has finished for PR 21857 at commit 96e2dc9.

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

gatorsmile · 2018-07-24T19:50:30Z

@dilipbiswal Also post the design doc in the PR description?

gatorsmile · 2018-07-24T19:50:44Z

cc @ueshin @maryannxue Please review this?

dilipbiswal · 2018-07-24T19:53:21Z

@gatorsmile I have the link to the design doc in the description ? Is there another way ?

SparkQA · 2018-07-24T21:33:03Z

Test build #93510 has finished for PR 21857 at commit c516f78.

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

ueshin · 2018-07-25T04:26:16Z

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

nit: there is no cnt?

@ueshin Thanks.. will change.

We can remove it from the pr description as well.

@ueshin Done. Thanks.

SparkQA · 2018-07-26T03:01:16Z

Test build #93573 has finished for PR 21857 at commit b201b88.

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

SparkQA · 2018-07-26T09:00:17Z

Test build #93587 has finished for PR 21857 at commit c49b15c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Except(

SparkQA · 2018-07-26T11:39:38Z

Test build #93595 has finished for PR 21857 at commit 93abd51.

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

gatorsmile · 2018-07-26T18:10:53Z

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

update this?

Ah.. sorry.. will change it to Except.

gatorsmile · 2018-07-26T18:12:44Z

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

qualifier = None, ....

@gatorsmile will change.

gatorsmile · 2018-07-26T18:14:45Z

sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala

isAll = true

@gatorsmile will change

viirya · 2018-07-26T18:29:33Z

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

Does replicate_rows's output include sum_val? I think it removes the multiplier value from output?

@viirya Yes it does. You had suggested to remove it in the generator PR. At that time it was an external function and i wanted to match with hive. Now that its an internal function i decided to remove it as per your suggestion.

So I think here it should be replicate_rows(sum_val, c1) AS c1?

yes.. thanks.. will fix.

Please don't forget to update the pr description as well.

viirya · 2018-07-26T18:34:26Z

sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala

I think it is good to mention resolves columns by position (not by name) here too.

viirya · 2018-07-26T18:36:52Z

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

there is no operator called "except all " ?

hmm, it looks no diff to above one. Maybe except (all) operator?

ok.. will change to except (all).. the difference was in the replaced operators, actually.

viirya · 2018-07-26T18:38:09Z

python/pyspark/sql/dataframe.py

Maybe it is better to add one more row to show the behavior of preserving duplicates.

SparkQA · 2018-07-26T21:49:00Z

Test build #93610 has finished for PR 21857 at commit a415f6a.

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

SparkQA · 2018-07-27T03:05:37Z

Test build #93639 has finished for PR 21857 at commit f0da978.

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

gatorsmile · 2018-07-27T04:05:49Z

sql/core/src/test/resources/sql-tests/inputs/except-all.sql

@gatorsmile Thank you. fixed.

SparkQA · 2018-07-27T07:05:02Z

Test build #93656 has finished for PR 21857 at commit 1f107aa.

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

gatorsmile

LGTM

SparkQA · 2018-07-27T20:37:09Z

Test build #93678 has finished for PR 21857 at commit 4e04883.

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

gatorsmile · 2018-07-27T20:59:45Z

Thanks! Merged to master.

dilipbiswal · 2018-07-27T22:00:00Z

Thank you very much @gatorsmile @maropu @viirya @ueshin @HyukjinKwon @kiszk

dilipbiswal changed the title ~~[SPARK-21274] Implement EXCEPT ALL clause.~~ [SPARK-21274][SQL] Implement EXCEPT ALL clause. Jul 24, 2018

HyukjinKwon reviewed Jul 24, 2018

View reviewed changes

kiszk reviewed Jul 24, 2018

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/generators.scala Outdated

Copy link

Member

kiszk Jul 24, 2018

Choose a reason for hiding this comment

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

nit: i -> _

kiszk reviewed Jul 24, 2018

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/generators.scala Outdated

Copy link

Member

kiszk Jul 24, 2018

Choose a reason for hiding this comment

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

nit: a internal -> an internal

maropu reviewed Jul 24, 2018

View reviewed changes

gatorsmile reviewed Jul 24, 2018

View reviewed changes

ueshin reviewed Jul 25, 2018

View reviewed changes

dilipbiswal force-pushed the dkb_except_all_final branch from 93abd51 to a415f6a Compare July 26, 2018 17:47

gatorsmile reviewed Jul 26, 2018

View reviewed changes

viirya reviewed Jul 26, 2018

View reviewed changes

gatorsmile reviewed Jul 27, 2018

View reviewed changes

dilipbiswal added 3 commits July 27, 2018 00:28

[SPARK-21274] Implement EXCEPT ALL clause.

a157482

python docstyle

62ab006

Code review + test failures

38a948f

dilipbiswal added 9 commits July 27, 2018 00:28

code review

072e0d7

Add a unit test to test rewrite

7e451e3

code review

c39f88e

Remove exceptall logical operator

83ea225

fix

f23bc47

minor

8e0c1ef

code review

a7ec7b6

missing ;

1e50b9d

rebase error

4e04883

dilipbiswal force-pushed the dkb_except_all_final branch from 1f107aa to 4e04883 Compare July 27, 2018 16:38

gatorsmile approved these changes Jul 27, 2018

View reviewed changes

asfgit closed this in 10f1f19 Jul 27, 2018

[SPARK-21274][SQL] Implement EXCEPT ALL clause. #21857

[SPARK-21274][SQL] Implement EXCEPT ALL clause. #21857

Uh oh!

Conversation

dilipbiswal commented Jul 24, 2018 • edited 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 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu left a comment

Choose a reason for hiding this comment

Uh oh!

maropu Jul 24, 2018 • 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!

maropu Jul 24, 2018 • 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!

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!

maropu Jul 24, 2018 • 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!

dilipbiswal Jul 24, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dilipbiswal Jul 24, 2018 • 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!

dilipbiswal commented Jul 24, 2018 •

edited

Loading

maropu Jul 24, 2018 •

edited

Loading

maropu Jul 24, 2018 •

edited

Loading

maropu Jul 24, 2018 •

edited

Loading

dilipbiswal Jul 24, 2018 •

edited

Loading

dilipbiswal Jul 24, 2018 •

edited

Loading

gatorsmile Jul 26, 2018 •

edited

Loading