[SPARK-6145][SQL] fix ORDER BY on nested fields #4904

cloud-fan · 2015-03-05T08:01:40Z

When resolve Sort with Project's output, we should use the base attribute but not the GetField chain in Project's output.

AmplabJenkins · 2015-03-05T08:02:10Z

Can one of the admins verify this patch?

chenghao-intel · 2015-03-05T08:12:32Z

Can you also try the test case:
https://github.com/apache/spark/pull/4892/files#diff-5a2e7f03d14856c8769fd3ddea8742bdR55

The root cause of the failure also due to the incorrect alias, see:
https://github.com/apache/spark/pull/4892/files#diff-27c76f96a7b2733ecfd6f46a1716e153R206

cloud-fan · 2015-03-05T10:04:48Z

Hi @chenghao-intel , your test is failed on my code, and I study into it, here is my thoughts.
First, I think the LogicalPlan#resolveChildren logic is not right for Sort.
For example, select a.b from t order by b.a on data {"a": {"b": 1}, "b": {"a": 1}}, currently we will try to resolve "b.a" on top of "a.b" instead of colunm "b", which is wrong.
Second, in ResolveSortReferences, we may expand the projection list if the field used by order by is not present. However, for something like "a.b.c", what should we add to projection? Just the bottom relation "a" or the whole expression "a.b.c"?
@marmbrus What do you think?
I'll dig into it tomorrow.

chenghao-intel · 2015-03-05T11:17:40Z

Why not review #4892 for me? :)

marmbrus · 2015-03-05T19:42:36Z

test this please

SparkQA · 2015-03-05T19:47:49Z

Test build #28305 has started for PR 4904 at commit 3eedbfc.

This patch merges cleanly.

SparkQA · 2015-03-05T19:48:53Z

Test build #28305 has finished for PR 4904 at commit 3eedbfc.

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

AmplabJenkins · 2015-03-05T19:48:54Z

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

marmbrus · 2015-03-05T20:22:34Z

Since I'd really like to include this in the next RC, I've opened #4918 with the style error fixed.

marmbrus · 2015-03-05T20:33:25Z

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

I'd rather use checkAnswer for these tests, especially if we are going to put them in SQLQuerySuite. When check answer fails it'll give nice exceptions and then we test end to end.

Based on #4904 with style errors fixed. `LogicalPlan#resolve` will not only produce `Attribute`, but also "`GetField` chain". So in `ResolveSortReferences`, after resolve the ordering expressions, we should not just collect the `Attribute` results, but also `Attribute` at the bottom of "`GetField` chain". Author: Wenchen Fan <[email protected]> Author: Michael Armbrust <[email protected]> Closes #4918 from marmbrus/pr/4904 and squashes the following commits: 997f84e [Michael Armbrust] fix style 3eedbfc [Wenchen Fan] fix 6145 (cherry picked from commit 5873c71) Signed-off-by: Michael Armbrust <[email protected]>

Based on #4904 with style errors fixed. `LogicalPlan#resolve` will not only produce `Attribute`, but also "`GetField` chain". So in `ResolveSortReferences`, after resolve the ordering expressions, we should not just collect the `Attribute` results, but also `Attribute` at the bottom of "`GetField` chain". Author: Wenchen Fan <[email protected]> Author: Michael Armbrust <[email protected]> Closes #4918 from marmbrus/pr/4904 and squashes the following commits: 997f84e [Michael Armbrust] fix style 3eedbfc [Wenchen Fan] fix 6145

cloud-fan · 2015-03-06T14:29:33Z

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

Can we just filter out everything except AttributeReference? I don't know all corner cases of ORDER BY and feel this way is safer.

cloud-fan · 2015-03-06T14:30:00Z

retest it please.

cloud-fan · 2015-03-06T14:34:16Z

Seems Jenkins doesn't listen to me :(

marmbrus · 2015-03-06T19:53:51Z

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

