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

chenghao-intel · 2015-03-04T15:26:53Z

No description provided.

SparkQA · 2015-03-04T15:27:42Z

Test build #28268 has started for PR 4892 at commit b82c8c5.

This patch merges cleanly.

chenghao-intel · 2015-03-04T15:33:34Z

cc @marmbrus @cloud-fan This is a quick fix, in long term, we should resolve the nested attribute sequence in one time.

What do you think?

chenghao-intel · 2015-03-04T15:45:03Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SqlParser.scala

val i1="a" val i2="b" val rest:Seq[String]=Nil println(i1 + "." + i2 + rest.mkString(".", ".", ""))

outputs a.b., what we expect is a.b

It was my mistake...didn't test the mkString method on Nil

how about
UnresolvedAttribute(i1 + "." + i2 + rest.mkString("."))

val i1="a" val i2="b" val rest:Seq[String]="c" :: Nil println(i1 + "." + i2 + rest.mkString("."))

Outputs a.bc

That make sense, thanks!

SparkQA · 2015-03-04T16:39:24Z

Test build #28268 has finished for PR 4892 at commit b82c8c5.

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

AmplabJenkins · 2015-03-04T16:39:28Z

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

marmbrus · 2015-03-04T18:49:16Z

Hmm, this solves some problems, but not all of them:

sqlContext.jsonRDD(sc.parallelize("""{"a": {"a": {"a": 1}}, "c": 1}""" :: Nil)).registerTempTable("nestedOrder")

sqlContext.sql("SELECT a.a.a FROM nestedOrder ORDER BY a.a.a")


org.apache.spark.sql.AnalysisException: GetField is not valid on fields of type LongType;
    at org.apache.spark.sql.catalyst.analysis.Analyzer$ResolveReferences$.resolveGetField(Analyzer.scala:307)
    at org.apache.spark.sql.catalyst.analysis.Analyzer$ResolveReferences$$anonfun$apply$7$$anonfun$applyOrElse$2.applyOrElse(Analyzer.scala:271)
    at org.apache.spark.sql.catalyst.analysis.Analyzer$ResolveReferences$$anonfun$apply$7$$anonfun$applyOrElse$2.applyOrElse(Analyzer.scala:260)
    at org.apache.spark.sql.catalyst.trees.TreeNode$$anonfun$transformUp$1.apply(TreeNode.scala:250)

SparkQA · 2015-03-05T03:22:43Z

Test build #28273 has started for PR 4892 at commit 44ac17d.

This patch merges cleanly.

SparkQA · 2015-03-05T03:34:30Z

Test build #28273 has finished for PR 4892 at commit 44ac17d.

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

AmplabJenkins · 2015-03-05T03:34:33Z

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

SparkQA · 2015-03-05T05:17:52Z

Test build #28276 has started for PR 4892 at commit 47db754.

This patch merges cleanly.

chenghao-intel · 2015-03-05T05:19:53Z

Thank you @marmbrus for the review, we do need to specify an alias for the nested attribute expression, but should not be the last attribute name. The code is updated, let's see if it break anything.

chenghao-intel · 2015-03-05T05:20:54Z

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

Moved this function into the LogicalPlan

SparkQA · 2015-03-05T05:57:08Z

Test build #28276 has finished for PR 4892 at commit 47db754.

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

AmplabJenkins · 2015-03-05T05:57:11Z

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

SparkQA · 2015-03-05T06:13:07Z

Test build #28279 has started for PR 4892 at commit 5de3b9e.

This patch merges cleanly.

SparkQA · 2015-03-05T07:14:21Z

Test build #28279 has finished for PR 4892 at commit 5de3b9e.

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

AmplabJenkins · 2015-03-05T07:14:24Z

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

cloud-fan · 2015-03-05T08:03:44Z

Hi @marmbrus @chenghao-intel , I have a simpler fix here

SparkQA · 2015-03-05T08:52:48Z

Test build #28284 has started for PR 4892 at commit 73eb346.

This patch merges cleanly.

viirya · 2015-03-05T09:21:51Z

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

The python test failure is caused by replacing aliasName with name here. Is it okay? SELECT a.b.c FROM table would get attribute named a.b.c instead of c before.

Thank you @viirya I've updated the python code.

I meant should it be that? In Hive it should be c instead of a.b.c?

I am not so sure how Hive handle that, but it can not be c; otherwise it may cause reference arbitrary for its parent logical plan.

e.g.
Assume we have table tbl with schema Struct < a : Struct < b : Int, c: Int>, b: int>

SELECT b FROM (SELECT a.b, b FROM tbl)

I don't think we can change the default alias when extracting nested fields. I believe we match hive behaviors now, and this would break existing queries.

