Document and enforce compatibility between an aggregate's accumulators

[masonh22]

Which issue does this PR close?

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

Rationale for this change

See #25660.

This change only documents/enforces this requirement for built-in aggregate UDF functions. Anyone who implements their own aggregate UDFs do not need to adhere to this requirement since vanilla datafusion doesn't require it. This does add a public test that user's can use to make sure this requirement is satisfied if they desire.

What changes are included in this PR?

This change adds:

• Internal documentation explaining this requirement and how to check for it using the provided test function.
• datafusion_functions_aggregate::testing::check_state_compatibility(): A generic test that identifies cases where the Accumulator and GroupsAccumulator of an AggregateUDF differ. This is exposed as a public function that can be used to test this requirement on AggregateUDFImpls implemented by downstream consumers. The test is run on all built-in default aggregation functions.
• Fixes for approx_distinct() and avg(), which both violated this requirement
  • This takes the changes from Allow the HLLAccumulator used by approx_distinct() to consume state produced by HllGroupsAccumulator #25659 to fix approx_distinct()
  • The Accumulator for avg() would return 0 for the count if it hasn't seen any non-null values. The GroupsAccumulator would return null instead. This change updates the Accumulators to also return null for the count in this case.

What is the testing strategy for this PR?

This adds a test to enforce this new requirement in a generic way.

Are there any user-facing changes?

This changes the intermediate state of the avg() accumulator to match the avg() groups accumulator, which some users may be affected by.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 76.19048% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.84%. Comparing base (8248a57) to head (acb05c9).
⚠️ Report is 101 commits behind head on main.

Files with missing lines	Patch %	Lines
...afusion/functions-aggregate/src/approx_distinct.rs	77.77%	7 Missing and 3 partials ⚠️
datafusion/functions-aggregate/src/average.rs	70.58%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #25710      +/-   ##
==========================================
+ Coverage   82.66%   82.84%   +0.17%     
==========================================
  Files        1147     1149       +2     
  Lines      446542   452813    +6271     
  Branches   446542   452813    +6271     
==========================================
+ Hits       369154   375150    +5996     
+ Misses      54986    54840     -146     
- Partials    22402    22823     +421     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[masonh22]

@alamb tagging you because you might be interested in the optimization this enables, given that github considers you a contributor to Datafusion Query Cache

[alamb]

Thank you @masonh22 and @thinkharderdev and @Dandandan

This looks like a nice improvement code and testing wise to me, though I am a little worried about the somewhat non standard testing feature (mostly that it add some tiny more amount of complexity to a system that is already pretty complicated).

I left some comments, but I also think this PR could be merged as is

Thank you for the cleanup 🙏

[alamb]

Thank you @masonh22

[masonh22]

@alamb it looks like CI failed in the hash collisions mode for the test I added because HyperLogLog doesn't do anything different when the hash collisions feature is enabled, but the groups accumulator for approx_distinct() does, so they end up generating different values.

I went ahead and updated HyperLogLog so that it produces hash collisions when the feature is enabled. The hash collision tests passed on my machine with this change.

Alternatively, I could disable the test I added when the hash collision feature is enabled. Let me know what you would prefer.

[alamb]

I went ahead and updated HyperLogLog so that it produces hash collisions when the feature is enabled. The hash collision tests passed on my machine with this change.

Sounds good to me -- thanks