zhuqi-lucas commented on issue #24603: URL: https://github.com/apache/datafusion/issues/24603#issuecomment-5390376902
The prefix-only limitation of `EmitTo::First(n)` is real, so +1 on separating retention from emission. One question on the contract: `retain_groups` has to apply the *same* permutation to `GroupValues` and to every accumulator for the new dense IDs to line up, and it returns `Result`. What's the contract if accumulator `k` fails after `0..k-1` have already remapped? At that point `GroupValues` and the accumulators disagree on what a group ID means, and there's no way to roll back. Worth pinning down one way or the other — either document it as poisoning (caller must discard the whole aggregation state on `Err`), or make it two-phase so validation/allocation can fail before anything is mutated. Silent divergence across accumulators would be nasty to debug. -- 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]
