GH-48679: [C++] Fix pivot_wider with non-monotonic group ids#50423
GH-48679: [C++] Fix pivot_wider with non-monotonic group ids#50423pitrou wants to merge 1 commit into
Conversation
|
@zanmato1984 Do you think this is ok, or should FastGrouperImpl be fixed to always produce monotonic group ids? (by the way, I was looking for a way to force group ids to always be non-monotonic in debug mode, but that seems quite non-trivial) |
|
@pitrou Although the current Grouper API contract does not explicitly specify the ordering of GetUniques(), the existing tests strongly suggest that it is expected to preserve first-seen order. If we agree that this should be part of the contract, then this issue should probably be treated as a FastGrouperImpl bug, and the pivot_wider fix would not be strictly necessary. That said, keeping the explicit mapping here as a safeguard seems reasonable. I am fine with keeping this PR for now. I will keep looking into whether we should make the ordering guarantee explicit, how to fix FastGrouperImpl accordingly, and whether that would have a significant performance impact. If the performance impact turns out to be unacceptable, then we may need to reconsider whether order preservation should be promoted to an API contract. |
Rationale for this change
The pivot_wider implementation was assuming that the
Grouperproduces monotonic group ids for the pivot key names.However, that turns out to not always be true when the
FastGrouperImplis involved (perhaps when hash collisions are involved?).What changes are included in this PR?
Compute a apply a mapping from pivot key group ids back to pivot key indices.
Are these changes tested?
Yes, by additional tests.
Are there any user-facing changes?
Only a bugfix.
hash_pivot_wider#48679