Skip to content

Remove GroupsAccumulator::supports_convert_to_state and make convert_to_state mandatory #23081

Closed

Description

@alamb

Is your feature request related to a problem or challenge?

Noticed while reviewing this PR from @2010YOUY01

GroupsAccumulator currently has two related APIs:

This optional capability adds branching and complexity in the group hash implementation (as the code has to handle the case when skip partial is not available)

It also means GroupsAccumulator implementations can silently miss the high-cardinality fast path.

I believe the original reason for this API was to allow incremental rollout of partial aggregate support (so we could introduce the API and then incrementally implement it for the accumulators)

Describe the solution you'd like

  1. Make convert_to_state(...) a required part of the GroupsAccumulator
  2. remove supports_convert_to_state()
  3. simplify hash aggregate so skip-partial aggregation no longer needs to check per-accumulator support.

Suggested Steps:

Acceptance criteria:

Describe alternatives you've considered

We could keep supports_convert_to_state() and continue enabling skip-partial aggregation only when all accumulators opt in. That preserves compatibility, but it keeps the hash aggregate code more complex and allows some accumulators to miss the high-cardinality fast path indefinitely.

Additional context

This is likely a breaking public API change for custom aggregate authors and FFI users, so it should be marked as an API change and documented in the upgrading guide.

Activity

  1. changed the title [-]Remove GroupsAccumulator::supports_convert_to_state and make convert_to_state mandatory[/-] [+]Remove `GroupsAccumulator::supports_convert_to_state` and make `convert_to_state` mandatory[/+] on Jun 22, 2026
  2. lyne7-sc commented on Jun 30, 2026

    @lyne7-sc
    Contributor

    take

  3. lyne7-sc commented on Jun 30, 2026

    @lyne7-sc
    Contributor

    After reviewing the codebase, there are indeed several GroupsAccumulator implementations missing convert_to_state(). So I plan to first complete the missing implementations, then remove supports_convert_to_state() and its call sites - to keep the pr small and easier to review.

  4. alamb commented on Jun 30, 2026

    @alamb
    ContributorAuthor

    Sounds like a great plan -- thank you @lyne7-sc

  5. added a commit that references this issue on Jul 24, 2026
    3f81613
  6. added a commit that references this issue on Aug 12, 2026
    6aa4759
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions