[SPARK-7067][SQL] fix bug when use complex nested fields in ORDER BY #5659

cloud-fan · 2015-04-23T09:58:33Z

This PR is a improvement for #5189.

The resolution rule for ORDER BY is: first resolve based on what comes from the select clause and then fall back on its child only when this fails.

There are 2 steps. First, try to resolve Sort in ResolveReferences based on select clause, and ignore exceptions. Second, try to resolve Sort in ResolveSortReferences and add missing projection.

However, the way we resolve SortOrder is wrong. We just resolve UnresolvedAttribute and use the result to indicate if we can resolve SortOrder. But UnresolvedAttribute is only part of GetField chain(broken by GetItem), so we need to go through the whole chain to indicate if we can resolve SortOrder.

With this change, we can also avoid re-throw GetField exception in CheckAnalysis which is little ugly.

AmplabJenkins · 2015-04-23T10:02:12Z

Can one of the admins verify this patch?

marmbrus · 2015-04-23T18:16:13Z

ok to test

SparkQA · 2015-04-23T18:18:47Z

Test build #30861 has started for PR 5659 at commit ef6039c.

SparkQA · 2015-04-23T19:58:23Z

Test build #30861 has finished for PR 5659 at commit ef6039c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

AmplabJenkins · 2015-04-23T19:58:27Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30861/
Test PASSed.

AmplabJenkins · 2015-04-27T18:18:01Z

Can one of the admins verify this patch?

cloud-fan · 2015-04-28T04:15:33Z

ping @marmbrus

cloud-fan · 2015-05-12T09:36:10Z

Retest this please.

marmbrus · 2015-05-13T18:59:06Z

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

Two suggestions here:

Can we share the code with the block below? and only add a try/catch around it?

I think we can probably avoid the changed optimization. The rule executor and transform already do checks to avoid churn when the plan does not change. Either way, I think its better to keep rules simple even if there is a small performance penalty.

cloud-fan · 2015-06-03T13:16:01Z

Retest this please.

cloud-fan · 2015-06-03T13:16:09Z

cc @marmbrus

cloud-fan · 2015-06-12T03:14:27Z

ping @marmbrus

cloud-fan · 2015-06-15T04:43:43Z

cc @marmbrus , is it OK to test?

marmbrus · 2015-06-15T04:52:01Z

add to whitelist

AmplabJenkins · 2015-06-15T04:52:13Z

Merged build triggered.

AmplabJenkins · 2015-06-15T04:52:20Z

Merged build started.

SparkQA · 2015-06-15T04:53:08Z

Test build #34909 has started for PR 5659 at commit 2ac76ea.

SparkQA · 2015-06-15T05:07:35Z

Test build #34909 has finished for PR 5659 at commit 2ac76ea.

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

AmplabJenkins · 2015-06-15T05:07:41Z

Merged build finished. Test FAILed.

AmplabJenkins · 2015-06-15T05:27:14Z

Merged build triggered.

AmplabJenkins · 2015-06-15T05:27:21Z

Merged build started.

SparkQA · 2015-06-15T05:30:02Z

Test build #34910 has started for PR 5659 at commit d71f022.

SparkQA · 2015-06-15T07:18:45Z

Test build #34910 has finished for PR 5659 at commit d71f022.

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

AmplabJenkins · 2015-06-15T07:18:57Z

Merged build finished. Test PASSed.

AmplabJenkins · 2015-06-16T14:02:17Z

Merged build triggered.

AmplabJenkins · 2015-06-16T14:02:22Z

Merged build started.

SparkQA · 2015-06-16T14:03:23Z

Test build #34984 has started for PR 5659 at commit e04b0e5.

SparkQA · 2015-06-16T14:20:10Z

Test build #34984 has finished for PR 5659 at commit e04b0e5.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class IsNull(child: Expression) extends UnaryExpression with Predicate
- case class IsNotNull(child: Expression) extends UnaryExpression with Predicate

AmplabJenkins · 2015-06-16T14:20:17Z

Merged build finished. Test FAILed.

AmplabJenkins · 2015-06-16T15:07:15Z

Merged build triggered.

AmplabJenkins · 2015-06-16T15:07:25Z

Merged build started.

SparkQA · 2015-06-16T15:08:26Z

Test build #34989 has started for PR 5659 at commit cfa79f8.

SparkQA · 2015-06-16T16:55:10Z

Test build #34989 has finished for PR 5659 at commit cfa79f8.

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

AmplabJenkins · 2015-06-16T16:55:21Z

Merged build finished. Test FAILed.

cloud-fan · 2015-06-17T00:47:28Z

retest it please.

marmbrus · 2015-06-17T19:25:44Z

test this please

AmplabJenkins · 2015-06-17T19:27:13Z

Merged build triggered.

AmplabJenkins · 2015-06-17T19:27:22Z

Merged build started.

SparkQA · 2015-06-17T19:30:29Z

Test build #35056 has started for PR 5659 at commit cfa79f8.

SparkQA · 2015-06-17T21:17:54Z

Test build #35056 has finished for PR 5659 at commit cfa79f8.

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

AmplabJenkins · 2015-06-17T21:18:06Z

