fix: Multiple bugfixes (DH-19237, DH-20195)

src/main/java/io/deephaven/hash/KHash.java

@@ -185,8 +185,9 @@ protected int setUp(int initialCapacity) {
    * @param capacity an <code>int</code> 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
   }
 

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;

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;

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;

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) {

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
    */

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
    */

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
    */