alamb commented on code in PR #15022:
URL: https://github.com/apache/datafusion/pull/15022#discussion_r2106160675
##########
datafusion/functions-aggregate-common/src/aggregate/groups_accumulator.rs:
##########
@@ -299,6 +424,17 @@ impl GroupsAccumulatorAdapter {
}
impl GroupsAccumulator for GroupsAccumulatorAdapter {
+ fn register_metadata(&mut self, metadata: GroupsAccumulatorMetadata) ->
Result<()> {
Review Comment:
I think I agree with @ctsk that this API is likely pretty easy to misuse
(people can forget to implement / not implement this correctly).
An alternate API would be to add a new parameter to `update_batch` .
I think the existing DataFusion APIs for passing a bunch of arguments to a
function involves a struct, for example
[ScalarFunctionArgs](https://docs.rs/datafusion/latest/datafusion/logical_expr/struct.ScalarFunctionArgs.html)
So instead of
```rust
fn update_batch(
&mut self,
values: &[ArrayRef],
group_indices: &[usize],
opt_filter: Option<&BooleanArray>,
total_num_groups: usize,
) -> Result<()> {
```
Perhaps something like
```rust
struct GroupsAccumulatorArgs<'a> {
values: &'a [ArrayRef],
group_indices: &'a [usize],
opt_filter: Option<&'a BooleanArray>,
total_num_groups: usize,
// new argument, for input_order_mode, describing how the input
groups
// are arranged. The accumulator could take advantage of this
information if desired
input_order_mode: InputOrderMode,
) -> Result<()> {
fn update_batch(
&mut self, args: GroupsAccumulatorArgs<'_>
) -> Result<()> {
```
--
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]