Merged build finished. Test PASSed.

marmbrus · 2015-06-17T21:46:35Z

Thanks! Merging to master.

This PR is a improvement for apache#5189. The resolution rule for ORDER BY is: first resolve based on what comes from the select clause and then fall back on its child only when this fails. There are 2 steps. First, try to resolve `Sort` in `ResolveReferences` based on select clause, and ignore exceptions. Second, try to resolve `Sort` in `ResolveSortReferences` and add missing projection. However, the way we resolve `SortOrder` is wrong. We just resolve `UnresolvedAttribute` and use the result to indicate if we can resolve `SortOrder`. But `UnresolvedAttribute` is only part of `GetField` chain(broken by `GetItem`), so we need to go through the whole chain to indicate if we can resolve `SortOrder`. With this change, we can also avoid re-throw GetField exception in `CheckAnalysis` which is little ugly. Author: Wenchen Fan <[email protected]> Closes apache#5659 from cloud-fan/order-by and squashes the following commits: cfa79f8 [Wenchen Fan] update test 3245d28 [Wenchen Fan] minor improve 465ee07 [Wenchen Fan] address comment 1fc41a2 [Wenchen Fan] fix SPARK-7067

cloud-fan force-pushed the order-by branch from 98cf467 to ef6039c Compare April 23, 2015 09:59

cloud-fan force-pushed the order-by branch from ef6039c to 86fa626 Compare May 12, 2015 09:31

marmbrus reviewed May 13, 2015
View reviewed changes

cloud-fan force-pushed the order-by branch 3 times, most recently from 8c2e600 to d75cef0 Compare May 14, 2015 04:47

cloud-fan force-pushed the order-by branch from d75cef0 to 6cfc017 Compare June 3, 2015 13:15

cloud-fan mentioned this pull request Jun 4, 2015

[SPARK-8104][SQL] auto alias expressions in analyzer #6647

Closed

cloud-fan force-pushed the order-by branch from 6cfc017 to 439e0ed Compare June 12, 2015 03:11

cloud-fan force-pushed the order-by branch from 439e0ed to 2ac76ea Compare June 15, 2015 04:40

cloud-fan added 3 commits June 16, 2015 21:40

fix SPARK-7067

1fc41a2

address comment

465ee07

minor improve

3245d28

cloud-fan force-pushed the order-by branch from d71f022 to e04b0e5 Compare June 16, 2015 13:59

update test

cfa79f8

cloud-fan force-pushed the order-by branch from e04b0e5 to cfa79f8 Compare June 16, 2015 15:03

asfgit closed this in 7f05b1f Jun 17, 2015

[SPARK-7067][SQL] fix bug when use complex nested fields in ORDER BY #5659

[SPARK-7067][SQL] fix bug when use complex nested fields in ORDER BY #5659

Uh oh!

Conversation

cloud-fan commented Apr 23, 2015

Uh oh!

AmplabJenkins commented Apr 23, 2015

Uh oh!

marmbrus commented Apr 23, 2015

Uh oh!

SparkQA commented Apr 23, 2015

Uh oh!

SparkQA commented Apr 23, 2015

Uh oh!

AmplabJenkins commented Apr 23, 2015

Uh oh!

AmplabJenkins commented Apr 27, 2015

Uh oh!

cloud-fan commented Apr 28, 2015

Uh oh!

cloud-fan commented May 12, 2015

Uh oh!

marmbrus May 13, 2015

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jun 3, 2015

Uh oh!

cloud-fan commented Jun 3, 2015

Uh oh!

cloud-fan commented Jun 12, 2015

Uh oh!

cloud-fan commented Jun 15, 2015

Uh oh!

marmbrus commented Jun 15, 2015

Uh oh!

AmplabJenkins commented Jun 15, 2015

Uh oh!

AmplabJenkins commented Jun 15, 2015

Uh oh!

SparkQA commented Jun 15, 2015

Uh oh!

SparkQA commented Jun 15, 2015

Uh oh!

AmplabJenkins commented Jun 15, 2015

Uh oh!

AmplabJenkins commented Jun 15, 2015

Uh oh!

AmplabJenkins commented Jun 15, 2015

Uh oh!

SparkQA commented Jun 15, 2015

Uh oh!

SparkQA commented Jun 15, 2015

Uh oh!

AmplabJenkins commented Jun 15, 2015

Uh oh!

AmplabJenkins commented Jun 16, 2015

Uh oh!

AmplabJenkins commented Jun 16, 2015

Uh oh!

SparkQA commented Jun 16, 2015

Uh oh!

SparkQA commented Jun 16, 2015

Uh oh!

AmplabJenkins commented Jun 16, 2015

Uh oh!

AmplabJenkins commented Jun 16, 2015

Uh oh!

AmplabJenkins commented Jun 16, 2015

Uh oh!

SparkQA commented Jun 16, 2015

Uh oh!

SparkQA commented Jun 16, 2015

Uh oh!

AmplabJenkins commented Jun 16, 2015

Uh oh!

cloud-fan commented Jun 17, 2015

Uh oh!

marmbrus commented Jun 17, 2015

Uh oh!

AmplabJenkins commented Jun 17, 2015