Ignore the seed hash of an empty B in theta/tuple a-not-b - #517
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>
|
C++ should follow the example of Java, which preceded C++ by quite a while. You are correct that the Empty Compact Sketch layout should set the seed hash to all zeros, to be ignored. Look carefully at the Java EmptyCompactSketch as it was designed for backward compatibility as well. If you vouch for that here, then I will approve. |
|
Vouched, and I went and read it rather than taking my own word for it.
//For backward compatibility, a candidate long must have Flags= compact, read-only,
// COMPACT-Family=3, SerVer=3, PreLongs=1, and be exactly 8 bytes long. The seedHash is ignored.
// NOTE: The empty and ordered flags may or may not be set
private static final long EMPTY_SKETCH_MASK = 0X00_00_EB_00_00_FF_FF_FFL;
private static final long EMPTY_SKETCH_TEST = 0X00_00_0A_00_00_03_03_01L;
static final byte[] EMPTY_COMPACT_SKETCH_ARR = { 1, 3, 3, 0, 0, 0x1E, 0, 0 };Two things line up with what you said. The serialized array carries On scope, this PR only does the read half. It stops Happy to do the write half too, emitting a zero seed hash for empty compact sketches to match |
|
Write half is up as #518, stacked on this branch. One thing came out of doing it that is worth flagging here, because it affects the order. On its own the write change is a regression, and this PR is what makes it safe. So #518 wants this one in first. For the record on the rest of the surface: After #518 an empty compact sketch serializes to |
leerho
left a comment
There was a problem hiding this comment.
Good Job! And thank you for catching this and this contribution! I really appreciate your thoroughness!
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.