Skip to content

[SPARK-24424][SQL] Support ANSI-SQL compliant syntax for GROUPING SET #21813

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

Closed
dilipbiswal wants to merge 5 commits into from
Closed

[SPARK-24424][SQL] Support ANSI-SQL compliant syntax for GROUPING SET #21813

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b5ada3f
[SPARK-24424] Support ANSI-SQL compliant syntax for GROUPING SET
dilipbiswal Jul 19, 2018
ac8f04f
code review
dilipbiswal Jul 19, 2018
7cf187d
Remove redundant test
dilipbiswal Jul 19, 2018
e0c57f7
Add comment
dilipbiswal Jul 19, 2018
2ecf3e1
Add one more negative test
dilipbiswal Jul 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add comment
  • Loading branch information
@dilipbiswal
dilipbiswal committed Jul 19, 2018
commit e0c57f73ec4c3e24e4af107cb457c9b1c13f4174
3 changes: 3 additions & 0 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,9 @@ class Analyzer(
child: LogicalPlan): LogicalPlan = {
val gid = AttributeReference(VirtualColumn.groupingIdName, IntegerType, false)()

// In case of ANSI-SQL compliant syntax for GROUPING SETS, groupByExprs is optional and
// can be null. In such case, we derive the groupByExprs from the user supplied values for
// grouping sets.
val finalGroupByExpressions = if (groupByExprs == Nil) {
Copy link
Member

@viirya viirya Jul 19, 2018

Choose a reason for hiding this comment

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

Shouldn't we do this in the branch of case x: GroupingSets if x.expressions.forall(_.resolved) =>? I think this constructAggregate method is also used by other clauses like Cube and Rollup.

Copy link
Contributor Author

@dilipbiswal dilipbiswal Jul 19, 2018

Choose a reason for hiding this comment

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

@viirya Yeah.. so for cube and rollup, we will always have groupByExprs setup right ? So i felt its better to keep the code consolidated here in this function. What do u think ?

Copy link
Member

@viirya viirya Jul 19, 2018

Choose a reason for hiding this comment

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

Ok. Mind to add a comment on this like SPARK-24424: this only happens for ANSI-SQL compliant syntax for GROUPING SET?

Copy link
Contributor Author

@dilipbiswal dilipbiswal Jul 19, 2018

Choose a reason for hiding this comment

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

@viirya Sure will do.

selectedGroupByExprs.flatten.foldLeft(Seq.empty[Expression]) { (result, currentExpr) =>
Copy link
Member

@viirya viirya Jul 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

What if GROUP BY GROUPING SETS (())? Is it a valid query?

Copy link
Contributor Author

@dilipbiswal dilipbiswal Jul 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@viirya No. We should be getting an error as we don't have a group by specification. I had tried this scenario against db2 to double check.

Copy link
Member

@viirya viirya Jul 20, 2018

Choose a reason for hiding this comment

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

Can we have a test case for it too?

Copy link
Contributor Author

@dilipbiswal dilipbiswal Jul 20, 2018

Choose a reason for hiding this comment

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

@viirya Yeah.. was already adding it .. knew u would ask :-)

Copy link
Member

@viirya viirya Jul 20, 2018

Choose a reason for hiding this comment

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

@dilipbiswal Thanks! :-)

// Only unique expressions are included in the group by expressions and is determined
Expand Down