kosiew commented on code in PR #23816:
URL: https://github.com/apache/datafusion/pull/23816#discussion_r3664383259
##########
datafusion/functions-aggregate/src/approx_distinct.rs:
##########
@@ -336,13 +367,14 @@ impl GroupHll {
/// Merge a serialized state (produced by [`Self::serialize`] or by the
/// per-group [`Accumulator`]) into this sketch.
- fn merge_serialized(&mut self, bytes: &[u8]) -> Result<isize> {
+ fn merge_serialized(&mut self, bytes: &[u8], p: usize) -> Result<isize> {
if bytes.is_empty() {
return Ok(0);
}
- if bytes.len() == NUM_REGISTERS {
+ let num_registers = 1_usize << p;
+ if bytes.len() == num_registers {
Review 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.
##########
datafusion/functions-aggregate/src/approx_distinct.rs:
##########
Review 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?
##########
datafusion/functions-aggregate/src/approx_distinct.rs:
##########
@@ -654,12 +701,29 @@ impl Default for ApproxDistinct {
#[derive(PartialEq, Eq, Hash)]
pub struct ApproxDistinct {
signature: Signature,
+ /// HLL register precision. Only used for types that take the HLL code path
+ /// (i.e. not Boolean / small-int types, which use exact bitmap counting).
+ hll_precision: usize,
}
impl ApproxDistinct {
pub fn new() -> Self {
+ Self::with_hll_precision(DEFAULT_HLL_P)
+ }
+
+ /// Creates an `ApproxDistinct` that uses HLL sketches with `2^p`
registers.
+ ///
+ /// 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 {
Review 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.
--
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]
