GH-48679: [C++] Fix pivot_wider with non-monotonic group ids

[pitrou]

Rationale for this change

The pivot_wider implementation was assuming that the Grouper produces monotonic group ids for the pivot key names.

However, that turns out to not always be true when the FastGrouperImpl is involved (perhaps when hash collisions are involved?).

What changes are included in this PR?

Compute and 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.

• GitHub Issue: [C++][Python] Incorrect results from hash_pivot_wider #48679

[pitrou]

@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)

[zanmato1984]

@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.

[pitrou]

@zanmato1984 Would you like to give this a review?

[zanmato1984]

LGTM, thanks @pitrou.

One note: this change removes what looks like the only production call site of Grouper::Populate(), replacing it with Consume() so we can build the key-index/group-id mapping. I don't think this is a problem: Populate() is still a reasonable public API ("consume without materializing ids") and may have future uses.

If/when we make Grouper's first-seen ordering a public contract, we can revisit this path and see whether pivot_wider can use Populate() again. Perhaps a TODO here would be useful, but I don't think this should block the PR.

[pitrou]

I'll merge now. Thanks for the review @zanmato1984

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 34647d2.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.