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: [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]
