[SPARK-13637][SQL] use more information to simplify the code in Expand builder #11485
Conversation
|
|
||
| (child.output :+ gid).map(expr => expr transformDown { | ||
| // TODO this causes a problem when a column is used both for grouping and aggregation. |
There was a problem hiding this comment.
Is this comment still valid?
There was a problem hiding this comment.
It isn't anymore. We had some trouble with such columns when I placed the comment.
There was a problem hiding this comment.
I believe this is for cube and rollup. Now, we covered all the cases.
There was a problem hiding this comment.
@gatorsmile are Cube and Rollup still broken? Or have they been fixed?
(edited)
There was a problem hiding this comment.
Just did a code history search. Sorry, this line was added by you. You know, cube and rollup also use Expand. Previously, cube and rollup had a couple of issues in grouping and aggregation. Now, all the issues have been fixed. Thanks!
There was a problem hiding this comment.
ah, so it's time to remove this TODO :)
|
@hvanhovell you might as well review this :) |
|
Test build #52370 has finished for PR 11485 at commit
|
|
retest this please |
|
Test build #52386 has finished for PR 11485 at commit
|
|
retest this please |
|
Test build #52394 has finished for PR 11485 at commit
|
| gid: Attribute, | ||
| child: LogicalPlan): Expand = { |
There was a problem hiding this comment.
Why not move the creation of the Project from the analyzer into this method? Then we can just take any logical plan as an argument.
|
Test build #52435 has finished for PR 11485 at commit
|
|
retest this please |
|
Test build #52666 has finished for PR 11485 at commit
|
|
LGTM |
|
Thanks for the review! Merging to master |
…d builder ## What changes were proposed in this pull request? The code in `Expand.apply` can be simplified by existing information: * the `groupByExprs` parameter are all `Attribute`s * the `child` parameter is a `Project` that append aliased group by expressions to its child's output ## How was this patch tested? by existing tests. Author: Wenchen Fan <wenchen@databricks.com> Closes apache#11485 from cloud-fan/expand.
What changes were proposed in this pull request?
The code in
Expand.applycan be simplified by existing information:groupByExprsparameter are allAttributeschildparameter is aProjectthat append aliased group by expressions to its child's outputHow was this patch tested?
by existing tests.