[
https://issues.apache.org/jira/browse/CODEC-268?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18098879#comment-18098879
]
Gary D. Gregory commented on CODEC-268:
---------------------------------------
[~aherbert]
Can this ticket be assigned a release version and closed?
> Deprecate methods in MurmurHash3 with no equivalent in the reference c++
> source.
> --------------------------------------------------------------------------------
>
> Key: CODEC-268
> URL: https://issues.apache.org/jira/browse/CODEC-268
> Project: Commons Codec
> Issue Type: Wish
> Affects Versions: 1.13
> Reporter: Alex Herbert
> Assignee: Alex Herbert
> Priority: Minor
>
> The following methods have no equivalent in the [MurmurHash3 c++ source
> code|https://github.com/aappleby/smhasher/blob/master/src/MurmurHash3.cpp]
> {code:java}
> public static long hash64(final long data) {
> public static long hash64(final int data) {
> public static long hash64(final short data) {
> public static long hash64(final byte[] data) {
> public static long hash64(final byte[] data, final int offset, final int
> length) {
> public static long hash64(final byte[] data, final int offset, final int
> length, final int seed) {
> {code}
> They are documented to return the upper 64-bits of the 128-bit hash method.
> This is false as the code neglects to mix in alternating blocks of 64-bits
> with the corresponding other hash that would be built in the 128-bit method.
> Thus this passes using a NotEquals check:
> {code:java}
> @Test
> public void testHash64InNotEqualToHash128() {
> for (int i = 0; i < 32; i++) {
> final byte[] bytes = Arrays.copyOf(RANDOM_BYTES, i);
> final long h1 = MurmurHash3.hash64(bytes);
> final long[] hash = MurmurHash3.hash128(bytes);
> Assert.assertNotEquals("Did not expect hash64 to match upper bits of
> hash128", hash[0], h1);
> Assert.assertNotEquals("Did not expect hash64 to match lower bits of
> hash128", hash[1], h1);
> }
> }
> {code}
> The following methods are convenience methods that use an arbitrary default
> seed:
> {code:java}
> public static final int DEFAULT_SEED = 104729;
> public static int hash32(final long data1, final long data2) {
> public static int hash32(final long data1, final long data2, final int
> seed) {
> public static int hash32(final long data) {
> public static int hash32(final long data, final int seed) {
> {code}
> Although they match calling hash32() with the equivalent bytes this assumes
> big-endian format. The reference c++ code is designed for little endian. If
> you hash the corresponding values using a Google Guava implementation with
> the same seed then they do not match as Guava faithfully converts primitives
> as little endian. Also note that the available methods for hashing primitives
> are not the same as those in hash64.
> These methods thus make very little sense and should be removed as an
> unwanted legacy from the port from Apache Hive.
> The following methods are convenience methods that use default encoding via
> String.getBytes():
> {code:java}
> public static int hash32(final String data) {
> public static long[] hash128(final String data) {
> {code}
> These effectively do nothing and save 0 lines of code for the caller:
> {code:java}
> String s;
> int hash1 = MurmurHash3.hash32(s);
> int hash2 = MurmurHash3.hash32(s.getBytes());
> assert hash1 == hash2;
> {code}
> This is not even used:
> {code:java}
> public static final long NULL_HASHCODE = 2862933555777941757L;
> {code}
> The following methods for hash32, hash64 and hash128 are not consistent with
> the available arguments:
> {code:java}
> public static int hash32(final byte[] data) {
> public static int hash32(final String data) {
> public static int hash32(final byte[] data, final int length) {
> public static int hash32(final byte[] data, final int length, final int
> seed) {
> public static int hash32(final byte[] data, final int offset, final int
> length, final int seed) {
> public static long hash64(final byte[] data) {
> // Two int arguments specify (offset, length) not (length, seed)
> public static long hash64(final byte[] data, final int offset, final int
> length) {
> public static long hash64(final byte[] data, final int offset, final int
> length, final int seed) {
> public static long[] hash128(final byte[] data) {
> public static long[] hash128(final String data) {
> // No other (offset, length, seed) combinations
> public static long[] hash128(final byte[] data, final int offset, final
> int length, final int seed) {
> {code}
> I would suggest deprecating:
> * All hash64 methods
> * The default seed
> * The null hashcode
> * The helper methods that hash primitives
> * The helper methods that hash String using default encoding
> * Those methods that accept various combinations of offset, length and seed
> This would leave a cleaner API:
> {code:java}
> public static int hash32(final byte[] data) {
> public static int hash32(final byte[] data, final int offset, final int
> length, final int seed) {
> public static long[] hash128(final byte[] data) {
> public static long[] hash128(final byte[] data, final int offset, final
> int length, final int seed) {
> {code}
> There are outstanding bugs in CODEC-264 and CODEC-267 for sign extension
> errors in hash32 and hash128. These should be fixed using a minimal API with
> names that reference the x86 and x64 names from the MurmurHash3 reference c++
> code:
> I would argue that the default seed should be zero (it is zero in Google
> Guava and Python mmh3 implementations of MurmurHash3), not an arbitrary
> random int.
> {code:java}
> public static int hash32x86(final byte[] data) {
> public static int hash32x86(final byte[] data, final int offset, final
> int length, final int seed) {
> public static long[] hash128x64(final byte[] data) {
> public static long[] hash128x64(final byte[] data, final int offset,
> final int length, final int seed) {
> {code}
> All deprecated methods should be referred to use these instead.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
