search

addthis / stream-lib

Stream summarizer and cardinality estimator.
Apache License 2.0
2.26k stars 556 forks source link

Test: Specify hash64 #127

Closed danglotb closed 7 years ago

danglotb commented 7 years ago

The proposed change specifies what the good hash code must be. With the current test, any change in "hash" would still make the test pass, incl. the changes that would result in an inefficient hash.

yuesong commented 7 years ago

Thanks for the PR. The purpose of the test, however, is not to verify "good" hash, but rather that the overloaded hash methods return the same value. In that regard, the existing tests are valid. That said, I think verifying the returned hash value is a valid test, so you are welcome to submit another PR to add the new test. If you did that, I'd suggest that you add some comments on how to obtain the baseline hash value that the test is checking against, e.g. a known good implementation of murmur hash.