Skip to content

Ignore the seed hash of an empty B in theta/tuple a-not-b - #517

Open
jaideeppyne wants to merge 1 commit into
apache:masterfrom
jaideeppyne:theta-anotb-empty-seed-hash
Open

Ignore the seed hash of an empty B in theta/tuple a-not-b#517
jaideeppyne wants to merge 1 commit into
apache:masterfrom
jaideeppyne:theta-anotb-empty-seed-hash

Conversation

@jaideeppyne

Copy link
Copy Markdown

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 check
  • theta_intersection_base::update: if (!sketch.is_empty() && sketch.get_seed_hash() != ...) throw

and 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::compute is the exception. Its early return only fires when a.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:

// B: empty, built with a different seed
auto b = update_theta_sketch::builder().set_seed(12345).build().compact();
// A: non-empty, zero retained (every hash exceeded theta)
auto a = update_theta_sketch::builder().set_p(1e-6f).build();
a.update(1); a.update(2); a.update(3);

compact_theta_sketch::deserialize(...);              // ACCEPTED
theta_union::builder().build().update(b);            // ACCEPTED
theta_intersection().update(b);                      // ACCEPTED
theta_a_not_b().compute(a.compact(), b);             // THREW -> B seed hash mismatch

It also breaks interop with datasketches-java, which serializes every empty compact sketch as the constant {1, 3, 3, 0, 0, 0x1E, 0, 0}. EmptyCompactSketch documents 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 what theta_intersection_base::update already 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 mismatch and 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, and tuple_test passes 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.

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>
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