Skip to content

Fix HLL union estimate/serialization regression from lazy KxQ rebuild - #512

Open
SavicStefan wants to merge 1 commit into
apache:masterfrom
SavicStefan:hll_bug_fix
Open

Fix HLL union estimate/serialization regression from lazy KxQ rebuild#512
SavicStefan wants to merge 1 commit into
apache:masterfrom
SavicStefan:hll_bug_fix

Conversation

@SavicStefan

Copy link
Copy Markdown
Contributor

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.

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ć <stefan.savic@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant