-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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.
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
[SPARK-6356][SQL] Support the ROLLUP/CUBE/GROUPING SETS in SQLContext #5080
Conversation
ok to test |
Test build #28767 has finished for PR 5080 at commit
|
Test build #28778 has finished for PR 5080 at commit
|
Test build #28987 has finished for PR 5080 at commit
|
| rep1sep(expression, ",") | ||
) | ||
|
||
private def calGroupingSets(child: LogicalPlan, aggregations: Seq[NamedExpression], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each argument per line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
groupByExprs
should always be valid (not None
) currently, let's say groupByExprs: Seq[Expression]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because I want to compat syntax GROUP BY GROUPING SETS(expression list)
in #5045 before, and I'll change to groupByExprs: Seq[Expression]
in this patch, thanks for review the code.
LGTM in general, except some small issues. |
238a782
to
3ba3fc2
Compare
Test build #29046 has finished for PR 5080 at commit
|
My biggest comment is that we probably should avoid adding new keywords to the SQLParser. While this might be a nice feature to have, you can already do it through HiveQL and you are going to break existing code that uses column names |
I would find the ROLLUP feature useful for some of my workflows. If we're trying to keep the SQLParser forward compatible indefinitely, what are your plans for achieving SQL92 (and later) compatibility in Spark SQL? The work with dialects happening at #4015 ? |
@marmbrus I see |
@ash211, exactly. I think the best tactic is to create a new SQL92 parser with something more robust than Scala parser combinators. @watermen, do other dialects actually disallow those words as identifiers? I think part of the problem here is that scala parser combinators are very rigid and our keywords can never be used as identifiers even where it would not cause ambiguity. That said, if there is a lot of demand for this, we can add it. I just want everyone to be aware that there is a cost to compatibility for every addition (and users have complained in the past). |
Can one of the admins verify this patch? |
Support for the syntax(also supported by HiveContext) below:
#5045
/cc @yhuai