HBASE-1347 HTable.incrementColumnValue does not take negative 'amount'

git-svn-id: https://svn.apache.org/repos/asf/hadoop/hbase/trunk@770991 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -99,6 +99,8 @@ Release 0.20.0 - Unreleased
                (Evgeny Ryabitskiy via Stack)
    HBASE-1322  hbase-1234 broke TestAtomicIncrement; fix and reenable
                (Evgeny Ryabitskiy and Ryan Rawson via Stack)
+   HBASE-1347  HTable.incrementColumnValue does not take negative 'amount'
+               (Evgeny Ryabitskiy via Stack)
 
   IMPROVEMENTS
    HBASE-1089  Add count of regions on filesystem to master UI; add percentage

src/java/org/apache/hadoop/hbase/regionserver/HRegion.java

@@ -2701,7 +2701,8 @@ public class HRegion implements HConstants {
           "/"+ Bytes.toString(column));
         value = Bytes.toBytes(amount);
       } else {
-        value = incrementBytes(value, amount);
+        if (amount == 0) return Bytes.toLong(value);
+        value = Bytes.incrementBytes(value, amount);
       }
 
       BatchUpdate b = new BatchUpdate(row, ts);
@@ -2713,34 +2714,4 @@ public class HRegion implements HConstants {
       releaseRowLock(lid);
     }
   }
-
+}
-  private byte [] incrementBytes(byte[] value, long amount) throws IOException {
-    // Hopefully this doesn't happen too often.
-    if (value.length < Bytes.SIZEOF_LONG) {
-      byte [] newvalue = new byte[Bytes.SIZEOF_LONG];
-      System.arraycopy(value, 0, newvalue, newvalue.length - value.length,
-        value.length);
-      value = newvalue;
-    } else if (value.length > Bytes.SIZEOF_LONG) {
-      throw new DoNotRetryIOException("Increment Bytes - value too big: " +
-        value.length);
-    }
-    return binaryIncrement(value, amount);
-  }
-  
-  private byte [] binaryIncrement(byte [] value, long amount) {
-    for(int i=0;i<value.length;i++) {
-      int cur = (int)(amount >> (8 * i)) % 256;
-      int val = value[value.length-i-1] & 0xff;
-      int total = cur + val;
-      if(total > 255) {
-        amount += ((long)256 << (8 * i));
-        total %= 256;
-      }
-      value[value.length-i-1] = (byte)total;
-      amount = (amount >> (8 * (i + 1))) << (8 * (i + 1));
-      if(amount == 0) return value;
-    }
-    return value;
-  }
-}

src/java/org/apache/hadoop/hbase/util/Bytes.java

@@ -27,6 +27,7 @@ import java.nio.ByteBuffer;
 import java.util.Comparator;
 import java.math.BigInteger;
 
+import org.apache.hadoop.hbase.DoNotRetryIOException;
 import org.apache.hadoop.hbase.HConstants;
 import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
 import org.apache.hadoop.io.RawComparator;
@@ -909,5 +910,90 @@ public class Bytes {
         return mid;
     }
     return - (low+1);
-  }      
+  }
-}
+
+  /**
+   * Bytewise binary increment/deincrement of long contained in byte array
+   * on given amount.
+   * 
+   * @param value - array of bytes containing long (length <= SIZEOF_LONG)
+   * @param amount value will be incremented on (deincremented if negative)
+   * @return array of bytes containing incremented long (length == SIZEOF_LONG)
+   * @throws IOException - if value.length > SIZEOF_LONG
+   */
+  public static byte [] incrementBytes(byte[] value, long amount) 
+  throws IOException {
+    byte[] val = value;
+    if (val.length < SIZEOF_LONG) {
+      // Hopefully this doesn't happen too often.
+      byte [] newvalue;
+      if (val[0] < 0) {
+        byte [] negativeValue = {-1, -1, -1, -1, -1, -1, -1, -1};
+        newvalue = negativeValue;
+      } else {
+        newvalue = new byte[SIZEOF_LONG];
+      }
+      System.arraycopy(val, 0, newvalue, newvalue.length - val.length,
+        val.length);
+      val = newvalue;
+    } else if (val.length > SIZEOF_LONG) {
+      throw new DoNotRetryIOException("Increment Bytes - value too big: " +
+        val.length);
+    }
+    if(amount == 0) return val;
+    if(val[0] < 0){
+      return binaryIncrementNeg(val, amount);
+    }
+    return binaryIncrementPos(val, amount);
+  }
+
+  /* increment/deincrement for positive value */
+  private static byte [] binaryIncrementPos(byte [] value, long amount) {
+    long amo = amount;
+    int sign = 1;
+    if (amount < 0) {
+      amo = -amount;
+      sign = -1;
+    } 
+    for(int i=0;i<value.length;i++) {
+      int cur = ((int)amo % 256) * sign;
+      amo = (amo >> 8);
+      int val = value[value.length-i-1] & 0x0ff;
+      int total = val + cur;
+      if(total > 255) {
+        amo += sign;
+        total %= 256;
+      } else if (total < 0) {
+        amo -= sign;
+      }
+      value[value.length-i-1] = (byte)total;
+      if (amo == 0) return value;
+    }
+    return value;
+  }
+
+  /* increment/deincrement for negative value */
+  private static byte [] binaryIncrementNeg(byte [] value, long amount) {
+    long amo = amount;
+    int sign = 1;
+    if (amount < 0) {
+      amo = -amount;
+      sign = -1;
+    }
+    for(int i=0;i<value.length;i++) {
+      int cur = ((int)amo % 256) * sign;
+      amo = (amo >> 8);
+      int val = ((~value[value.length-i-1]) & 0x0ff) + 1;
+      int total = cur - val;
+      if(total >= 0) {
+        amo += sign;
+      } else if (total < -256) {
+        amo -= sign;
+        total %= 256;
+      }
+      value[value.length-i-1] = (byte)total;
+      if (amo == 0) return value;
+    }
+    return value;
+  }
+}

