Skip to content

[SPARK-9830] [SQL] Remove AggregateExpression1 and Aggregate Operator used to evaluate AggregateExpression1s #9556

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

Jump to bottom
Closed
wants to merge 35 commits into from
Closed

[SPARK-9830] [SQL] Remove AggregateExpression1 and Aggregate Operator used to evaluate AggregateExpression1s #9556

wants to merge 35 commits into from

Conversation

@yhuai
Copy link
Contributor

@yhuai yhuai commented Nov 9, 2015

https://issues.apache.org/jira/browse/SPARK-9830

This PR contains the following main changes.

  • Removing AggregateExpression1.
  • Removing Aggregate operator, which is used to evaluate AggregateExpression1.
  • Removing planner rule used to plan Aggregate.
  • Linking MultipleDistinctRewriter to analyzer.
  • Renaming AggregateExpression2 to AggregateExpression and AggregateFunction2 to AggregateFunction.
  • Updating places where we create aggregate expression. The way to create aggregate expressions is AggregateExpression(aggregateFunction, mode, isDistinct).
  • Changing vals in DeclarativeAggregates that touch children of this function to lazy vals (when we create aggregate expression in DataFrame API, children of an aggregate function can be unresolved).

yhuai added 24 commits November 6, 2015 16:08
@yhuai
Remove agg1 from the planner.
96e3e36
@yhuai
Add needed constructors.
9921733
@yhuai
Delete old agg functions.
f126520
@yhuai
Update function registry and DataFrame functions.
5a177ec
@yhuai
Update analyzer.
f5a389f
@yhuai
Update optimizer.
3d06cfd
@yhuai
Update parsers, check analysis, and hive type coercion.
7e77628
@yhuai
Update catalyst dsl.
55841a5
@yhuai
Update HiveTypeCoercion
d2ca290
@yhuai
Update logical operators.
aa8daa9
@yhuai
Merge remote-tracking branch 'upstream/master' into removeAgg1
10630e1
@yhuai
Remove the mapping from old agg functions to new agg functions.
e8d39d1
@yhuai
Remove old agg operator.
a3df1fd
@yhuai
Throw exception when we have count distinct. Once #9409 is in, we wil…
5ed4970 
…l remove this limitation.
@yhuai
Update tests.
881afe8
@yhuai
Style.
2451233
@yhuai
Check analysis.
1ef0b92
@yhuai
Fix AggregateQuerySuite.
35dfe93
@yhuai
Merge remote-tracking branch 'upstream/master' into removeAgg1
6d29195 
Conflicts:
	sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Utils.scala
@yhuai
Fix test.
3835110
@yhuai
Since an agg function's children can be unresolved (when we use them …
c88c97d 
…in DF's API), making all fields that may use children lazy vals.
@yhuai
Fix tests.
c6c6c09
@yhuai
style
b73156a
@yhuai
Renaming:
cb3f4d3 
AggregateExpression2 => AggregateExpression
AggregateFunction2 => AggregateFunction
@rxin
Copy link
Contributor

rxin commented Nov 9, 2015

YAY.

@yhuai
Copy link
Contributor Author

yhuai commented Nov 9, 2015

test this please

@SparkQA
Copy link

SparkQA commented Nov 9, 2015

Test build #45342 has finished for PR 9556 at commit cb3f4d3.

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

@hvanhovell
Copy link
Contributor

hvanhovell commented Nov 9, 2015

retest this please

@SparkQA
Copy link

SparkQA commented Nov 9, 2015

Test build #45350 has finished for PR 9556 at commit cb3f4d3.

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

hvanhovell
hvanhovell reviewed Nov 9, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala Outdated
Copy link
Contributor

@hvanhovell hvanhovell Nov 9, 2015

Choose a reason for hiding this comment

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

The comment and the error message mention a new aggregation path. We should update this.

Copy link
Contributor Author

@yhuai yhuai Nov 9, 2015

Choose a reason for hiding this comment

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

will update this.

yhuai
yhuai reviewed Nov 10, 2015
View reviewed changes
R/pkg/R/functions.R
Copy link
Contributor Author

@yhuai yhuai Nov 10, 2015

Choose a reason for hiding this comment

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

@shivaram Seems this default value should be 0.05 instead of 0.95, right?

Copy link
Contributor

@shivaram shivaram Nov 10, 2015

Choose a reason for hiding this comment

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

Yeah looks like it should be 0.05 -- cc @davies
This seems to have been added back when SparkR was a separate code-base in davies/SparkR-pkg@d7b17a4

Copy link
Contributor

@davies davies Nov 10, 2015

Choose a reason for hiding this comment

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

Good catch, thanks!

@yhuai
Copy link
Contributor Author

yhuai commented Nov 10, 2015

test this please

@SparkQA
Copy link

SparkQA commented Nov 10, 2015

Test build #45474 has finished for PR 9556 at commit fb64896.

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

@SparkQA
Copy link

SparkQA commented Nov 10, 2015

Test build #2027 has finished for PR 9556 at commit fb64896.

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

@SparkQA
Copy link

SparkQA commented Nov 10, 2015

Test build #45476 has finished for PR 9556 at commit fb64896.

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

yhuai
yhuai reviewed Nov 10, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala
Copy link
Contributor Author

@yhuai yhuai Nov 10, 2015

Choose a reason for hiding this comment

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

@rxin @marmbrus It is better to change the prettyString of UnresolvedFunction to get rid of that ' (e.g. 'abs). The impact of this change is that if any user rely on that ', his/her code will break.

@SparkQA
Copy link

SparkQA commented Nov 10, 2015

Test build #45492 has finished for PR 9556 at commit 16826e6.

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

davies
davies reviewed Nov 10, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
Copy link
Contributor

@davies davies Nov 10, 2015

Choose a reason for hiding this comment

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

agg2 -> agg

Copy link
Contributor Author

@yhuai yhuai Nov 10, 2015

Choose a reason for hiding this comment

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

Done

@marmbrus
Copy link
Contributor

marmbrus commented Nov 10, 2015

We should keep reviewing and address any comments in a follow-up, but I'm going to merge this now to unblock other work. Thanks!

asfgit pushed a commit that referenced this pull request Nov 10, 2015
@yhuai @marmbrus
[SPARK-9830][SQL] Remove AggregateExpression1 and Aggregate Operator …
7c4ade0 
…used to evaluate AggregateExpression1s

https://issues.apache.org/jira/browse/SPARK-9830

This PR contains the following main changes.
* Removing `AggregateExpression1`.
* Removing `Aggregate` operator, which is used to evaluate `AggregateExpression1`.
* Removing planner rule used to plan `Aggregate`.
* Linking `MultipleDistinctRewriter` to analyzer.
* Renaming `AggregateExpression2` to `AggregateExpression` and `AggregateFunction2` to `AggregateFunction`.
* Updating places where we create aggregate expression. The way to create aggregate expressions is `AggregateExpression(aggregateFunction, mode, isDistinct)`.
* Changing `val`s in `DeclarativeAggregate`s that touch children of this function to `lazy val`s (when we create aggregate expression in DataFrame API, children of an aggregate function can be unresolved).

Author: Yin Huai <[email protected]>

Closes #9556 from yhuai/removeAgg1.

(cherry picked from commit e0701c7)
Signed-off-by: Michael Armbrust <[email protected]>
@asfgit asfgit closed this in e0701c7 Nov 10, 2015
@yhuai yhuai mentioned this pull request Nov 10, 2015
Closed
davies
davies reviewed Nov 10, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Sum.scala
Copy link
Contributor

@davies davies Nov 10, 2015

Choose a reason for hiding this comment

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

Should we remove the NullType here?

Copy link
Contributor Author

@yhuai yhuai Nov 10, 2015

Choose a reason for hiding this comment

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

yeah, we should remove it. I somehow missed it.

Copy link
Contributor Author

@yhuai yhuai Nov 10, 2015

Choose a reason for hiding this comment

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

Done

@yhuai yhuai mentioned this pull request Nov 10, 2015
Closed
asfgit pushed a commit that referenced this pull request Nov 11, 2015
@yhuai @rxin
[SPARK-9830][SPARK-11641][SQL][FOLLOW-UP] Remove AggregateExpression1…
ff7d869 
… and update toString of Exchange

https://issues.apache.org/jira/browse/SPARK-9830

This is the follow-up pr for #9556 to address davies' comments.

Author: Yin Huai <[email protected]>

Closes #9607 from yhuai/removeAgg1-followup.

(cherry picked from commit 3121e78)
Signed-off-by: Reynold Xin <[email protected]>
asfgit pushed a commit that referenced this pull request Nov 11, 2015
@yhuai @rxin
[SPARK-9830][SPARK-11641][SQL][FOLLOW-UP] Remove AggregateExpression1…
3121e78 
… and update toString of Exchange

https://issues.apache.org/jira/browse/SPARK-9830

This is the follow-up pr for #9556 to address davies' comments.

Author: Yin Huai <[email protected]>

Closes #9607 from yhuai/removeAgg1-followup.
kiszk pushed a commit to kiszk/spark-gpu that referenced this pull request Dec 26, 2015
@yhuai @kiszk
[SPARK-9830][SPARK-11641][SQL][FOLLOW-UP] Remove AggregateExpression1…
e8e02e4 
… and update toString of Exchange

https://issues.apache.org/jira/browse/SPARK-9830

This is the follow-up pr for apache/spark#9556 to address davies' comments.

Author: Yin Huai <[email protected]>

Closes #9607 from yhuai/removeAgg1-followup.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@yhuai @rxin @SparkQA @hvanhovell @marmbrus @davies @shivaram