You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@datasketches.apache.org by "proost (via GitHub)" <gi...@apache.org> on 2026/08/01 08:01:35 UTC
[PR] test: count min sketch serialization test [datasketches-go]
proost opened a new pull request, #163:
URL: https://github.com/apache/datasketches-go/pull/163
Add cross language compatibility test code.
There is limitation of compatibility test for the CM sketch.
internal hash seeds are randomly generated all the implementations(Java, C++, Go), So we can't expect interoperability correctly. random generation algorithms differs from language by language.
So CM sketch cross language compatibility test just check that each binary format is same.
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] test: count min sketch serialization test [datasketches-go]
Posted by "proost (via GitHub)" <gi...@apache.org>.
proost commented on PR #163:
URL: https://github.com/apache/datasketches-go/pull/163#issuecomment-5150567462
@tisonkun
> But anyway a CountMinSketch snapshot is self-described to recover and continue to use?
Yes, If use it's own language. So i generate CM sketch in go and deserialized from rust, It constructed well. But we can't guarantee that estimation of value is within range.
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] test: count min sketch serialization test [datasketches-go]
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on PR #163:
URL: https://github.com/apache/datasketches-go/pull/163#issuecomment-5840830996
I have to rely on Claude for this since I don't do Go :) Nonetheless, it would be prudent to pay attention to what Claude has to say:
**Claude:**
I found no real bugs in [apache/datasketches-go#163 · test: count min sketch…](https://github.com/apache/datasketches-go/pull/163), only three test-quality and API issues.
I decoded the snapshot files the PR adds to check the description's claim that the Go and C++ binary formats match. It holds:
- Empty sketch: the Go and C++ files are byte-identical.
- Non-empty sketch: both files are 24,600 bytes and their first 24 bytes (the header, including the total weight of 2850) are identical. The counters differ, which is expected because each language generates its internal hash seeds randomly. Every row of counters sums to 2850 in both files, as it should.
**Findings:**
1. count/count_min_sketch_serialization_test.go:109 — The C++ non-empty test never checks the 3,072 counters. The GetSeed check always passes, because the deserializer just returns the seed you pass in and never compares it with the stored seed hash. So a bug that misreads the counters would still pass. **A check that works despite the different hash seeds: assert that each row sums to the total weight. I confirmed both files satisfy this.**
2. count/count_min_sketch_serialization_test.go:82 — Errors are checked with assert.NoError, which doesn't stop the test. If the file fails to deserialize, result is nil and the next line panics instead of failing cleanly. **Same at line 109; use require.NoError.**
3. count/count_min_sketch.go:32 — The PR adds an exported constant, DefaultSeed, with no doc comment, and only tests use it. Once released, removing it breaks the public API. **Either document it as the library default (it matches C++'s 9001) or make it a constant inside the test.**
Two small notes that aren't bugs:
- TestCPPCompact looks like a **name copied from the theta tests; nothing here is "compact".**
- The deserializer never checks the stored seed hash against the seed passed in. **This predates the PR, but it's why the seed assertion tests nothing.**
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] test: count min sketch serialization test [datasketches-go]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #163:
URL: https://github.com/apache/datasketches-go/pull/163#issuecomment-5150726323
> But we can't guarantee that estimation of value is within range.
Sorry, I don't dive into CMSketch deeply. Could you elaborate this point a bit? Or provide a concrete example to demonstrate the issue.
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] test: count min sketch serialization test [datasketches-go]
Posted by "proost (via GitHub)" <gi...@apache.org>.
proost commented on PR #163:
URL: https://github.com/apache/datasketches-go/pull/163#issuecomment-6171855634
@leerho
Thanks for review.
1. I prefer that test doesn't stop even if some cases failed. Because seeing all failures is preferred. But your review makes sense to me, so i added assertion is passed or not.
2. Exporting default seed isn't good. So i added new constructors and deserialization method without what is seed needed.
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] test: count min sketch serialization test [datasketches-go]
Posted by "proost (via GitHub)" <gi...@apache.org>.
proost commented on PR #163:
URL: https://github.com/apache/datasketches-go/pull/163#issuecomment-5150776059
@tisonkun
Each implementation derives the per-row Murmur3 hash seeds from the user-provided sketch seed using its language-specific random number generator.
Java:
```java
final Random rand = new Random(seed);
for (int i = 0; i < numHashes; i++) {
hashSeeds_[i] = rand.nextLong();
}
```
C++:
```cpp
std::default_random_engine rng(_seed);
std::uniform_int_distribution<uint64_t> extra_hash_seeds(
0, std::numeric_limits<uint64_t>::max());
for (uint64_t i = 0; i < num_hashes; ++i) {
hash_seeds.push_back(extra_hash_seeds(rng) + _seed);
}
```
Go:
```go
rng := rand.New(rand.NewSource(seed))
hashSeeds := make([]int64, numHashes)
for i := range int(numHashes) {
hashSeeds[i] = int64(rng.Int()) + seed
}
```
The serialized form contains the counter array and a hash of the user-provided seed, but it does not contain the generated per-row `hashSeeds`.
During deserialization, each implementation reconstructs `hashSeeds` from the provided seed using its own random number generator. Because these random number generators do not produce the same sequence across Java, C++, and Go, the reconstructed Murmur3 seeds can differ even when the same input seed is provided.
For example:
1. A C++ sketch updates item `x`, placing its counts into buckets selected using the C++-generated `hashSeeds`.
2. Go successfully deserializes the C++ counter array.
3. Go regenerates different `hashSeeds`.
4. Calling `GetEstimate(x)` in Go may inspect different buckets from those updated by C++.
Therefore, the binary format can be deserialized across languages, but we cannot currently guarantee that queries, subsequent updates, or merges behave correctly across language boundaries.
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] test: count min sketch serialization test [datasketches-go]
Posted by "proost (via GitHub)" <gi...@apache.org>.
proost commented on PR #163:
URL: https://github.com/apache/datasketches-go/pull/163#issuecomment-5150532344
@leerho
CM sketch has limitation for cross language compatibility. Is intended? Or do we have to introduce same random algorithm for all the languages?
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org