Skip to content

Remove GroupsAccumulator::supports_convert_to_state and make convert_to_state mandatory #23081

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions