rluvaton opened a new pull request, #15022:
URL: https://github.com/apache/datafusion/pull/15022
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases. You can
link an issue to this PR using the GitHub syntax. For example `Closes #123`
indicates that this PR will close issue #123.
-->
- Closes #14991.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in
the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your
changes and offer better suggestions for fixes.
-->
### why we need to know if the group indices are contiguous
See the linked issue.
### Why add `register_metadata` function
The options I see for how to pass that information to the group accumulators
are:
1. Adding new field for `AccumulatorArgs`
the problem here is that, that field is only relevant for
`GroupsAccumulator` and `AccumulatorArgs` is used for other Accumulators as
well.
and adding that field force us to propagate it to `AggregateFunctionExpr`
and possibly also `AggregateExprBuilder` which doesn't make sense.
this will also introduce a breaking change
2. Add new argument to `update_batch`/`merge_batch` in `GroupsAccumulator`
implementations
This give the assumption that the value can be changed between calls and
this will make the function have too many arguments.
this will also introduce a breaking change
3. this approach
Clean approach with minimal changes to make it work, only applicable to
`GroupsAccumulator`, does not introduce a breaking change.
the disadvantages are:
1. make the flow hard to follow
2. does not allow to create specialized `GroupsAccumulator` impl and
instead handle that option in the existing impl
## What changes are included in this PR?
1. Added `register_metadata` function for the `GroupsAccumulator` trait with
default implementation that does nothing to avoid breaking changes
2. Added `GroupsAccumulatorMetadata` with `contiguous_group_indices` field.
3. Pass `contiguous_group_indices` true when the `input_order_mode` is
sorted.
4. Call register metadata in the `create_group_accumulator` function
5. Added specialized implementation for `GroupsAccumulatorAdapter` when
invoking each accumulator in `invoke_per_accumulator` depending on the
`contiguous_group_indices` value
<!--
There is no need to duplicate the description in the issue here but it is
sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
For the specialized implementation for `GroupsAccumulatorAdapter` existing
tests cover it.
For if we call `register_metadata` or the value we call it with than no, I
don't see any "unit" test that initiate with sorted, will try to make it soon
## Are there any user-facing changes?
Yes.
1. New function for `GroupsAccumulator` trait (have default impl so no
breaking change)
2. New struct for `GroupsAccumulatorMetadata`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
