Repository navigation
Fix test failures, pom cross-language group exclusion, and empty tuple seed hash - #772
Conversation
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 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:
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. |
|
@tisonkun, Question 1: What concrete problem does zeroing the seed hash solve?Definitions
Architectural Concepts and the Theta SketchBig Data tends to be power-law distributedIt 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 BigThe Theta Sketch is a big sketch compared to HLL (about 8x to 16x bigger). Throughput is inversely related to serialized sizeThis 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:
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?
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. |
|
@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? |
|
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. |
Great! Please let me know if we have a spec or any further changes I could or should bring to datasketches-rust. |
Summary
Four independent fixes found while getting
mvn clean testgreen again.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 survivedifference(), far below the test's 25% threshold. RaisednumBitsto 65536 (~7.5% load, ~68% expected retention). No library change.testng.*group properties added in 9951cf8 were concatenated without commas, soexcludedGroupsonly matchedgenerate_java_filesandcheck_java_files; every C++/Go/Rust/CL-binary/historical cross-language group ran on a plainmvn test(the source of the "File not found" warnings). Also pointed thecheck_CL_binary_filesandcheck_cpp_historical_filesprofiles at the renamed properties (they referenced undefined ones and selected nothing).Math.addExactoverflow check, so a merge that throwsArithmeticExceptionstill mutated the digest (caught byweightOverflowDoesNotChangeDigest). Moved the check back first.compact_tuple_sketchalready 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/3andThetaSketchCrossLanguageTest.checkBinaries2/3: Go-generatedhll6_n1000_go.skandtheta_n0_go.skdiffer 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