src/test/org/apache/hadoop/hbase/regionserver/TestAtomicIncrement.java

@@ -88,6 +88,9 @@ public class TestAtomicIncrement extends HBaseClusterTestCase {
       assertEquals(3L, table.incrementColumnValue(row, column, 1));
 
       assertEquals(-2L, table.incrementColumnValue(row, column, -5));
+      assertEquals(-502L, table.incrementColumnValue(row, column, -500));
+      assertEquals(1500L, table.incrementColumnValue(row, column, 2002));
+      assertEquals(1501L, table.incrementColumnValue(row, column, 1));
 
       row = Bytes.toBytes("foo3");
       byte[] value2 = {1,2,3,4,5,6,7,8,9};

src/test/org/apache/hadoop/hbase/util/TestBytes.java

@@ -19,6 +19,7 @@
  */
 package org.apache.hadoop.hbase.util;
 
+import java.io.IOException;
 import java.util.Arrays;
 
 import junit.framework.TestCase;
@@ -113,4 +114,40 @@ public class TestBytes extends TestCase {
     assertEquals(5, Bytes.binarySearch(arr, key3, 1, 1,
       Bytes.BYTES_RAWCOMPARATOR));
   }
-}
+  
+  public void testIncrementBytes() throws IOException {
+
+    assertTrue(checkTestIncrementBytes(10, 1));
+    assertTrue(checkTestIncrementBytes(12, 123435445));
+    assertTrue(checkTestIncrementBytes(124634654, 1));
+    assertTrue(checkTestIncrementBytes(10005460, 5005645));
+    assertTrue(checkTestIncrementBytes(1, -1));
+    assertTrue(checkTestIncrementBytes(10, -1));
+    assertTrue(checkTestIncrementBytes(10, -5));
+    assertTrue(checkTestIncrementBytes(1005435000, -5));
+    assertTrue(checkTestIncrementBytes(10, -43657655));
+    assertTrue(checkTestIncrementBytes(-1, 1));
+    assertTrue(checkTestIncrementBytes(-26, 5034520));
+    assertTrue(checkTestIncrementBytes(-10657200, 5));
+    assertTrue(checkTestIncrementBytes(-12343250, 45376475));
+    assertTrue(checkTestIncrementBytes(-10, -5));
+    assertTrue(checkTestIncrementBytes(-12343250, -5));
+    assertTrue(checkTestIncrementBytes(-12, -34565445));
+    assertTrue(checkTestIncrementBytes(-1546543452, -34565445));
+  }
+  
+  private static boolean checkTestIncrementBytes(long val, long amount) 
+  throws IOException {
+    byte[] value = Bytes.toBytes(val);
+    byte [] testValue = {-1, -1, -1, -1, -1, -1, -1, -1};
+    if (value[0] > 0) {
+      testValue = new byte[Bytes.SIZEOF_LONG];
+    }
+    System.arraycopy(value, 0, testValue, testValue.length - value.length,
+        value.length);
+
+    long incrementResult = Bytes.toLong(Bytes.incrementBytes(value, amount));
+
+    return (Bytes.toLong(testValue) + amount) == incrementResult;
+  }
+}