Skip to content

Document and enforce compatibility of the intermediate state produced by an AggregateUDFImpl's Accumulator and GroupsAccumulator #25660

Description

@masonh22

Is your feature request related to a problem or challenge?

At Coralogix, we have an optimization to share partial aggregations between queries where possible. Because of this, we will occasionally feed the state from an AggregateUDFImpl's groups accumulator into it's regular, non-grouped accumulator.

This is very useful when querying time-series data. As new data comes in, you can re-use the partial results from prior queries to avoid scanning huge amounts of data.

#22768 added an optimized groups accumulator for approx_distinct() that uses a different state format for groups with few distinct values. Trying to feed this state into the non-grouped accumulator gives an internal error: "Impossibly got invalid binary array from states".

Describe the solution you'd like

I think we should document, and enforce with tests, that the Accumulator and GroupsAccumulator for all AggregateUDFImpls are able to consume the state produced by their counterpart.

As far as I can tell, there should never be such a fundamental difference in the intermediate values produced by the two accumulators that would make it infeasible to make them compatible with each other. In the one case I'm aware of where there's an incompatibility, the fix is trivial and shouldn't affect performance (PR: #25659).

Adding tests to enforce this might be tricky. I think we could try to iterate over all the registered aggregate functions, try feeding inputs into their accumulators, and making sure that both variants produce the same results when consuming each other's intermediate values. Or, we can have the test force all aggregate functions to "register" valid inputs we can use to test their accumulators.

Describe alternatives you've considered

  1. We can fork/wrap UDFs that break this contract upstream, but that requires a lot of extra work on our end compared to the small amount of work to fix the cases where this contract is broken.

  2. We could also always try using the GroupsAccumulator instead of the Accumulator, but for approx_distinct(), this leads to a ~30% degradation in performance.

  3. Since I expect this to be very little work to enforce upstream, I can also submit patches whenever this invariant is broken upstream. However, I got pushback on the first such PR because this behavior isn't supported upstream: Allow the HLLAccumulator used by approx_distinct() to consume state produced by HllGroupsAccumulator #25659 (comment).

Additional context

The optimization we use is similar to the one implemented in Datafusion Query Cache. One limitation from that implementation that ours addresses is this:

Aggregation queries (no GROUP BY) with a dynamic lower bound - this is harder, we probably have to rewrite the aggregation to include a group_by clause, then filter, then aggregate again???

It is exactly this rewriting of the aggregation to include a group_by clause that leads to issues when the groups accumulator state is not compatible with the regular accumulator.

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

    enhancementNew feature or request

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions