Repository navigation
fix: hll_union kept memory of input sketches' allocators - #556
Open
rustyconover wants to merge 1 commit into
Open
rustyconover wants to merge 1 commit into
rustyconover wants to merge 1 commit into
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When an
hll_unionabsorbs 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 gadgetcopy_or_downsample(), used for HLL sketches; it also allocated the downsampled array withsrc->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:
Hll8Arrayallocated 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 allocatoruses a small stateful allocator that counts bytes per instance. It covers:In every case, once the input is destroyed, none of its allocator's memory may be left, and the estimate must match a
std::allocatorunion. Without the fix it fails atREQUIRE( 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