[SPARK-13732] [SPARK-13797] [SQL] Remove projectList from Window and Eliminate useless Window #11565

gatorsmile · 2016-03-07T22:33:49Z

What changes were proposed in this pull request?

projectList is useless. Its value is always the same as the child.output. Remove it from the class Window. Removal can simplify the codes in Analyzer and Optimizer.

This PR is based on the discussion started by @cloud-fan in a separate PR:
#5604 (comment)

This PR also eliminates useless Window.

@cloud-fan @yhuai

How was this patch tested?

Existing test cases cover it.

cloud-fan · 2016-03-10T05:12:54Z

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

+    // Prune windowExpressions and child of Window
+    case p @ Project(_, w: Window) if (w.outputSet -- p.references).nonEmpty =>
+      val newWindowExprs = w.windowExpressions.filter(p.references.contains)
+      val newGrandChild = prunedChild(w.child, w.references ++ p.references)


w.references ++ p.references are not the attributes we need from child right? First, we may filter out some window expressions and w.references is out of date. Second, w.child.output will never contains p.reference, as Window operator only produce window function results.

I see your points. Yeah, you are right. We should use the newWindowExprs.

For the second part, we still need p.references. After this PR, the output of Window is child.output ++ windowExpression.map(_.toAttribute). Thus, the attributes used in Project have to be kept. Is my understanding right?

How about this?

prunedChild(w.child, p.references ++ AttributeSet(newWindowExprs.flatMap(_.references)))

haha, based on your suggestion, I found another rule to eliminate useless Window. : )

Then, we also can convert Aggregate to Project if Aggregate does not have any aggregate function.

SparkQA · 2016-03-10T06:32:57Z

Test build #52815 has finished for PR 11565 at commit 4dd3e66.

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

cloud-fan · 2016-03-10T07:25:30Z

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

