Ignore the seed hash of an empty B in theta/tuple a-not-b - #517
Open
jaideeppyne wants to merge 1 commit into
Open
Ignore the seed hash of an empty B in theta/tuple a-not-b#517jaideeppyne wants to merge 1 commit into
jaideeppyne wants to merge 1 commit into
Conversation
An empty sketch retains no hashes, so its seed hash carries no
information and must not be validated. Deserialization
(deserialize_v3/deserialize_v4), theta_union_base::update() and
theta_intersection_base::update() all already skip the seed hash check
for empty inputs, and the existing seed mismatch tests record the intent
with the comment "non-empty should not be ignored".
theta_set_difference_base::compute() was the one path that still checked
it. When A is non-empty with zero retained entries, the early return does
not fire, and an empty B whose seed hash differs makes a-not-b throw
"B seed hash mismatch" where union and intersection accept the same pair.
This also breaks Java/C++ interop: datasketches-java serializes every
empty compact sketch as the constant {1,3,3,0,0,0x1E,0,0}, with a seed
hash of 0 that is documented as ignored, so any empty sketch coming from
Java hits this path.
The behavior is reproducible in C++ alone using two different seeds.
Generated-by: Claude Code (Claude Opus 4.8)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
While investigating #460 (Java/C++ theta byte differences) I found a related bug that is not about bytes.
An empty sketch retains no hashes, so its seed hash carries no information. Three of the four C++ paths that validate a seed hash already know this:
compact_theta_sketch::deserialize_v3/deserialize_v4:if (!is_empty) checker<true>::check_seed_hash(...)theta_union_base::update:if (sketch.is_empty()) return;before the checktheta_intersection_base::update:if (!sketch.is_empty() && sketch.get_seed_hash() != ...) throwand all three existing seed mismatch tests record the intent in a comment:
sketch.update(1); // non-empty should not be ignored.theta_set_difference_base::computeis the exception. Its early return only fires whena.get_num_retained() > 0, so when A is non-empty with zero retained entries the check is reached and an empty B with a different seed hash throws.Reproducible in C++ alone, no Java involved:
It also breaks interop with datasketches-java, which serializes every empty compact sketch as the constant
{1, 3, 3, 0, 0, 0x1E, 0, 0}.EmptyCompactSketchdocuments that seed hash of 0 as ignored, so every empty sketch arriving from Java trips this path. In a 20 case Java/C++ differential I ran, all 6 cases that produce an empty sketch failed here and now pass.The fix guards B's check with
!b.is_empty(), matching whattheta_intersection_base::updatealready does. A's check is left alone because A is guaranteed non-empty by the early return above it.Verification: the added test fails on master with
B seed hash mismatchand passes with the fix. Full suite green afterwards, 17/17 ctest suites, 20,245,913 assertions in the theta suite. This code is shared with the tuple sketches, andtuple_testpasses too.I used Claude Code to help run the differential harness and prepare this change. I verified the behavior, the fix and the test results myself against actual build and test output.