[SPARK-9778][SQL] remove unnecessary evaluation from SortOrder #8066

cloud-fan · 2015-08-10T08:04:58Z

If the order-by expression is already in child's output, we can just reference it instead of evaluate it agian.

cloud-fan · 2015-08-10T08:05:54Z

cc @marmbrus

cloud-fan · 2015-08-10T13:01:38Z

retest this please.

SparkQA · 2015-08-10T19:06:39Z

Test build #40290 has finished for PR 8066 at commit eaa6002.

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

marmbrus · 2015-08-10T23:20:58Z

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

I would be good if we could just do this as part of CleanAliases in #7957. Basically it seems that there is no reason to have an Alias except as a top level expression in Project or Aggregate.

cloud-fan · 2015-08-15T13:38:54Z

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

We can't do this optimization in Analyzer.ResolveSortReferences as the aggregate expressions inside ordering expression are unresolved at that time. We need to add them to underlying Aggregate first and let other rules to resolve them.

SparkQA · 2015-08-15T13:59:15Z

Test build #40960 has finished for PR 8066 at commit abc0fc3.

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

SparkQA · 2015-08-15T16:00:27Z

Test build #40965 has finished for PR 8066 at commit 30fceb9.

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

SparkQA · 2015-08-15T16:07:25Z

Test build #40962 has finished for PR 8066 at commit 28eb6bb.

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

SparkQA · 2015-08-15T17:18:37Z

Test build #40964 has finished for PR 8066 at commit 9f2c1c3.

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

chenghao-intel · 2015-08-17T05:25:11Z

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

Use semanticEquals or sameRef instead?

cloud-fan · 2015-09-07T15:19:23Z

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

We should not do this optimization during analysis. Logically optimizations should go into Optimizer, and for this case, the match may fail because of non-complete Cast expressions caused by type coercion rules

SparkQA · 2015-09-07T16:40:25Z

Test build #42098 has finished for PR 8066 at commit a2626fb.

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

cloud-fan · 2015-09-07T17:13:27Z

ping @marmbrus

SparkQA · 2015-09-23T02:18:59Z

Test build #42870 has finished for PR 8066 at commit 57405b3.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class WriterThread(

cloud-fan · 2015-12-02T15:48:45Z

Hi @marmbrus , I updated it and did some simplification, could you take a look? Thanks!

SparkQA · 2015-12-02T17:32:07Z

Test build #47071 has finished for PR 8066 at commit 772dc7f.

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

marmbrus · 2015-12-02T18:18:41Z

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

nit: could you do e.asc instead?

SparkQA · 2015-12-02T18:21:08Z

Test build #47073 has finished for PR 8066 at commit a3e1313.

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

marmbrus · 2015-12-02T18:23:48Z

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

Nit: to follow the other suites this would probably be SortOptimizationSuite

marmbrus · 2015-12-02T18:25:25Z

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

Would this be simpler as a find?

marmbrus · 2015-12-02T18:26:15Z

A few minor comments, overall LGTM.

SparkQA · 2015-12-03T15:28:20Z

Test build #47138 has finished for PR 8066 at commit 79f2dc9.

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

SparkQA · 2015-12-03T15:47:35Z

Test build #47141 has finished for PR 8066 at commit d5fdeff.

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

SparkQA · 2015-12-03T15:53:00Z

Test build #47142 has finished for PR 8066 at commit 1cc52f2.

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

SparkQA · 2016-04-21T08:59:03Z

Test build #56519 has finished for PR 8066 at commit e023482.

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

SparkQA · 2016-04-21T10:18:25Z

Test build #56521 has finished for PR 8066 at commit af1a83d.

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

cloud-fan · 2016-04-22T01:18:56Z

retest this please.

SparkQA · 2016-04-22T02:38:07Z

Test build #56624 has finished for PR 8066 at commit af1a83d.

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

SparkQA · 2016-04-22T08:10:44Z

Test build #56666 has finished for PR 8066 at commit 4baaf55.

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

