2010YOUY01 commented on PR #25659: URL: https://github.com/apache/datafusion/pull/25659#issuecomment-5902730808
The issue is that the requirement for `GroupsAccumulator` to have the same physical representation as `Accumulator` is currently neither documented nor tested, the only tests should be the UTs added in this PR. For simpler aggregate functions like `avg()`, I believe they already happen to use the same representation. I understand that this is doable, and the PR looks quite clean now. What I’m still unclear about is why `Accumulator <---> GroupsAccumulator` compatibility is necessary in the first place. For example, would it be possible for your app to always use `GroupsAccumulator`? If there are use cases that require this stronger guarantee, I’d suggest: - Explain from the end usage side why is it necessary - documenting it explicitly as part of the `Accumulator` / `GroupsAccumulator` contract; - adding a test harness to make the invariant easy to verify for aggregate implementations. -- 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]
