[SPARK-6466][SQL] Remove unnecessary attributes when resolving GroupingSets

[viirya]

When resolving GroupingSets, we currently list all outputs of GroupingSets's child plan. However, the columns that are not in groupBy expressions and not used by aggregation expressions are unnecessary and can be removed.

[SparkQA]

Test build #28988 has finished for PR 5134 at commit 8e16206.

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

[chenghao-intel]

Seems more reasonable to me if we do this in Optimizer, what do you think?

[viirya]

Good suggestion. I will do that later. Thanks.

[SparkQA]

Test build #29080 has finished for PR 5134 at commit a2734b3.

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

[viirya]

/cc @liancheng @marmbrus

[marmbrus]

Why do we need special handling here?

[viirya]

Because there are some constant null values and bitmasks we need to keep them.

[marmbrus]

This change should also be accompanied by test cases. There are several examples in the optimizer section of the tests.

[SparkQA]

Test build #31045 has started for PR 5134 at commit a2734b3.

[chenghao-intel]

I don't think it's the correct way for this optimization. Aggregate takes the output of Expand for expression evaluation, not the projections!
It probably need to refactor the GroupingSet a little bit before this optimization, I will submit the code soon.

Sorry @viirya , I will ping you once the refactoring finished.

[viirya]

@chenghao-intel Expand's output is modified too in this optimization.

[chenghao-intel]

I mean projections.map{ groupExpr.collect is not necessary, as output will be exactly match the projections.
Since we will pruning the output anyway, why don't just remove the associated columns from projections directly?

Specifying the output for a logical node is actually quit confusing, as catalyst will take it as referenced attribute also, that's why we need to refactor it.

[viirya]

@chenghao-intel I originally remove the unnecessary columns from projections in Analyzer as you said. However, you suggest that it is more reasonable to move it to Optimizer.

[chenghao-intel]

Sorry, I didn't make it clear, we definitely should do the column pruning in Optimizer. And also the Expand need to be refactor.

[viirya]

ok. I will update the unit test first.

[SparkQA]

Test build #31137 has finished for PR 5134 at commit d5dadec.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[viirya]

@chenghao-intel do you think we still need this? If no, I should close it.

[chenghao-intel]

@viirya The #5780 should fix that also, can you jump there for the code review? Sorry, I will update that PR asap.

[viirya]

@chenghao-intel OK. then I close this now.