You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/05/10 14:49:30 UTC
[GitHub] [spark] AngersZhuuuu opened a new pull request #28490: [SPARK-31670][WIP]Struct Field in groupByExpr with CUBE
AngersZhuuuu opened a new pull request #28490:
URL: https://github.com/apache/spark/pull/28490
### What changes were proposed in this pull request?
Will wrong with below SQL
```
test("TEST STRUCT FIELD WITH GROUP BY with CUBE") {
withTable("t1") {
sql(
"""create table t1(
|a string,
|b int,
|c array<struct<row_id:int,json_string:string>>)
|using orc""".stripMargin)
sql(
"""
|select a, coalesce(get_json_object(each.json_string,'$.iType'),'-127') as iType, sum(b)
|from t1
|LATERAL VIEW explode(c) x AS each
|group by a, get_json_object(each.json_string,'$.iType')
|with cube
|""".stripMargin).explain(true)
}
}
```
ErrorĀ
```
expression 'x.`each`' is neither present in the group by, nor is it an aggregate function. Add to group by or wrap in first() (or first_value) if you don't care which value you get.;;
Aggregate [a#230, get_json_object(each#222.json_string AS json_string#223, $.iType)#231, spark_grouping_id#229L], [a#230, coalesce(get_json_object(each#222.json_string, $.iType), -127) AS iType#218, sum(cast(b#220 as bigint)) AS sum(b)#226L]
```
It is because for Struct type Field, it will add a Alias by default but with different exprId,
This make `ResolveGroupingAnalytics.constructAggregateExprs()` failed to replace expression by expand output attribute.
### Why are the changes needed?
Fix analyze bug
### Does this PR introduce _any_ user-facing change?
NO
### How was this patch tested?
Added UT
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685319247
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480200524
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1583,6 +1611,35 @@ class Analyzer(
failAnalysis(s"Invalid usage of '*' in expression '${o.prettyName}'")
}
}
+
+ // For aggregateExpression we need NamedExpression, so for top level Struct field
+ // Alias, we won't trim it, for non top level Struct field Alias, we need to trim
+ // it since we have trimmed groupByExpressions.
Review comment:
the comment relies on `trimStructFieldAlias`, can we move this method after that?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684371491
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684090975
**[Test build #128120 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128120/testReport)** for PR 28490 at commit [`82f3876`](https://github.com/apache/spark/commit/82f38766867cb1719fd0601da1332caa5b864a82).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684622980
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684367153
**[Test build #128135 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128135/testReport)** for PR 28490 at commit [`72dc305`](https://github.com/apache/spark/commit/72dc3056654131a942672e7bad54100eb99ec3ea).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r434369978
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -3495,6 +3495,59 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
assert(df4.schema.head.name === "randn(1)")
checkIfSeedExistsInExplain(df2)
}
+
+ test("SPARK-31670: Struct Field in groupByExpr with CUBE") {
+ withTable("t1") {
Review comment:
nit: `t1` -> `t`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640168470
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684180427
**[Test build #128120 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128120/testReport)** for PR 28490 at commit [`82f3876`](https://github.com/apache/spark/commit/82f38766867cb1719fd0601da1332caa5b864a82).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685394588
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-637325327
cc @maropu Can you have a review ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480210406
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1428,8 +1428,36 @@ class Analyzer(
// SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
// `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
// names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
+ case appendColumns: AppendColumns => appendColumns
+ case _ => a
+ }
+
+ val resolvedGroupingExprs =
Review comment:
> nit:
>
> ```
> val resolvedGroupingExprs = a.groupingExpressions
> .map...
> .map...
> ```
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683873997
**[Test build #128104 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128104/testReport)** for PR 28490 at commit [`d63613f`](https://github.com/apache/spark/commit/d63613fadb7ad82c815a6e0114518f6c6eca3235).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685391981
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640163983
**[Test build #123597 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123597/testReport)** for PR 28490 at commit [`1ee0542`](https://github.com/apache/spark/commit/1ee0542e20eea131ff27e4114e3547d32191a6a2).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640168470
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r436322459
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,11 +1486,70 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case p: LogicalPlan if needResolveStructField(p) =>
+ logTrace(s"Attempting to resolve ${p.simpleString(SQLConf.get.maxToStringFields)}")
+ val resolved = p.mapExpressions(resolveExpressionTopDown(_, p))
+ val structFieldMap = new mutable.HashMap[String, Alias]
+ resolved.transformExpressions {
+ case a @ Alias(struct: GetStructField, _) =>
+ if (structFieldMap.contains(struct.sql)) {
+ val exprId = structFieldMap.getOrElse(struct.sql, a).exprId
+ Alias(a.child, a.name)(exprId, a.qualifier, a.explicitMetadata)
+ } else {
+ structFieldMap.put(struct.sql, a)
+ a
+ }
+ case e => e
+ }
+
case q: LogicalPlan =>
logTrace(s"Attempting to resolve ${q.simpleString(SQLConf.get.maxToStringFields)}")
q.mapExpressions(resolveExpressionTopDown(_, q))
}
+ def needResolveStructField(plan: LogicalPlan): Boolean = {
+ plan match {
+ case UnresolvedHaving(havingCondition, a: Aggregate)
+ if containSameStructFields(a.groupingExpressions.flatMap(_.references),
+ a.aggregateExpressions.flatMap(_.references),
+ Some(havingCondition.references.toSeq)) => true
+ case Aggregate(groupingExpressions, aggregateExpressions, _)
+ if containSameStructFields(groupingExpressions.flatMap(_.references),
+ aggregateExpressions.flatMap(_.references)) => true
+ case GroupingSets(selectedGroupByExprs, groupByExprs, _, aggregations)
+ if containSameStructFields(groupByExprs.flatMap(_.references),
+ aggregations.flatMap(_.references),
+ Some(selectedGroupByExprs.flatMap(_.flatMap(_.references)))) => true
+ case _ => false
+ }
+ }
+
+ def containSameStructFields(
Review comment:
> It is not enough just to check if both sides (`grpExprs` and `aggExprs`) have struct fields here?
GroupingSets need check selected cols too, so all need to check.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640160868
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][WIP]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-626339809
**[Test build #122477 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122477/testReport)** for PR 28490 at commit [`27c495b`](https://github.com/apache/spark/commit/27c495b4da348e2d6bb461afec22d8088cf141c2).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r436324728
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -3496,6 +3496,88 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkIfSeedExistsInExplain(df2)
}
+ test("SPARK-31670: Struct Field in groupByExpr with CUBE") {
+ withTable("t") {
+ sql(
+ """CREATE TABLE t(
+ |a STRING,
+ |b INT,
+ |c ARRAY<STRUCT<row_id:INT,json_string:STRING>>,
+ |d ARRAY<ARRAY<STRING>>,
+ |e ARRAY<MAP<STRING, INT>>)
+ |USING ORC""".stripMargin)
+
+ checkAnswer(
+ sql(
+ """
+ |SELECT a, each.json_string, SUM(b)
+ |FROM t
+ |LATERAL VIEW EXPLODE(c) x AS each
+ |GROUP BY a, each.json_string
+ |WITH CUBE
+ |""".stripMargin), Nil)
+
+ checkAnswer(
+ sql(
+ """
+ |SELECT a, get_json_object(each.json_string, '$.i'), SUM(b)
+ |FROM t
+ |LATERAL VIEW EXPLODE(c) X AS each
+ |GROUP BY a, get_json_object(each.json_string, '$.i')
+ |WITH CUBE
+ |""".stripMargin), Nil)
+
+ checkAnswer(
+ sql(
+ """
+ |SELECT a, each.json_string AS json_string, SUM(b)
+ |FROM t
+ |LATERAL VIEW EXPLODE(c) x AS each
+ |GROUP BY a, each.json_string
+ |WITH CUBE
+ |""".stripMargin), Nil)
+
+ checkAnswer(
+ sql(
+ """
+ |SELECT a, each.json_string as js, SUM(b)
+ |FROM t
+ |LATERAL VIEW EXPLODE(c) X AS each
+ |GROUP BY a, each.json_string
+ |WITH CUBE
+ |""".stripMargin), Nil)
+
+ checkAnswer(
+ sql(
+ """
+ |SELECT a, each.json_string as js, SUM(b)
+ |FROM t
+ |LATERAL VIEW EXPLODE(c) X AS each
+ |GROUP BY a, each.json_string
+ |WITH ROLLUP
+ |""".stripMargin), Nil)
+
+ sql(
+ """
+ |SELECT a, each.json_string, SUM(b)
+ |FROM t
+ |LATERAL VIEW EXPLODE(c) X AS each
+ |GROUP BY a, each.json_string
+ |GROUPING sets((a),(a, each.json_string))
+ |""".stripMargin).explain(true)
+
+ checkAnswer(
+ sql(
+ """
+ |SELECT a, each.json_string, SUM(b)
+ |FROM t
+ |LATERAL VIEW EXPLODE(c) X AS each
+ |GROUP BY a, each.json_string
+ |GROUPING sets((a),(a, each.json_string))
+ |""".stripMargin), Nil)
Review comment:
> Could you add tests having queries with `HAVING` clauses?
Seems I make a mistake, having won't have duplicate field since it won't have grouping keys in having condition
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640212326
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685299929
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684622980
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-671608286
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683768816
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640163983
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684775900
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683423634
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685064277
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r470509589
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,33 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case agg @ (_: Aggregate | _: GroupingSets) =>
+ val resolved = agg.mapExpressions(resolveExpressionTopDown(_, agg))
+ val hasStructField = resolved.expressions.exists {
+ _.collectFirst { case gsf: GetStructField => gsf }.isDefined
+ }
+ if (hasStructField) {
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause same struct field
+ // in aggExprs/groupExprs/selectedGroupByExprs will be resolved divided
+ // with different ExprId of Alias and replace failed when construct
+ // Aggregate in ResolveGroupingAnalytics, so we resolve duplicated struct
+ // field here with same ExprId
Review comment:
> ah i see, then can we remove aliases in grouping expressions here?
Tried, No, since a lot api need param as `NamedExpression`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu removed a comment on pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-626340051
gentle ping @cloud-fan
Another analyze bug, a little band-fix now, hope some good suggestion and better place to fix this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684002502
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480853032
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,24 +1325,43 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e the expression need to be resolved.
+ * @param q the LogicalPlan who's children used to resolve expression's attribute.
Review comment:
> `whose children are used to ...`
emmm..pool english skill..., changed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480844760
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,24 +1325,43 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e the expression need to be resolved.
+ * @param q the LogicalPlan use to resolve expression's attribute from.
+ * @param trimAlias whether need to trim alias of Struct field. When true, we will trim
+ * Struct field alias. When isTopLevel = true, we won't trim top-level
+ * Struct field alias.
+ * @param isTopLevel whether need to trim top-level alias of Struct field. this param is
Review comment:
nit: this -> This
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,24 +1325,43 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e the expression need to be resolved.
+ * @param q the LogicalPlan use to resolve expression's attribute from.
Review comment:
nit: use -> used
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,24 +1325,43 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e the expression need to be resolved.
+ * @param q the LogicalPlan use to resolve expression's attribute from.
+ * @param trimAlias whether need to trim alias of Struct field. When true, we will trim
+ * Struct field alias. When isTopLevel = true, we won't trim top-level
+ * Struct field alias.
+ * @param isTopLevel whether need to trim top-level alias of Struct field. this param is
+ * controlled by this method itself to make sure we won't trim top-level
+ * Struct field alias. If need to trim top-level Struct field alias,
+ * we can do that outside of this method.
Review comment:
Can we rephase this param doc too? Do you mean, this param is used by this method to know whether it is resolving top-level expression or not, if it is top-level, we skip trimming alias of struct field.
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,24 +1325,43 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e the expression need to be resolved.
+ * @param q the LogicalPlan use to resolve expression's attribute from.
+ * @param trimAlias whether need to trim alias of Struct field. When true, we will trim
+ * Struct field alias. When isTopLevel = true, we won't trim top-level
+ * Struct field alias.
Review comment:
This param doc reads weird. Do you mean, when `trimAlias` is true, the method will trim alias of a struct field. But this method won't trim alias if it is top-level expression?
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1428,8 +1428,46 @@ class Analyzer(
// SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
// `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
// names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
+ case appendColumns: AppendColumns => appendColumns
+ case _ => a
+ }
+
+ val resolvedGroupingExprs =
+ a.groupingExpressions.map(resolveExpressionTopDown(_, planForResolve))
+ .map(trimStructFieldAlias)
+
+ val resolvedAggExprs = a.aggregateExpressions
+ .map(resolveExpressionTopDown(_, planForResolve))
+ .map {
Review comment:
Hmm, where is `trimNonTopLevelStructFieldAlias`?
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1425,11 +1444,48 @@ class Analyzer(
// rule: ResolveDeserializer.
case plan if containsDeserializer(plan.expressions) => plan
- // SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
- // `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
- // names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
+ case appendColumns: AppendColumns => appendColumns
+ case _ => a
+ }
+
+ val resolvedGroupingExprs = a.groupingExpressions
+ .map(resolveExpressionTopDown(_, planForResolve, trimAlias = true))
+ .map {
+ // trim Alias over top-level GetStructField
+ case Alias(s: GetStructField, _) => s
+ case other => other
+ }
+
+ val resolvedAggExprs = a.aggregateExpressions
+ .map(resolveExpressionTopDown(_, planForResolve, trimAlias = true))
+ .map(_.asInstanceOf[NamedExpression])
+
+ a.copy(resolvedGroupingExprs, resolvedAggExprs, a.child)
+
+ case g: GroupingSets =>
+ val resolvedSelectedExprs = g.selectedGroupByExprs
+ .map(_.map(resolveExpressionTopDown(_, g, trimAlias = true))
+ .map {
+ // trim Alias over top-level GetStructField
+ case Alias(s: GetStructField, _) => s
+ case other => other
+ })
Review comment:
This is somehow hard to understand for reader. Why we need to trim alias for these expressions? Can you explain or maybe add an example as comment in the code?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r481162813
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,25 +1325,54 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e The expression need to be resolved.
+ * @param q The LogicalPlan whose children are used to resolve expression's attribute.
+ * @param trimAlias When true, we will trim Struct field alias. When isTopLevel = true,
Review comment:
> For `innerResolve`, the `isTopLevel` parameter is just an internal state to indicate top-level `GetStructField`, too trivial to comment.
Yea, above comment is enough
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r434961465
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -506,31 +506,55 @@ class Analyzer(
aggregations: Seq[NamedExpression],
groupByAliases: Seq[Alias],
groupingAttrs: Seq[Expression],
- gid: Attribute): Seq[NamedExpression] = aggregations.map {
- // collect all the found AggregateExpression, so we can check an expression is part of
- // any AggregateExpression or not.
- val aggsBuffer = ArrayBuffer[Expression]()
- // Returns whether the expression belongs to any expressions in `aggsBuffer` or not.
- def isPartOfAggregation(e: Expression): Boolean = {
- aggsBuffer.exists(a => a.find(_ eq e).isDefined)
+ gid: Attribute): Seq[NamedExpression] = {
+ val resolvedGroupByAliases = groupByAliases.map(_.transformDown {
Review comment:
> A root cause seems to be that `ResolveReferences` assigns different exprIds to `each#30.json_string AS json_string`s (`#31` vs `#32`);
Yea, StructGetFiled will construct with a alias in ResolveReference and got different ExprID
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684767332
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r479962914
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -570,18 +570,44 @@ class Analyzer(
s"Grouping sets size cannot be greater than ${GroupingID.dataType.defaultSize * 8}")
}
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause the same struct fields
Review comment:
> It's simpler to fix the problem earlier. Let's try it.
Ok, update later.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r436360282
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1481,7 +1486,35 @@ class Analyzer(
case q: LogicalPlan =>
logTrace(s"Attempting to resolve ${q.simpleString(SQLConf.get.maxToStringFields)}")
Review comment:
> w/ some code cleanup;
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r436324690
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -3496,6 +3496,88 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkIfSeedExistsInExplain(df2)
}
+ test("SPARK-31670: Struct Field in groupByExpr with CUBE") {
Review comment:
> Could you please make the title more correct? I think we don't need the word `with CUBE`.
It's ok for current PR title?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480987402
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1326,24 +1326,49 @@ class Analyzer(
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
*/
- private def resolveExpressionTopDown(e: Expression, q: LogicalPlan): Expression = {
- if (e.resolved) return e
- e match {
- case f: LambdaFunction if !f.bound => f
- case u @ UnresolvedAttribute(nameParts) =>
- // Leave unchanged if resolution fails. Hopefully will be resolved next round.
- val result =
- withPosition(u) {
- q.resolveChildren(nameParts, resolver)
- .orElse(resolveLiteralFunction(nameParts, u, q))
- .getOrElse(u)
+ private def resolveExpressionTopDown(
+ e: Expression,
+ q: LogicalPlan,
+ trimAlias: Boolean = false): Expression = {
+
+ // Explain for param trimAlias and isTopLevel:
+ // 1. trimAlias = false:
+ // We won't trim Struct Field alias.
+ // 2. trimAlias = true && isTopLevel = false:
+ // We will trim all Struct field alias.
+ // 3. trimAlias = true && isTopLevel = true
+ // Trim unnecessary alias of `GetStructField`. Note that, we cannot trim the alias of
+ // top-level `GetStructField`, as we should resolve `UnresolvedAttribute` to a named
+ // expression. The caller side can trim the alias of top-level `GetStructField` if needed.
+ def innerResolve(
+ e: Expression,
+ q: LogicalPlan,
+ trimAlias: Boolean = false,
+ isTopLevel: Boolean = true): Expression = {
Review comment:
> this parameter doesn't need a default value.
Done
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1326,24 +1326,49 @@ class Analyzer(
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
*/
- private def resolveExpressionTopDown(e: Expression, q: LogicalPlan): Expression = {
- if (e.resolved) return e
- e match {
- case f: LambdaFunction if !f.bound => f
- case u @ UnresolvedAttribute(nameParts) =>
- // Leave unchanged if resolution fails. Hopefully will be resolved next round.
- val result =
- withPosition(u) {
- q.resolveChildren(nameParts, resolver)
- .orElse(resolveLiteralFunction(nameParts, u, q))
- .getOrElse(u)
+ private def resolveExpressionTopDown(
+ e: Expression,
+ q: LogicalPlan,
+ trimAlias: Boolean = false): Expression = {
+
+ // Explain for param trimAlias and isTopLevel:
+ // 1. trimAlias = false:
+ // We won't trim Struct Field alias.
+ // 2. trimAlias = true && isTopLevel = false:
+ // We will trim all Struct field alias.
+ // 3. trimAlias = true && isTopLevel = true
+ // Trim unnecessary alias of `GetStructField`. Note that, we cannot trim the alias of
+ // top-level `GetStructField`, as we should resolve `UnresolvedAttribute` to a named
+ // expression. The caller side can trim the alias of top-level `GetStructField` if needed.
+ def innerResolve(
+ e: Expression,
+ q: LogicalPlan,
Review comment:
> inner method can access the parameters of outer method. We don't need this parameter.
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-638554177
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/123516/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684329349
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685323122
**[Test build #128188 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128188/testReport)** for PR 28490 at commit [`e6fb91f`](https://github.com/apache/spark/commit/e6fb91fc8124c60e27899846b675853c22537791).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480210028
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1428,8 +1428,36 @@ class Analyzer(
// SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
// `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
// names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
Review comment:
> nit: move the original comment `// [SPARK-25942](https://issues.apache.org/jira/browse/SPARK-25942): ....` above this line.
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685402725
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480838768
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1425,11 +1433,48 @@ class Analyzer(
// rule: ResolveDeserializer.
case plan if containsDeserializer(plan.expressions) => plan
- // SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
- // `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
- // names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
+ case appendColumns: AppendColumns => appendColumns
+ case _ => a
+ }
+
+ val resolvedGroupingExprs = a.groupingExpressions
+ .map(resolveExpressionTopDown(_, planForResolve, true))
Review comment:
> nit `trimAlias = true`
Done
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1425,11 +1433,48 @@ class Analyzer(
// rule: ResolveDeserializer.
case plan if containsDeserializer(plan.expressions) => plan
- // SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
- // `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
- // names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
+ case appendColumns: AppendColumns => appendColumns
+ case _ => a
+ }
+
+ val resolvedGroupingExprs = a.groupingExpressions
+ .map(resolveExpressionTopDown(_, planForResolve, true))
+ .map {
+ // trim Alias over top-level GetStructField
+ case Alias(s: GetStructField, _) => s
+ case other => other
+ }
+
+ val resolvedAggExprs = a.aggregateExpressions
+ .map(resolveExpressionTopDown(_, planForResolve, true, true))
Review comment:
> `isTopLevel` should be controlled by the method itself, not caller side.
Updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683397866
**[Test build #128042 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128042/testReport)** for PR 28490 at commit [`7ecc8ad`](https://github.com/apache/spark/commit/7ecc8addfb9bd631d67c0fa3a83c89968b30839b).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683423634
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685394621
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/128181/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r436311072
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -510,10 +510,12 @@ class Analyzer(
// collect all the found AggregateExpression, so we can check an expression is part of
// any AggregateExpression or not.
val aggsBuffer = ArrayBuffer[Expression]()
+
Review comment:
nit: plz revert the unencessary changes. (Unrelated changes might lead to revert/backport failures sometimes...)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480148059
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1428,8 +1428,46 @@ class Analyzer(
// SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
// `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
// names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
+ case appendColumns: AppendColumns => appendColumns
+ case _ => a
+ }
+
+ val resolvedGroupingExprs =
+ a.groupingExpressions.map(resolveExpressionTopDown(_, planForResolve))
+ .map(trimStructFieldAlias)
+
+ val resolvedAggExprs = a.aggregateExpressions
+ .map(resolveExpressionTopDown(_, planForResolve))
+ .map {
Review comment:
Can we just stop here and don't change the `resolvedAggExprs`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on pull request #28490: [SPARK-31670][WIP]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-626340051
gentle ping @cloud-fan
Another analyze bug, a little band-fix now, hope some good suggestion and better place to fix this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684001367
**[Test build #128103 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128103/testReport)** for PR 28490 at commit [`f846539`](https://github.com/apache/spark/commit/f846539a81312b1c3864917c024bbd97b1a65b17).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480183578
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1428,8 +1428,46 @@ class Analyzer(
// SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
// `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
// names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
+ case appendColumns: AppendColumns => appendColumns
+ case _ => a
+ }
+
+ val resolvedGroupingExprs =
+ a.groupingExpressions.map(resolveExpressionTopDown(_, planForResolve))
+ .map(trimStructFieldAlias)
+
+ val resolvedAggExprs = a.aggregateExpressions
+ .map(resolveExpressionTopDown(_, planForResolve))
+ .map {
Review comment:
> and clearly document it.
Done, and please review the comment
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640160759
**[Test build #123595 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123595/testReport)** for PR 28490 at commit [`e28b084`](https://github.com/apache/spark/commit/e28b0845c5b172635b1d3b1ae4dd41da55f1b1cf).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r481119430
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,25 +1325,54 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e The expression need to be resolved.
+ * @param q The LogicalPlan whose children are used to resolve expression's attribute.
+ * @param trimAlias When true, we will trim Struct field alias. When isTopLevel = true,
+ * we won't trim top-level Struct field alias.
+ * @return resolved Expression.
*/
- private def resolveExpressionTopDown(e: Expression, q: LogicalPlan): Expression = {
- if (e.resolved) return e
- e match {
- case f: LambdaFunction if !f.bound => f
- case u @ UnresolvedAttribute(nameParts) =>
- // Leave unchanged if resolution fails. Hopefully will be resolved next round.
- val result =
- withPosition(u) {
- q.resolveChildren(nameParts, resolver)
- .orElse(resolveLiteralFunction(nameParts, u, q))
- .getOrElse(u)
+ private def resolveExpressionTopDown(
+ e: Expression,
+ q: LogicalPlan,
+ trimAlias: Boolean = false): Expression = {
+
+ // Explain for param trimAlias and isTopLevel:
+ // 1. trimAlias = false:
+ // We won't trim Struct Field alias.
+ // 2. trimAlias = true && isTopLevel = false:
+ // We will trim all Struct field alias.
+ // 3. trimAlias = true && isTopLevel = true
+ // Trim unnecessary alias of `GetStructField`. Note that, we cannot trim the alias of
+ // top-level `GetStructField`, as we should resolve `UnresolvedAttribute` to a named
+ // expression. The caller side can trim the alias of top-level `GetStructField` if needed.
+ def innerResolve(
+ e: Expression,
+ isTopLevel: Boolean): Expression = {
Review comment:
nit: we can put the above 3 lines into one line
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684500942
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480960791
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1326,24 +1326,49 @@ class Analyzer(
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
*/
- private def resolveExpressionTopDown(e: Expression, q: LogicalPlan): Expression = {
- if (e.resolved) return e
- e match {
- case f: LambdaFunction if !f.bound => f
- case u @ UnresolvedAttribute(nameParts) =>
- // Leave unchanged if resolution fails. Hopefully will be resolved next round.
- val result =
- withPosition(u) {
- q.resolveChildren(nameParts, resolver)
- .orElse(resolveLiteralFunction(nameParts, u, q))
- .getOrElse(u)
+ private def resolveExpressionTopDown(
+ e: Expression,
+ q: LogicalPlan,
+ trimAlias: Boolean = false): Expression = {
+
+ // Explain for param trimAlias and isTopLevel:
+ // 1. trimAlias = false:
+ // We won't trim Struct Field alias.
+ // 2. trimAlias = true && isTopLevel = false:
+ // We will trim all Struct field alias.
+ // 3. trimAlias = true && isTopLevel = true
+ // Trim unnecessary alias of `GetStructField`. Note that, we cannot trim the alias of
+ // top-level `GetStructField`, as we should resolve `UnresolvedAttribute` to a named
+ // expression. The caller side can trim the alias of top-level `GetStructField` if needed.
+ def innerResolve(
+ e: Expression,
+ q: LogicalPlan,
Review comment:
inner method can access the parameters of outer method. We don't need this parameter.
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1326,24 +1326,49 @@ class Analyzer(
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
*/
- private def resolveExpressionTopDown(e: Expression, q: LogicalPlan): Expression = {
- if (e.resolved) return e
- e match {
- case f: LambdaFunction if !f.bound => f
- case u @ UnresolvedAttribute(nameParts) =>
- // Leave unchanged if resolution fails. Hopefully will be resolved next round.
- val result =
- withPosition(u) {
- q.resolveChildren(nameParts, resolver)
- .orElse(resolveLiteralFunction(nameParts, u, q))
- .getOrElse(u)
+ private def resolveExpressionTopDown(
+ e: Expression,
+ q: LogicalPlan,
+ trimAlias: Boolean = false): Expression = {
+
+ // Explain for param trimAlias and isTopLevel:
+ // 1. trimAlias = false:
+ // We won't trim Struct Field alias.
+ // 2. trimAlias = true && isTopLevel = false:
+ // We will trim all Struct field alias.
+ // 3. trimAlias = true && isTopLevel = true
+ // Trim unnecessary alias of `GetStructField`. Note that, we cannot trim the alias of
+ // top-level `GetStructField`, as we should resolve `UnresolvedAttribute` to a named
+ // expression. The caller side can trim the alias of top-level `GetStructField` if needed.
+ def innerResolve(
+ e: Expression,
+ q: LogicalPlan,
Review comment:
same to `trimAlias`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683833419
**[Test build #128103 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128103/testReport)** for PR 28490 at commit [`f846539`](https://github.com/apache/spark/commit/f846539a81312b1c3864917c024bbd97b1a65b17).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480848629
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,24 +1325,43 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e the expression need to be resolved.
+ * @param q the LogicalPlan use to resolve expression's attribute from.
Review comment:
it's inaccurate. We resolve the given expression using `q.children`
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,24 +1325,43 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e the expression need to be resolved.
+ * @param q the LogicalPlan use to resolve expression's attribute from.
Review comment:
it's inaccurate. We resolve the given expression using `q.children`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685299929
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683747499
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685064277
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480565224
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1425,11 +1436,33 @@ class Analyzer(
// rule: ResolveDeserializer.
case plan if containsDeserializer(plan.expressions) => plan
- // SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
- // `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
- // names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
+ case appendColumns: AppendColumns => appendColumns
+ case _ => a
+ }
+
+ val resolvedGroupingExprs = a.groupingExpressions
+ .map(resolveExpressionTopDown(_, planForResolve, true, false))
+
+ val resolvedAggExprs = a.aggregateExpressions
+ .map(resolveExpressionTopDown(_, planForResolve, true, true))
+ .map(_.asInstanceOf[NamedExpression])
Review comment:
Updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685182140
**[Test build #128170 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128170/testReport)** for PR 28490 at commit [`84e65af`](https://github.com/apache/spark/commit/84e65afb5de5a001693b97b4ad47cbef380c3bef).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r518469886
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1425,11 +1450,49 @@ class Analyzer(
// rule: ResolveDeserializer.
case plan if containsDeserializer(plan.expressions) => plan
- // SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
- // `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
- // names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ // SPARK-31607: Resolve Struct field in groupByExpressions and aggregateExpressions
Review comment:
> In the comment, [SPARK-31607](https://issues.apache.org/jira/browse/SPARK-31607) seems to be a typo of [SPARK-31670](https://issues.apache.org/jira/browse/SPARK-31670) because [SPARK-31607](https://issues.apache.org/jira/browse/SPARK-31607) is `Improve the perf of CTESubstitution`.
Yeaļ¼sorry for my mistake
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684496697
cc @viirya @cloud-fan please review at latest commit.
Have raise a new commit follow https://github.com/apache/spark/pull/28490#discussion_r480856230
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684390648
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685278034
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480201905
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1583,6 +1611,35 @@ class Analyzer(
failAnalysis(s"Invalid usage of '*' in expression '${o.prettyName}'")
}
}
+
+ // For aggregateExpression we need NamedExpression, so for top level Struct field
+ // Alias, we won't trim it, for non top level Struct field Alias, we need to trim
+ // it since we have trimmed groupByExpressions.
+ def trimNonTopLevelStructFieldAlias(expr: Expression): NamedExpression = {
+ expr match {
+ case alias @ Alias(_: GetStructField, _) => alias
+ case e => trimStructFieldAlias(e).asInstanceOf[NamedExpression]
+ }
+ }
+
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause the same struct fields
+ // in aggExprs/groupExprs/selectedGroupByExprs be treated as different ones due to different
+ // ExprIds in Alias, and stops us finding the grouping expressions in aggExprs. Here we
+ // will use this method to remove Alias with different ExprId of GetStructField
+ // in groupByAlias/selectedGroupByExprs and aggregateExpressions
+ def trimStructFieldAlias(e: Expression): Expression = {
+ var fixed = ArrayBuffer[Alias]()
+ e.transformDown {
+ case a @ Alias(Alias(struct, _), name) if struct.isInstanceOf[GetStructField] =>
Review comment:
ok things get complicated. How about we add a new flag to `resolveExpressionTopDown`, so that we can strip alias only for resolved column?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685319750
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640179013
**[Test build #123603 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123603/testReport)** for PR 28490 at commit [`1ee0542`](https://github.com/apache/spark/commit/1ee0542e20eea131ff27e4114e3547d32191a6a2).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683782493
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683398038
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480121736
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -3383,6 +3419,25 @@ object CleanupAliases extends Rule[LogicalPlan] {
}
}
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause the same struct fields
+ // in aggExprs/groupExprs/selectedGroupByExprs be treated as different ones due to different
+ // ExprIds in Alias, and stops us finding the grouping expressions in aggExprs. Here we
+ // will use this method to remove Alias with different ExprId of GetStructField
+ // in groupByAlias/selectedGroupByExprs and aggregateExpressions
+ def trimStructFieldAlias(e: Expression): Expression = {
Review comment:
It's only called in `ResolveReferences`, let's define it there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684885938
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r469923244
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,33 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case agg @ (_: Aggregate | _: GroupingSets) =>
+ val resolved = agg.mapExpressions(resolveExpressionTopDown(_, agg))
+ val hasStructField = resolved.expressions.exists {
+ _.collectFirst { case gsf: GetStructField => gsf }.isDefined
+ }
+ if (hasStructField) {
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause same struct field
+ // in aggExprs/groupExprs/selectedGroupByExprs will be resolved divided
+ // with different ExprId of Alias and replace failed when construct
+ // Aggregate in ResolveGroupingAnalytics, so we resolve duplicated struct
+ // field here with same ExprId
Review comment:
I don't get it. `CleanupAliases` will remove aliases from the grouping expressions. Why do we hit the bug?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684499698
**[Test build #128140 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128140/testReport)** for PR 28490 at commit [`891fd1b`](https://github.com/apache/spark/commit/891fd1be604e9d1d2874e7c17cf60be09d1b6741).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640072138
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r436329103
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,11 +1486,70 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case p: LogicalPlan if needResolveStructField(p) =>
+ logTrace(s"Attempting to resolve ${p.simpleString(SQLConf.get.maxToStringFields)}")
+ val resolved = p.mapExpressions(resolveExpressionTopDown(_, p))
+ val structFieldMap = new mutable.HashMap[String, Alias]
+ resolved.transformExpressions {
+ case a @ Alias(struct: GetStructField, _) =>
+ if (structFieldMap.contains(struct.sql)) {
+ val exprId = structFieldMap.getOrElse(struct.sql, a).exprId
+ Alias(a.child, a.name)(exprId, a.qualifier, a.explicitMetadata)
+ } else {
+ structFieldMap.put(struct.sql, a)
+ a
+ }
+ case e => e
+ }
+
case q: LogicalPlan =>
logTrace(s"Attempting to resolve ${q.simpleString(SQLConf.get.maxToStringFields)}")
q.mapExpressions(resolveExpressionTopDown(_, q))
}
+ def needResolveStructField(plan: LogicalPlan): Boolean = {
+ plan match {
+ case UnresolvedHaving(havingCondition, a: Aggregate)
+ if containSameStructFields(a.groupingExpressions.flatMap(_.references),
+ a.aggregateExpressions.flatMap(_.references),
+ Some(havingCondition.references.toSeq)) => true
+ case Aggregate(groupingExpressions, aggregateExpressions, _)
+ if containSameStructFields(groupingExpressions.flatMap(_.references),
+ aggregateExpressions.flatMap(_.references)) => true
+ case GroupingSets(selectedGroupByExprs, groupByExprs, _, aggregations)
+ if containSameStructFields(groupByExprs.flatMap(_.references),
+ aggregations.flatMap(_.references),
+ Some(selectedGroupByExprs.flatMap(_.flatMap(_.references)))) => true
+ case _ => false
+ }
+ }
+
+ def containSameStructFields(
Review comment:
> I think its better to avoid comparing unresolved attributes.... could we resolve them then detect the mismatched exprIds?
How about current? not check, just reoslve.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r470508893
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,33 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case agg @ (_: Aggregate | _: GroupingSets) =>
+ val resolved = agg.mapExpressions(resolveExpressionTopDown(_, agg))
+ val hasStructField = resolved.expressions.exists {
+ _.collectFirst { case gsf: GetStructField => gsf }.isDefined
+ }
+ if (hasStructField) {
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause same struct field
+ // in aggExprs/groupExprs/selectedGroupByExprs will be resolved divided
+ // with different ExprId of Alias and replace failed when construct
+ // Aggregate in ResolveGroupingAnalytics, so we resolve duplicated struct
+ // field here with same ExprId
Review comment:
ah i see, then can we remove aliases in grouping expressions here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r470362613
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,33 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case agg @ (_: Aggregate | _: GroupingSets) =>
+ val resolved = agg.mapExpressions(resolveExpressionTopDown(_, agg))
+ val hasStructField = resolved.expressions.exists {
+ _.collectFirst { case gsf: GetStructField => gsf }.isDefined
+ }
+ if (hasStructField) {
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause same struct field
+ // in aggExprs/groupExprs/selectedGroupByExprs will be resolved divided
+ // with different ExprId of Alias and replace failed when construct
+ // Aggregate in ResolveGroupingAnalytics, so we resolve duplicated struct
+ // field here with same ExprId
Review comment:
> I don't get it. `CleanupAliases` will remove aliases from the grouping expressions. Why do we hit the bug?
This error happen when `ResolveGroupingAnalytics` construct Grouping Analytics Aggregation,
When expand expression, match error because of different ExprID
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683768816
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684400922
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r481662587
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,25 +1325,45 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e The expression need to be resolved.
+ * @param q The LogicalPlan whose children are used to resolve expression's attribute.
+ * @param trimAlias When true, trim unnecessary alias of `GetStructField`. Note that,
+ * we cannot trim the alias of top-level `GetStructField`, as we should
+ * resolve `UnresolvedAttribute` to a named expression. The caller side
+ * can trim the alias of top-level `GetStructField` if it's safe to do so.
+ * @return resolved Expression.
*/
- private def resolveExpressionTopDown(e: Expression, q: LogicalPlan): Expression = {
- if (e.resolved) return e
- e match {
- case f: LambdaFunction if !f.bound => f
- case u @ UnresolvedAttribute(nameParts) =>
- // Leave unchanged if resolution fails. Hopefully will be resolved next round.
- val result =
- withPosition(u) {
- q.resolveChildren(nameParts, resolver)
- .orElse(resolveLiteralFunction(nameParts, u, q))
- .getOrElse(u)
+ private def resolveExpressionTopDown(
+ e: Expression,
+ q: LogicalPlan,
+ trimAlias: Boolean = false): Expression = {
+
+ def innerResolve(e: Expression, isTopLevel: Boolean): Expression = {
+ if (e.resolved) return e
+ e match {
+ case f: LambdaFunction if !f.bound => f
+ case u @ UnresolvedAttribute(nameParts) =>
+ // Leave unchanged if resolution fails. Hopefully will be resolved next round.
+ val resolved =
+ withPosition(u) {
+ q.resolveChildren(nameParts, resolver)
+ .orElse(resolveLiteralFunction(nameParts, u, q))
+ .getOrElse(u)
+ }
+ val result = resolved match {
+ case Alias(s: GetStructField, _) if trimAlias && !isTopLevel => s
+ case others => others
}
Review comment:
As we don't merge this yet, can you also add a comment here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684325215
**[Test build #128132 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128132/testReport)** for PR 28490 at commit [`d0f89af`](https://github.com/apache/spark/commit/d0f89afc57b497cdb77cc20d24dd6dd6df5d8919).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480121972
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,42 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case a: Aggregate =>
+ val resolvedGroupingExprs =
+ a.groupingExpressions.map(resolveExpressionTopDown(_, a))
+ .map(CleanupAliases.trimStructFieldAlias)
+
+ val resolvedAggExprs = a.aggregateExpressions
+ .map(resolveExpressionTopDown(_, a))
+ .map {
+ // Here we want to trim StructField Alias in complex expression
+ // Only Alias(GetStructField, name) can't be trimmed since we need NamedExpression.
+ case alias@Alias(_: GetStructField, _) => alias
+ case e => CleanupAliases.trimStructFieldAlias(e).asInstanceOf[NamedExpression]
Review comment:
> why do we need to do it?
As i have said in comment. If not get rid of that case will got error
```
SPARK-31670: Resolve Struct Field in Grouping Aggregate with same ExprId *** FAILED *** (185 milliseconds)
[info] java.lang.ClassCastException: org.apache.spark.sql.catalyst.expressions.aggregate.AggregateExpression cannot be cast to org.apache.spark.sql.catalyst.expressions.NamedExpression
[info]
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r479954858
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -570,18 +570,44 @@ class Analyzer(
s"Grouping sets size cannot be greater than ${GroupingID.dataType.defaultSize * 8}")
}
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause the same struct fields
Review comment:
Similar like https://github.com/apache/spark/pull/28490#discussion_r470555710.
Need to try if it can solve all problem use `CleanupAlias.trimAliases()`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480183333
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1428,8 +1428,46 @@ class Analyzer(
// SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
// `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
// names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
+ case appendColumns: AppendColumns => appendColumns
+ case _ => a
+ }
+
+ val resolvedGroupingExprs =
+ a.groupingExpressions.map(resolveExpressionTopDown(_, planForResolve))
+ .map(trimStructFieldAlias)
+
+ val resolvedAggExprs = a.aggregateExpressions
+ .map(resolveExpressionTopDown(_, planForResolve))
+ .map {
Review comment:
> ok, let's create a method with name `trimNonTopLevelStructFieldAlias `
Uodated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640179128
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r470660102
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,33 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case agg @ (_: Aggregate | _: GroupingSets) =>
+ val resolved = agg.mapExpressions(resolveExpressionTopDown(_, agg))
+ val hasStructField = resolved.expressions.exists {
+ _.collectFirst { case gsf: GetStructField => gsf }.isDefined
+ }
+ if (hasStructField) {
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause same struct field
+ // in aggExprs/groupExprs/selectedGroupByExprs will be resolved divided
+ // with different ExprId of Alias and replace failed when construct
+ // Aggregate in ResolveGroupingAnalytics, so we resolve duplicated struct
+ // field here with same ExprId
+ val structFieldMap = mutable.Map[String, Alias]()
+ resolved.transformExpressionsDown {
Review comment:
> another concern: for `SELECT a.b, a.b FROM t GROUP BY a.b`, will we end up with an `Aggregate` operator whose output has duplicated ExprId? It's probably not an issue as the two output columns always have the same value.
struct.sql in UT show as `t`.`c`.`json_string`, [table].[column].[field], so seems won't have case that same `struct.sql` mean different column?
New way in https://github.com/apache/spark/pull/28490#discussion_r470555710 only handle current node to avoid same struct.sql show different column because of alias in different sql level.
emm, for two output `a.b`, it's hard to say which one is the one in cube? right?, so in this case same output with duplicated exprId seems ok.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684090975
**[Test build #128120 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128120/testReport)** for PR 28490 at commit [`82f3876`](https://github.com/apache/spark/commit/82f38766867cb1719fd0601da1332caa5b864a82).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684181119
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683843579
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480963443
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1326,24 +1326,49 @@ class Analyzer(
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
*/
- private def resolveExpressionTopDown(e: Expression, q: LogicalPlan): Expression = {
- if (e.resolved) return e
- e match {
- case f: LambdaFunction if !f.bound => f
- case u @ UnresolvedAttribute(nameParts) =>
- // Leave unchanged if resolution fails. Hopefully will be resolved next round.
- val result =
- withPosition(u) {
- q.resolveChildren(nameParts, resolver)
- .orElse(resolveLiteralFunction(nameParts, u, q))
- .getOrElse(u)
+ private def resolveExpressionTopDown(
+ e: Expression,
+ q: LogicalPlan,
+ trimAlias: Boolean = false): Expression = {
+
+ // Explain for param trimAlias and isTopLevel:
Review comment:
The previous parameter doc of `resolveExpressionTopDown` is good. Why we turn it to comment here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684022504
**[Test build #128104 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128104/testReport)** for PR 28490 at commit [`d63613f`](https://github.com/apache/spark/commit/d63613fadb7ad82c815a6e0114518f6c6eca3235).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r479848494
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,33 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case agg @ (_: Aggregate | _: GroupingSets) =>
+ val resolved = agg.mapExpressions(resolveExpressionTopDown(_, agg))
+ val hasStructField = resolved.expressions.exists {
+ _.collectFirst { case gsf: GetStructField => gsf }.isDefined
+ }
+ if (hasStructField) {
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause same struct field
+ // in aggExprs/groupExprs/selectedGroupByExprs will be resolved divided
+ // with different ExprId of Alias and replace failed when construct
+ // Aggregate in ResolveGroupingAnalytics, so we resolve duplicated struct
+ // field here with same ExprId
Review comment:
also ping @maropu
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683397361
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][WIP]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-626339987
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480805872
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1425,11 +1433,48 @@ class Analyzer(
// rule: ResolveDeserializer.
case plan if containsDeserializer(plan.expressions) => plan
- // SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
- // `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
- // names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
+ case appendColumns: AppendColumns => appendColumns
+ case _ => a
+ }
+
+ val resolvedGroupingExprs = a.groupingExpressions
+ .map(resolveExpressionTopDown(_, planForResolve, true))
+ .map {
+ // trim Alias over top-level GetStructField
+ case Alias(s: GetStructField, _) => s
+ case other => other
+ }
+
+ val resolvedAggExprs = a.aggregateExpressions
+ .map(resolveExpressionTopDown(_, planForResolve, true, true))
Review comment:
`isTopLevel` should be controlled by the method itself, not caller side.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r481661602
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1425,11 +1445,39 @@ class Analyzer(
// rule: ResolveDeserializer.
case plan if containsDeserializer(plan.expressions) => plan
- // SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
- // `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
- // names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
Review comment:
> We need a high-level comment to explain why we trim alias here, e.g.
>
> ```
> // [SPARK-31670](https://issues.apache.org/jira/browse/SPARK-31670): ...
> ```
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r436357558
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1259,6 +1259,11 @@ class Analyzer(
attr.withExprId(exprId)
}
+ private def dedupStructField(attr: Alias, structFieldMap: Map[String, Attribute]) = {
Review comment:
> Not used now?
Oh, yea
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480231511
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1583,6 +1611,35 @@ class Analyzer(
failAnalysis(s"Invalid usage of '*' in expression '${o.prettyName}'")
}
}
+
+ // For aggregateExpression we need NamedExpression, so for top level Struct field
+ // Alias, we won't trim it, for non top level Struct field Alias, we need to trim
+ // it since we have trimmed groupByExpressions.
+ def trimNonTopLevelStructFieldAlias(expr: Expression): NamedExpression = {
+ expr match {
+ case alias @ Alias(_: GetStructField, _) => alias
+ case e => trimStructFieldAlias(e).asInstanceOf[NamedExpression]
+ }
+ }
+
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause the same struct fields
+ // in aggExprs/groupExprs/selectedGroupByExprs be treated as different ones due to different
+ // ExprIds in Alias, and stops us finding the grouping expressions in aggExprs. Here we
+ // will use this method to remove Alias with different ExprId of GetStructField
+ // in groupByAlias/selectedGroupByExprs and aggregateExpressions
+ def trimStructFieldAlias(e: Expression): Expression = {
+ var fixed = ArrayBuffer[Alias]()
+ e.transformDown {
+ case a @ Alias(Alias(struct, _), name) if struct.isInstanceOf[GetStructField] =>
Review comment:
> ok things get complicated. How about we add a new flag to resolveExpressionTopDown, so that we can strip alias only for resolved column?
> and Add top-level Alias manually after calling resolveExpressionTopDown.
Updated..really complicated...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r479961994
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -570,18 +570,44 @@ class Analyzer(
s"Grouping sets size cannot be greater than ${GroupingID.dataType.defaultSize * 8}")
}
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause the same struct fields
Review comment:
It's simpler to fix the problem earlier. Let's try it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683843591
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/128097/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685391625
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685288939
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640212326
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685299896
**[Test build #128184 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128184/testReport)** for PR 28490 at commit [`e6fb91f`](https://github.com/apache/spark/commit/e6fb91fc8124c60e27899846b675853c22537791).
* This patch **fails MiMa tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685319750
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480853788
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,24 +1325,43 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e the expression need to be resolved.
+ * @param q the LogicalPlan whose children are used to resolve expression's attribute.
+ * @param trimAlias whether need to trim alias of Struct field. When true, we will trim
Review comment:
```
When true, trim unnecessary alias of `GetStructField`. Note that, we cannot trim
the alias of top-level `GetStructField`, as we should resolve `UnresolvedAttribute` to
a named expression. The caller side can trim the alias of top-level `GetStructField` if
it's safe to do so.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r481650746
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1425,11 +1445,39 @@ class Analyzer(
// rule: ResolveDeserializer.
case plan if containsDeserializer(plan.expressions) => plan
- // SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
- // `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
- // names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
Review comment:
We need a high-level comment to explain why we trim alias here, e.g.
```
// SPARK-31670: ...
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684874621
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r471409620
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,33 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case agg @ (_: Aggregate | _: GroupingSets) =>
+ val resolved = agg.mapExpressions(resolveExpressionTopDown(_, agg))
+ val hasStructField = resolved.expressions.exists {
+ _.collectFirst { case gsf: GetStructField => gsf }.isDefined
+ }
+ if (hasStructField) {
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause same struct field
+ // in aggExprs/groupExprs/selectedGroupByExprs will be resolved divided
+ // with different ExprId of Alias and replace failed when construct
+ // Aggregate in ResolveGroupingAnalytics, so we resolve duplicated struct
+ // field here with same ExprId
Review comment:
But `Aggregate/GroupingSets` does not need `NamedExpression` for grouping expressions, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683874626
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/32728/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r434366334
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -506,31 +506,55 @@ class Analyzer(
aggregations: Seq[NamedExpression],
groupByAliases: Seq[Alias],
groupingAttrs: Seq[Expression],
- gid: Attribute): Seq[NamedExpression] = aggregations.map {
- // collect all the found AggregateExpression, so we can check an expression is part of
- // any AggregateExpression or not.
- val aggsBuffer = ArrayBuffer[Expression]()
- // Returns whether the expression belongs to any expressions in `aggsBuffer` or not.
- def isPartOfAggregation(e: Expression): Boolean = {
- aggsBuffer.exists(a => a.find(_ eq e).isDefined)
+ gid: Attribute): Seq[NamedExpression] = {
+ val resolvedGroupByAliases = groupByAliases.map(_.transformDown {
Review comment:
cc: @cloud-fan @viirya
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683424444
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640249149
**[Test build #123605 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123605/testReport)** for PR 28490 at commit [`0af3166`](https://github.com/apache/spark/commit/0af316675f376472d6deab40c82401a55a765e20).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684798931
**[Test build #128140 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128140/testReport)** for PR 28490 at commit [`891fd1b`](https://github.com/apache/spark/commit/891fd1be604e9d1d2874e7c17cf60be09d1b6741).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684767332
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684346327
**[Test build #128134 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128134/testReport)** for PR 28490 at commit [`3ebec5f`](https://github.com/apache/spark/commit/3ebec5f0a5f8312e8ccfcf8fbdb6d4bd69d33a99).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685299941
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/128184/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683856477
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/128099/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r471496799
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,33 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case agg @ (_: Aggregate | _: GroupingSets) =>
+ val resolved = agg.mapExpressions(resolveExpressionTopDown(_, agg))
+ val hasStructField = resolved.expressions.exists {
+ _.collectFirst { case gsf: GetStructField => gsf }.isDefined
+ }
+ if (hasStructField) {
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause same struct field
+ // in aggExprs/groupExprs/selectedGroupByExprs will be resolved divided
+ // with different ExprId of Alias and replace failed when construct
+ // Aggregate in ResolveGroupingAnalytics, so we resolve duplicated struct
+ // field here with same ExprId
Review comment:
Sorry I didn't make myself clear. Can we remove aliases only in `Aggregate.groupingExpressions`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684390683
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/128135/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683829229
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640213213
**[Test build #123605 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123605/testReport)** for PR 28490 at commit [`0af3166`](https://github.com/apache/spark/commit/0af316675f376472d6deab40c82401a55a765e20).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-638554174
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-638553797
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640163490
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r434975787
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -506,31 +506,55 @@ class Analyzer(
aggregations: Seq[NamedExpression],
groupByAliases: Seq[Alias],
groupingAttrs: Seq[Expression],
- gid: Attribute): Seq[NamedExpression] = aggregations.map {
- // collect all the found AggregateExpression, so we can check an expression is part of
- // any AggregateExpression or not.
- val aggsBuffer = ArrayBuffer[Expression]()
- // Returns whether the expression belongs to any expressions in `aggsBuffer` or not.
- def isPartOfAggregation(e: Expression): Boolean = {
- aggsBuffer.exists(a => a.find(_ eq e).isDefined)
+ gid: Attribute): Seq[NamedExpression] = {
+ val resolvedGroupByAliases = groupByAliases.map(_.transformDown {
Review comment:
> Probably, we should not fix this issue in `ResolveGroupingAnalytics`, but in `ResolveRefences` just like this;
>
> ```
> object ResolveReferences extends Rule[LogicalPlan] {
> ...
> def apply(plan: LogicalPlan): LogicalPlan = plan.resolveOperatorsUp {
> case p: LogicalPlan if !p.childrenResolved => p
> ....
>
> case a @ Aggregate(groupingExprs, aggExprs, _) if both `aggExprs` and `aggExprs` have the same struct field =>
> val newAgg = resolve expressions so that they have the same exprIds
> newAgg
> ```
Also having clause.. may have this problem. I didn't thinking enough case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-638554169
**[Test build #123516 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123516/testReport)** for PR 28490 at commit [`4c0b04c`](https://github.com/apache/spark/commit/4c0b04c7ed11ea8c60424b9abd21cb8d05dda19e).
* This patch **fails Scala style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-671465677
**[Test build #127288 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/127288/testReport)** for PR 28490 at commit [`0af3166`](https://github.com/apache/spark/commit/0af316675f376472d6deab40c82401a55a765e20).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684620818
**[Test build #128143 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128143/testReport)** for PR 28490 at commit [`281096a`](https://github.com/apache/spark/commit/281096ab5ca212d12171a9f278d6d63eab4605bb).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480853788
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,24 +1325,43 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e the expression need to be resolved.
+ * @param q the LogicalPlan whose children are used to resolve expression's attribute.
+ * @param trimAlias whether need to trim alias of Struct field. When true, we will trim
Review comment:
```
When true, trim unnecessary alias of `GetStructField`. Note that, we cannot trim
the alias of top-level `GetStructField`, as we should resolve `UnresolvedAttribute` to
a named expression. The caller side can trim the alias of top-level `GetStructField` if
needed.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684181119
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683781806
**[Test build #128099 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128099/testReport)** for PR 28490 at commit [`cf31ab4`](https://github.com/apache/spark/commit/cf31ab4fb8e649ca860c6be28142da946eccc4b9).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683873997
**[Test build #128104 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128104/testReport)** for PR 28490 at commit [`d63613f`](https://github.com/apache/spark/commit/d63613fadb7ad82c815a6e0114518f6c6eca3235).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640217133
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640216881
**[Test build #123603 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123603/testReport)** for PR 28490 at commit [`1ee0542`](https://github.com/apache/spark/commit/1ee0542e20eea131ff27e4114e3547d32191a6a2).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683856466
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r479932237
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -3496,6 +3496,98 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkIfSeedExistsInExplain(df2)
}
+ test("SPARK-31670: Resolve Struct Field in Grouping Aggregate with same ExprId") {
+ withTempView("t") {
+ sql(
+ """
+ |CREATE TEMPORARY VIEW t(a, b, c) AS
+ |SELECT * FROM VALUES
+ |('A', 1, NAMED_STRUCT('row_id', 1, 'json_string', '{"i": 1}')),
+ |('A', 2, NAMED_STRUCT('row_id', 2, 'json_string', '{"i": 1}')),
+ |('A', 2, NAMED_STRUCT('row_id', 2, 'json_string', '{"i": 2}')),
+ |('B', 1, NAMED_STRUCT('row_id', 3, 'json_string', '{"i": 1}')),
+ |('C', 3, NAMED_STRUCT('row_id', 4, 'json_string', '{"i": 1}'))
+ """.stripMargin)
+
+ checkAnswer(
+ sql(
+ """
+ |SELECT a, c.json_string, SUM(b)
+ |FROM t
+ |GROUP BY a, c.json_string
+ |WITH CUBE
Review comment:
does normal GROUP BY have this bug?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480122040
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,42 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case a: Aggregate =>
Review comment:
there is a `case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns)` above, please merge that into this case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-638553517
**[Test build #123516 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123516/testReport)** for PR 28490 at commit [`4c0b04c`](https://github.com/apache/spark/commit/4c0b04c7ed11ea8c60424b9abd21cb8d05dda19e).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r479942446
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -3496,6 +3496,98 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkIfSeedExistsInExplain(df2)
}
+ test("SPARK-31670: Resolve Struct Field in Grouping Aggregate with same ExprId") {
+ withTempView("t") {
+ sql(
+ """
+ |CREATE TEMPORARY VIEW t(a, b, c) AS
+ |SELECT * FROM VALUES
+ |('A', 1, NAMED_STRUCT('row_id', 1, 'json_string', '{"i": 1}')),
+ |('A', 2, NAMED_STRUCT('row_id', 2, 'json_string', '{"i": 1}')),
+ |('A', 2, NAMED_STRUCT('row_id', 2, 'json_string', '{"i": 2}')),
+ |('B', 1, NAMED_STRUCT('row_id', 3, 'json_string', '{"i": 1}')),
+ |('C', 3, NAMED_STRUCT('row_id', 4, 'json_string', '{"i": 1}'))
+ """.stripMargin)
+
+ checkAnswer(
+ sql(
+ """
+ |SELECT a, c.json_string, SUM(b)
+ |FROM t
+ |GROUP BY a, c.json_string
+ |WITH CUBE
Review comment:
> does normal GROUP BY have this bug?
No, this happened when construct group Analytics, and changed code only fix this too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683874611
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683424163
**[Test build #128042 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128042/testReport)** for PR 28490 at commit [`7ecc8ad`](https://github.com/apache/spark/commit/7ecc8addfb9bd631d67c0fa3a83c89968b30839b).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683856466
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684620818
**[Test build #128143 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128143/testReport)** for PR 28490 at commit [`281096a`](https://github.com/apache/spark/commit/281096ab5ca212d12171a9f278d6d63eab4605bb).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r434955113
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -506,31 +506,55 @@ class Analyzer(
aggregations: Seq[NamedExpression],
groupByAliases: Seq[Alias],
groupingAttrs: Seq[Expression],
- gid: Attribute): Seq[NamedExpression] = aggregations.map {
- // collect all the found AggregateExpression, so we can check an expression is part of
- // any AggregateExpression or not.
- val aggsBuffer = ArrayBuffer[Expression]()
- // Returns whether the expression belongs to any expressions in `aggsBuffer` or not.
- def isPartOfAggregation(e: Expression): Boolean = {
- aggsBuffer.exists(a => a.find(_ eq e).isDefined)
+ gid: Attribute): Seq[NamedExpression] = {
+ val resolvedGroupByAliases = groupByAliases.map(_.transformDown {
Review comment:
> Based on above analysis, seems it is caused by `ResolveReferences`, instead of `ResolveGroupingAnalytics`? Can we possibly have a reproducible case without CUBE?
This happened with cube when construct cube LogicalPlan.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r481120900
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,25 +1325,54 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e The expression need to be resolved.
+ * @param q The LogicalPlan whose children are used to resolve expression's attribute.
+ * @param trimAlias When true, we will trim Struct field alias. When isTopLevel = true,
Review comment:
For `innerResolve`, the `isTopLevel` parameter is just an internal state to indicate top-level `GetStructField`, too trivial to comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684766244
**[Test build #128134 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128134/testReport)** for PR 28490 at commit [`3ebec5f`](https://github.com/apache/spark/commit/3ebec5f0a5f8312e8ccfcf8fbdb6d4bd69d33a99).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r470523167
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,33 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case agg @ (_: Aggregate | _: GroupingSets) =>
+ val resolved = agg.mapExpressions(resolveExpressionTopDown(_, agg))
+ val hasStructField = resolved.expressions.exists {
+ _.collectFirst { case gsf: GetStructField => gsf }.isDefined
+ }
+ if (hasStructField) {
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause same struct field
+ // in aggExprs/groupExprs/selectedGroupByExprs will be resolved divided
+ // with different ExprId of Alias and replace failed when construct
+ // Aggregate in ResolveGroupingAnalytics, so we resolve duplicated struct
+ // field here with same ExprId
+ val structFieldMap = mutable.Map[String, Alias]()
+ resolved.transformExpressionsDown {
+ case a @ Alias(struct: GetStructField, _) =>
+ if (structFieldMap.contains(struct.sql)) {
Review comment:
is it safe to use sql string as key?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684002502
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685402725
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684789863
**[Test build #128132 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128132/testReport)** for PR 28490 at commit [`d0f89af`](https://github.com/apache/spark/commit/d0f89afc57b497cdb77cc20d24dd6dd6df5d8919).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684775900
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684023594
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685401930
**[Test build #128195 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128195/testReport)** for PR 28490 at commit [`e6fb91f`](https://github.com/apache/spark/commit/e6fb91fc8124c60e27899846b675853c22537791).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640071894
**[Test build #123589 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123589/testReport)** for PR 28490 at commit [`c4ff823`](https://github.com/apache/spark/commit/c4ff823171c80f157a1306713af71521168588ee).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640160868
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640104812
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684885030
**[Test build #128151 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128151/testReport)** for PR 28490 at commit [`51cea07`](https://github.com/apache/spark/commit/51cea0781049f0e39c80a546e1e1f0f059c52971).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684400922
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684023606
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/128104/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683782493
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685063270
**[Test build #128151 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128151/testReport)** for PR 28490 at commit [`51cea07`](https://github.com/apache/spark/commit/51cea0781049f0e39c80a546e1e1f0f059c52971).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685182537
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480838692
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1326,7 +1326,11 @@ class Analyzer(
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
*/
- private def resolveExpressionTopDown(e: Expression, q: LogicalPlan): Expression = {
+ private def resolveExpressionTopDown(
+ e: Expression,
+ q: LogicalPlan,
+ trimAlias: Boolean = false,
+ isTopLevel: Boolean = false): Expression = {
Review comment:
> `isTopLevel` should be true by default, otherwise the semantic is wrong when callers call `resolveExpressionTopDown` without specifying `isTopLevel`.
Got your point, make this param controlled by this method make this method more safety.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r481667174
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,25 +1325,45 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e The expression need to be resolved.
+ * @param q The LogicalPlan whose children are used to resolve expression's attribute.
+ * @param trimAlias When true, trim unnecessary alias of `GetStructField`. Note that,
+ * we cannot trim the alias of top-level `GetStructField`, as we should
+ * resolve `UnresolvedAttribute` to a named expression. The caller side
+ * can trim the alias of top-level `GetStructField` if it's safe to do so.
+ * @return resolved Expression.
*/
- private def resolveExpressionTopDown(e: Expression, q: LogicalPlan): Expression = {
- if (e.resolved) return e
- e match {
- case f: LambdaFunction if !f.bound => f
- case u @ UnresolvedAttribute(nameParts) =>
- // Leave unchanged if resolution fails. Hopefully will be resolved next round.
- val result =
- withPosition(u) {
- q.resolveChildren(nameParts, resolver)
- .orElse(resolveLiteralFunction(nameParts, u, q))
- .getOrElse(u)
+ private def resolveExpressionTopDown(
+ e: Expression,
+ q: LogicalPlan,
+ trimAlias: Boolean = false): Expression = {
+
+ def innerResolve(e: Expression, isTopLevel: Boolean): Expression = {
+ if (e.resolved) return e
+ e match {
+ case f: LambdaFunction if !f.bound => f
+ case u @ UnresolvedAttribute(nameParts) =>
+ // Leave unchanged if resolution fails. Hopefully will be resolved next round.
+ val resolved =
+ withPosition(u) {
+ q.resolveChildren(nameParts, resolver)
+ .orElse(resolveLiteralFunction(nameParts, u, q))
+ .getOrElse(u)
+ }
+ val result = resolved match {
+ case Alias(s: GetStructField, _) if trimAlias && !isTopLevel => s
+ case others => others
}
Review comment:
> As we don't merge this yet, can you also add a comment here?
Yea
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480121157
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,42 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case a: Aggregate =>
+ val resolvedGroupingExprs =
+ a.groupingExpressions.map(resolveExpressionTopDown(_, a))
+ .map(CleanupAliases.trimStructFieldAlias)
+
+ val resolvedAggExprs = a.aggregateExpressions
+ .map(resolveExpressionTopDown(_, a))
+ .map {
+ // Here we want to trim StructField Alias in complex expression
+ // Only Alias(GetStructField, name) can't be trimmed since we need NamedExpression.
+ case alias@Alias(_: GetStructField, _) => alias
+ case e => CleanupAliases.trimStructFieldAlias(e).asInstanceOf[NamedExpression]
Review comment:
why do we need to do it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685182537
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480873342
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1428,8 +1428,36 @@ class Analyzer(
// SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
// `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
// names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
Review comment:
> The comment is lost?
Oh make a mistake, add back
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r481649564
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1525,6 +1573,17 @@ class Analyzer(
AttributeSet(projectList.collect { case a: Alias => a.toAttribute })
}
+ // This method is used to trim groupByExpressions/selectedGroupByExpressions's top-level
+ // GetStructField Alias. Since these expression are not NamedExpression originally,
+ // we are safely to trim top-level GetStructField Alias.
Review comment:
nit: `we are safe to ...`
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1525,6 +1573,17 @@ class Analyzer(
AttributeSet(projectList.collect { case a: Alias => a.toAttribute })
}
+ // This method is used to trim groupByExpressions/selectedGroupByExpressions's top-level
+ // GetStructField Alias. Since these expression are not NamedExpression originally,
+ // we are safely to trim top-level GetStructField Alias.
+ def trimTopLevelGetStructFieldAlias(e: Expression): Expression = {
+ e match {
+ // trim Alias over top-level GetStructField
Review comment:
we can remove this now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684791382
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480801106
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1326,7 +1326,11 @@ class Analyzer(
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
*/
- private def resolveExpressionTopDown(e: Expression, q: LogicalPlan): Expression = {
+ private def resolveExpressionTopDown(
Review comment:
let's add comments to explain what the new parameters do
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684985030
Thanks for updating the PR description. It looks better now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r479953325
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -570,18 +570,44 @@ class Analyzer(
s"Grouping sets size cannot be greater than ${GroupingID.dataType.defaultSize * 8}")
}
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause the same struct fields
Review comment:
Can we fix the problem earlier in `ResolveReferences`? e.g.Ā·
```
case a: Aggregate =>
// comment
val resolvedGroupingExprs = CleanupAliases.trimAliases(
a.groupingExpressions.map(resolveExpressionTopDown(_, a))
val resolvedAggExprs = a.aggregateExpressions.map(resolveExpressionTopDown(_, a)
a.makeCopy(resolvedGroupingExprs, resolvedAggExprs, a.child)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685390640
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685697711
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685697711
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r436327053
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -3496,6 +3496,88 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkIfSeedExistsInExplain(df2)
}
+ test("SPARK-31670: Struct Field in groupByExpr with CUBE") {
+ withTable("t") {
+ sql(
+ """CREATE TABLE t(
+ |a STRING,
+ |b INT,
+ |c ARRAY<STRUCT<row_id:INT,json_string:STRING>>,
+ |d ARRAY<ARRAY<STRING>>,
+ |e ARRAY<MAP<STRING, INT>>)
+ |USING ORC""".stripMargin)
Review comment:
> See: [#28490 (comment)](https://github.com/apache/spark/pull/28490#discussion_r434371366)
Test case change to view and with actual data.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683616308
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685293971
**[Test build #128184 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128184/testReport)** for PR 28490 at commit [`e6fb91f`](https://github.com/apache/spark/commit/e6fb91fc8124c60e27899846b675853c22537791).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480129864
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -3383,6 +3419,25 @@ object CleanupAliases extends Rule[LogicalPlan] {
}
}
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause the same struct fields
+ // in aggExprs/groupExprs/selectedGroupByExprs be treated as different ones due to different
+ // ExprIds in Alias, and stops us finding the grouping expressions in aggExprs. Here we
+ // will use this method to remove Alias with different ExprId of GetStructField
+ // in groupByAlias/selectedGroupByExprs and aggregateExpressions
+ def trimStructFieldAlias(e: Expression): Expression = {
Review comment:
> It's only called in `ResolveReferences`, let's define it there.
Ok
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480120727
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -570,18 +570,44 @@ class Analyzer(
s"Grouping sets size cannot be greater than ${GroupingID.dataType.defaultSize * 8}")
}
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause the same struct fields
Review comment:
cc @cloud-fan
Updated. since it's special, we can't just use CleanupAlias.trimAlias()
Add a new method to fit for GetStructField
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640178688
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685292014
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480961849
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1326,24 +1326,49 @@ class Analyzer(
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
*/
- private def resolveExpressionTopDown(e: Expression, q: LogicalPlan): Expression = {
- if (e.resolved) return e
- e match {
- case f: LambdaFunction if !f.bound => f
- case u @ UnresolvedAttribute(nameParts) =>
- // Leave unchanged if resolution fails. Hopefully will be resolved next round.
- val result =
- withPosition(u) {
- q.resolveChildren(nameParts, resolver)
- .orElse(resolveLiteralFunction(nameParts, u, q))
- .getOrElse(u)
+ private def resolveExpressionTopDown(
+ e: Expression,
+ q: LogicalPlan,
+ trimAlias: Boolean = false): Expression = {
+
+ // Explain for param trimAlias and isTopLevel:
+ // 1. trimAlias = false:
+ // We won't trim Struct Field alias.
+ // 2. trimAlias = true && isTopLevel = false:
+ // We will trim all Struct field alias.
+ // 3. trimAlias = true && isTopLevel = true
+ // Trim unnecessary alias of `GetStructField`. Note that, we cannot trim the alias of
+ // top-level `GetStructField`, as we should resolve `UnresolvedAttribute` to a named
+ // expression. The caller side can trim the alias of top-level `GetStructField` if needed.
+ def innerResolve(
+ e: Expression,
+ q: LogicalPlan,
+ trimAlias: Boolean = false,
+ isTopLevel: Boolean = true): Expression = {
Review comment:
this parameter doesn't need a default value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684800147
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r479953325
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -570,18 +570,44 @@ class Analyzer(
s"Grouping sets size cannot be greater than ${GroupingID.dataType.defaultSize * 8}")
}
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause the same struct fields
Review comment:
Can we fix the problem earlier in `ResolveReferences`? e.g.Ā·
```
case a: Aggregate =>
val resolvedGroupingExprs = CleanupAliases.trimAliases(
a.groupingExpressions.map(resolveExpressionTopDown(_, a))
val resolvedAggExprs = a.aggregateExpressions.map(resolveExpressionTopDown(_, a)
a.makeCopy(resolvedGroupingExprs, resolvedAggExprs, a.child)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480202463
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1583,6 +1611,35 @@ class Analyzer(
failAnalysis(s"Invalid usage of '*' in expression '${o.prettyName}'")
}
}
+
+ // For aggregateExpression we need NamedExpression, so for top level Struct field
+ // Alias, we won't trim it, for non top level Struct field Alias, we need to trim
+ // it since we have trimmed groupByExpressions.
+ def trimNonTopLevelStructFieldAlias(expr: Expression): NamedExpression = {
+ expr match {
+ case alias @ Alias(_: GetStructField, _) => alias
+ case e => trimStructFieldAlias(e).asInstanceOf[NamedExpression]
+ }
+ }
+
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause the same struct fields
+ // in aggExprs/groupExprs/selectedGroupByExprs be treated as different ones due to different
+ // ExprIds in Alias, and stops us finding the grouping expressions in aggExprs. Here we
+ // will use this method to remove Alias with different ExprId of GetStructField
+ // in groupByAlias/selectedGroupByExprs and aggregateExpressions
+ def trimStructFieldAlias(e: Expression): Expression = {
+ var fixed = ArrayBuffer[Alias]()
+ e.transformDown {
+ case a @ Alias(Alias(struct, _), name) if struct.isInstanceOf[GetStructField] =>
Review comment:
and Add top-level Alias manually after calling `resolveExpressionTopDown`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683843579
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480131732
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,42 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case a: Aggregate =>
Review comment:
> there is a `case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns)` above, please merge that into this case.
Updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r436354138
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1259,6 +1259,11 @@ class Analyzer(
attr.withExprId(exprId)
}
+ private def dedupStructField(attr: Alias, structFieldMap: Map[String, Attribute]) = {
Review comment:
Not used now?
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1481,7 +1486,35 @@ class Analyzer(
case q: LogicalPlan =>
logTrace(s"Attempting to resolve ${q.simpleString(SQLConf.get.maxToStringFields)}")
Review comment:
w/ some code cleanup;
```
case agg @ (_: Aggregate | _: GroupingSets) =>
val resolved = agg.mapExpressions(resolveExpressionTopDown(_, agg))
val hasStructField = resolved.expressions.exists {
_.collectFirst { case gsf: GetStructField => gsf }.isDefined
}
if (hasStructField) {
// Plz describe some comments why we need this handling...
val structFieldMap = mutable.Map[String, Alias]()
resolved.transformExpressionsDown {
case a @ Alias(struct: GetStructField, _) =>
if (structFieldMap.contains(struct.sql)) {
val exprId = structFieldMap.getOrElse(struct.sql, a).exprId
Alias(a.child, a.name)(exprId, a.qualifier, a.explicitMetadata)
} else {
structFieldMap += (struct.sql -> a)
a
}
}
} else {
resolved
}
```
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1481,7 +1486,35 @@ class Analyzer(
case q: LogicalPlan =>
logTrace(s"Attempting to resolve ${q.simpleString(SQLConf.get.maxToStringFields)}")
Review comment:
Could you write this handling in an independent patten like this?
```
case agg @ (_: Aggregate | _: GroupingSets) =>
val resolved = agg.mapExpressions(resolveExpressionTopDown(_, agg))
val structFieldMap = mutable.Map[String, Alias]()
resolved.transformExpressionsDown {
// Plz describe some comments why we need this handling...
case a @ Alias(struct: GetStructField, _) =>
if (structFieldMap.contains(struct.sql)) {
val exprId = structFieldMap.getOrElse(struct.sql, a).exprId
Alias(a.child, a.name)(exprId, a.qualifier, a.explicitMetadata)
} else {
structFieldMap += (struct.sql -> a)
a
}
case e => e
}
case q: LogicalPlan =>
logTrace(s"Attempting to resolve ${q.simpleString(SQLConf.get.maxToStringFields)}")
q.mapExpressions(resolveExpressionTopDown(_, q))
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480155321
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1428,8 +1428,46 @@ class Analyzer(
// SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
// `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
// names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
+ case appendColumns: AppendColumns => appendColumns
+ case _ => a
+ }
+
+ val resolvedGroupingExprs =
+ a.groupingExpressions.map(resolveExpressionTopDown(_, planForResolve))
+ .map(trimStructFieldAlias)
+
+ val resolvedAggExprs = a.aggregateExpressions
+ .map(resolveExpressionTopDown(_, planForResolve))
+ .map {
Review comment:
> Can we just stop here and don't change the `resolvedAggExprs`?
No, since when we have complex expression in `GROU BY` and aggregateExpression. if we don't resolve this will make aggregateExpression can't match with groupByExpression's expr.
```
checkAnswer(
sql(
"""
|SELECT a, get_json_object(c.json_string, '$.i'), SUM(b)
|FROM t
|GROUP BY a, get_json_object(c.json_string, '$.i')
|WITH CUBE
|""".stripMargin),
Row("A", "1", 3) :: Row("A", "2", 2) :: Row("A", null, 5) ::
Row("B", "1", 1) :: Row("B", null, 1) ::
Row("C", "1", 3) :: Row("C", null, 3) ::
Row(null, "1", 7) :: Row(null, "2", 2) :: Row(null, null, 9) :: Nil)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640072138
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r479944701
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -3496,6 +3496,98 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkIfSeedExistsInExplain(df2)
}
+ test("SPARK-31670: Resolve Struct Field in Grouping Aggregate with same ExprId") {
+ withTempView("t") {
+ sql(
+ """
+ |CREATE TEMPORARY VIEW t(a, b, c) AS
+ |SELECT * FROM VALUES
+ |('A', 1, NAMED_STRUCT('row_id', 1, 'json_string', '{"i": 1}')),
+ |('A', 2, NAMED_STRUCT('row_id', 2, 'json_string', '{"i": 1}')),
+ |('A', 2, NAMED_STRUCT('row_id', 2, 'json_string', '{"i": 2}')),
+ |('B', 1, NAMED_STRUCT('row_id', 3, 'json_string', '{"i": 1}')),
+ |('C', 3, NAMED_STRUCT('row_id', 4, 'json_string', '{"i": 1}'))
+ """.stripMargin)
+
+ checkAnswer(
+ sql(
+ """
+ |SELECT a, c.json_string, SUM(b)
+ |FROM t
+ |GROUP BY a, c.json_string
+ |WITH CUBE
Review comment:
> does normal GROUP BY have this bug?
Add a test case for normal GROUP BY
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683397244
**[Test build #128041 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128041/testReport)** for PR 28490 at commit [`5f0562c`](https://github.com/apache/spark/commit/5f0562c7f2ecf8baa7454a8fb6c74633a608ebc0).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r436324734
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -3496,6 +3496,88 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkIfSeedExistsInExplain(df2)
}
+ test("SPARK-31670: Struct Field in groupByExpr with CUBE") {
+ withTable("t") {
+ sql(
+ """CREATE TABLE t(
+ |a STRING,
+ |b INT,
+ |c ARRAY<STRUCT<row_id:INT,json_string:STRING>>,
+ |d ARRAY<ARRAY<STRING>>,
+ |e ARRAY<MAP<STRING, INT>>)
+ |USING ORC""".stripMargin)
+
+ checkAnswer(
+ sql(
+ """
+ |SELECT a, each.json_string, SUM(b)
+ |FROM t
+ |LATERAL VIEW EXPLODE(c) x AS each
+ |GROUP BY a, each.json_string
+ |WITH CUBE
+ |""".stripMargin), Nil)
+
+ checkAnswer(
+ sql(
+ """
+ |SELECT a, get_json_object(each.json_string, '$.i'), SUM(b)
+ |FROM t
+ |LATERAL VIEW EXPLODE(c) X AS each
+ |GROUP BY a, get_json_object(each.json_string, '$.i')
+ |WITH CUBE
+ |""".stripMargin), Nil)
+
+ checkAnswer(
+ sql(
+ """
+ |SELECT a, each.json_string AS json_string, SUM(b)
+ |FROM t
+ |LATERAL VIEW EXPLODE(c) x AS each
Review comment:
> btw, we must need `lateral view` to reproduce this issue? I mean, this issue cannot happen without `lateral view`?
No, I changed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][WIP]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-626377586
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480121375
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,42 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case a: Aggregate =>
+ val resolvedGroupingExprs =
+ a.groupingExpressions.map(resolveExpressionTopDown(_, a))
+ .map(CleanupAliases.trimStructFieldAlias)
+
+ val resolvedAggExprs = a.aggregateExpressions
+ .map(resolveExpressionTopDown(_, a))
+ .map {
+ // Here we want to trim StructField Alias in complex expression
+ // Only Alias(GetStructField, name) can't be trimmed since we need NamedExpression.
+ case alias@Alias(_: GetStructField, _) => alias
+ case e => CleanupAliases.trimStructFieldAlias(e).asInstanceOf[NamedExpression]
+ }
+
+ a.copy(resolvedGroupingExprs, resolvedAggExprs, a.child)
+
+ case g: GroupingSets =>
+ val resolvedSelectedExprs = g.selectedGroupByExprs
+ .map(_.map(resolveExpressionTopDown(_, g))
+ .map(CleanupAliases.trimStructFieldAlias))
+
+ val resolvedGroupingExprs = g.groupByExprs
+ .map(resolveExpressionTopDown(_, g))
+ .map(CleanupAliases.trimStructFieldAlias)
+
+ val resolvedAggExprs = g.aggregations
+ .map(resolveExpressionTopDown(_, g))
+ .map {
+ // Here we want to trim StructField Alias in complex expression
+ // Only Alias(GetStructField, name) can't be trimmed since we need NamedExpression.
+ case alias@Alias(_: GetStructField, _) => alias
+ case e => CleanupAliases.trimStructFieldAlias(e).asInstanceOf[NamedExpression]
Review comment:
ditto
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683398038
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r481651662
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1525,6 +1573,17 @@ class Analyzer(
AttributeSet(projectList.collect { case a: Alias => a.toAttribute })
}
+ // This method is used to trim groupByExpressions/selectedGroupByExpressions's top-level
+ // GetStructField Alias. Since these expression are not NamedExpression originally,
+ // we are safely to trim top-level GetStructField Alias.
Review comment:
Done
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1525,6 +1573,17 @@ class Analyzer(
AttributeSet(projectList.collect { case a: Alias => a.toAttribute })
}
+ // This method is used to trim groupByExpressions/selectedGroupByExpressions's top-level
+ // GetStructField Alias. Since these expression are not NamedExpression originally,
+ // we are safely to trim top-level GetStructField Alias.
+ def trimTopLevelGetStructFieldAlias(e: Expression): Expression = {
+ e match {
+ // trim Alias over top-level GetStructField
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685292014
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480855693
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,24 +1325,43 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e the expression need to be resolved.
+ * @param q the LogicalPlan whose children are used to resolve expression's attribute.
+ * @param trimAlias whether need to trim alias of Struct field. When true, we will trim
+ * Struct field alias. When isTopLevel = true, we won't trim top-level
+ * Struct field alias.
+ * @param isTopLevel whether need to trim top-level alias of Struct field. this param is
Review comment:
how about hiding this parameter from the caller side
```
def resolveExpressionTopDown(e: Expression, q: LogicalPlan, trimAlias: Boolean = false) = {
if (e.resolved) return e
def innerResolve(expr: Expression, isTopLevel: Boolean) = expr match {
case ...
...
}
innerResolve(e, isTopLevel = true)
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684091331
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640249398
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684398610
**[Test build #128138 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128138/testReport)** for PR 28490 at commit [`f17dd53`](https://github.com/apache/spark/commit/f17dd53e96d5e56366d6516a813f1581fb4f151e).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684398610
**[Test build #128138 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128138/testReport)** for PR 28490 at commit [`f17dd53`](https://github.com/apache/spark/commit/f17dd53e96d5e56366d6516a813f1581fb4f151e).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683615560
**[Test build #128082 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128082/testReport)** for PR 28490 at commit [`53fd03a`](https://github.com/apache/spark/commit/53fd03a8bb1d4c72badd555e08b935576fc0dbf4).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684390648
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684404933
don't forget https://github.com/apache/spark/pull/28490/files#r480799758
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684874621
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r434952757
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -506,31 +506,55 @@ class Analyzer(
aggregations: Seq[NamedExpression],
groupByAliases: Seq[Alias],
groupingAttrs: Seq[Expression],
- gid: Attribute): Seq[NamedExpression] = aggregations.map {
- // collect all the found AggregateExpression, so we can check an expression is part of
- // any AggregateExpression or not.
- val aggsBuffer = ArrayBuffer[Expression]()
- // Returns whether the expression belongs to any expressions in `aggsBuffer` or not.
- def isPartOfAggregation(e: Expression): Boolean = {
- aggsBuffer.exists(a => a.find(_ eq e).isDefined)
+ gid: Attribute): Seq[NamedExpression] = {
+ val resolvedGroupByAliases = groupByAliases.map(_.transformDown {
Review comment:
Based on above analysis, seems it is caused by `ResolveReferences`, instead of `ResolveGroupingAnalytics`? Can we possibly have a reproducible case without CUBE?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480200056
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1428,8 +1428,36 @@ class Analyzer(
// SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
// `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
// names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
+ case appendColumns: AppendColumns => appendColumns
+ case _ => a
+ }
+
+ val resolvedGroupingExprs =
Review comment:
nit:
```
val resolvedGroupingExprs = a.groupingExpressions
.map...
.map...
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683856277
**[Test build #128099 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128099/testReport)** for PR 28490 at commit [`cf31ab4`](https://github.com/apache/spark/commit/cf31ab4fb8e649ca860c6be28142da946eccc4b9).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-671608286
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480265324
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1425,11 +1436,33 @@ class Analyzer(
// rule: ResolveDeserializer.
case plan if containsDeserializer(plan.expressions) => plan
- // SPARK-25942: Resolves aggregate expressions with `AppendColumns`'s children, instead of
- // `AppendColumns`, because `AppendColumns`'s serializer might produce conflict attribute
- // names leading to ambiguous references exception.
- case a @ Aggregate(groupingExprs, aggExprs, appendColumns: AppendColumns) =>
- a.mapExpressions(resolveExpressionTopDown(_, appendColumns))
+ case a: Aggregate =>
+ val planForResolve = a.child match {
+ case appendColumns: AppendColumns => appendColumns
+ case _ => a
+ }
+
+ val resolvedGroupingExprs = a.groupingExpressions
+ .map(resolveExpressionTopDown(_, planForResolve, true, false))
+
+ val resolvedAggExprs = a.aggregateExpressions
+ .map(resolveExpressionTopDown(_, planForResolve, true, true))
+ .map(_.asInstanceOf[NamedExpression])
Review comment:
I'd expect something simpler
```
// If `trimAlias` is true, trim Aliases over non-top-level GetStructField.
def resolveExpressionTopDown(..., trimAlias: Boolean = false, isTopLevel: Boolean = true) = {
...
result match {
case Alias(s: GetStructField, _) if trimAlias && !isTopLevel => s
case _ => result
}
...
case _ => e.mapChildren(resolveExpressionTopDown(_, q, trimAlias, isTopLevel = false))
}
```
and here
```
val resolvedGroupingExprs = a.groupingExpressions
.map(resolveExpressionTopDown(_, planForResolve, trimAlias = true))
.map {
// trim Alias over top-level GetStructField
case Alias(s: GetStructField, _) => s
case other => other
}
val resolvedAggExprs = a.aggregateExpressions
.map(resolveExpressionTopDown(_, planForResolve, trimAlias = true))
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r434971770
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -3495,6 +3495,59 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
assert(df4.schema.head.name === "randn(1)")
checkIfSeedExistsInExplain(df2)
}
+
+ test("SPARK-31670: Struct Field in groupByExpr with CUBE") {
+ withTable("t1") {
+ sql(
+ """create table t1(
+ |a string,
+ |b int,
+ |c array<struct<row_id:int,json_string:string>>,
+ |d array<array<string>>,
+ |e array<map<string, int>>)
+ |using orc""".stripMargin)
+
+ checkAnswer(
+ sql(
+ """
+ |select a, each.json_string, sum(b)
+ |from t1
+ |LATERAL VIEW explode(c) x AS each
+ |group by a, each.json_string
+ |with cube
+ |""".stripMargin), Nil)
+
+ checkAnswer(
+ sql(
+ """
+ |select a, get_json_object(each.json_string, '$.i'), sum(b)
+ |from t1
+ |LATERAL VIEW explode(c) x AS each
+ |group by a, get_json_object(each.json_string, '$.i')
+ |with cube
+ |""".stripMargin), Nil)
+
+ checkAnswer(
+ sql(
+ """
+ |select a, each.json_string as json_string, sum(b)
+ |from t1
+ |LATERAL VIEW explode(c) x AS each
+ |group by a, each.json_string
+ |with cube
+ |""".stripMargin), Nil)
+
+ checkAnswer(
+ sql(
+ """
+ |select a, each.json_string as js, sum(b)
+ |from t1
+ |LATERAL VIEW explode(c) x AS each
+ |group by a, each.json_string
+ |with cube
Review comment:
> Could you check the other analytics grouping, too, e.g., GROUPING SETS and ROLLUP?
emmm grouping sets still have problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640168489
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-638554174
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-671607389
**[Test build #127288 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/127288/testReport)** for PR 28490 at commit [`0af3166`](https://github.com/apache/spark/commit/0af316675f376472d6deab40c82401a55a765e20).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r436322246
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -510,10 +510,12 @@ class Analyzer(
// collect all the found AggregateExpression, so we can check an expression is part of
// any AggregateExpression or not.
val aggsBuffer = ArrayBuffer[Expression]()
+
Review comment:
> nit: plz revert the unencessary changes. (Unrelated changes might lead to revert/backport failures sometimes...)
Sorry..., forgot to check diff.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640104812
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r436325004
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -3496,6 +3496,88 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkIfSeedExistsInExplain(df2)
}
+ test("SPARK-31670: Struct Field in groupByExpr with CUBE") {
Review comment:
Yea, looks fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685696518
**[Test build #128195 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128195/testReport)** for PR 28490 at commit [`e6fb91f`](https://github.com/apache/spark/commit/e6fb91fc8124c60e27899846b675853c22537791).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r481476702
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,25 +1325,45 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e The expression need to be resolved.
+ * @param q The LogicalPlan whose children are used to resolve expression's attribute.
+ * @param trimAlias When true, trim unnecessary alias of `GetStructField`. Note that,
+ * we cannot trim the alias of top-level `GetStructField`, as we should
+ * resolve `UnresolvedAttribute` to a named expression. The caller side
+ * can trim the alias of top-level `GetStructField` if it's safe to do so.
+ * @return resolved Expression.
*/
- private def resolveExpressionTopDown(e: Expression, q: LogicalPlan): Expression = {
- if (e.resolved) return e
- e match {
- case f: LambdaFunction if !f.bound => f
- case u @ UnresolvedAttribute(nameParts) =>
- // Leave unchanged if resolution fails. Hopefully will be resolved next round.
- val result =
- withPosition(u) {
- q.resolveChildren(nameParts, resolver)
- .orElse(resolveLiteralFunction(nameParts, u, q))
- .getOrElse(u)
+ private def resolveExpressionTopDown(
+ e: Expression,
+ q: LogicalPlan,
+ trimAlias: Boolean = false): Expression = {
+
+ def innerResolve(e: Expression, isTopLevel: Boolean): Expression = {
+ if (e.resolved) return e
+ e match {
+ case f: LambdaFunction if !f.bound => f
+ case u @ UnresolvedAttribute(nameParts) =>
+ // Leave unchanged if resolution fails. Hopefully will be resolved next round.
+ val resolved =
+ withPosition(u) {
+ q.resolveChildren(nameParts, resolver)
+ .orElse(resolveLiteralFunction(nameParts, u, q))
+ .getOrElse(u)
+ }
+ val result = resolved match {
+ case Alias(s: GetStructField, _) if trimAlias && !isTopLevel => s
+ case others => others
}
Review comment:
> Actually I'm wondering if there is any cases we don't want to trim nested (i.e., non top-level) `Alias`? Such `Alias` is useless and could possibly cause unexpected issue.
Since we will call CleanupAlias later in Analyzer, any way this field will be trimmed, but if we don't handle it here, we can't pass `ResolveGroupingAnalytics.constructAggregateExprs()`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r436322434
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,11 +1486,70 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case p: LogicalPlan if needResolveStructField(p) =>
+ logTrace(s"Attempting to resolve ${p.simpleString(SQLConf.get.maxToStringFields)}")
+ val resolved = p.mapExpressions(resolveExpressionTopDown(_, p))
+ val structFieldMap = new mutable.HashMap[String, Alias]
+ resolved.transformExpressions {
+ case a @ Alias(struct: GetStructField, _) =>
+ if (structFieldMap.contains(struct.sql)) {
+ val exprId = structFieldMap.getOrElse(struct.sql, a).exprId
+ Alias(a.child, a.name)(exprId, a.qualifier, a.explicitMetadata)
+ } else {
+ structFieldMap.put(struct.sql, a)
+ a
+ }
+ case e => e
+ }
+
case q: LogicalPlan =>
logTrace(s"Attempting to resolve ${q.simpleString(SQLConf.get.maxToStringFields)}")
q.mapExpressions(resolveExpressionTopDown(_, q))
}
+ def needResolveStructField(plan: LogicalPlan): Boolean = {
+ plan match {
+ case UnresolvedHaving(havingCondition, a: Aggregate)
+ if containSameStructFields(a.groupingExpressions.flatMap(_.references),
+ a.aggregateExpressions.flatMap(_.references),
+ Some(havingCondition.references.toSeq)) => true
+ case Aggregate(groupingExpressions, aggregateExpressions, _)
+ if containSameStructFields(groupingExpressions.flatMap(_.references),
+ aggregateExpressions.flatMap(_.references)) => true
+ case GroupingSets(selectedGroupByExprs, groupByExprs, _, aggregations)
+ if containSameStructFields(groupByExprs.flatMap(_.references),
+ aggregations.flatMap(_.references),
+ Some(selectedGroupByExprs.flatMap(_.flatMap(_.references)))) => true
+ case _ => false
+ }
+ }
+
+ def containSameStructFields(
Review comment:
> It is not enough just to check if both sides (`grpExprs` and `aggExprs`) have struct fields here? We need to confirm the identity by using unresolved attributes?
Yea, hear attribute is still unresolved attributes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r470522985
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,33 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case agg @ (_: Aggregate | _: GroupingSets) =>
+ val resolved = agg.mapExpressions(resolveExpressionTopDown(_, agg))
+ val hasStructField = resolved.expressions.exists {
+ _.collectFirst { case gsf: GetStructField => gsf }.isDefined
+ }
+ if (hasStructField) {
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause same struct field
+ // in aggExprs/groupExprs/selectedGroupByExprs will be resolved divided
+ // with different ExprId of Alias and replace failed when construct
+ // Aggregate in ResolveGroupingAnalytics, so we resolve duplicated struct
Review comment:
```
... will cause the same struct fields in aggExprs/groupExprs/selectedGroupByExprs
be treated as different ones due to different ExprIds in Alias, and stops us
finding the grouping expressions in aggExprs. Here we resolve ...
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][WIP]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-626339987
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683397866
**[Test build #128042 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128042/testReport)** for PR 28490 at commit [`7ecc8ad`](https://github.com/apache/spark/commit/7ecc8addfb9bd631d67c0fa3a83c89968b30839b).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-683616308
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][WIP]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-626377586
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684885938
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684499698
**[Test build #128140 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128140/testReport)** for PR 28490 at commit [`891fd1b`](https://github.com/apache/spark/commit/891fd1be604e9d1d2874e7c17cf60be09d1b6741).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685401930
**[Test build #128195 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128195/testReport)** for PR 28490 at commit [`e6fb91f`](https://github.com/apache/spark/commit/e6fb91fc8124c60e27899846b675853c22537791).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r481118979
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,25 +1325,54 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e The expression need to be resolved.
+ * @param q The LogicalPlan whose children are used to resolve expression's attribute.
+ * @param trimAlias When true, we will trim Struct field alias. When isTopLevel = true,
Review comment:
there is no `isTopLevel` parameter here.
```
When true, trim unnecessary alias of `GetStructField`. Note that, we cannot trim
the alias of top-level `GetStructField`, as we should resolve `UnresolvedAttribute` to
a named expression. The caller side can trim the alias of top-level `GetStructField` if
it's safe to do so.
```
Just let the caller side know that this method only trims alias of non-top-level `GetStructField`, and why.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r470660102
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,6 +1479,33 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case agg @ (_: Aggregate | _: GroupingSets) =>
+ val resolved = agg.mapExpressions(resolveExpressionTopDown(_, agg))
+ val hasStructField = resolved.expressions.exists {
+ _.collectFirst { case gsf: GetStructField => gsf }.isDefined
+ }
+ if (hasStructField) {
+ // For struct field, it will be resolve as Alias(GetStructField, name),
+ // In Aggregate/GroupingSets this behavior will cause same struct field
+ // in aggExprs/groupExprs/selectedGroupByExprs will be resolved divided
+ // with different ExprId of Alias and replace failed when construct
+ // Aggregate in ResolveGroupingAnalytics, so we resolve duplicated struct
+ // field here with same ExprId
+ val structFieldMap = mutable.Map[String, Alias]()
+ resolved.transformExpressionsDown {
Review comment:
> another concern: for `SELECT a.b, a.b FROM t GROUP BY a.b`, will we end up with an `Aggregate` operator whose output has duplicated ExprId? It's probably not an issue as the two output columns always have the same value.
struct.sql in UT show as `t`.`c`.`json_string`, [table].[column].[field], so seems won't have case that same `struct.sql` mean different column?
New way in https://github.com/apache/spark/pull/28490#discussion_r470555710 only handle current node to avoid same struct.sql show different column because of alias in different sql level.
emm, for two output `a.b`, it's hard to say which one is the one in cube. so in this case same output with duplicated exprId seems ok.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-640071894
**[Test build #123589 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/123589/testReport)** for PR 28490 at commit [`c4ff823`](https://github.com/apache/spark/commit/c4ff823171c80f157a1306713af71521168588ee).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685278034
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-685391625
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480838228
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1326,7 +1326,11 @@ class Analyzer(
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
*/
- private def resolveExpressionTopDown(e: Expression, q: LogicalPlan): Expression = {
+ private def resolveExpressionTopDown(
Review comment:
> let's add comments to explain what the new parameters do
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-684774848
**[Test build #128138 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128138/testReport)** for PR 28490 at commit [`f17dd53`](https://github.com/apache/spark/commit/f17dd53e96d5e56366d6516a813f1581fb4f151e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-671682512
Any update for this? cc @cloud-fan @viirya @HyukjinKwon
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r480851500
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1325,24 +1325,43 @@ class Analyzer(
*
* Note : In this routine, the unresolved attributes are resolved from the input plan's
* children attributes.
+ *
+ * @param e the expression need to be resolved.
+ * @param q the LogicalPlan use to resolve expression's attribute from.
Review comment:
> it's inaccurate. We resolve the given expression using `q.children`
changed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28490: [SPARK-31670][WIP]Struct Field in groupByExpr with CUBE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28490:
URL: https://github.com/apache/spark/pull/28490#issuecomment-626377134
**[Test build #122477 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122477/testReport)** for PR 28490 at commit [`27c495b`](https://github.com/apache/spark/commit/27c495b4da348e2d6bb461afec22d8088cf141c2).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #28490: [SPARK-31670][SQL] Trim unnecessary Struct field alias in Aggregate/GroupingSets
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #28490:
URL: https://github.com/apache/spark/pull/28490
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #28490: [SPARK-31670][SQL]Resolve Struct Field in Grouping Aggregate with same ExprId
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #28490:
URL: https://github.com/apache/spark/pull/28490#discussion_r436327006
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,11 +1486,70 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case p: LogicalPlan if needResolveStructField(p) =>
+ logTrace(s"Attempting to resolve ${p.simpleString(SQLConf.get.maxToStringFields)}")
+ val resolved = p.mapExpressions(resolveExpressionTopDown(_, p))
+ val structFieldMap = new mutable.HashMap[String, Alias]
+ resolved.transformExpressions {
+ case a @ Alias(struct: GetStructField, _) =>
+ if (structFieldMap.contains(struct.sql)) {
+ val exprId = structFieldMap.getOrElse(struct.sql, a).exprId
+ Alias(a.child, a.name)(exprId, a.qualifier, a.explicitMetadata)
+ } else {
+ structFieldMap.put(struct.sql, a)
+ a
+ }
+ case e => e
+ }
+
case q: LogicalPlan =>
logTrace(s"Attempting to resolve ${q.simpleString(SQLConf.get.maxToStringFields)}")
q.mapExpressions(resolveExpressionTopDown(_, q))
}
+ def needResolveStructField(plan: LogicalPlan): Boolean = {
+ plan match {
+ case UnresolvedHaving(havingCondition, a: Aggregate)
+ if containSameStructFields(a.groupingExpressions.flatMap(_.references),
+ a.aggregateExpressions.flatMap(_.references),
+ Some(havingCondition.references.toSeq)) => true
+ case Aggregate(groupingExpressions, aggregateExpressions, _)
+ if containSameStructFields(groupingExpressions.flatMap(_.references),
+ aggregateExpressions.flatMap(_.references)) => true
+ case GroupingSets(selectedGroupByExprs, groupByExprs, _, aggregations)
+ if containSameStructFields(groupByExprs.flatMap(_.references),
+ aggregations.flatMap(_.references),
+ Some(selectedGroupByExprs.flatMap(_.flatMap(_.references)))) => true
+ case _ => false
+ }
+ }
+
+ def containSameStructFields(
+ grpExprs: Seq[Attribute],
Review comment:
> nit: `grpExprs` -> `groupExprs` for consistency.
Done
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -1479,11 +1486,70 @@ class Analyzer(
// Skip the having clause here, this will be handled in ResolveAggregateFunctions.
case h: UnresolvedHaving => h
+ case p: LogicalPlan if needResolveStructField(p) =>
+ logTrace(s"Attempting to resolve ${p.simpleString(SQLConf.get.maxToStringFields)}")
+ val resolved = p.mapExpressions(resolveExpressionTopDown(_, p))
+ val structFieldMap = new mutable.HashMap[String, Alias]
Review comment:
> `mutable.HashMap` -> `mutable.Map`
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org