avantgardnerio commented on PR #25659:
URL: https://github.com/apache/datafusion/pull/25659#issuecomment-5895222776
> this isn't a supported pattern in DataFusion
I'd push back on this a bit. This file treats the two accumulators' state as
interchangeable:
1. `GroupHll::merge_serialized` is [documented to accept state produced by
the per-group
`Accumulator`](https://github.com/apache/datafusion/blob/2cac713274496ba8ee08e58655e80f0d464f6d48/datafusion/functions-aggregate/src/approx_distinct.rs#L338-L339)
2. The dense sketch is [documented as wire-compatible with the per-group
`Accumulator`](https://github.com/apache/datafusion/blob/2cac713274496ba8ee08e58655e80f0d464f6d48/datafusion/functions-aggregate/src/approx_distinct.rs#L411-L412)
3. Both accumulators share a single state field, [declared as
`hll_registers`](https://github.com/apache/datafusion/blob/2cac713274496ba8ee08e58655e80f0d464f6d48/datafusion/functions-aggregate/src/approx_distinct.rs#L743).
Since #22768 (first released in 55.0.0), that column sometimes holds a list of
hashes instead of registers, and the upgrade guide doesn't mention it.
So I read this PR as restoring the other half of a symmetry the file already
documents, rather than adding a special case.
I agree the two `merge_serialized` implementations should be de-duplicated.
With that change, I don't think this adds any maintenance burden.
--
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]
