Skip to content

fix: density_sketch did not work with a custom allocator - #562

Open
rustyconover wants to merge 2 commits into
apache:masterfrom
rustyconover:fix/density-allocator
Open

rustyconover wants to merge 2 commits into
apache:masterfrom
rustyconover:fix/density-allocator

Conversation

@rustyconover

Copy link
Copy Markdown
Contributor

density_sketch<T, Kernel, Allocator> takes an allocator, but a sketch with anything other than std::allocator didn't work, for two reasons:

  1. The kernel only accepted std::vector<T>. gaussian_kernel::operator() took const std::vector<T>&, but the sketch stores its points as std::vector<T, Allocator>. Compaction and get_estimate() call the kernel on those points, so the sketch didn't compile:

    density_sketch_impl.hpp:120:41: error: no match for call to '(const datasketches::gaussian_kernel<double>) (const std::vector<double, ...test_allocator<double>>&, ...)'
    

    The kernel now takes any two vectors of T. Existing callers that pass std::vector<T> are unaffected.

  2. An unused member was always default-constructed. The class had an Allocator allocator_ member that neither constructor initialized. That fails for allocators that require an instance, such as test_allocator, whose default constructor throws for exactly this reason. Nothing reads the member, and get_allocator() already returns the levels' allocator, so this removes it.

Test

Added density sketch: custom allocator to density_sketch_test.cpp, using test_allocator. It covers:

  • 1,000 updates, which puts the sketch into estimation mode
  • get_estimate
  • merging a second sketch
  • a serialize/deserialize round trip with equal estimates
  • checking that every byte is released at the end

Without these changes it doesn't compile; with them, all 17 test suites pass under CTest (GCC 14, aarch64).

We found this in the DuckDB DataSketches extension. There, aggregate states keep their sketches in DuckDB's per-query arena through a custom allocator, so they count toward the memory limit and can spill.

🤖 Generated with Claude Code

rustyconover and others added 2 commits October 29, 2026 12:00
gaussian_kernel only accepted std::vector<T>, but the sketch's points are
std::vector<T, Allocator>, so a sketch with any other allocator did not
compile (get_estimate and compaction call the kernel on them). The kernel now
accepts any two vectors of T.

The sketch also had an unused Allocator member that neither constructor
initialized, so it was always default-constructed, which fails for
allocators that need an instance. It is removed; get_allocator() already
returns the levels' allocator.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rustyconover
rustyconover force-pushed the fix/density-allocator branch from e87d82f to dd37b4f Compare October 29, 2026 16:07

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