Merge pull request #2 from lbooker42/nightly/DH-18265-lab-kohash-bugfix

fix: DH-18265 - correct bugs in `replace()` function

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

@@ -251,19 +251,25 @@ public synchronized V replace(double key, V value) {
   }
 
   public synchronized boolean replace(Double key, V oldValue, V newValue) {
+    if (oldValue == null) {
+      throw new NullPointerException("oldValue is null, but this map cannot hold null values");
+    }
     if (!doubleKeyDef.equalDoubleKey(key, newValue)) {
       throw new IllegalArgumentException(
           "key and value are inconsistent:" + key + " and " + doubleKeyDef.getDoubleKey(newValue));
     }
-    return internalPut(newValue, KeyedDoubleObjectHash.REPLACE, oldValue) != null;
+    return internalPut(newValue, KeyedDoubleObjectHash.REPLACE, oldValue).equals(oldValue);
   }
 
   public synchronized boolean replace(double key, V oldValue, V newValue) {
+    if (oldValue == null) {
+      throw new NullPointerException("oldValue is null, but this map cannot hold null values");
+    }
     if (!doubleKeyDef.equalDoubleKey(key, newValue)) {
       throw new IllegalArgumentException(
           "key and value are inconsistent:" + key + " and " + doubleKeyDef.getDoubleKey(newValue));
     }
-    return internalPut(newValue, KeyedDoubleObjectHash.REPLACE, oldValue) != null;
+    return internalPut(newValue, KeyedDoubleObjectHash.REPLACE, oldValue).equals(oldValue);
   }
 
   private static final int NORMAL = 0;
