masonh22 commented on PR #25659: URL: https://github.com/apache/datafusion/pull/25659#issuecomment-5911723145
> 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. I agree with you on this, and going forward this is something I want to do. I filed #25660 to do what you describe, and if you have feedback I would appreciate if you would share it there. I also have a prototype test harness for testing the invariant in #25710. The reason I made this separate PR is that I believed the fix here was small and wouldn't be too controversial. I thought that what I proposed in #25660 would be met with much more skepticism. -- 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]
