Skip to content

Fix test failures, pom cross-language group exclusion, and empty tuple seed hash - #772

Merged
leerho merged 4 commits into
mainfrom
fix-test-failures
Sep 28, 2026
Merged

leerho merged 4 commits into
mainfrom
fix-test-failures

Conversation

@leerho

@leerho leerho commented Sep 26, 2026

Copy link
Copy Markdown
Member

Summary

Four independent fixes found while getting mvn clean test green again.

  1. BloomFilterTest.basicDifferenceTest — the test (added with difference() in c20bd80) could essentially never pass: with 8192 bits, the right filter's load was ~46.5%, so only ~4.4% of left-only items can survive difference(), far below the test's 25% threshold. Raised numBits to 65536 (~7.5% load, ~68% expected retention). No library change.
  2. pom.xml — the composite testng.* group properties added in 9951cf8 were concatenated without commas, so excludedGroups only matched generate_java_files and check_java_files; every C++/Go/Rust/CL-binary/historical cross-language group ran on a plain mvn test (the source of the "File not found" warnings). Also pointed the check_CL_binary_files and check_cpp_historical_files profiles at the renamed properties (they referenced undefined ones and selected nothing).
  3. TDigestDouble.merge — 9bf19ff moved the min/max update ahead of the Math.addExact overflow check, so a merge that throws ArithmeticException still mutated the digest (caught by weightOverflowDoesNotChangeDigest). Moved the check back first.
  4. CompactTupleSketch — a serialized empty compact tuple sketch (8 bytes, preLongs = 1) now writes a zero seed hash, following the theta rule. C++ compact_tuple_sketch already does this; no Java or C++ tuple reader checks the seed hash of an empty image. ArrayOfDoubles is a separate, older format and is intentionally left for later.

Testing

  • mvn clean test: 2286 tests, 0 failures.
  • mvn clean test -Pcheck_all_language_files (with current C++ snapshots from datasketches-cpp master): 5 known failures, not addressed here:
    • HllSketchCrossLanguageTest.checkBinaries2/3 and ThetaSketchCrossLanguageTest.checkBinaries2/3: Go-generated hll6_n1000_go.sk and theta_n0_go.sk differ from Java and C++ (being fixed by the Go team).
    • AodSketchCrossLanguageTest.checkCpp: C++ writes a zero seed hash for an empty ArrayOfDoubles compact image, which the Java AoD reader rejects. Deferred until the AoD format is revisited.

🤖 Generated with Claude Code

leerho and others added 4 commits September 26, 2026 14:16
With 8192 bits, right's load was ~46.5%, so only ~4.4% of left-only
items could survive difference(), far below the test's 25% threshold.
Raise numBits to 65536 (~7.5% load, ~68% expected retention).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The composite testng.* group properties were concatenated without
commas, so surefire's excludedGroups only matched generate_java_files
and check_java_files; C++/Go/Rust/CL-binary/historical groups ran on
every 'mvn test'. Also point the check_CL_binary_files and
check_cpp_historical_files profiles at the renamed properties.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
9bf19ff moved the min/max update ahead of the Math.addExact check,
so a merge that throws ArithmeticException still mutated the digest,
failing weightOverflowDoesNotChangeDigest.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Follow the theta rule: a serialized empty compact sketch (8 bytes,
preLongs = 1) carries a zero seed hash. C++ compact_tuple_sketch already
does this, and no Java or C++ tuple reader checks the seed hash of an
empty image, so this only brings the Java bytes in line with C++.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@leerho
leerho merged commit 39e5f74 into main Sep 28, 2026
6 checks passed
@leerho
leerho deleted the fix-test-failures branch September 28, 2026 00:24
@tisonkun

Copy link
Copy Markdown
Member

@leerho While evaluating the corresponding Rust change (apache/datasketches-rust#283), I would like to understand the rationale for the empty Theta/Tuple seed-hash rule. Is this an intentional semantic distinction, or is the Tuple change primarily following the existing C++ encoding?

I have two questions:

  1. What concrete problem does zeroing the seed hash solve? An empty sketch can still have a configured seed. As this PR notes, the existing nonzero seed hash is already accepted by the empty Tuple readers, so retaining it does not appear to break deserialization compatibility. Writing zero also does not reduce the image size. Ignoring an empty input during a set operation and discarding its seed fingerprint during serialization seem like separate decisions: the former does not require the latter. Is there a documented requirement for a unique, seed-independent empty encoding, or another benefit that justifies this special case?

  2. Why does this apply specifically to Theta/Tuple? I checked both the Java code at this PR's head and C++ at 70e462f. Other seeded families preserve and validate their configuration even when empty:

Family Java C++
CPC Empty serialization retains the seed hash; heapify/uncompress validates it. Writes the seed hash before the non-empty payload branch; deserialization validates it.
Count-Min Writes the seed hash before returning the empty image; deserialization checks it before the empty return. Writes the seed hash for empty images; deserialization checks it before the empty return.
Bloom Stores the full seed rather than a seed hash: empty serialization preserves it, and union/intersection require compatibility, including matching seeds. Empty serialization preserves the full seed; union/intersection check seed compatibility without an empty-input exception.

CPC is an especially direct contrast: Java's union checks the seed hash before returning for an empty serialized input, its object-input path does the same with the seed, and C++ also checks before its empty-input return. Thus even an empty input can produce a seed-mismatch error in both implementations. That is a configuration invariant, despite the empty input contributing no data.

I see that Java Theta uses an empty compact singleton and a fixed byte representation. Is the intended boundary that immutable compact Theta/Tuple results discard seed identity when empty, while updateable sketches retain configuration? Or is this a historical format convention that Tuple is following for byte-level consistency? The differing policies may be intentional, but it would help to state the distinction explicitly rather than infer a general rule that emptiness makes the seed irrelevant.

@leerho

leerho commented Sep 28, 2026

Copy link
Copy Markdown
Member Author

@tisonkun,
Very fair questions.

Question 1: What concrete problem does zeroing the seed hash solve?

Definitions

  • A hashSeed Is a confusing name and should just be called seed. It is a constant that configures a Hash-Function $hash = f(value, seed)$, to return a deterministic, pseudorandom, (64 or 128bit) hash that is unique to the value and the seed. Once a user selects a seed it should almost never be changed.
      Inside large organizations, different departments may want to choose different seeds to prevent accidental merging of sketches. This is not cryptographic security by any means, since the hash functions themselves are not cryptographic.

    • The CountMinSketch declares a "private final long[] hashSeeds_", which is very confusing and should have been just a long array of seeds_ .
    • The BloomFilter declares a single long seed like this:
      "private final long seed; // hash seed_".
      This is marginally OK. The two separated words in the comment imply "seed for the hash function". Given all the confusion around these words I would prefer the longer phrase for the comment instead.
  • A seedHash is the result of applying the same Hash-Function to the seed itself and is trimmed to 16 bits: $seedHash = (short)hash = f(seed, seed)$.

    • The seedHash is only relevant to serialized sketches. And only used in Theta, Tuple, CPC, CountMin Sketches.
    • The reason it was introduced in Theta/Tuple (about 12 years ago), was to warn the user if an incoming serialized sketch was generated using a different seed than the user's choice of seed. This assumes the same hash function, of course, and for all of our sketches that use hash functions, the hash function is hard wired for a lot of reasons, but a different topic.
    • The use of seedHash is a bit strange in the CountMinSketch. It could have been incorporated into the first preamble long, but instead it is in the second long. This sketch is quite new and has not had the scrutiny of the Theta/Tuple/CPC sketches.

Architectural Concepts and the Theta Sketch

Big Data tends to be power-law distributed

It is also important to understand that big data tends to be power-law distributed. This means that when analyzing massive data it is very likely to have many millions of empty streams (sometimes null), 100s of thousands of single item streams, and so on down to very few streams of millions of items.

The Theta Sketch is Big

The Theta Sketch is a big sketch compared to HLL (about 8x to 16x bigger).

Throughput is inversely related to serialized size

This becomes very evident when processing millions of sketches. As a result we designed the serialized formats to only contain the required preamble elements, and focus on the smallest of these formats to be as fast as possible to process. You can observe this when you look at the documentation in the Java Theta PreambleUtil class. There are seven different serialization formats for the Theta sketch.

In particular look at these 4 formats for serialized Compact Theta Sketches:

Sketch Type Comment
Empty No: SeedHash, Theta, #Entries, p
Single Item, Empty No: Theta, #Entries, p
Exact No Theta
Estimating Full Preamble

The serialized, empty CompactThetaSketch has no hashes so it doesn't need a SeedHash. This 8-byte sketch can be represented as a static final long constant, which makes it extremely fast to detect and process (and the same size as a null value in a 64bit, >32GB JVM).

What concrete problem does zeroing the seedHash solve?

  1. Speed. Making the empty sketch independent of the chosen seed allows it to be a constant, thus very fast.
  2. Cross-Language Binary (CLB) Testing. We have already standardized on the hash function, the default seed value, and default ordered hash array. Standardizing on the empty sketch format was the last remaining hurdle to achieve full binary compatibility.

Question 2: Why does this apply specifically to Theta/Tuple/CPC?

Part of this is history. The Theta/Tuple families were the very first sketches and we had a lot of focus on throughput. CPC came a little later but was also a unique counting sketch and sort of a blend of HLL and Theta. When compressed and serialized was also very small so we felt focusing on its speed and size performance made sense, and it had similar hash seed compatibility issues as Theta. Nonetheless, its consistency with our improving standardization across the library is lacking and could be improved.

The Theta and HLL sketches are just the first of the deterministic sketches I have looked at for the CLB property, which should evolve to the other deterministic sketches as well.

The other sketches came much later and had different kinds of hash issues and trade-offs. The CountMin and Bloom are relatively new and were initially contributed from the outside, and frankly have not had the same level of attention paid to consistency with other sketches in the library. In that regard they can be improved quite a bit.

@tisonkun

Copy link
Copy Markdown
Member

@leerho Thanks for explaining the throughput and cross-language binary compatibility motivation. I’ve updated Rust’s empty compact Theta and Tuple serialization to use the canonical zero seed hash, with fixed 8-byte images for the empty serialization path: apache/datasketches-rust#283.

My preference for the other families is still to preserve the configured seed identity and retain compatibility checks, including for empty sketches.

Specifically, for CPC and CountMin, I’d keep writing the configured seed hash and validating it against the caller-supplied seed during deserialization, even when the sketch is empty. Since the seed hash is only a fingerprint, the actual seed still needs to come from the caller. For Bloom filters, I’d keep serializing and restoring the full seed unchanged, with seed compatibility checked when combining filters.

Does this family-specific contract sound reasonable? Or do you intend seed-independent empty encodings to become a cross-language requirement for CPC and CountMin as well?

@leerho

leerho commented Sep 29, 2026

Copy link
Copy Markdown
Member Author

I haven't had the chance to look deeply into CPC yet. But ds-cpp issue #460 has identified some other binary discrepancies between C++ and Java for Theta sketch that I want to look at next.
My ultimate goal is to try to achieve Cross-Language Binary (CLB) compatibility for all of our deterministic sketches. It will take some time to get there, but ultimately, it will make our libraries more robust, easier to test and more trustworthy.

@tisonkun

Copy link
Copy Markdown
Member

Cross-Language Binary (CLB) compatibility

Great! Please let me know if we have a spec or any further changes I could or should bring to datasketches-rust.

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.

2 participants