sandeshkr419 commented on PR #23816:
URL: https://github.com/apache/datafusion/pull/23816#issuecomment-5259358458
Thanks @2010YOUY01 for the review and sharing benchmark data. I was able to
reproduce the regression on my end and find the issue and add new benhcmarks.
**On the regression:** Root cause was `changing registers from [u8; 16384]
to Vec<u8>` (double allocation per accumulator) and `p/q/p_mask` becoming
struct fields loaded on every add_hashed call instead of compile-time
constants.
Fixed in a follow-up commit: registers are back to an inline array (single
allocation as before), and a `hll_update!` macro uses compile-time constants
for the `p=14` path. Valid range is also tightened to
`HLL_P_MIN..=DEFAULT_HLL_P (4..=14)` since p>14 was outside the feature intent
of memory consumption and performance improvement.
Special handling of p=14 is to avoid performance hit on default case.
**On benchmarks**: Added two new groups: `approx_distinct_precision` and
`approx_distinct_grouped_precision`, running across p ∈ {8, 10, 12, 14} for
i64, utf8, and Decimal128. Confirms the expected ordering: p=12 is ~40% faster
than p=14, p=8 ~50% faster, primarily from count() scanning 4× and 64× fewer
registers respectively.
Please help review the changes and re-trigger benchmarks for another round
of validation!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
