Add option to add limit to number of groups for khyperloglog_agg function

[feilong-liu]

Description

Add an option to limit the number of groups for the khyperloglog_agg. This limit is set in feature config, with property khyperloglog-agg-group-limit.

Motivation and Context

This is to solve the same problem as mentioned in #9553 for the khyperloglog_agg function.
In this aggregation function, it will create multiple HyperLogLog objects for the same group. It can lead to cross heap memory region reference, which increase the native memory usage as described in the issue.
In this PR, I add a limit on the number of groups this aggregation can have. This limit defaults to 0 which means no limit, i.e. the current behavior.

Impact

Have a way to limit native memory usage for this aggregation.

Test Plan

Existing unit tests
Also run verifier suites to make sure that the aggregation function still returns the same results

Contributor checklist

• Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
• PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
• Documented new properties (with its default value), SQL syntax, functions, or other functionality.
• If release notes are required, they follow the release notes guidelines.
• Adequate tests were added if applicable.
• CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Add an feature config property `khyperloglog-agg-group-limit` to set the maximum number of groups `khyperloglog_agg` can have. It will fail the query when the limit is exceeded. The default is 0 which means no limit.

[mlyublena]

LGTM

[agrawaldevesh]

I am curious how would we generalize this to cover say other expensive agg states down the line with minimal code ? Not something we ought to fix now, but what are your thoughts ? Should these checks go elsewhere then ?

[agrawaldevesh]

Can you explain more about these changes, is this some new style of writing agg functions ?

[feilong-liu]

This is an easier way to write aggregation function, corresponding to the ParametricAggregation class. The input, combine and output function together with the annotations can be used to generate the aggregation function. However, it's also not flexible. As we need to specify the limit on the number of groups, I choose to refactor the code to override functions directly. Both ways are widely used in our codebase.

[feilong-liu]

I am curious how would we generalize this to cover say other expensive agg states down the line with minimal code ? Not something we ought to fix now, but what are your thoughts ? Should these checks go elsewhere then ?

All aggregations are compiled by AccumulatorCompiler to generate bytecode, we may need to change here if want to add support for all functions in one place.

[tdcmeehan]

I am curious how would we generalize this to cover say other expensive agg states down the line with minimal code ? Not something we ought to fix now, but what are your thoughts ? Should these checks go elsewhere then ?

In the past, we've rewritten aggregation functions to "flatten" their internal state. For example, for array_agg, we put all the values in one giant array for all groups, and do some record keeping to track which sections of the array go to which group. This is expensive and not generalizable, though.

One potential option suggested by @ZacBlanco is to modify the code generation to specify a max grouping and make that configurable per-function, so that we don't need to do this type of code change for each aggregation function that we discover is expensive. It is unfortunate in this PR that we revert from using the aggregation function framework, as it makes the code harder to read.

[feilong-liu]

I am curious how would we generalize this to cover say other expensive agg states down the line with minimal code ? Not something we ought to fix now, but what are your thoughts ? Should these checks go elsewhere then ?

In the past, we've rewritten aggregation functions to "flatten" their internal state. For example, for array_agg, we put all the values in one giant array for all groups, and do some record keeping to track which sections of the array go to which group. This is expensive and not generalizable, though.

One potential option suggested by @ZacBlanco is to modify the code generation to specify a max grouping and make that configurable per-function, so that we don't need to do this type of code change for each aggregation function that we discover is expensive. It is unfortunate in this PR that we revert from using the aggregation function framework, as it makes the code harder to read.

The ideal case is to fix the algorithm if possible, like the arrray_agg, and apply limit if cannot avoid. Fortunately, this will not be a problem for Prestissimo. Change accumulator interface is a more general solution, but with more complex changes. Since this is the only aggregation we found expensive and cannot be simplified with algorithm change, I incline to keep the changes minimal and generalize it if we find more cases

[ajaygeorge]

LGTM