@@ -307,7 +313,8 @@ protected V internalPut(V value, int mode, V oldValue) {
           }
           return null;
         } else if (candidate != DELETED && doubleKeyDef.equalDoubleKey(key, candidate)) {
-          if (mode != KeyedDoubleObjectHash.IF_ABSENT) {
+          if (mode != KeyedDoubleObjectHash.IF_ABSENT
+              && (oldValue == null || candidate.equals(oldValue))) {
             state[index] = value;
             _indexableList = null;
           }

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

@@ -248,19 +248,25 @@ public synchronized V replace(int key, V value) {
   }
 
   public synchronized boolean replace(Integer key, V oldValue, V newValue) {
+    if (oldValue == null) {
+      throw new NullPointerException("oldValue is null, but this map cannot hold null values");
+    }
     if (!intKeyDef.equalIntKey(key, newValue)) {
       throw new IllegalArgumentException(
           "key and value are inconsistent:" + key + " and " + intKeyDef.getIntKey(newValue));
     }
-    return internalPut(newValue, KeyedIntObjectHash.REPLACE, oldValue) != null;
+    return internalPut(newValue, KeyedIntObjectHash.REPLACE, oldValue).equals(oldValue);
   }
 
   public synchronized boolean replace(int key, V oldValue, V newValue) {
+    if (oldValue == null) {
+      throw new NullPointerException("oldValue is null, but this map cannot hold null values");
+    }
     if (!intKeyDef.equalIntKey(key, newValue)) {
       throw new IllegalArgumentException(
           "key and value are inconsistent:" + key + " and " + intKeyDef.getIntKey(newValue));
     }
-    return internalPut(newValue, KeyedIntObjectHash.REPLACE, oldValue) != null;
+    return internalPut(newValue, KeyedIntObjectHash.REPLACE, oldValue).equals(oldValue);
   }
 
   private static final int NORMAL = 0;
@@ -304,7 +310,8 @@ protected V internalPut(V value, int mode, V oldValue) {
           }
           return null;
         } else if (candidate != DELETED && intKeyDef.equalIntKey(key, candidate)) {
-          if (mode != KeyedIntObjectHash.IF_ABSENT) {
+          if (mode != KeyedIntObjectHash.IF_ABSENT
+              && (oldValue == null || candidate.equals(oldValue))) {
             state[index] = value;
             _indexableList = null;
           }

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

@@ -249,19 +249,25 @@ public synchronized V replace(long key, V value) {
   }
 
   public synchronized boolean replace(Long key, V oldValue, V newValue) {
+    if (oldValue == null) {
+      throw new NullPointerException("oldValue is null, but this map cannot hold null values");
+    }
     if (!longKeyDef.equalLongKey(key, newValue)) {
       throw new IllegalArgumentException(
           "key and value are inconsistent:" + key + " and " + longKeyDef.getLongKey(newValue));
     }
-    return internalPut(newValue, KeyedLongObjectHash.REPLACE, oldValue) != null;
+    return internalPut(newValue, KeyedLongObjectHash.REPLACE, oldValue).equals(oldValue);
   }
 
   public synchronized boolean replace(long key, V oldValue, V newValue) {
+    if (oldValue == null) {
+      throw new NullPointerException("oldValue is null, but this map cannot hold null values");
+    }
     if (!longKeyDef.equalLongKey(key, newValue)) {
       throw new IllegalArgumentException(
           "key and value are inconsistent:" + key + " and " + longKeyDef.getLongKey(newValue));
     }
-    return internalPut(newValue, KeyedLongObjectHash.REPLACE, oldValue) != null;
+    return internalPut(newValue, KeyedLongObjectHash.REPLACE, oldValue).equals(oldValue);
   }
 
   private static final int NORMAL = 0;
@@ -305,7 +311,8 @@ protected V internalPut(V value, int mode, V oldValue) {
           }
           return null;
         } else if (candidate != DELETED && longKeyDef.equalLongKey(key, candidate)) {
-          if (mode != KeyedLongObjectHash.IF_ABSENT) {
+          if (mode != KeyedLongObjectHash.IF_ABSENT
+              && (oldValue == null || candidate.equals(oldValue))) {
             state[index] = value;
             _indexableList = null;
           }

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

@@ -700,11 +700,14 @@ public synchronized V replace(K key, V value) {
   }
 
   public synchronized boolean replace(K key, V oldValue, V newValue) {
+    if (oldValue == null) {
+      throw new NullPointerException("oldValue is null, but this map cannot hold null values");
+    }
     if (!keyDef.equalKey(key, newValue)) {
       throw new IllegalArgumentException(
           "key and value are inconsistent:" + key + " and " + keyDef.getKey(newValue));
     }
-    return internalPut(newValue, REPLACE, oldValue) != null;
+    return internalPut(newValue, REPLACE, oldValue).equals(oldValue);
   }
 
   public synchronized boolean add(V value) {
@@ -761,7 +764,7 @@ protected V internalPut(V value, int mode, V oldValue) {
           }
           return null;
         } else if (candidate != DELETED && keyDef.equalKey(key, candidate)) {
-          if (mode != IF_ABSENT) {
+          if (mode != IF_ABSENT && (oldValue == null || candidate.equals(oldValue))) {
             state[index] = value;
             _indexableList = null;
           }

src/test/java/io/deephaven/hash/KeyedDoubleTestObject.java

@@ -0,0 +1,93 @@
+/*
+ Copyright (C) 2021 Deephaven Data Labs (https://deephaven.io).
+
+ This program is free software: you can redistribute it and/or modify it under the terms of the
+ GNU Lesser General Public License as published by the Free Software Foundation, either version 3
+ of the License, or (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY;
+ without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU Lesser General Public License for more details.
+*/
+package io.deephaven.hash;
+
+/** Test class. */
+class KeyedDoubleTestObject {
+  private final double id;
+
+  public KeyedDoubleTestObject(double id) {
+    this.id = id;
+  }
+
+  public double getId() {
+    return id;
+  }
+
+  public boolean equals(Object other) {
+    return other instanceof KeyedDoubleTestObject && id == ((KeyedDoubleTestObject) other).id;
+  }
+
+  public int hashCode() {
+    return ~Double.hashCode(id); // do something different that gnu.trove.HashFunctions.hash(double)
+  }
+
+  public String toString() {
+    return "[KeyedDoubleTestObject:" + id + "]";
+  }
+
+  public static final KeyedDoubleObjectKey<KeyedDoubleTestObject> keyDef =
+      new KeyedDoubleObjectKey<KeyedDoubleTestObject>() {
+        public Double getKey(KeyedDoubleTestObject v) {
+          return v.getId();
+        }
+
+        public double getDoubleKey(KeyedDoubleTestObject v) {
+          return v.getId();
+        }
+
+        public int hashKey(Double k) {
+          return k.hashCode();
+        }
+
+        @Override
+        public boolean equalKey(Double k, KeyedDoubleTestObject v) {
+          return v.getId() == k;
+        }
+
+        public int hashDoubleKey(double k) {
+          return Double.hashCode(k);
+        }
+
+        public boolean equalDoubleKey(double k, KeyedDoubleTestObject v) {
+          return v.getId() == k;
+        }
+      };
+
+  public static final KeyedIntObjectHash.ValueFactory<KeyedDoubleTestObject> factory =
+      new KeyedIntObjectHash.ValueFactory<KeyedDoubleTestObject>() {
+        public KeyedDoubleTestObject newValue(Integer key) {
+          return new KeyedDoubleTestObject(key);
+        }
+
+        public KeyedDoubleTestObject newValue(int key) {
+          return new KeyedDoubleTestObject(key);
+        }
+      };
+
+  // for intrusive chained maps
+
+  private KeyedDoubleTestObject next;
+
+  public static final IntrusiveChainedHashAdapter<KeyedDoubleTestObject> adapter =
+      new IntrusiveChainedHashAdapter<KeyedDoubleTestObject>() {
+        @Override
+        public KeyedDoubleTestObject getNext(KeyedDoubleTestObject self) {
+          return self.next;
+        }
+
+        @Override
+        public void setNext(KeyedDoubleTestObject self, KeyedDoubleTestObject next) {
+          self.next = next;
+        }
+      };
+}

src/test/java/io/deephaven/hash/KeyedDoubleTestObjectMap.java

@@ -0,0 +1,31 @@
+/*
+ Copyright (C) 2021 Deephaven Data Labs (https://deephaven.io).
+
+ This program is free software: you can redistribute it and/or modify it under the terms of the
+ GNU Lesser General Public License as published by the Free Software Foundation, either version 3
+ of the License, or (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY;
+ without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU Lesser General Public License for more details.
+*/
+package io.deephaven.hash;
+
+/** Instantiation of KeyedDoubleObjectHashMap on the test class. */
+class KeyedDoubleTestObjectMap extends KeyedDoubleObjectHashMap<KeyedDoubleTestObject> {
+  public KeyedDoubleTestObjectMap() {
+    super(KeyedDoubleTestObject.keyDef);
+  }
+
+  public KeyedDoubleTestObjectMap(int initialCapacity) {
+    super(initialCapacity, KeyedDoubleTestObject.keyDef);
+  }
+
+  public KeyedDoubleTestObjectMap(int initialCapacity, float loadFactor) {
+    super(initialCapacity, loadFactor, KeyedDoubleTestObject.keyDef);
+  }
+
+  public final double getId(KeyedDoubleTestObject obj) {
+    return obj.getId();
+  }
+}