rxin · 2016-07-07T00:59:46Z

Close this?

HyukjinKwon · 2017-02-01T12:34:06Z

Do you mind if I ask why it was asked to be closed? (I am just purely curious while looking through PRs and JIRAs).

marmbrus reviewed Aug 10, 2015
View reviewed changes

cloud-fan force-pushed the order-by branch 2 times, most recently from a1fabce to abc0fc3 Compare August 15, 2015 13:32

cloud-fan reviewed Aug 15, 2015
View reviewed changes

cloud-fan force-pushed the order-by branch from 9f2c1c3 to 30fceb9 Compare August 15, 2015 15:28

chenghao-intel reviewed Aug 17, 2015
View reviewed changes

cloud-fan force-pushed the order-by branch from 30fceb9 to a2626fb Compare September 7, 2015 14:51

cloud-fan reviewed Sep 7, 2015
View reviewed changes

cloud-fan force-pushed the order-by branch from a2626fb to 57405b3 Compare September 22, 2015 23:30

cloud-fan force-pushed the order-by branch from 57405b3 to 772dc7f Compare December 2, 2015 15:46

marmbrus reviewed Dec 2, 2015
View reviewed changes

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

Copy link

Contributor

marmbrus Dec 2, 2015

Choose a reason for hiding this comment

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

Would this be simpler as a find?

cloud-fan force-pushed the order-by branch 3 times, most recently from d5fdeff to 1cc52f2 Compare December 3, 2015 14:02

cloud-fan force-pushed the order-by branch from 1cc52f2 to e023482 Compare April 21, 2016 08:55

remove unnecessary evaluation from SortOrder

af1a83d

cloud-fan force-pushed the order-by branch from e023482 to af1a83d Compare April 21, 2016 09:07

fix tests

4baaf55

cloud-fan closed this Jul 7, 2016

[SPARK-9778][SQL] remove unnecessary evaluation from SortOrder #8066

[SPARK-9778][SQL] remove unnecessary evaluation from SortOrder #8066

Uh oh!

Conversation

cloud-fan commented Aug 10, 2015

Uh oh!

cloud-fan commented Aug 10, 2015

Uh oh!

cloud-fan commented Aug 10, 2015

Uh oh!

SparkQA commented Aug 10, 2015

Uh oh!

marmbrus Aug 10, 2015

Choose a reason for hiding this comment

Uh oh!

cloud-fan Aug 15, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 15, 2015

Uh oh!

SparkQA commented Aug 15, 2015

Uh oh!

SparkQA commented Aug 15, 2015

Uh oh!

SparkQA commented Aug 15, 2015

Uh oh!

chenghao-intel Aug 17, 2015

Choose a reason for hiding this comment

Uh oh!

cloud-fan Sep 7, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 7, 2015

Uh oh!

cloud-fan commented Sep 7, 2015

Uh oh!

SparkQA commented Sep 23, 2015

Uh oh!

cloud-fan commented Dec 2, 2015

Uh oh!

SparkQA commented Dec 2, 2015

Uh oh!

marmbrus Dec 2, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 2, 2015

Uh oh!

marmbrus Dec 2, 2015

Choose a reason for hiding this comment

Uh oh!

marmbrus Dec 2, 2015

Choose a reason for hiding this comment

Uh oh!

marmbrus commented Dec 2, 2015

Uh oh!

SparkQA commented Dec 3, 2015

Uh oh!

SparkQA commented Dec 3, 2015

Uh oh!

SparkQA commented Dec 3, 2015

Uh oh!

SparkQA commented Apr 21, 2016

Uh oh!

SparkQA commented Apr 21, 2016

Uh oh!

cloud-fan commented Apr 22, 2016

Uh oh!

SparkQA commented Apr 22, 2016

Uh oh!

SparkQA commented Apr 22, 2016

Uh oh!

rxin commented Jul 7, 2016

Uh oh!

HyukjinKwon commented Feb 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone