[SPARK-20636] Add new optimization rule to transpose adjacent Window expressions. #17899

ptkool · 2017-05-08T10:35:47Z

What changes were proposed in this pull request?

Add new optimization rule to eliminate unnecessary shuffling by flipping adjacent Window expressions.

How was this patch tested?

Tested with unit tests, integration tests, and manual tests.

gatorsmile · 2017-05-08T20:30:15Z

ok to test

SparkQA · 2017-05-08T20:34:35Z

Test build #76591 has finished for PR 17899 at commit f90c857.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-05-08T20:34:51Z

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

This condition might not be enough. w1 might depend on the outputs of w2, right?

You are also changing the order of the columns. You will need to add a projection on top to be sure.

SparkQA · 2017-05-09T12:39:44Z

Test build #76678 has finished for PR 17899 at commit 300edf0.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-05-09T12:49:31Z

Test build #76679 has finished for PR 17899 at commit eb2def2.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-05-09T14:11:51Z

Test build #76680 has finished for PR 17899 at commit f6a4e47.

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

SparkQA · 2017-05-09T21:03:30Z

Test build #76690 has finished for PR 17899 at commit 1ab81ca.

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

hvanhovell · 2017-05-16T09:16:19Z

retest this please

hvanhovell · 2017-05-16T09:21:45Z

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

This probably warrants a follow-up that tries to move projections that are wedged in between two window clauses.

hvanhovell · 2017-05-16T09:35:18Z

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

You need to check that the windows are independent, e.g.: w1.references.intersect(w2.windowOutputSet).isEmpty

We might be able to get a little more milage out of the rule by using semanticEquals for comparing the partition expressions, e.g.:

def sliceSemanticEquals(ps1: Seq[Expression], ps2: Seq[Expression]): Boolean = ps1.zip(ps2).forall { case (l, r) => l.semanticEquals(r) } ... sliceSemanticEquals(ps1, ps2)

You could even get more leverage if you do not consider the order of the partition spec.

hvanhovell · 2017-05-16T11:36:06Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameWindowFunctionsSuite.scala

Why this test? It does not really add anything new.

Agreed. I'll remove it.

hvanhovell · 2017-05-16T11:37:16Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/TransposeWindowSuite.scala

comparePlans(optimized, analyzed)?

hvanhovell · 2017-05-16T11:38:36Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/TransposeWindowSuite.scala

I am not entirely sure if we need this test.

Agreed. I'll remove it.

hvanhovell

@ptkool this looks pretty good. One thing need to be addressed though: we need to factor in that a parent window can depend on its child window.

SparkQA · 2017-05-16T17:48:02Z

Test build #76970 has finished for PR 17899 at commit 1ab81ca.

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

SparkQA · 2017-05-20T14:59:04Z

Test build #77126 has finished for PR 17899 at commit f472bfe.

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

ptkool · 2017-06-03T21:41:38Z

@hvanhovell @gatorsmile Can you have another look at this?

gatorsmile · 2017-06-30T05:32:19Z

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

No test case covers the condition w1.references.intersect(w2.windowOutputSet).isEmpty

I will add one.

gatorsmile · 2017-06-30T05:36:49Z

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

The expressions in both w1.expressions and w2.expressions must be deterministic. If not, we should not flip

Why? This seems overly restrictive to me.

Just to ensure the results are still the same with and without the rule.

SparkQA · 2017-06-30T09:34:31Z

Test build #78973 has finished for PR 17899 at commit e2f24c2.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-10-28T05:27:01Z

@ptkool Can you address the conflicts? We will review it.

SparkQA · 2017-10-29T12:04:21Z

Test build #83189 has finished for PR 17899 at commit 54d88fa.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-10-29T12:39:24Z

Test build #83191 has finished for PR 17899 at commit 82d7390.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-10-29T12:49:19Z

Test build #83193 has finished for PR 17899 at commit f840c69.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-10-29T15:52:51Z

Test build #83194 has finished for PR 17899 at commit e9f6928.

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

SparkQA · 2018-01-19T00:04:28Z

Test build #86355 has finished for PR 17899 at commit 72a4b3a.

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

SparkQA · 2018-09-06T15:16:39Z

Test build #95753 has finished for PR 17899 at commit 94e8115.

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