@@ -384,9 +374,25 @@ object ColumnPruning extends Rule[LogicalPlan] {
    // Eliminate no-op Projects
    case p @ Project(projectList, child) if sameOutput(child.output, p.output) => child

+    // Eliminate no-op Window
+    case w: Window if sameOutput(w.child.output, w.output) => w.child


w.windowExpressions.empty looks simplier?

yeah, will do

SparkQA · 2016-03-10T07:49:56Z

Test build #52818 has finished for PR 11565 at commit 6cf6f44.

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

SparkQA · 2016-03-10T09:18:35Z

Test build #52820 has finished for PR 11565 at commit 44326f1.

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

cloud-fan · 2016-03-10T10:58:42Z

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

+            UnspecifiedFrame)).as('window) :: Nil,
+          'a :: Nil, 'b.asc :: Nil)
+        .select('a, 'c, 'window).select('a, 'c, 'window, 'window)
+        .select('a, 'c, 'window).where('window > 1).select('a, 'c).analyze


It's weird to see 3 selects here, we can add CollapseProject rule into the optimizer in this test suite.

Will do. Thanks!

cloud-fan · 2016-03-10T11:00:25Z

LGTM exception 2 small comments. It will be better if we can add more DSL to build window expression, but I'm ok to do it in follow-ups.

cc @yhuai

gatorsmile · 2016-03-10T16:30:18Z

Will add more DSL for Window in a separate PR #11635, which is doing predicate push down for Window. Thank you for your detailed review! @cloud-fan

SparkQA · 2016-03-10T18:15:54Z

Test build #52831 has finished for PR 11565 at commit fc96d84.

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

cloud-fan · 2016-03-11T01:01:05Z

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

    // Can't prune the columns on LeafNode
    case p @ Project(_, l: LeafNode) => p

+    // Prune windowExpressions and child of Window
+    case p @ Project(_, w: Window) if (w.outputSet -- p.references).nonEmpty =>
+      val newWindowExprs = w.windowExpressions.filter(p.references.contains)


After rethink about it, seems we can still separate it into 2 rules.
We can add a def windowOutputSet = AttributeSet(windowExpressions.map(_.toAttribute)) to Window operator, and change the first case to case p @ Project(_, w: Window) if (w.windowOutputSet -- p.references).nonEmpty => and filter out useless window expressions

Will do it.Thanks!

cloud-fan · 2016-03-11T02:13:01Z

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

@@ -384,6 +381,9 @@ object ColumnPruning extends Rule[LogicalPlan] {
    // Eliminate no-op Projects
    case p @ Project(projectList, child) if sameOutput(child.output, p.output) => child

+    // Eliminate no-op Window
+    case w: Window if w.windowExpressions.isEmpty => w.child


move this rule near the rule that filter out useless window expressions, which makes people eaiser to understand.

cloud-fan · 2016-03-11T02:40:57Z

LGTM, thanks for working on it!
will merge after tests pass.

gatorsmile · 2016-03-11T03:34:30Z

Thank you for your detailed review! Actually, the problem was first found by you. I thought my PR can save your time, but it sounds like you still waste a lot of time on the review. Anyway, really thank you! @cloud-fan

SparkQA · 2016-03-11T03:46:17Z

Test build #52883 has finished for PR 11565 at commit 6a59b42.

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

cloud-fan · 2016-03-11T04:00:03Z

nvm, that's my job:)
Thanks, merging to master!

SparkQA · 2016-03-11T04:06:11Z

Test build #52885 has finished for PR 11565 at commit bd35ee7.

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

…iminate useless Window #### What changes were proposed in this pull request? `projectList` is useless. Its value is always the same as the child.output. Remove it from the class `Window`. Removal can simplify the codes in Analyzer and Optimizer. This PR is based on the discussion started by cloud-fan in a separate PR: apache#5604 (comment) This PR also eliminates useless `Window`. cloud-fan yhuai #### How was this patch tested? Existing test cases cover it. Author: gatorsmile <gatorsmile@gmail.com> Author: xiaoli <lixiao1983@gmail.com> Author: Xiao Li <xiaoli@Xiaos-MacBook-Pro.local> Closes apache#11565 from gatorsmile/removeProjListWindow.

gatorsmile and others added 30 commits November 13, 2015 14:50

Merge remote-tracking branch 'upstream/master'

01e4cdf

Merge remote-tracking branch 'upstream/master'

6835704

Merge remote-tracking branch 'upstream/master'

9180687

SPARK-11633

b38a21e

Merge remote-tracking branch 'upstream/master' into joinMakeCopy

d2b84af

Merge remote-tracking branch 'upstream/master'

fda8025

Merge branch 'master' of https://github.com/gatorsmile/spark

ac0dccd

Merge remote-tracking branch 'upstream/master'

6e0018b

converge

0546772

converge

b37a64f

Merge remote-tracking branch 'upstream/master'

c2a872c

Merge remote-tracking branch 'upstream/master'

ab6dbd7

Merge remote-tracking branch 'upstream/master'

4276356

Merge remote-tracking branch 'upstream/master'

2dab708

Merge remote-tracking branch 'upstream/master'

0458770

Merge remote-tracking branch 'upstream/master'

1debdfa

Merge remote-tracking branch 'upstream/master'

763706d

Merge remote-tracking branch 'upstream/master'

4de6ec1

Merge remote-tracking branch 'upstream/master'

9422a4f

Merge remote-tracking branch 'upstream/master'

52bdf48

Merge remote-tracking branch 'upstream/master'

1e95df3

Merge remote-tracking branch 'upstream/master'

fab24cf

Merge remote-tracking branch 'upstream/master'

8b2e33b

Merge remote-tracking branch 'upstream/master'

2ee1876

Merge remote-tracking branch 'upstream/master'

b9f0090

Merge remote-tracking branch 'upstream/master'

ade6f7e

Merge remote-tracking branch 'upstream/master'

9fd63d2

Merge remote-tracking branch 'upstream/master'

5199d49

Merge remote-tracking branch 'upstream/master'

404214c

Merge remote-tracking branch 'upstream/master'

c001dd9

cloud-fan reviewed Mar 10, 2016
View reviewed changes

gatorsmile changed the title ~~[SPARK-13732] [SQL] Remove projectList from Window~~ [SPARK-13732] [SPARK-13797] [SPARK-13798] [SQL] Remove projectList from Window and Eliminate useless Aggregate and Window Mar 10, 2016

eliminate useless Aggregate and Window

6cf6f44

cloud-fan reviewed Mar 10, 2016
View reviewed changes

gatorsmile changed the title ~~[SPARK-13732] [SPARK-13797] [SPARK-13798] [SQL] Remove projectList from Window and Eliminate useless Aggregate and Window~~ [SPARK-13732] [SPARK-13797] [SQL] Remove projectList from Window and Eliminate useless Aggregate and Window Mar 10, 2016

remove aggregate replacement.

44326f1

cloud-fan reviewed Mar 10, 2016
View reviewed changes

gatorsmile changed the title ~~[SPARK-13732] [SPARK-13797] [SQL] Remove projectList from Window and Eliminate useless Aggregate and Window~~ [SPARK-13732] [SPARK-13797] [SQL] Remove projectList from Window and Eliminate useless Window Mar 10, 2016

address comments.

fc96d84

gatorsmile mentioned this pull request Mar 10, 2016

[SPARK-13739] [SQL] Push Predicate Through Window #11635

Closed

2 tasks

cloud-fan reviewed Mar 11, 2016
View reviewed changes

address comments.

6a59b42

cloud-fan reviewed Mar 11, 2016
View reviewed changes

address comments.

bd35ee7

asfgit closed this in 560489f Mar 11, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-13732] [SPARK-13797] [SQL] Remove projectList from Window and Eliminate useless Window #11565

[SPARK-13732] [SPARK-13797] [SQL] Remove projectList from Window and Eliminate useless Window #11565

gatorsmile commented Mar 7, 2016

cloud-fan Mar 10, 2016

gatorsmile Mar 10, 2016

gatorsmile Mar 10, 2016

gatorsmile Mar 10, 2016

SparkQA commented Mar 10, 2016

cloud-fan Mar 10, 2016

gatorsmile Mar 10, 2016

SparkQA commented Mar 10, 2016

SparkQA commented Mar 10, 2016

cloud-fan Mar 10, 2016

gatorsmile Mar 10, 2016

cloud-fan commented Mar 10, 2016

gatorsmile commented Mar 10, 2016

SparkQA commented Mar 10, 2016

cloud-fan Mar 11, 2016

gatorsmile Mar 11, 2016

cloud-fan Mar 11, 2016

gatorsmile Mar 11, 2016

cloud-fan commented Mar 11, 2016

gatorsmile commented Mar 11, 2016

SparkQA commented Mar 11, 2016

cloud-fan commented Mar 11, 2016

SparkQA commented Mar 11, 2016

[SPARK-13732] [SPARK-13797] [SQL] Remove projectList from Window and Eliminate useless Window #11565

[SPARK-13732] [SPARK-13797] [SQL] Remove projectList from Window and Eliminate useless Window #11565

Conversation

gatorsmile commented Mar 7, 2016

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 10, 2016

SparkQA commented Mar 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Mar 10, 2016

gatorsmile commented Mar 10, 2016

SparkQA commented Mar 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Mar 11, 2016

gatorsmile commented Mar 11, 2016

SparkQA commented Mar 11, 2016

cloud-fan commented Mar 11, 2016

SparkQA commented Mar 11, 2016