Yes, I agree we shouldn't break the existed logic, but I believe this is a bug of Hive.

hive>create table struct1 as select named_struct("a",key, "b", value) as a, key as b from src limit 1; hive>select a.b, b from struct1; -- Works hive>create table struct2 as select a.b, b from struct1; FAILED: SemanticException [Error 10036]: Duplicate column name: b

I am wondering if we can break the naming rule of Hive for nested data type references, which always causes ambiguous.

How is this a bug? These are pretty contrived examples. How often do you actually have nested structures where the outside name is the same as the inside name?

Yea, I shouldn't say "always", but "possible", it maybe quite often while with join.

SparkQA · 2015-03-05T10:29:07Z

Test build #28284 has finished for PR 4892 at commit 73eb346.

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

AmplabJenkins · 2015-03-05T10:29:11Z

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

cloud-fan · 2015-03-05T13:44:38Z

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

The type of resolved is Seq[NamedExpression], so we don't need call collect here.

Good Point, I will update that.

cloud-fan · 2015-03-05T13:53:55Z

Hi @chenghao-intel , have you considered about something like a.b[0].c? It will be parsed into UnresolvedGetField(GetItem(UnresolvedAttribute("a.b"), 0), "c"), so you can't resolve the GetField chain in one time at LogicalPlan#resolve

chenghao-intel · 2015-03-05T14:10:11Z

@cloud-fan I don't think we need to consider the GetItem here, See https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SqlParser.scala#L374

I was thinking we probably can remove the class UnresolvedGetField also, but keep it as future improvement, that will be a big change.

SparkQA · 2015-03-05T14:17:44Z

Test build #28292 has started for PR 4892 at commit 9209ac1.

This patch merges cleanly.

cloud-fan · 2015-03-05T15:35:29Z

Hi @chenghao-intel , I just ran a quick test, it failed.

  test("quick") {
    jsonRDD(sparkContext.makeRDD(
      """{"a": [{"b": 1}], "c0": {"a": 1}}""" :: Nil)).registerTempTable("t")
    sql("SELECT a[0].b FROM t ORDER BY c0.a").queryExecution.analyzed
  }

Actually the GetItem matters, it will make the alias name of a.a[0].a become "c0"(auto generated). Otherwise, with your fix, the alias name of a.a.a will be "a.a.a"

SparkQA · 2015-03-05T15:56:14Z

Test build #28292 has finished for PR 4892 at commit 9209ac1.

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

AmplabJenkins · 2015-03-05T15:56:18Z

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

chenghao-intel · 2015-03-09T08:47:46Z

I am closing this PR.

fix the bug of nested data type resolving in sort by

b82c8c5

chenghao-intel reviewed Mar 4, 2015
View reviewed changes

marmbrus mentioned this pull request Mar 4, 2015

[SPARK-5278][SQL] Introduce UnresolvedGetField and complete the check of ambiguous reference to fields #4068

Closed

nested a.a.a which returns expression with alias name 'a'

44ac17d

fix the bug in unittest

47db754

chenghao-intel reviewed Mar 5, 2015
View reviewed changes

fix another bug in unittest

5de3b9e

update the output column attribute name for nested data query

73eb346

viirya reviewed Mar 5, 2015
View reviewed changes

This was referenced Mar 5, 2015

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

Closed

[SPARK-6184][SQL] Relocate logDebug to correct location in ResolveSortReferences #4905

Closed

cloud-fan reviewed Mar 5, 2015
View reviewed changes

update as feedback

9209ac1

chenghao-intel mentioned this pull request Mar 6, 2015

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

Closed

chenghao-intel closed this Mar 9, 2015

chenghao-intel deleted the orderby branch July 2, 2015 08:38

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

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

Uh oh!

Conversation

chenghao-intel commented Mar 4, 2015

Uh oh!

SparkQA commented Mar 4, 2015

Uh oh!

chenghao-intel commented Mar 4, 2015

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!

SparkQA commented Mar 4, 2015

Uh oh!

AmplabJenkins commented Mar 4, 2015

Uh oh!

marmbrus commented Mar 4, 2015

Uh oh!

SparkQA commented Mar 5, 2015

Uh oh!

SparkQA commented Mar 5, 2015

Uh oh!

AmplabJenkins commented Mar 5, 2015

Uh oh!

SparkQA commented Mar 5, 2015

Uh oh!

chenghao-intel commented Mar 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 5, 2015

Uh oh!

AmplabJenkins 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!

cloud-fan commented Mar 5, 2015

Uh oh!

SparkQA commented Mar 5, 2015

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!

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 Mar 5, 2015

Uh oh!

AmplabJenkins commented Mar 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Mar 5, 2015

Uh oh!

chenghao-intel commented Mar 5, 2015

Uh oh!