feat(functions-aggregate): make approx_distinct HLL precision configurable - #23816
feat(functions-aggregate): make approx_distinct HLL precision configurable#23816sandeshkr419 wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23816 +/- ##
==========================================
+ Coverage 80.67% 80.69% +0.01%
==========================================
Files 1095 1095
Lines 372460 372746 +286
Branches 372460 372746 +286
==========================================
+ Hits 300488 300769 +281
- Misses 54045 54046 +1
- Partials 17927 17931 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
5188279 to
834c2a9
Compare
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for the configurable HLL precision work. I think the overall direction looks good, but I found two correctness issues that should be addressed before merging. I also have one API suggestion that is not blocking.
| } | ||
| if bytes.len() == NUM_REGISTERS { | ||
| let num_registers = 1_usize << p; | ||
| if bytes.len() == num_registers { |
There was a problem hiding this comment.
I think there is a correctness issue with the grouped state encoding at lower precisions.
merge_serialized treats any state whose length is 1 << p as a dense sketch, but serialize writes sparse state as raw u64 hashes. For p <= 11, those encodings can have the same length. For example, at p = 4, two hashes serialize to 2 * 8 == 16 == 1 << p, and at p = 10, 128 hashes serialize to 1024 == 1 << p.
In those cases, the final aggregation interprets sparse hashes as register bytes, which produces incorrect cardinality estimates.
Could we make the encoding unambiguous by adding a dense/sparse tag, or alternatively force sparse state to be serialized as dense whenever its serialized length could match a dense sketch? It would also be good to add a round trip test for at least one p <= 11 case.
There was a problem hiding this comment.
Fixed now, serialize now promotes to dense whenever the sparse byte length would equal 1 << p, avoiding the ambiguity; added roundtrip regression tests at both collision points (p=4 with 2 hashes, p=10 with 128 hashes).
| Ok(ScalarValue::UInt64(Some(self.hll.count() as u64))) | ||
| } | ||
|
|
||
| fn size(&self) -> usize { |
There was a problem hiding this comment.
I think the memory accounting regressed when HyperLogLog changed from an inline [u8; NUM_REGISTERS] to a Vec<u8>.
size_of_val(self) now only includes the Vec header, so both HLLAccumulator::size here and NumericHLLAccumulator::size around line 280 no longer account for the register buffer itself. That under reports each sketch by 1 << p bytes, which can be as much as 256 KiB per accumulator at p = 18.
This could allow memory limited aggregations to exceed their configured limits. Could we include the register buffer in size(), perhaps via a HyperLogLog::size() helper, and add a regression test for the reported size?
There was a problem hiding this comment.
Added HyperLogLog::register_heap_size() and included it in both HLLAccumulator::size() and NumericHLLAccumulator::size(); HllGroupsAccumulator was already correct; added regression tests asserting size() >= 1 << p.
| /// This only has effect for types that use the HLL accumulator path. Small | ||
| /// integer and boolean types use exact bitmap counting regardless of this | ||
| /// value. Valid range: `HLL_P_MIN..=HLL_P_MAX` (4..=18). | ||
| pub fn with_hll_precision(p: usize) -> Self { |
There was a problem hiding this comment.
with_hll_precision currently panics for invalid input. Since this is a public Rust API, would it make sense to provide a fallible constructor instead? That would let callers return a normal DataFusion error instead of panicking. If the panic is intentional, it would be helpful to document that callers are expected to validate the precision first.
There was a problem hiding this comment.
Replaced the assert! with plan_err! so invalid precision surfaces as a clean DataFusion error rather than a panic.
…rable HyperLogLog precision was hardcoded at p=14 (16 384 registers, 16 KiB per sketch). Lower precision reduces memory and inter-shard state size at the cost of higher estimation error (p=12 → 4 KiB, ~1.6% error; p=10 → 1 KiB, ~3.3%). Changes: - `HyperLogLog::with_precision(p)` constructs a sketch at any p in 4..=18; `new()` keeps p=14 as the default (no behaviour change). - `HyperLogLog::from_registers(vec)` replaces `new_with_registers([u8; 16384])` and infers p from the slice length (must be a power of two). - `register_for_hash` and `count_from_hashes` take a `p` argument. - `ApproxDistinct::with_precision(p)` / `HllGroupsAccumulator::with_precision(p)` / `HLLAccumulator::with_precision(p)` / `NumericHLLAccumulator::with_precision(p)` propagate the chosen precision through every accumulator path. - Wire format is unchanged: serialized state is still raw register bytes; the length encodes p implicitly (len == 1 << p), so merge_serialized / TryFrom accept any valid precision automatically. - All 167 existing unit tests pass; 9 new tests cover precision construction, accuracy at p=10/12, roundtrip serialization, cross-precision merge panic, and boundary validation.
- make `try_with_hll_precision` fallible (plan_err instead of assert) - fix sparse/dense encoding collision for p<=11 in GroupHll::serialize - fix memory accounting regression: include register heap buffer in size()
50085b3 to
cb392bb
Compare
|
Thanks for the thorough review @kosiew! |
3 participants
Which issue does this PR close?
Resolves #23815
Rationale for this change
Adds runtime-configurable HLL precision to approx_distinct. Previously the register count was a compile-time constant (p=14, 16 KiB per sketch); now any precision between 4 to 18 is supported.
What changes are included in this PR?
Are these changes tested?
9 new tests cover: construction at non-default precisions, accuracy within expected error bounds at p=10 and p=12, roundtrip serialization at p=10/12/14, cross-precision merge panic (programming error guard), and boundary validation.
Are there any user-facing changes?
None for existing callers. Old ApproxDistinct::new() and HyperLogLog::new() continue to use p=14.