From 041670bcb27ab3b1d7e418ce478b16773478650e Mon Sep 17 00:00:00 2001 From: Larry Booker Date: Thu, 21 Aug 2025 11:45:24 -0700 Subject: [PATCH] Multiple bugfixes --- src/main/java/io/deephaven/hash/KHash.java | 5 +- .../deephaven/hash/KeyedDoubleObjectHash.java | 5 +- .../io/deephaven/hash/KeyedIntObjectHash.java | 5 +- .../deephaven/hash/KeyedLongObjectHash.java | 5 +- .../io/deephaven/hash/KeyedObjectHash.java | 3 +- .../hash/TestKeyedDoubleObjectHashMap.java | 77 +++++++++++++++++++ .../hash/TestKeyedIntObjectHashMap.java | 77 +++++++++++++++++++ .../hash/TestKeyedLongObjectHashMap.java | 77 +++++++++++++++++++ .../hash/TestKeyedObjectHashMap.java | 64 +++++++++++++++ 9 files changed, 309 insertions(+), 9 deletions(-) diff --git a/src/main/java/io/deephaven/hash/KHash.java b/src/main/java/io/deephaven/hash/KHash.java index acb02df..18478f6 100644 --- a/src/main/java/io/deephaven/hash/KHash.java +++ b/src/main/java/io/deephaven/hash/KHash.java @@ -185,8 +185,9 @@ protected int setUp(int initialCapacity) { * @param capacity an int value */ private final void computeMaxSize(int capacity) { - // need at least one free slot for open addressing - _maxSize = Math.min(capacity - 1, (int) Math.floor(capacity * _loadFactor)); + // need at least two free slots for open addressing since we rehash when only one free slot + // remains + _maxSize = Math.min(capacity - 2, (int) Math.floor(capacity * _loadFactor)); _free = capacity - _size; // reset the free element count } diff --git a/src/main/java/io/deephaven/hash/KeyedDoubleObjectHash.java b/src/main/java/io/deephaven/hash/KeyedDoubleObjectHash.java index 4cbf526..e9fda19 100644 --- a/src/main/java/io/deephaven/hash/KeyedDoubleObjectHash.java +++ b/src/main/java/io/deephaven/hash/KeyedDoubleObjectHash.java @@ -12,6 +12,7 @@ of the License, or (at your option) any later version. package io.deephaven.hash; import java.io.Serializable; +import java.util.Objects; /** * This collection implements a hashed set of objects identified by a key; the characteristics of @@ -258,7 +259,7 @@ public synchronized boolean replace(Double key, V oldValue, V newValue) { throw new IllegalArgumentException( "key and value are inconsistent:" + key + " and " + doubleKeyDef.getDoubleKey(newValue)); } - return internalPut(newValue, KeyedDoubleObjectHash.REPLACE, oldValue).equals(oldValue); + return Objects.equals(internalPut(newValue, KeyedDoubleObjectHash.REPLACE, oldValue), oldValue); } public synchronized boolean replace(double key, V oldValue, V newValue) { @@ -269,7 +270,7 @@ public synchronized boolean replace(double key, V oldValue, V newValue) { throw new IllegalArgumentException( "key and value are inconsistent:" + key + " and " + doubleKeyDef.getDoubleKey(newValue)); } - return internalPut(newValue, KeyedDoubleObjectHash.REPLACE, oldValue).equals(oldValue); + return Objects.equals(internalPut(newValue, KeyedDoubleObjectHash.REPLACE, oldValue), oldValue); } private static final int NORMAL = 0; diff --git a/src/main/java/io/deephaven/hash/KeyedIntObjectHash.java b/src/main/java/io/deephaven/hash/KeyedIntObjectHash.java index 4dc4f78..463a85e 100644 --- a/src/main/java/io/deephaven/hash/KeyedIntObjectHash.java +++ b/src/main/java/io/deephaven/hash/KeyedIntObjectHash.java @@ -12,6 +12,7 @@ of the License, or (at your option) any later version. package io.deephaven.hash; import java.io.Serializable; +import java.util.Objects; /** * This collection implements a hashed set of objects identified by a key; the characteristics of @@ -255,7 +256,7 @@ public synchronized boolean replace(Integer key, V oldValue, V newValue) { throw new IllegalArgumentException( "key and value are inconsistent:" + key + " and " + intKeyDef.getIntKey(newValue)); } - return internalPut(newValue, KeyedIntObjectHash.REPLACE, oldValue).equals(oldValue); + return Objects.equals(internalPut(newValue, KeyedIntObjectHash.REPLACE, oldValue), oldValue); } public synchronized boolean replace(int key, V oldValue, V newValue) { @@ -266,7 +267,7 @@ public synchronized boolean replace(int key, V oldValue, V newValue) { throw new IllegalArgumentException( "key and value are inconsistent:" + key + " and " + intKeyDef.getIntKey(newValue)); } - return internalPut(newValue, KeyedIntObjectHash.REPLACE, oldValue).equals(oldValue); + return Objects.equals(internalPut(newValue, KeyedIntObjectHash.REPLACE, oldValue), oldValue); } private static final int NORMAL = 0; diff --git a/src/main/java/io/deephaven/hash/KeyedLongObjectHash.java b/src/main/java/io/deephaven/hash/KeyedLongObjectHash.java index 708a30d..535d8aa 100644 --- a/src/main/java/io/deephaven/hash/KeyedLongObjectHash.java +++ b/src/main/java/io/deephaven/hash/KeyedLongObjectHash.java @@ -12,6 +12,7 @@ of the License, or (at your option) any later version. package io.deephaven.hash; import java.io.Serializable; +import java.util.Objects; /** * This collection implements a hashed set of objects identified by a key; the characteristics of @@ -256,7 +257,7 @@ public synchronized boolean replace(Long key, V oldValue, V newValue) { throw new IllegalArgumentException( "key and value are inconsistent:" + key + " and " + longKeyDef.getLongKey(newValue)); } - return internalPut(newValue, KeyedLongObjectHash.REPLACE, oldValue).equals(oldValue); + return Objects.equals(internalPut(newValue, KeyedLongObjectHash.REPLACE, oldValue), oldValue); } public synchronized boolean replace(long key, V oldValue, V newValue) { @@ -267,7 +268,7 @@ public synchronized boolean replace(long key, V oldValue, V newValue) { throw new IllegalArgumentException( "key and value are inconsistent:" + key + " and " + longKeyDef.getLongKey(newValue)); } - return internalPut(newValue, KeyedLongObjectHash.REPLACE, oldValue).equals(oldValue); + return Objects.equals(internalPut(newValue, KeyedLongObjectHash.REPLACE, oldValue), oldValue); } private static final int NORMAL = 0; diff --git a/src/main/java/io/deephaven/hash/KeyedObjectHash.java b/src/main/java/io/deephaven/hash/KeyedObjectHash.java index eb68d15..d32a21c 100644 --- a/src/main/java/io/deephaven/hash/KeyedObjectHash.java +++ b/src/main/java/io/deephaven/hash/KeyedObjectHash.java @@ -22,6 +22,7 @@ of the License, or (at your option) any later version. import java.util.Iterator; import java.util.Map; import java.util.NoSuchElementException; +import java.util.Objects; import java.util.Set; import java.util.function.Predicate; @@ -734,7 +735,7 @@ public synchronized boolean replace(K key, V oldValue, V newValue) { throw new IllegalArgumentException( "key and value are inconsistent:" + key + " and " + keyDef.getKey(newValue)); } - return internalPut(newValue, REPLACE, oldValue).equals(oldValue); + return Objects.equals(internalPut(newValue, REPLACE, oldValue), oldValue); } public synchronized boolean add(V value) { diff --git a/src/test/java/io/deephaven/hash/TestKeyedDoubleObjectHashMap.java b/src/test/java/io/deephaven/hash/TestKeyedDoubleObjectHashMap.java index bfb3df6..ba16793 100644 --- a/src/test/java/io/deephaven/hash/TestKeyedDoubleObjectHashMap.java +++ b/src/test/java/io/deephaven/hash/TestKeyedDoubleObjectHashMap.java @@ -98,6 +98,83 @@ public void tearDown() throws Exception { super.tearDown(); } + /** + * Test a matrix of (low) capacity starting conditions and a range of load factors. This test will + * expose starting conditions that are likely to cause problems with the hash table + * implementation. + */ + public void testCapacityAndLoadFactor() { + // Test some very high LF edge cases + testCapacityAndLoadFactor(0, 0.99999999f); + testCapacityAndLoadFactor(1, 0.99999999f); + testCapacityAndLoadFactor(2, 0.99999999f); + testCapacityAndLoadFactor(3, 0.99999999f); + testCapacityAndLoadFactor(4, 0.99999999f); + testCapacityAndLoadFactor(5, 0.99999999f); + + // Test a matrix of starting conditions. + for (float loadFactor = 0.001f; loadFactor < 1.0f; loadFactor += 0.001f) { + for (int capacity = 0; capacity < 100; ++capacity) { + testCapacityAndLoadFactor(capacity, loadFactor); + } + } + } + + private void testCapacityAndLoadFactor(final int capacity, final float loadFactor) { + final KeyedDoubleTestObjectMap m = new KeyedDoubleTestObjectMap(capacity, loadFactor); + + for (int i = 0; i < capacity * 2; ++i) { + if (m.size() >= m.capacity() - 1) { + // remove the first key + final KeyedDoubleTestObject o = m.getByIndex(0); + m.remove(o.getId()); + } + + // add a random key + final long key = random.nextInt(10); + final KeyedDoubleTestObject o = new KeyedDoubleTestObject((double) key); + try { + m.add(o); + } catch (IllegalStateException ex) { + throw new IllegalStateException( + "testAddRemove: capacity = " + capacity + ", loadFactor = " + loadFactor, ex); + } + } + } + + /** + * Reproducer for bug documented in DH-19237, where a normal replace() call incorrectly throws NPE + */ + public void testDH19237() { + final KeyedDoubleTestObjectMap m1 = new KeyedDoubleTestObjectMap(); + KeyedDoubleTestObject o1 = new KeyedDoubleTestObject(1); + KeyedDoubleTestObject o2 = new KeyedDoubleTestObject(1); + + boolean replaced; + + replaced = m1.replace(1, o2, o1); + assertFalse("replace() should not return true when value not found", replaced); + assertSame(m1.get(1), null); // confirm replace failed + + m1.add(o1); + replaced = m1.replace(1, o1, o2); + assertTrue("replace() should return true when value found and replaced", replaced); + assertSame(m1.get(1), o2); + + // Repeat using boxed datatype + final KeyedDoubleTestObjectMap m2 = new KeyedDoubleTestObjectMap(); + final Double d1 = 1.0; + + replaced = m2.replace(d1, o1, o2); + assertFalse("replace() should not return true when value not found", replaced); + assertSame(m2.get(d1), null); // confirm replace failed + + m2.add(o1); + replaced = m2.replace(d1, o2, o1); + assertTrue("replace() should return true when value found and replaced", replaced); + assertSame(m2.get(d1), o1); + } + /* ** tests the unboxed putIfAbsent call */ diff --git a/src/test/java/io/deephaven/hash/TestKeyedIntObjectHashMap.java b/src/test/java/io/deephaven/hash/TestKeyedIntObjectHashMap.java index 6272f88..d861a49 100644 --- a/src/test/java/io/deephaven/hash/TestKeyedIntObjectHashMap.java +++ b/src/test/java/io/deephaven/hash/TestKeyedIntObjectHashMap.java @@ -97,6 +97,83 @@ public void tearDown() throws Exception { super.tearDown(); } + /** + * Test a matrix of (low) capacity starting conditions and a range of load factors. This test will + * expose starting conditions that are likely to cause problems with the hash table + * implementation. + */ + public void testCapacityAndLoadFactor() { + // Test some very high LF edge cases + testCapacityAndLoadFactor(0, 0.99999999f); + testCapacityAndLoadFactor(1, 0.99999999f); + testCapacityAndLoadFactor(2, 0.99999999f); + testCapacityAndLoadFactor(3, 0.99999999f); + testCapacityAndLoadFactor(4, 0.99999999f); + testCapacityAndLoadFactor(5, 0.99999999f); + + // Test a matrix of starting conditions. + for (float loadFactor = 0.001f; loadFactor < 1.0f; loadFactor += 0.001f) { + for (int capacity = 0; capacity < 100; ++capacity) { + testCapacityAndLoadFactor(capacity, loadFactor); + } + } + } + + private void testCapacityAndLoadFactor(final int capacity, final float loadFactor) { + final KeyedIntTestObjectMap m = new KeyedIntTestObjectMap(capacity, loadFactor); + + for (int i = 0; i < capacity * 2; ++i) { + if (m.size() >= m.capacity() - 1) { + // remove the first key + final KeyedIntTestObject o = m.getByIndex(0); + m.remove(o.getId()); + } + + // add a random key + final int key = random.nextInt(10); + final KeyedIntTestObject o = new KeyedIntTestObject(key); + try { + m.add(o); + } catch (IllegalStateException ex) { + throw new IllegalStateException( + "testAddRemove: capacity = " + capacity + ", loadFactor = " + loadFactor, ex); + } + } + } + + /** + * Reproducer for bug documented in DH-19237, where a normal replace() call incorrectly throws NPE + */ + public void testDH19237() { + final KeyedIntTestObjectMap m1 = new KeyedIntTestObjectMap(); + KeyedIntTestObject o1 = new KeyedIntTestObject(1); + KeyedIntTestObject o2 = new KeyedIntTestObject(1); + + boolean replaced; + + replaced = m1.replace(1, o2, o1); + assertFalse("replace() should not return true when value not found", replaced); + assertSame(m1.get(1), null); // confirm replace failed + + m1.add(o1); + replaced = m1.replace(1, o1, o2); + assertTrue("replace() should return true when value found and replaced", replaced); + assertSame(m1.get(1), o2); + + // Repeat using boxed datatype + final KeyedIntTestObjectMap m2 = new KeyedIntTestObjectMap(); + final Integer i1 = 1; + + replaced = m2.replace(i1, o1, o2); + assertFalse("replace() should not return true when value not found", replaced); + assertSame(m2.get(i1), null); // confirm replace failed + + m2.add(o1); + replaced = m2.replace(i1, o2, o1); + assertTrue("replace() should return true when value found and replaced", replaced); + assertSame(m2.get(i1), o1); + } + /* ** tests the unboxed putIfAbsent call */ diff --git a/src/test/java/io/deephaven/hash/TestKeyedLongObjectHashMap.java b/src/test/java/io/deephaven/hash/TestKeyedLongObjectHashMap.java index 70ca19a..0d2d01c 100644 --- a/src/test/java/io/deephaven/hash/TestKeyedLongObjectHashMap.java +++ b/src/test/java/io/deephaven/hash/TestKeyedLongObjectHashMap.java @@ -100,6 +100,83 @@ public void tearDown() throws Exception { super.tearDown(); } + /** + * Test a matrix of (low) capacity starting conditions and a range of load factors. This test will + * expose starting conditions that are likely to cause problems with the hash table + * implementation. + */ + public void testCapacityAndLoadFactor() { + // Test some very high LF edge cases + testCapacityAndLoadFactor(0, 0.99999999f); + testCapacityAndLoadFactor(1, 0.99999999f); + testCapacityAndLoadFactor(2, 0.99999999f); + testCapacityAndLoadFactor(3, 0.99999999f); + testCapacityAndLoadFactor(4, 0.99999999f); + testCapacityAndLoadFactor(5, 0.99999999f); + + // Test a matrix of starting conditions. + for (float loadFactor = 0.001f; loadFactor < 1.0f; loadFactor += 0.001f) { + for (int capacity = 0; capacity < 100; ++capacity) { + testCapacityAndLoadFactor(capacity, loadFactor); + } + } + } + + private void testCapacityAndLoadFactor(final int capacity, final float loadFactor) { + final KeyedLongTestObjectMap m = new KeyedLongTestObjectMap(capacity, loadFactor); + + for (int i = 0; i < capacity * 2; ++i) { + if (m.size() >= m.capacity() - 1) { + // remove the first key + final KeyedLongTestObject o = m.getByIndex(0); + m.remove(o.getId()); + } + + // add a random key + final long key = random.nextInt(10); + final KeyedLongTestObject o = new KeyedLongTestObject(key); + try { + m.add(o); + } catch (IllegalStateException ex) { + throw new IllegalStateException( + "testAddRemove: capacity = " + capacity + ", loadFactor = " + loadFactor, ex); + } + } + } + + /** + * Reproducer for bug documented in DH-19237, where a normal replace() call incorrectly throws NPE + */ + public void testDH19237() { + final KeyedLongTestObjectMap m1 = new KeyedLongTestObjectMap(); + KeyedLongTestObject o1 = new KeyedLongTestObject(1); + KeyedLongTestObject o2 = new KeyedLongTestObject(1); + + boolean replaced; + + replaced = m1.replace(1, o2, o1); + assertFalse("replace() should not return true when value not found", replaced); + assertSame(m1.get(1), null); // confirm replace failed + + m1.add(o1); + replaced = m1.replace(1, o1, o2); + assertTrue("replace() should return true when value found and replaced", replaced); + assertSame(m1.get(1), o2); + + // Repeat using boxed datatype + final KeyedLongTestObjectMap m2 = new KeyedLongTestObjectMap(); + final Long l1 = 1L; + + replaced = m2.replace(l1, o1, o2); + assertFalse("replace() should not return true when value not found", replaced); + assertSame(m2.get(l1), null); // confirm replace failed + + m2.add(o1); + replaced = m2.replace(l1, o2, o1); + assertTrue("replace() should return true when value found and replaced", replaced); + assertSame(m2.get(l1), o1); + } + /* ** tests the unboxed putIfAbsent call */ diff --git a/src/test/java/io/deephaven/hash/TestKeyedObjectHashMap.java b/src/test/java/io/deephaven/hash/TestKeyedObjectHashMap.java index b1d573f..688a5ae 100644 --- a/src/test/java/io/deephaven/hash/TestKeyedObjectHashMap.java +++ b/src/test/java/io/deephaven/hash/TestKeyedObjectHashMap.java @@ -88,6 +88,70 @@ public void tearDown() throws Exception { super.tearDown(); } + /** + * Test a matrix of (low) capacity starting conditions and a range of load factors. This test will + * expose starting conditions that are likely to cause problems with the hash table + * implementation. + */ + public void testCapacityAndLoadFactor() { + // Test some very high LF edge cases + testCapacityAndLoadFactor(0, 0.99999999f); + testCapacityAndLoadFactor(1, 0.99999999f); + testCapacityAndLoadFactor(2, 0.99999999f); + testCapacityAndLoadFactor(3, 0.99999999f); + testCapacityAndLoadFactor(4, 0.99999999f); + testCapacityAndLoadFactor(5, 0.99999999f); + + // Test a matrix of starting conditions. + for (float loadFactor = 0.001f; loadFactor < 1.0f; loadFactor += 0.001f) { + for (int capacity = 0; capacity < 100; ++capacity) { + testCapacityAndLoadFactor(capacity, loadFactor); + } + } + } + + private void testCapacityAndLoadFactor(final int capacity, final float loadFactor) { + final KeyedTestObjectMap m = new KeyedTestObjectMap(capacity, loadFactor); + + for (int i = 0; i < capacity * 2; ++i) { + if (m.size() >= m.capacity() - 1) { + // remove the first key + final KeyedTestObject o = m.getByIndex(0); + m.remove(o.getId()); + } + + // add a random key + final long key = random.nextInt(10); + final KeyedTestObject o = new KeyedTestObject(Long.toString(key)); + try { + m.add(o); + } catch (IllegalStateException ex) { + throw new IllegalStateException( + "testAddRemove: capacity = " + capacity + ", loadFactor = " + loadFactor, ex); + } + } + } + + /** + * Reproducer for bug documented in DH-19237, where a normal replace() call incorrectly throws NPE + */ + public void testDH19237() { + final KeyedTestObjectMap m1 = new KeyedTestObjectMap(); + KeyedTestObject o1 = new KeyedTestObject("1"); + KeyedTestObject o2 = new KeyedTestObject("1"); + + boolean replaced; + + replaced = m1.replace("1", o2, o1); + assertFalse("replace() should not return true when value not found", replaced); + assertSame(m1.get("1"), null); // confirm replace failed + + m1.add(o1); + replaced = m1.replace("1", o1, o2); + assertTrue("replace() should return true when value found and replaced", replaced); + assertSame(m1.get("1"), o2); + } + /** * If the test subject is an indexable map, make sure the getByIndex method returns identical * objects