This feels kind of hacky to me as its mixing the particulars of analysis for SQL into the logical plan. Could we instead just make resolve not do partial resolution, where we can resolve the base attribute but not the GetFields that are on top. I think this change is the root cause of the regression.

marmbrus · 2015-03-06T19:54:14Z

ok to test

SparkQA · 2015-03-06T19:58:22Z

Test build #28354 has started for PR 4904 at commit e383154.

This patch merges cleanly.

SparkQA · 2015-03-06T20:39:19Z

Test build #28354 has finished for PR 4904 at commit e383154.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class BinaryClassificationMetrics(JavaModelWrapper):

AmplabJenkins · 2015-03-06T20:39:23Z

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

cloud-fan · 2015-03-08T15:36:36Z

Hi @marmbrus , it feels hard for me to resolve the base attribute but not the GetFields that are on top. When we get into LogicalPlan#resolve, the Attributes are all AttributeReference, and we don't know whether it's a base attribute or GetField chain. To make it less hacky, I moved the particular code to Analyzer, is that OK?

SparkQA · 2015-03-08T15:38:02Z

Test build #28372 has started for PR 4904 at commit eab0b43.

This patch merges cleanly.

SparkQA · 2015-03-08T16:56:23Z

Test build #28372 has finished for PR 4904 at commit eab0b43.

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

AmplabJenkins · 2015-03-08T16:56:26Z

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

SparkQA · 2015-03-16T02:18:11Z

Test build #28634 has started for PR 4904 at commit 7b56fb9.

This patch merges cleanly.

SparkQA · 2015-03-16T03:37:35Z

Test build #28634 has finished for PR 4904 at commit 7b56fb9.

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

AmplabJenkins · 2015-03-16T03:37:39Z

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

cloud-fan · 2015-03-16T04:21:17Z

Hi @marmbrus , any more comments?

cloud-fan · 2015-03-25T02:59:43Z

ping @marmbrus

yhuai · 2015-03-25T18:01:05Z

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

I think it will not work when we have a order by in a subquery and the outer query block try to access the those filtered out fields.

I see. You are creating a fake Sort node in order to resolve the attribute. If the attribute is resolved, use the resolved one to replace the unresolved one.

marmbrus · 2015-03-25T22:07:31Z

Thanks for working on this! This is a pretty tricky case that involves several rules interacting, and your test cases have been incredibly helpful.

Here are my thoughts on the proposed solution:

My biggest problem is that we are spreading the logic about Sort's special resolution across several rules. I think it can be confusing to add things in one rule and then filter them out in another. Instead, I propose that we just finalize the resolution in the ResolveSortReferences rules, instead of adding projects there while deferring the actual resolution to another rule. At the point where we construct the projects, we know the resolution, so I think we can just use it there. This way even if we introduce ambiguity, it does not matter.
Separately, I think it is a problem that with our current logic we break UnresolvedReferences into UnresolvedGetFields, before know if it is possible for them to be successfully resolved. The problem here is that . is overloaded both to do scoping and to do extraction of fields. I think UnresolvedGetField is a good abstraction to have, but should only be used by the parser to handle cases where it must be field extraction (i.e. because the chain has been broken by a GetItem or other expression).

I've taken your changes and made an alternative solution (#5189). Would appreciate your feedback there.

This PR is based on work by cloud-fan in #4904, but with two differences: - We isolate the logic for Sort's special handling into `ResolveSortReferences` - We avoid creating UnresolvedGetField expressions during resolution. Instead we either resolve GetField or we return None. This avoids us going down the wrong path early on. Author: Michael Armbrust <[email protected]> Closes #5189 from marmbrus/nestedOrderBy and squashes the following commits: b8cae45 [Michael Armbrust] fix another test 0f36a11 [Michael Armbrust] WIP 91820cd [Michael Armbrust] Fix bug.

