Skip to content

Add integration regression test for terminal hash aggregate drain #24849

Description

@hhhizzz

Problem

#23182 fixed the performance regression in #23178 by avoiding repeated destructive EmitTo::First calls during terminal hash aggregate output.

The remaining MaterializedAggregateOutput unit test only verifies the slicing helper. It does not exercise AggregateExec, and it will not provide useful coverage when the current workaround is replaced by blocked/chunked state management.

This was a performance/complexity regression rather than a correctness bug. A result-only integration test would pass both the regressed and fixed implementations, while a wall-clock assertion would be unreliable in CI.

Proposed test

Add an integration-style regression test for the unordered Single/Final aggregate output path:

  • Run AggregateExec with more groups than batch_size.
  • Use a test GroupsAccumulator that fails if terminal drain falls back to destructive EmitTo::First.
  • Verify that output batches remain bounded and contain the correct results.
  • Keep the test independent of MaterializedAggregateOutput, so direct blocked emission can replace the current workaround.

This can follow the same pattern as the partial aggregate regression test added in #23250.

The actual timing and memory behavior should continue to be validated by the drain benchmark in #24795.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions