Skip to content

[SPARK-20710][SQL] Support aliases in CUBE/ROLLUP/GROUPING SETS#17948

Closed
maropu wants to merge 4 commits intoapache:masterfrom
maropu:SPARK-20710
Closed

[SPARK-20710][SQL] Support aliases in CUBE/ROLLUP/GROUPING SETS#17948
maropu wants to merge 4 commits intoapache:masterfrom
maropu:SPARK-20710

Conversation

@maropu
Copy link
Copy Markdown
Member

@maropu maropu commented May 11, 2017

What changes were proposed in this pull request?

This pr added Analyzer code for supporting aliases in CUBE/ROLLUP/GROUPING SETS (This is follow-up of #17191).

How was this patch tested?

Added tests in SQLQueryTestSuite.

@SparkQA
Copy link
Copy Markdown

SparkQA commented May 11, 2017

Test build #76796 has finished for PR 17948 at commit 08cef63.

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

@maropu
Copy link
Copy Markdown
Member Author

maropu commented May 12, 2017

@cloud-fan Could you check this? Thanks!

cloud-fan
cloud-fan reviewed May 12, 2017
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated
*/
object ResolveAggAliasInGroupBy extends Rule[LogicalPlan] {

// This is a strict check though, we put this to apply the rule only in alias expressions
Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan May 12, 2017

Choose a reason for hiding this comment

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

... only if the expression is not resolvable by child

Copy link
Copy Markdown
Member Author

@maropu maropu May 12, 2017

Choose a reason for hiding this comment

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

ok

@cloud-fan
Copy link
Copy Markdown
Contributor

cloud-fan commented May 12, 2017

LGTM

@SparkQA
Copy link
Copy Markdown

SparkQA commented May 12, 2017

Test build #76848 has started for PR 17948 at commit 0163656.

gatorsmile
gatorsmile reviewed May 12, 2017
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
// This is a strict check though, we put this to apply the rule only if the expression is not
// resolvable by child.
private def notResolvableByChild(attrName: String, child: LogicalPlan): Boolean =
!child.output.exists(a => resolver(a.name, attrName))
Copy link
Copy Markdown
Member

@gatorsmile gatorsmile May 12, 2017

Choose a reason for hiding this comment

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

Nit: style

    private def notResolvableByChild(attrName: String, child: LogicalPlan): Boolean = {
      !child.output.exists(a => resolver(a.name, attrName))
    }

Copy link
Copy Markdown
Member Author

@maropu maropu May 12, 2017

Choose a reason for hiding this comment

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

Thanks! Fixed.

@SparkQA
Copy link
Copy Markdown

SparkQA commented May 12, 2017

Test build #76849 has started for PR 17948 at commit a809274.

@gatorsmile
Copy link
Copy Markdown
Member

gatorsmile commented May 12, 2017

LGTM too. : )

viirya
viirya reviewed May 12, 2017
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated

case gs @ GroupingSets(selectedGroups, groups, child, aggs)
if conf.groupByAliases && child.resolved && aggs.forall(_.resolved) &&
(selectedGroups :+ groups).exists(_.exists(_.isInstanceOf[UnresolvedAttribute])) =>
Copy link
Copy Markdown
Member

@viirya viirya May 12, 2017

Choose a reason for hiding this comment

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

groups should cover selectedGroups. So we may not need to add selectedGroups here.

Copy link
Copy Markdown
Member Author

@maropu maropu May 12, 2017

Choose a reason for hiding this comment

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

Aha, I see. It looks reasonable. I'll update. Thanks!

Copy link
Copy Markdown
Member

@viirya viirya May 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Are we sure that grouping expressions are all pure attributes? If not, this check might fail.

Copy link
Copy Markdown
Member

@viirya viirya May 12, 2017

Choose a reason for hiding this comment

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

Oh. nvm. It is of course.

Copy link
Copy Markdown
Member Author

@maropu maropu May 12, 2017

Choose a reason for hiding this comment

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

yea, thanks!

viirya
viirya reviewed May 12, 2017
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated
case gs @ GroupingSets(selectedGroups, groups, child, aggs)
if conf.groupByAliases && child.resolved && aggs.forall(_.resolved) &&
(selectedGroups :+ groups).exists(_.exists(_.isInstanceOf[UnresolvedAttribute])) =>
def mayResolveAttrByAggregateExprs(exprs: Seq[Expression]): Seq[Expression] = exprs.map {
Copy link
Copy Markdown
Member

@viirya viirya May 12, 2017

Choose a reason for hiding this comment

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

I think we should do exprs.map { _.transform { ... like above.

@viirya
Copy link
Copy Markdown
Member

viirya commented May 12, 2017

LGTM

@SparkQA
Copy link
Copy Markdown

SparkQA commented May 12, 2017

Test build #76855 has finished for PR 17948 at commit ea12790.

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

asfgit pushed a commit that referenced this pull request May 12, 2017
@maropu @cloud-fan
[SPARK-20710][SQL] Support aliases in CUBE/ROLLUP/GROUPING SETS
eed3a5a 
## What changes were proposed in this pull request?
This pr added  `Analyzer` code for supporting aliases in CUBE/ROLLUP/GROUPING SETS (This is follow-up of #17191).

## How was this patch tested?
Added tests in `SQLQueryTestSuite`.

Author: Takeshi Yamamuro <yamamuro@apache.org>

Closes #17948 from maropu/SPARK-20710.

(cherry picked from commit 92ea7fd)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@cloud-fan
Copy link
Copy Markdown
Contributor

cloud-fan commented May 12, 2017

thanks, merging to master/2.2

@asfgit asfgit closed this in 92ea7fd May 12, 2017
robert3005 pushed a commit to palantir/spark that referenced this pull request May 19, 2017
@maropu
[SPARK-20710][SQL] Support aliases in CUBE/ROLLUP/GROUPING SETS
53c52d1 
## What changes were proposed in this pull request?
This pr added  `Analyzer` code for supporting aliases in CUBE/ROLLUP/GROUPING SETS (This is follow-up of apache#17191).

## How was this patch tested?
Added tests in `SQLQueryTestSuite`.

Author: Takeshi Yamamuro <yamamuro@apache.org>

Closes apache#17948 from maropu/SPARK-20710.
lycplus pushed a commit to lycplus/spark that referenced this pull request May 24, 2017
@maropu @lycplus
[SPARK-20710][SQL] Support aliases in CUBE/ROLLUP/GROUPING SETS
c03f0b3 
## What changes were proposed in this pull request?
This pr added  `Analyzer` code for supporting aliases in CUBE/ROLLUP/GROUPING SETS (This is follow-up of apache#17191).

## How was this patch tested?
Added tests in `SQLQueryTestSuite`.

Author: Takeshi Yamamuro <yamamuro@apache.org>

Closes apache#17948 from maropu/SPARK-20710.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@viirya viirya viirya left review comments

@cloud-fan cloud-fan cloud-fan left review comments

@gatorsmile gatorsmile gatorsmile left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maropu @SparkQA @cloud-fan @gatorsmile @viirya