From 46025e9aeed8368b1184cbde9634dd99d0ee47c0 Mon Sep 17 00:00:00 2001 From: SavicStefan <50296686+SavicStefan@users.noreply.github.com> Date: Thu, 13 Aug 2026 18:03:24 +0000 Subject: [PATCH 1/2] Fix HLL union estimate/serialization regression from lazy KxQ rebuild MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since the lazy KxQ/curMin rebuild was introduced ("skip updating kxq in HLL merge"), Hll8Array::mergeHll only sets rebuild_kxq_curmin_ and defers the recompute. Several consumers read that deferred state without honoring the flag, so unioning HLL sketches with different lgConfigK could: - discard accumulated data: isEmpty() reads curMin_/numAtCurMin_, which stay at empty-sketch defaults after a downsampling merge, so a populated gadget looks empty and the next union update overwrites it; - corrupt the estimate: internalCouponUpdate does incremental KxQ/HIP updates against the stale base; - serialize non-deterministically: copyAs() rebuilds via register replay (convertToHll8) while estimate/bounds rebuild via the direct sum, so equivalent merges could serialize to different bytes. The effect was estimates collapsing to ~one input's cardinality (merge-order dependent) and merge-order-dependent serialization; 4.0.1 had neither. Fix, keeping the lazy-merge optimization: - isEmpty(): a pending rebuild means the array is non-empty. - internalCouponUpdate(): rebuild before an incremental update reads KxQ, only when the coupon changes a register (duplicate coupons stay lazy). - copyAs(): make every same-type result use the direct-sum rebuild so all paths agree, keeping serialization merge-order independent. Adds a regression test covering estimate order-independence, is_empty() after a downsampling merge, scalar-after-merge accumulation, and merge-order-independent serialization. Co-authored-by: Isaac Co-authored-by: Stefan Savić --- hll/include/Hll8Array-internal.hpp | 4 ++ hll/include/HllArray-internal.hpp | 16 +++++-- hll/test/HllUnionTest.cpp | 69 ++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 4 deletions(-) diff --git a/hll/include/Hll8Array-internal.hpp b/hll/include/Hll8Array-internal.hpp index 2c5823a6..154788d3 100644 --- a/hll/include/Hll8Array-internal.hpp +++ b/hll/include/Hll8Array-internal.hpp @@ -98,6 +98,10 @@ void Hll8Array::internalCouponUpdate(uint32_t coupon) { const uint8_t curVal = this->hllByteArr_[slotNo]; if (newVal > curVal) { + // A prior HLL merge may have deferred rebuilding the estimator state. Rebuild only when + // this coupon changes a register, immediately before the incremental update needs KxQ and + // numAtCurMin_. Duplicate coupons therefore preserve the lazy-merge optimization. + if (this->rebuild_kxq_curmin_) this->check_rebuild_kxq_cur_min(); this->hllByteArr_[slotNo] = newVal; this->hipAndKxQIncrementalUpdate(curVal, newVal); this->numAtCurMin_ -= curVal == 0; // interpret numAtCurMin as num zeros diff --git a/hll/include/HllArray-internal.hpp b/hll/include/HllArray-internal.hpp index 62ea7f78..9db0df32 100644 --- a/hll/include/HllArray-internal.hpp +++ b/hll/include/HllArray-internal.hpp @@ -64,10 +64,14 @@ HllArray::HllArray(const HllArray& other, target_hll_type tgtHllType) : template HllArray* HllArray::copyAs(target_hll_type tgtHllType) const { - // we may need to recompute KxQ and curMin data for a union gadget, - // so only use a direct copy if we have a valid sketch - if (tgtHllType == this->getTgtHllType() && !this->isRebuildKxqCurminFlag()) { - return static_cast(copy()); + if (tgtHllType == this->getTgtHllType()) { + // Preserve lazy merging in the source, but make every same-type result use the direct-sum + // rebuild. Replaying registers through the conversion constructor produces slightly + // different floating-point KxQ values, making serialization depend on when the lazy state + // escaped through copyAs(). + HllArray* result = static_cast(copy()); + result->check_rebuild_kxq_cur_min(); + return result; } // the factory methods replay the coupons and will always rebuild @@ -465,6 +469,10 @@ bool HllArray::isCompact() const { template bool HllArray::isEmpty() const { + // mergeHll() is only called with non-empty sketches and sets this flag after updating the + // register array. The cached curMin_/numAtCurMin_ may still have their empty-sketch values, + // but a pending rebuild therefore proves that this array is not empty. + if (rebuild_kxq_curmin_) return false; const uint32_t configK = 1 << this->lgConfigK_; return (curMin_ == 0) && (numAtCurMin_ == configK); } diff --git a/hll/test/HllUnionTest.cpp b/hll/test/HllUnionTest.cpp index ceaef12f..6e0a81b4 100644 --- a/hll/test/HllUnionTest.cpp +++ b/hll/test/HllUnionTest.cpp @@ -314,4 +314,73 @@ TEST_CASE("hll union: check hll to hll", "[hll_union]") { union_two_sketches_with_overlap(1000000, 11, HLL_4); } +static hll_sketch::vector_bytes serialize_flat_union( + const hll_sketch& first, const hll_sketch& second, const hll_sketch& third) { + hll_union u(8); + u.update(first); + u.update(second); + u.update(third); + return u.get_result(HLL_8).serialize_updatable(); +} + +static hll_sketch::vector_bytes serialize_nested_union( + const hll_sketch& first, const hll_sketch& second, const hll_sketch& third) { + hll_union prefix(8); + prefix.update(first); + prefix.update(second); + + hll_union u(8); + u.update(prefix.get_result(HLL_8)); + u.update(third); + return u.get_result(HLL_8).serialize_updatable(); +} + +// Regression test for a lazy KxQ/curMin rebuild bug in HLL union. +// When an HLL-mode sketch with a larger lgConfigK is merged into a union first, the union +// downsamples it via mergeHll(), which defers the KxQ/curMin recompute (rebuild_kxq_curmin_). +// That left numAtCurMin_/curMin_ at their empty-sketch defaults, so is_empty() wrongly reported +// the populated gadget as empty and the next update discarded the accumulated result; a scalar +// update afterwards instead corrupted the HIP accumulator (read because the downsampled gadget +// keeps oooFlag_ == false). Both made the estimate collapse to roughly a single input's +// cardinality. Estimates and serialized results must be independent of merge order. +TEST_CASE("hll union: mixed lgConfigK merge order independence", "[hll_union]") { + const uint64_t n = 100000; // large enough to put all inputs in HLL mode at these lgConfigK + hll_sketch a(15); + hll_sketch b(8); + for (uint64_t i = 0; i < n; ++i) a.update(i); + for (uint64_t i = n; i < 2 * n; ++i) b.update(i); + const double truth = 2.0 * n; + + // A single downsampling merge must not report the populated gadget as empty. + hll_union u0(8); + u0.update(a); + REQUIRE_FALSE(u0.is_empty()); + + // Larger-lgConfigK sketch merged first (forces downsample-first), then the smaller one. + hll_union u1(8); + u1.update(a); + u1.update(b); + REQUIRE(u1.get_estimate() == Approx(truth).epsilon(0.1)); + + // Smaller-lgConfigK first must give the same answer. + hll_union u2(8); + u2.update(b); + u2.update(a); + REQUIRE(u2.get_estimate() == Approx(truth).epsilon(0.1)); + + // Scalar updates after a downsample-first merge must also accumulate correctly. + hll_union u3(8); + u3.update(a); + for (uint64_t i = n; i < 2 * n; ++i) u3.update(i); + REQUIRE(u3.get_estimate() == Approx(truth).epsilon(0.1)); + + // Extracting an intermediate lazy result must use the same KxQ rebuild as the flat union. + // Otherwise equivalent register arrays can serialize different floating-point KxQ values. + hll_sketch c(11); + for (uint64_t i = n / 2; i < n + n / 2; ++i) c.update(i); + REQUIRE(serialize_nested_union(b, c, a) == serialize_flat_union(b, c, a)); + REQUIRE(serialize_nested_union(a, c, b) == serialize_flat_union(a, c, b)); + REQUIRE(serialize_nested_union(a, b, c) == serialize_flat_union(a, b, c)); +} + } /* namespace datasketches */ From ed731c28cb83766df410558f5bd966a40cc3184a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Savi=C4=87?= Date: Fri, 14 Aug 2026 12:39:01 +0000 Subject: [PATCH 2/2] Trimed comments and refactor the tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Savić --- hll/include/Hll8Array-internal.hpp | 7 ++- hll/test/HllUnionTest.cpp | 83 +++++++++++++++--------------- 2 files changed, 45 insertions(+), 45 deletions(-) diff --git a/hll/include/Hll8Array-internal.hpp b/hll/include/Hll8Array-internal.hpp index 154788d3..14a2bbd7 100644 --- a/hll/include/Hll8Array-internal.hpp +++ b/hll/include/Hll8Array-internal.hpp @@ -98,10 +98,9 @@ void Hll8Array::internalCouponUpdate(uint32_t coupon) { const uint8_t curVal = this->hllByteArr_[slotNo]; if (newVal > curVal) { - // A prior HLL merge may have deferred rebuilding the estimator state. Rebuild only when - // this coupon changes a register, immediately before the incremental update needs KxQ and - // numAtCurMin_. Duplicate coupons therefore preserve the lazy-merge optimization. - if (this->rebuild_kxq_curmin_) this->check_rebuild_kxq_cur_min(); + // A prior HLL merge may have left the estimator state stale. Rebuild it before applying + // an incremental update for a changed register. + this->check_rebuild_kxq_cur_min(); this->hllByteArr_[slotNo] = newVal; this->hipAndKxQIncrementalUpdate(curVal, newVal); this->numAtCurMin_ -= curVal == 0; // interpret numAtCurMin as num zeros diff --git a/hll/test/HllUnionTest.cpp b/hll/test/HllUnionTest.cpp index 6e0a81b4..1d065d19 100644 --- a/hll/test/HllUnionTest.cpp +++ b/hll/test/HllUnionTest.cpp @@ -314,6 +314,12 @@ TEST_CASE("hll union: check hll to hll", "[hll_union]") { union_two_sketches_with_overlap(1000000, 11, HLL_4); } +static hll_sketch make_hll_sketch(uint8_t lg_config_k, uint64_t start, uint64_t end) { + hll_sketch sketch(lg_config_k); + for (uint64_t i = start; i < end; ++i) sketch.update(i); + return sketch; +} + static hll_sketch::vector_bytes serialize_flat_union( const hll_sketch& first, const hll_sketch& second, const hll_sketch& third) { hll_union u(8); @@ -335,49 +341,44 @@ static hll_sketch::vector_bytes serialize_nested_union( return u.get_result(HLL_8).serialize_updatable(); } -// Regression test for a lazy KxQ/curMin rebuild bug in HLL union. -// When an HLL-mode sketch with a larger lgConfigK is merged into a union first, the union -// downsamples it via mergeHll(), which defers the KxQ/curMin recompute (rebuild_kxq_curmin_). -// That left numAtCurMin_/curMin_ at their empty-sketch defaults, so is_empty() wrongly reported -// the populated gadget as empty and the next update discarded the accumulated result; a scalar -// update afterwards instead corrupted the HIP accumulator (read because the downsampled gadget -// keeps oooFlag_ == false). Both made the estimate collapse to roughly a single input's -// cardinality. Estimates and serialized results must be independent of merge order. -TEST_CASE("hll union: mixed lgConfigK merge order independence", "[hll_union]") { - const uint64_t n = 100000; // large enough to put all inputs in HLL mode at these lgConfigK - hll_sketch a(15); - hll_sketch b(8); - for (uint64_t i = 0; i < n; ++i) a.update(i); - for (uint64_t i = n; i < 2 * n; ++i) b.update(i); +TEST_CASE("hll union: downsampling merge is not empty", "[hll_union]") { + const hll_sketch sketch = make_hll_sketch(15, 0, 100000); + hll_union u(8); + u.update(sketch); + REQUIRE_FALSE(u.is_empty()); +} + +TEST_CASE("hll union: mixed lgConfigK estimate is merge-order independent", "[hll_union]") { + const uint64_t n = 100000; + const hll_sketch a = make_hll_sketch(15, 0, n); + const hll_sketch b = make_hll_sketch(8, n, 2 * n); const double truth = 2.0 * n; - // A single downsampling merge must not report the populated gadget as empty. - hll_union u0(8); - u0.update(a); - REQUIRE_FALSE(u0.is_empty()); - - // Larger-lgConfigK sketch merged first (forces downsample-first), then the smaller one. - hll_union u1(8); - u1.update(a); - u1.update(b); - REQUIRE(u1.get_estimate() == Approx(truth).epsilon(0.1)); - - // Smaller-lgConfigK first must give the same answer. - hll_union u2(8); - u2.update(b); - u2.update(a); - REQUIRE(u2.get_estimate() == Approx(truth).epsilon(0.1)); - - // Scalar updates after a downsample-first merge must also accumulate correctly. - hll_union u3(8); - u3.update(a); - for (uint64_t i = n; i < 2 * n; ++i) u3.update(i); - REQUIRE(u3.get_estimate() == Approx(truth).epsilon(0.1)); - - // Extracting an intermediate lazy result must use the same KxQ rebuild as the flat union. - // Otherwise equivalent register arrays can serialize different floating-point KxQ values. - hll_sketch c(11); - for (uint64_t i = n / 2; i < n + n / 2; ++i) c.update(i); + hll_union larger_first(8); + larger_first.update(a); + larger_first.update(b); + REQUIRE(larger_first.get_estimate() == Approx(truth).epsilon(0.1)); + + hll_union smaller_first(8); + smaller_first.update(b); + smaller_first.update(a); + REQUIRE(smaller_first.get_estimate() == Approx(truth).epsilon(0.1)); +} + +TEST_CASE("hll union: scalar update after downsampling merge", "[hll_union]") { + const uint64_t n = 100000; + const hll_sketch sketch = make_hll_sketch(15, 0, n); + hll_union u(8); + u.update(sketch); + for (uint64_t i = n; i < 2 * n; ++i) u.update(i); + REQUIRE(u.get_estimate() == Approx(2.0 * n).epsilon(0.1)); +} + +TEST_CASE("hll union: serialization is grouping independent", "[hll_union]") { + const uint64_t n = 100000; + const hll_sketch a = make_hll_sketch(15, 0, n); + const hll_sketch b = make_hll_sketch(8, n, 2 * n); + const hll_sketch c = make_hll_sketch(11, n / 2, n + n / 2); REQUIRE(serialize_nested_union(b, c, a) == serialize_flat_union(b, c, a)); REQUIRE(serialize_nested_union(a, c, b) == serialize_flat_union(a, c, b)); REQUIRE(serialize_nested_union(a, b, c) == serialize_flat_union(a, b, c));