Skip to content

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

Draft
pitrou wants to merge 1 commit into
apache:mainfrom
pitrou:gh48679-pivot-wider-bug
Draft

GH-48679: [C++] Fix pivot_wider with non-monotonic group ids#50423
pitrou wants to merge 1 commit into
apache:mainfrom
pitrou:gh48679-pivot-wider-bug

Conversation

@pitrou

@pitrou pitrou commented Jul 8, 2026

Copy link
Copy Markdown
Member

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

@pitrou pitrou changed the title GH-48679: [C++] Fix pivot_wider bug with non-monotonic group ids GH-48679: [C++] Fix pivot_wider with non-monotonic group ids Jul 8, 2026
@github-actions github-actions Bot added the awaiting review Awaiting review label Jul 8, 2026
@pitrou

pitrou commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

@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

Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review Awaiting review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants