jaideeppyne opened a new pull request, #517:
URL: https://github.com/apache/datasketches-cpp/pull/517
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:
```cpp
// 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