This PR is based on work by cloud-fan in #4904, but with two differences: - We isolate the logic for Sort's special handling into `ResolveSortReferences` - We avoid creating UnresolvedGetField expressions during resolution. Instead we either resolve GetField or we return None. This avoids us going down the wrong path early on. Author: Michael Armbrust <[email protected]> Closes #5189 from marmbrus/nestedOrderBy and squashes the following commits: b8cae45 [Michael Armbrust] fix another test 0f36a11 [Michael Armbrust] WIP 91820cd [Michael Armbrust] Fix bug. (cherry picked from commit cd48ca5) Signed-off-by: Michael Armbrust <[email protected]> Conflicts: sql/core/src/main/scala/org/apache/spark/sql/SQLContext.scala

cloud-fan mentioned this pull request Mar 5, 2015

[SPARK-6145] [SQL] Fix the bug of nested data type resolving in ORDER BY #4892

Closed

marmbrus mentioned this pull request Mar 5, 2015

[SPARK-6145][SQL] fix ORDER BY on nested fields #4918

Closed

marmbrus reviewed Mar 5, 2015
View reviewed changes

fix 6145 completely

8d6b324

cloud-fan force-pushed the 6145 branch from 3eedbfc to 8d6b324 Compare March 6, 2015 13:33

handle special cases

e383154

cloud-fan reviewed Mar 6, 2015
View reviewed changes

marmbrus reviewed Mar 6, 2015
View reviewed changes

cloud-fan added 2 commits March 7, 2015 08:45

feedback for marmbrus

187d97e

fix hive tests caused by a mistake

eab0b43

minor enhance

7b56fb9

yhuai reviewed Mar 25, 2015
View reviewed changes

marmbrus mentioned this pull request Mar 25, 2015

[SPARK-6145][SQL] fix ORDER BY on nested fields #5189

Closed

cloud-fan closed this Apr 8, 2015

cloud-fan deleted the 6145 branch April 8, 2015 07:30

[SPARK-6145][SQL] fix ORDER BY on nested fields #4904

[SPARK-6145][SQL] fix ORDER BY on nested fields #4904

Uh oh!

Conversation

cloud-fan commented Mar 5, 2015

Uh oh!

AmplabJenkins commented Mar 5, 2015

Uh oh!

chenghao-intel commented Mar 5, 2015

Uh oh!

cloud-fan commented Mar 5, 2015

Uh oh!

chenghao-intel commented Mar 5, 2015

Uh oh!

marmbrus commented Mar 5, 2015

Uh oh!

SparkQA commented Mar 5, 2015

Uh oh!

SparkQA commented Mar 5, 2015

Uh oh!

AmplabJenkins commented Mar 5, 2015

Uh oh!

marmbrus commented Mar 5, 2015

Uh oh!

marmbrus Mar 5, 2015

Choose a reason for hiding this comment

Uh oh!

cloud-fan Mar 6, 2015

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Mar 6, 2015

Uh oh!

cloud-fan commented Mar 6, 2015

Uh oh!

marmbrus Mar 6, 2015

Choose a reason for hiding this comment

Uh oh!

marmbrus commented Mar 6, 2015

Uh oh!

SparkQA commented Mar 6, 2015

Uh oh!

SparkQA commented Mar 6, 2015

Uh oh!

AmplabJenkins commented Mar 6, 2015

Uh oh!

cloud-fan commented Mar 8, 2015

Uh oh!

SparkQA commented Mar 8, 2015

Uh oh!

SparkQA commented Mar 8, 2015

Uh oh!

AmplabJenkins commented Mar 8, 2015

Uh oh!

SparkQA commented Mar 16, 2015

Uh oh!

SparkQA commented Mar 16, 2015

Uh oh!

AmplabJenkins commented Mar 16, 2015

Uh oh!

cloud-fan commented Mar 16, 2015

Uh oh!

cloud-fan commented Mar 25, 2015

Uh oh!

yhuai Mar 25, 2015

Choose a reason for hiding this comment

Uh oh!

yhuai Mar 25, 2015

Choose a reason for hiding this comment

Uh oh!

marmbrus commented Mar 25, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants