Skip to content

[SPARK-36275][SQL] ResolveAggregateFunctions should works with nested fields #33498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
allisonwang-db wants to merge 2 commits into from
Closed

[SPARK-36275][SQL] ResolveAggregateFunctions should works with nested fields #33498

allisonwang-db wants to merge 2 commits into from

Conversation

@allisonwang-db
Copy link
Contributor

@allisonwang-db allisonwang-db commented Jul 23, 2021

What changes were proposed in this pull request?

This PR fixes an issue in ResolveAggregateFunctions where non-aggregated nested fields in ORDER BY and HAVING are not resolved correctly. This is because nested fields are resolved as aliases that fail to be semantically equal to any grouping/aggregate expressions.

Why are the changes needed?

To fix an analyzer issue.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unit tests.

@github-actions github-actions bot added the SQL label Jul 23, 2021
@SparkQA
Copy link

SparkQA commented Jul 23, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46088/

@SparkQA
Copy link

SparkQA commented Jul 23, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46088/

@SparkQA
Copy link

SparkQA commented Jul 23, 2021

Test build #141569 has finished for PR 33498 at commit ded1a42.

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

@allisonwang-db
Copy link
Contributor Author

allisonwang-db commented Jul 26, 2021

cc @cloud-fan

cloud-fan
cloud-fan reviewed Jul 26, 2021
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated
Copy link
Contributor

@cloud-fan cloud-fan Jul 26, 2021

Choose a reason for hiding this comment

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

shall we update TempResolvedColumn to only take Attribute?

Copy link
Contributor Author

@allisonwang-db allisonwang-db Jul 26, 2021

Choose a reason for hiding this comment

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

Here actually we shouldn't transform attributes, because for nested fields this u.nameParts will not correspond to the attribute. Updated to specifically match Alias.

cloud-fan
cloud-fan approved these changes Jul 26, 2021
View reviewed changes
Copy link
Contributor

@cloud-fan cloud-fan left a comment

Choose a reason for hiding this comment

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

good catch!

@SparkQA
Copy link

SparkQA commented Jul 26, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46174/

@SparkQA
Copy link

SparkQA commented Jul 26, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46174/

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Test build #141658 has finished for PR 33498 at commit 1889fe4.

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

@cloud-fan cloud-fan mentioned this pull request Jul 27, 2021
Closed
@allisonwang-db allisonwang-db force-pushed the spark-36275-resolve-agg-func branch from 1889fe4 to fb3931c Compare July 27, 2021 18:14
@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46241/

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46241/

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Test build #141728 has finished for PR 33498 at commit fb3931c.

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

@cloud-fan
Copy link
Contributor

cloud-fan commented Jul 28, 2021

thanks, merging to master/3.2!

cloud-fan pushed a commit that referenced this pull request Jul 28, 2021
@allisonwang-db @cloud-fan
[SPARK-36275][SQL] ResolveAggregateFunctions should works with nested…
993ffaf 
… fields

### What changes were proposed in this pull request?
This PR fixes an issue in `ResolveAggregateFunctions` where non-aggregated nested fields in ORDER BY and HAVING are not resolved correctly. This is because nested fields are resolved as aliases that fail to be semantically equal to any grouping/aggregate expressions.

### Why are the changes needed?
To fix an analyzer issue.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Unit tests.

Closes #33498 from allisonwang-db/spark-36275-resolve-agg-func.

Authored-by: allisonwang-db <allison.wang@databricks.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
(cherry picked from commit 23a6ffa)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@cloud-fan cloud-fan closed this in 23a6ffa Jul 28, 2021
HyukjinKwon
HyukjinKwon reviewed Aug 13, 2021
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
// should undo it later and fail with "Column c2 not found".
agg.child.resolve(u.nameParts, resolver).map(TempResolvedColumn(_, u.nameParts))
.getOrElse(u)
agg.child.resolve(u.nameParts, resolver).map({
Copy link
Member

@HyukjinKwon HyukjinKwon Aug 13, 2021

Choose a reason for hiding this comment

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

Let's use a single brace next time for map in this case: https://github.com/databricks/scala-style-guide#anonymous-methods

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HyukjinKwon HyukjinKwon HyukjinKwon left review comments

@cloud-fan cloud-fan cloud-fan approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@allisonwang-db @SparkQA @cloud-fan @HyukjinKwon