HyukjinKwon · 2018-09-07T04:34:38Z

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

 object CollapseWindow extends Rule[LogicalPlan] {
  def apply(plan: LogicalPlan): LogicalPlan = plan transformUp {
    case w1 @ Window(we1, ps1, os1, w2 @ Window(we2, ps2, os2, grandChild))
-        if ps1 == ps2 && os1 == os2 && w1.references.intersect(w2.windowOutputSet).isEmpty &&


I wouldn't include style changes

SparkQA · 2018-09-07T20:05:38Z

Test build #95802 has finished for PR 17899 at commit 4328831.

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

gatorsmile

LGTM

Thanks! Merged to master.

cloud-fan · 2018-12-05T07:06:48Z

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

 }

+/**
+ * Transpose Adjacent Window Expressions.


why is this rule useful?

## What changes were proposed in this pull request? This PR is a follow-up of the PR apache#17899. It is to add the rule TransposeWindow the optimizer batch. ## How was this patch tested? The existing tests. Closes apache#23222 from gatorsmile/followupSPARK-20636. Authored-by: gatorsmile <[email protected]> Signed-off-by: gatorsmile <[email protected]>

gatorsmile reviewed May 8, 2017

View reviewed changes

ptkool force-pushed the adjacent_window_optimization branch from f6a4e47 to 1ab81ca Compare May 9, 2017 18:38

hvanhovell reviewed May 16, 2017

View reviewed changes

hvanhovell requested changes May 16, 2017

View reviewed changes

ptkool force-pushed the adjacent_window_optimization branch from 1ab81ca to f472bfe Compare May 20, 2017 12:37

ptkool changed the title ~~[SPARK-20636] Add new optimization rule to flip adjacent Window expressions.~~ [SPARK-20636] Add new optimization rule to transpose adjacent Window expressions. Jun 7, 2017

gatorsmile reviewed Jun 30, 2017

View reviewed changes

ptkool force-pushed the adjacent_window_optimization branch from f472bfe to e2f24c2 Compare June 30, 2017 09:31

ptkool added 3 commits October 29, 2017 08:42

Add new optimization rule to flip adjacent Window expressions.

f444b2c

Changes based on feedback.

6f8c036

Resolve Scala style errors.

b7793cd

ptkool added 5 commits October 29, 2017 08:42

Resolve more Scala style issues.

f183b78

Small tweak to rule.

1f21841

Changes based on feedback.

9a28535

New test case.

76f1721

Fix scalastyle errors

f840c69

ptkool force-pushed the adjacent_window_optimization branch from 82d7390 to f840c69 Compare October 29, 2017 12:43

Fix another scalastyle error

e9f6928

Resolve conflicts

72a4b3a

Resolve conflicts

94e8115

HyukjinKwon reviewed Sep 7, 2018

View reviewed changes

Remove style changes

4328831

gatorsmile reviewed Sep 8, 2018

View reviewed changes

asfgit closed this in 78981ef Sep 8, 2018

gatorsmile mentioned this pull request Dec 5, 2018

[SPARK-20636] Add the rule TransposeWindow to the optimization batch #23222

Closed

cloud-fan reviewed Dec 5, 2018

View reviewed changes

ptkool deleted the adjacent_window_optimization branch January 18, 2020 12:14

[SPARK-20636] Add new optimization rule to transpose adjacent Window expressions. #17899

[SPARK-20636] Add new optimization rule to transpose adjacent Window expressions. #17899

Uh oh!

Conversation

ptkool commented May 8, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

gatorsmile commented May 8, 2017

Uh oh!

SparkQA commented May 8, 2017

Uh oh!

gatorsmile May 8, 2017 • 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!

SparkQA commented May 9, 2017

Uh oh!

SparkQA commented May 9, 2017

Uh oh!

SparkQA commented May 9, 2017

Uh oh!

SparkQA commented May 9, 2017

Uh oh!

hvanhovell commented May 16, 2017

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!

ptkool May 18, 2017 • 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!

hvanhovell left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 16, 2017

Uh oh!

SparkQA commented May 20, 2017

Uh oh!

ptkool commented Jun 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile Jun 30, 2017 • 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!

SparkQA commented Jun 30, 2017

Uh oh!

gatorsmile commented Oct 28, 2017

Uh oh!

SparkQA commented Oct 29, 2017

Uh oh!

SparkQA commented Oct 29, 2017

Uh oh!

SparkQA commented Oct 29, 2017

gatorsmile May 8, 2017 •

edited

Loading

ptkool May 18, 2017 •

edited

Loading

gatorsmile Jun 30, 2017 •

edited

Loading