Add optional trim flag to update_theta_sketch::compact() - #524
Open
leerho wants to merge 1 commit into
Open
Conversation
Coverage Report for CI Build 34086187188Coverage increased (+0.01%) to 82.321%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
An update sketch retains more than the nominal size k between rebuilds.
Callers who need a result bounded by k currently have to copy the sketch,
call trim() on the copy, then compact() it, which mutates state and
allocates twice.
This adds an optional second flag to the existing compact():
compact_theta_sketch_alloc<Allocator> compact(bool ordered = true,
bool trim = false) const;
The trim path copies the retained entries (the method is const, and quick
select would permute the live table), partitions with std::nth_element at
0-based index k so that entries[k] becomes the new theta, keeps the k
entries below it, and sorts only when an ordered result is requested. This
matches the pivot convention already used by rebuild(), so trimming and
rebuilding cannot drift apart.
Trimming stays opt-in because it is lossy. Relative error scales with
1 / sqrt(retained), so discarding entries always widens the confidence
bounds: measured 1.03x to 1.27x here, and up to about sqrt(15/8) (~37%)
worst case for a sketch grown to just under the 15/16 * 2k rebuild
threshold. A sketch in exact mode that retains more than k entries also
loses exactness and is returned in estimation mode. Both effects are
documented on the parameter and pinned by tests.
Tests cover the four ordered/trim combinations, that the source sketch is
unmodified, that trimmed output matches trim() + compact() exactly, that
every retained hash is below the new theta, the exact-to-estimation
conversion, the bounds widening, and the empty and below-k cases.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017EDHa7UhfW4eSj5L82panJ
leerho
force-pushed
the
theta-compact-trim
branch
from
September 7, 2026 05:16
dc7a52a to
b3f2c9b
Compare
proost
reviewed
Sep 8, 2026
proost
left a comment
Member
There was a problem hiding this comment.
For the double-check; is same reason not to call "shrink_to_fit"?
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.
What changed
Adds an optional second flag to the existing
compact()onupdate_theta_sketch_alloc:trim = truereduces the returned sketch to at most the nominal sizek. The default is unchanged, and because the parameter is defaulted andcompact()is not virtual here, every existing call site compiles untouched.Thanks to @stojkomilos for raising the underlying need in #515. This takes a different approach — a flag on the existing method rather than a new
get_result()— for two reasons: it avoids adding public API surface, and trimming turns out to be lossy enough that it should be an explicit opt-in rather than the natural way to obtain a result.Why trimming is opt-in
Relative error scales with
1 / sqrt(retained), so discarding entries always widens the confidence bounds, whatever mode the source is in. Measured at the defaultlg_k, comparingcompact()againstcompact(true, true):The cost depends on where in the rebuild cycle the sketch is caught, which a caller cannot predict. Worst case is bounded: a sketch grown to just under the
15/16 * 2krebuild threshold loses nearly half its entries, widening the bounds by aboutsqrt(15/8), roughly 37%.Separately, a sketch in exact mode can retain more than
kentries — nothing has been evicted, so theta is still 1.0. Trimming there discards real data and returns an estimating sketch:Both effects are documented on the
trimparameter and pinned by tests.Implementation
The trim path copies the retained entries first — the method is
const, and quick select would permute the live table — then partitions withstd::nth_elementat 0-based indexk, soentries[k]becomes the new theta and thekentries below it are kept. Sorting happens only when an ordered result is requested. This is the same pivot convention already used byrebuild()intheta_update_sketch_base_impl.hpp, so trimming and rebuilding cannot drift apart.How tested
New cases in
theta/test/theta_sketch_test.cpp:trim()+compact()on theta and retained set, and that every retained hash is strictly below the new thetaNote on the Java side
datasketches-javashould get the same capability, but it cannot use the same mechanism:ThetaSketch.compact(boolean dstOrdered, MemorySegment dstSeg)isabstractand already occupies the two-argument slot, so Java needs an overload rather than an added parameter, andcompact(true, null)would be ambiguous. Also worth care there:QuickSelect.selectExcludingZerosis 1-based whilestd::nth_elementis 0-based, so the Java port should assertretained == kthe way these tests do. That work is deliberately left to a separate PR.🤖 Generated with Claude Code