Allow theta sketches with lg_k = 4 to match Java - #514
Open
SavicStefan wants to merge 2 commits into
Open
Conversation
added 2 commits
August 20, 2026 10:00
The C++ theta implementation used a single constant, theta_constants::MIN_LG_K = 5, for two distinct roles: the floor on the user-requested nominal size (K) and the floor on the internal hash table (cache) size. Java keeps these separate -- ThetaUtil.MIN_LG_NOM_LONGS = 4 and MIN_LG_ARR_LONGS = 5 -- so Java accepts lg_k = 4 (nominal 16) while C++ rejected it in theta_base_builder:: set_lg_k. As a result, C++ could not *construct* a theta/tuple update sketch or a union at lg_k = 4, even though Java can. (Reading was unaffected: the compact serialization format does not carry lg_nom, so C++ already deserializes Java-produced lg = 4 compact sketches -- this only concerns building them.) Split the constant: MIN_LG_K becomes 4 (the nominal floor validated by set_lg_k, shared by update/union/tuple builders), and a new MIN_LG_ARR = 5 keeps the hash table floored at 32 slots via starting_sub_multiple(). A lg_k = 4 sketch therefore starts with lg_cur_size (5) > lg_nom_size (4) and only ever rebuilds down to the nominal 16, which the existing update/rebuild path already handles. Add tests: building at the minimum lg_k (and rejecting below it), estimation + trim + serialization round trip for an update sketch, and a union built at the minimum lg_k. Co-authored-by: Isaac
Signed-off-by: Stefan Savić <stefan.savic@databricks.com>
SavicStefan
marked this pull request as ready for review
August 20, 2026 12:40
vranes
approved these changes
Aug 20, 2026
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.
Java allows theta and tuple sketches down to lg_k = 4 (16 nominal entries), but C++ rejected anything below lg_k = 5, so the two libraries disagreed on the smallest sketch you can build.
The C++ theta implementation used a single constant, theta_constants::MIN_LG_K = 5, for two distinct roles: the floor on the user-requested nominal size (K) and the floor on the internal hash table (cache) size. Java keeps these separate ThetaUtil.MIN_LG_NOM_LONGS = 4 and MIN_LG_ARR_LONGS = 5, so Java accepts lg_k = 4 (nominal 16) while C++ rejected it in theta_base_builder::set_lg_k.
Because C++ used 5 for both, theta_base_builder::set_lg_k(4) threw, and there was no way to build a theta/tuple update sketch or union at lg_k = 4.
This only affected construction. Reading was already fine: the compact serialization format does not store lg_nom, so C++ has always been able to deserialize Java-produced lg = 4 compact sketches.
Fix, split the one constant into the two that Java has:
A lg_k = 4 sketch now starts with lg_cur_size (5) > lg_nom_size (4): the hash table keeps its full 32 slots and simply rebuilds down to the nominal 16, a path the existing update/rebuild logic already handles. There is no format or behavior change for lg_k >= 5.
The fix covers tuple sketches as well as theta, with no tuple-specific change. Tuple sketches have no size constants of their own their builder inherits theta_base_builder (sharing the same set_lg_k validation and starting_lg_size()), and their internal hash table is a theta_update_sketch_base (the exact table this PR resizes). So lg_k = 4 now works for tuple update sketches and unions too.
Added new min lg_k tests in both theta_sketch_test.cpp and tuple_sketch_test.cpp. Each test: