aherbert commented on issue #27: Murmur3fix URL: https://github.com/apache/commons-codec/pull/27#issuecomment-555543154 With a bit more time to look at the new CharSequence encoding method I found it to be broken. This test demonstrates how it does not handle bad code points correctly: ``` @Test public void testHash32x86_UTF8_encoding_InvalidCodePoints() { // Since RFC 3629 (November 2003), the high and low surrogate halves used by // UTF-16 (U+D800 through U+DFFF) and code points not encodable by UTF-16 // (those after U+10FFFF) are not legal Unicode values, and their UTF-8 // encoding must be treated as an invalid byte sequence. String invalid = String.valueOf('\uD801'); int seed = 0; // Correct answer byte[] bytes = invalid.getBytes(StandardCharsets.UTF_8); int h1 = MurmurHash3.hash32x86(bytes, 0, bytes.length, seed); // This is NOT EQUAL int h2 = MurmurHash3.hash32x86((CharSequence) invalid, 0, invalid.length(), seed); Assert.assertNotEquals("Did not expected the encoding to be correct", h1, h2); // This is what the encoding should do. Represent bad code points as unknown. int h3 = MurmurHash3.hash32x86((CharSequence) "?", 0, 1, seed); Assert.assertEquals("The invalid code point should be encoded as '?'", h1, h3); } ``` I think we should leave charset encoding out of this. I'm also confused as to why the default seed has been set at that value. This should be changed for the new methods. IMO we should have the following new methods and class: ``` public static int hash32x86(byte[] data) public static int hash32x86(byte[] data, int offset, int length) public static int hash32x86(byte[] data, int seed) public static int hash32x86(byte[] data, int offset, int length, int seed) public static int hash128x64(byte[] data) public static int hash128x64(byte[] data, int offset, int length) public static int hash128x64(byte[] data, int seed) public static int hash128x64(byte[] data, int offset, int length, int seed) IncrementalHash32x86 ``` The default seed for the new methods should be zero. This is what is done in Google Guava and the python library `mmh3`. This removes adding new methods for primitives since these are not broken. They are just strangely configured helper methods. These should be documented correctly with their equivalent code and scheduled for removal e.g. ``` /** * Generates 32 bit hash from two longs with default seed value. * This is a helper method that will produce the same result as: * * <pre> * int seed = 104729; * int hash = hash32x86(ByteBuffer.allocate(16) * .putLong(l0) * .putLong(l1) * .array(), seed); * </pre> * * @param l0 long to hash * @param l1 long to hash * @return 32 bit hash * @deprecated This method to be removed in 2.0 */ @Deprecated public static int hash32(final long l0, final long l1) { ``` The hash64 methods should not be documented as `"MSB 8 bytes of Murmur3 128-bit variant"`. They do not return the same value as and the 128-bit method since they neglect the second hash to be mixed into the first. This has would have an effect as it should be initialised with the default seed (which is not zero). The methods are just plain wrong and they also do not use little endian format for the equivalent calls via a ByteBuffer to the byte[] hash method. The methods bloat the class. If removed in 2.0 and someone wants them back then this can be addressed then. I can do documentation changes for all methods to be deprecated in master if you want and then you can rebase the PR to bring in the fixed hash implementations. Thanks for your patience.
---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
