[SPARK-16200][SQL] Rename AggregateFunction#supportsPartial by maropu · Pull Request #13852 · apache/spark

maropu · 2016-06-22T18:03:03Z

What changes were proposed in this pull request?

Update a doc because it's stale.
This is a trivial fix, so I didn't create a JIRA ticket.
This fix was suggested in #13802 by @hvanhovell.

How was this patch tested?

N/A

maropu · 2016-06-22T18:08:02Z

+ * Indicates if this function needs to aggregate values group-by-group in a single step.
+ * If false, we must always use a `SortAggregateExec` operator without partial aggregates.
 */
 def supportsPartial: Boolean = true


We'd be also better to rename this variable instead of supportsPartial because it's kinds of misleading; forceSortAggregate, needsSequentialAggregate, ...

I do like the names, they describe however the inverse of what this flag does. We could invert the flag.

okay. Since the suggested fix changes the code, I'll create a new jira ticket and attach this pr with it.

maropu · 2016-06-22T18:08:25Z

@hvanhovell ping

SparkQA · 2016-06-22T18:14:21Z

Test build #61056 has finished for PR 13852 at commit a4d26b3.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2016-06-24T08:49:33Z

@hvanhovell ping

hvanhovell · 2016-06-24T17:37:55Z

 /**
- * Indicates if this function supports partial aggregation.
- * Currently Hive UDAF is the only one that doesn't support partial aggregation.
+ * Indicates if this function needs to aggregate values group-by-group in a single step.


This is also the inverse.

SparkQA · 2016-06-25T03:37:58Z

Test build #61215 has finished for PR 13852 at commit 00aac26.

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

maropu · 2016-06-30T18:01:10Z

@hvanhovell ping

rxin · 2016-07-11T01:36:42Z

are we overloading the semantics? I think it's actually useful to have a supportsPartial, which is what this was for.

maropu · 2016-07-11T01:38:18Z

you mean we need two funcs: supportPartial and forceSortAggregate?

rxin · 2016-07-11T01:40:02Z

Do we have two requirements here?

One is whether an aggregate function supports partial aggregation, and the other is whether the order should be enforced right ?

maropu · 2016-07-11T01:57:51Z

Yes. Currently, we have three functions with supportPartial=false: hive_udaf, collect, and window functions. The former two functions cannot support this because we do not have byte-backed mutable and growable ArrayData and MapData. I think, if we implement these data structures, we can set true at supportPartial. However, window functions need ordered evaluation. So, since the name is kinda misleading, I think.

hvanhovell · 2016-08-25T10:49:04Z

@maropu we are close to merging PR #14753. This will eliminate the need for a per-group (sorted) aggregation approach. On the longer run, it might also eliminate the need for a supportsPartial flag.

Shall we close this one?

maropu · 2016-08-25T10:57:37Z

okay, thanks!

yhuai · 2016-08-25T17:45:22Z

 override def inputTypes: Seq[AbstractDataType] = Seq(AnyDataType)

- override def supportsPartial: Boolean = false
+ override def forceSortAggregate: Boolean = true


oh, after changing this name, it will not show that we do not partial agg for this function.

yea. either way, it seems partial agg. becomes meaningless in future.

Update descriptions for supportPartial
a4d26b3

maropu reviewed Jun 22, 2016
View reviewed changes

hvanhovell reviewed Jun 24, 2016
View reviewed changes

Apply comments
00aac26

maropu force-pushed the FixDocInAggIf branch from e315db2 to 00aac26Compare June 25, 2016 01:46

maropu changed the title ~~[SQL][DOC] Update a description for AggregateFunction#supportsPartial~~[SPARK-16200][SQL] Rename AggregateFunction#supportsPartialJun 25, 2016

maropu mentioned this pull request Aug 25, 2016
[SPARK-12978][SQL] Skip unnecessary final group-by when input data already clustered with group-by keys #10896
Closed

maropu closed this Aug 25, 2016

yhuai reviewed Aug 25, 2016
View reviewed changes

maropu deleted the FixDocInAggIf branch July 5, 2017 11:47

Uh oh!

Conversation

maropu commented Jun 22, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

maropuJun 22, 2016

Choose a reason for hiding this comment

Uh oh!

hvanhovellJun 24, 2016

Choose a reason for hiding this comment

Uh oh!

maropuJun 24, 2016

Choose a reason for hiding this comment

Uh oh!

maropu commented Jun 22, 2016

Uh oh!

SparkQA commented Jun 22, 2016

Uh oh!

maropu commented Jun 24, 2016

Uh oh!

hvanhovellJun 24, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 25, 2016

Uh oh!

maropu commented Jun 30, 2016

Uh oh!

rxin commented Jul 11, 2016

Uh oh!

maropu commented Jul 11, 2016

Uh oh!

rxin commented Jul 11, 2016

Uh oh!

maropu commented Jul 11, 2016

Uh oh!

hvanhovell commented Aug 25, 2016

Uh oh!

maropu commented Aug 25, 2016

Uh oh!

yhuaiAug 25, 2016

Choose a reason for hiding this comment

Uh oh!

maropuAug 25, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maropu commented Jun 22, 2016•
edited
Loading