[SPARK-19060][SQL] remove the supportsPartial flag in AggregateFunction #16461
Conversation
| @@ -436,7 +436,6 @@ abstract class AggregateWindowFunction extends DeclarativeAggregate with WindowF | |||
| override val frame = SpecifiedWindowFrame(RowFrame, UnboundedPreceding, CurrentRow) | |||
| override def dataType: DataType = IntegerType | |||
| override def nullable: Boolean = true | |||
| override def supportsPartial: Boolean = false | |||
There was a problem hiding this comment.
@hvanhovell is it allowed to use aggregate window functions in normal aggregate?
There was a problem hiding this comment.
No, it is not. These function should calculate a result for each value in a group for a single key. They also require that the group is ordered. Using these in a regular aggregate does not make much sense since they would degrade into a count, a distinct count (if the groups are ordered, random otherwise), or some constant (1 probably).
I think throwing an NotImplementedError is fine in this case, there is logic in Catalyst to prevent a user from using these function in a regular aggregate.
|
Shouldn't we also remove the actual flag? :) |
|
Test build #70821 has finished for PR 16461 at commit
|
|
Test build #70856 has finished for PR 16461 at commit
|
|
LGTM. Merging to master. Thanks! |
## What changes were proposed in this pull request? Now all aggregation functions support partial aggregate, we can remove the `supportsPartual` flag in `AggregateFunction` ## How was this patch tested? existing tests. Author: Wenchen Fan <wenchen@databricks.com> Closes apache#16461 from cloud-fan/partial.
## What changes were proposed in this pull request? Now all aggregation functions support partial aggregate, we can remove the `supportsPartual` flag in `AggregateFunction` ## How was this patch tested? existing tests. Author: Wenchen Fan <wenchen@databricks.com> Closes apache#16461 from cloud-fan/partial.
What changes were proposed in this pull request?
Now all aggregation functions support partial aggregate, we can remove the
supportsPartualflag inAggregateFunctionHow was this patch tested?
existing tests.