Skip to content

fix: hll_union kept memory of input sketches' allocators - #556

Open
rustyconover wants to merge 1 commit into
apache:masterfrom
rustyconover:fix/hll-union-allocator
Open

rustyconover wants to merge 1 commit into
apache:masterfrom
rustyconover:fix/hll-union-allocator

Conversation

@rustyconover

Copy link
Copy Markdown

When an hll_union absorbs a sketch, it may copy that sketch into its gadget. The copy helpers keep the source's allocator:

  • copyAs(HLL_8), used for a LIST/SET sketch going into an empty gadget
  • copy_or_downsample(), used for HLL sketches; it also allocated the downsampled array with src->getAllocator()

In addition, update(hll_sketch&&) moves the input into the gadget outright.

With a stateful allocator, the union then holds memory owned by a different allocator instance than its own. That's a problem when the input's allocator is an arena or pool released before the union, and it breaks accounting that tracks memory per allocator.

This change:

  • copies, or takes over a moved sketch, only when the input's allocator compares equal to the union's
  • otherwise uses the paths the union already has for building into its own memory: inserting coupons for LIST/SET inputs, or merging into a new Hll8Array allocated with the union's allocator (the existing downsampling path)

std::allocator, and any allocator that always compares equal, behaves exactly as before.

Test

hll union: keeps no memory of an input sketch's allocator uses a small stateful allocator that counts bytes per instance. It covers:

  • LIST, SET and HLL inputs
  • lg_k below, equal to and above the union's
  • HLL_4, HLL_6 and HLL_8 inputs
  • empty and non-empty unions
  • const& and && updates

In every case, once the input is destroyed, none of its allocator's memory may be left, and the estimate must match a std::allocator union. Without the fix it fails at REQUIRE( pool_bytes[1] == 0 ); with it, all HLL tests pass (65 test cases).

We found this in the DuckDB DataSketches extension. We're moving aggregate states into DuckDB's per-query arena so they count toward its memory limit and can spill to disk, and HLL unions kept heap memory from deserialized input sketches.

🤖 Generated with Claude Code

union_impl copied input sketches with copyAs() and copy_or_downsample(), which
allocate with the source's allocator, and update(hll_sketch&&) took over the
input's memory outright. With a stateful allocator the union's gadget then
held memory owned by another allocator instance, which could be released or
reset before the union.

Copy only when the input uses the union's allocator; otherwise use the existing
paths that build into the union's own memory (inserting coupons, or merging into
a new HLL_8 array allocated with the union's allocator). Allocators that always
compare equal, such as std::allocator, keep the current behavior.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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