diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CellFlatMap.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CellFlatMap.java index 6d26785f9ea..c83a382d670 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CellFlatMap.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CellFlatMap.java @@ -28,6 +28,8 @@ import java.util.Map; import java.util.NavigableSet; import java.util.NavigableMap; import java.util.Set; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; /** @@ -42,7 +44,7 @@ import java.util.Set; */ @InterfaceAudience.Private public abstract class CellFlatMap implements NavigableMap { - + private static final Log LOG = LogFactory.getLog(CellFlatMap.class); private final Comparator comparator; protected int minCellIdx = 0; // the index of the minimal cell (for sub-sets) protected int maxCellIdx = 0; // the index of the cell after the maximal cell (for sub-sets) @@ -88,8 +90,9 @@ public abstract class CellFlatMap implements NavigableMap { if (compareRes == 0) { return mid; // 0 means equals. We found the key } - - if (compareRes < 0) { + // Key not found. Check the comparison results; reverse the meaning of + // the comparison in case the order is descending (using XOR) + if ((compareRes < 0) ^ descending) { // midCell is less than needle so we need to look at farther up begin = mid + 1; } else { @@ -101,37 +104,37 @@ public abstract class CellFlatMap implements NavigableMap { return (-1 * begin)-1; } - /* Get the index of the given anchor key for creating subsequent set. - ** It doesn't matter whether the given key exists in the set or not. - ** - ** taking into consideration whether - ** the key should be inclusive or exclusive */ + /** + * Get the index of the given anchor key for creating subsequent set. + * It doesn't matter whether the given key exists in the set or not. + * taking into consideration whether + * the key should be inclusive or exclusive. + */ private int getValidIndex(Cell key, boolean inclusive, boolean tail) { - int index = find(key); - int result = -1; + final int index = find(key); + // get the valid (positive) insertion point from the output of the find() method + int insertionPoint = index < 0 ? ~index : index; - // if the key is found and to be included, for all possibilities, the answer is the found index - if (index >= 0 && inclusive) result = index; - - // The compliment Operator (~) converts the returned insertion point to the real one - if (index<0) result = ~index; - - if (tail && result==-1) { - if (index >= 0 && !inclusive) - result = (descending) ? index - 1 : index + 1; - } else if (result==-1) { - if (index >= 0 && !inclusive) - result = (descending) ? index + 1 : index - 1; + // correct the insertion point in case the given anchor key DOES EXIST in the set + if (index >= 0) { + if ( descending && !(tail ^ inclusive)) { + // for the descending case + // if anchor for head set (tail=false) AND anchor is not inclusive -> move the insertion pt + // if anchor for tail set (tail=true) AND the keys is inclusive -> move the insertion point + // because the end index of a set is the index of the cell after the maximal cell + insertionPoint += 1; + } else if ( !descending && (tail ^ inclusive)) { + // for the ascending case + // if anchor for head set (tail=false) AND anchor is inclusive -> move the insertion point + // because the end index of a set is the index of the cell after the maximal cell + // if anchor for tail set (tail=true) AND the keys is not inclusive -> move the insertion pt + insertionPoint += 1; + } } - - if (result < minCellIdx || result > maxCellIdx) { - throw new IllegalArgumentException("Index " + result + " (initial index " + index + ") " - + " out of boundary, when looking for key " + key + ". The minCellIdx is " + minCellIdx - + " and the maxCellIdx is " + maxCellIdx + ". Finally, descending? " + descending - + " and was the key requested inclusively? " + inclusive); - } - return result; - } + // insert the insertion point into the valid range, + // as we may enlarge it too much in the above correction + return Math.min(Math.max(insertionPoint, minCellIdx), maxCellIdx); +} @Override public Comparator comparator() { @@ -155,27 +158,31 @@ public abstract class CellFlatMap implements NavigableMap { boolean fromInclusive, Cell toKey, boolean toInclusive) { - int toIndex = getValidIndex(toKey, toInclusive, false); - int fromIndex = (getValidIndex(fromKey, fromInclusive, true)); - - if (fromIndex > toIndex) { - throw new IllegalArgumentException("Inconsistent range, when looking from " - + fromKey + " to " + toKey); + final int lessCellIndex = getValidIndex(fromKey, fromInclusive, true); + final int greaterCellIndex = getValidIndex(toKey, toInclusive, false); + if (descending) { + return createSubCellFlatMap(greaterCellIndex, lessCellIndex, descending); + } else { + return createSubCellFlatMap(lessCellIndex, greaterCellIndex, descending); } - return createSubCellFlatMap(fromIndex, toIndex+1, descending); } @Override public NavigableMap headMap(Cell toKey, boolean inclusive) { - int index = getValidIndex(toKey, inclusive, false); - // "+1" because the max index is one after the true index - return createSubCellFlatMap(minCellIdx, index+1, descending); + if (descending) { + return createSubCellFlatMap(getValidIndex(toKey, inclusive, false), maxCellIdx, descending); + } else { + return createSubCellFlatMap(minCellIdx, getValidIndex(toKey, inclusive, false), descending); + } } @Override public NavigableMap tailMap(Cell fromKey, boolean inclusive) { - int index = (getValidIndex(fromKey, inclusive, true)); - return createSubCellFlatMap(index, maxCellIdx, descending); + if (descending) { + return createSubCellFlatMap(minCellIdx, getValidIndex(fromKey, inclusive, true), descending); + } else { + return createSubCellFlatMap(getValidIndex(fromKey, inclusive, true), maxCellIdx, descending); + } } @Override @@ -403,7 +410,7 @@ public abstract class CellFlatMap implements NavigableMap { private final class CellFlatMapCollection implements Collection { @Override - public int size() { + public int size() { return CellFlatMap.this.size(); } @@ -466,8 +473,5 @@ public abstract class CellFlatMap implements NavigableMap { public boolean retainAll(Collection collection) { throw new UnsupportedOperationException(); } - - } - } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CellSet.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CellSet.java index 6f70bac64c3..531bf66db87 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CellSet.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CellSet.java @@ -18,6 +18,7 @@ */ package org.apache.hadoop.hbase.regionserver; +import com.google.common.annotations.VisibleForTesting; import java.util.Collection; import java.util.Comparator; import java.util.Iterator; @@ -54,6 +55,11 @@ public class CellSet implements NavigableSet { this.delegatee = m; } + @VisibleForTesting + NavigableMap getDelegatee() { + return delegatee; + } + public Cell ceiling(Cell e) { throw new UnsupportedOperationException("Not implemented"); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCellFlatSet.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCellFlatSet.java index cd5788e8d53..2905a5b3372 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCellFlatSet.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCellFlatSet.java @@ -18,31 +18,37 @@ */ package org.apache.hadoop.hbase.regionserver; +import java.util.Iterator; +import java.util.NavigableMap; +import java.util.NavigableSet; +import java.util.SortedSet; import junit.framework.TestCase; import org.apache.hadoop.conf.Configuration; -import org.apache.hadoop.hbase.*; +import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.CellComparator; +import org.apache.hadoop.hbase.CellUtil; +import org.apache.hadoop.hbase.KeyValue; import org.apache.hadoop.hbase.testclassification.RegionServerTests; import org.apache.hadoop.hbase.testclassification.SmallTests; import org.apache.hadoop.hbase.util.Bytes; +import org.junit.Test; import org.junit.experimental.categories.Category; -import java.util.Iterator; -import java.util.NavigableMap; -import java.util.SortedSet; -import static org.junit.Assert.assertTrue; + @Category({RegionServerTests.class, SmallTests.class}) public class TestCellFlatSet extends TestCase { - private static final int NUM_OF_CELLS = 4; - - private Cell cells[]; - private CellArrayMap cbOnHeap; - - private final static Configuration conf = new Configuration(); + private Cell ascCells[]; + private CellArrayMap ascCbOnHeap; + private Cell descCells[]; + private CellArrayMap descCbOnHeap; + private final static Configuration CONF = new Configuration(); private HeapMemStoreLAB mslab; + private KeyValue lowerOuterCell; + private KeyValue upperOuterCell; - + @Override protected void setUp() throws Exception { super.setUp(); @@ -60,22 +66,78 @@ public class TestCellFlatSet extends TestCase { final KeyValue kv2 = new KeyValue(two, f, q, 20, v); final KeyValue kv3 = new KeyValue(three, f, q, 30, v); final KeyValue kv4 = new KeyValue(four, f, q, 40, v); - - cells = new Cell[] {kv1,kv2,kv3,kv4}; - cbOnHeap = new CellArrayMap(CellComparator.COMPARATOR,cells,0,NUM_OF_CELLS,false); - - conf.setBoolean(SegmentFactory.USEMSLAB_KEY, true); - conf.setFloat(MemStoreChunkPool.CHUNK_POOL_MAXSIZE_KEY, 0.2f); + lowerOuterCell = new KeyValue(Bytes.toBytes(10), f, q, 10, v); + upperOuterCell = new KeyValue(Bytes.toBytes(50), f, q, 10, v); + ascCells = new Cell[] {kv1,kv2,kv3,kv4}; + ascCbOnHeap = new CellArrayMap(CellComparator.COMPARATOR,ascCells,0,NUM_OF_CELLS,false); + descCells = new Cell[] {kv4,kv3,kv2,kv1}; + descCbOnHeap = new CellArrayMap(CellComparator.COMPARATOR,descCells,0,NUM_OF_CELLS,true); + CONF.setBoolean(SegmentFactory.USEMSLAB_KEY, true); + CONF.setFloat(MemStoreChunkPool.CHUNK_POOL_MAXSIZE_KEY, 0.2f); MemStoreChunkPool.chunkPoolDisabled = false; - mslab = new HeapMemStoreLAB(conf); + mslab = new HeapMemStoreLAB(CONF); } /* Create and test CellSet based on CellArrayMap */ public void testCellBlocksOnHeap() throws Exception { - CellSet cs = new CellSet(cbOnHeap); + CellSet cs = new CellSet(ascCbOnHeap); testCellBlocks(cs); testIterators(cs); } + @Test + public void testAsc() throws Exception { + CellSet ascCs = new CellSet(ascCbOnHeap); + assertEquals(NUM_OF_CELLS, ascCs.size()); + testSubSet(ascCs); + } + @Test + public void testDesc() throws Exception { + CellSet descCs = new CellSet(descCbOnHeap); + assertEquals(NUM_OF_CELLS, descCs.size()); + testSubSet(descCs); + } + + private void testSubSet(CellSet cs) throws Exception { + for (int i = 0; i != ascCells.length; ++i) { + NavigableSet excludeTail = cs.tailSet(ascCells[i], false); + NavigableSet includeTail = cs.tailSet(ascCells[i], true); + assertEquals(ascCells.length - 1 - i, excludeTail.size()); + assertEquals(ascCells.length - i, includeTail.size()); + Iterator excludeIter = excludeTail.iterator(); + Iterator includeIter = includeTail.iterator(); + for (int j = 1 + i; j != ascCells.length; ++j) { + assertEquals(true, CellUtil.equals(excludeIter.next(), ascCells[j])); + } + for (int j = i; j != ascCells.length; ++j) { + assertEquals(true, CellUtil.equals(includeIter.next(), ascCells[j])); + } + } + assertEquals(NUM_OF_CELLS, cs.tailSet(lowerOuterCell, false).size()); + assertEquals(0, cs.tailSet(upperOuterCell, false).size()); + for (int i = 0; i != ascCells.length; ++i) { + NavigableSet excludeHead = cs.headSet(ascCells[i], false); + NavigableSet includeHead = cs.headSet(ascCells[i], true); + assertEquals(i, excludeHead.size()); + assertEquals(i + 1, includeHead.size()); + Iterator excludeIter = excludeHead.iterator(); + Iterator includeIter = includeHead.iterator(); + for (int j = 0; j != i; ++j) { + assertEquals(true, CellUtil.equals(excludeIter.next(), ascCells[j])); + } + for (int j = 0; j != i + 1; ++j) { + assertEquals(true, CellUtil.equals(includeIter.next(), ascCells[j])); + } + } + assertEquals(0, cs.headSet(lowerOuterCell, false).size()); + assertEquals(NUM_OF_CELLS, cs.headSet(upperOuterCell, false).size()); + + NavigableMap sub = cs.getDelegatee().subMap(lowerOuterCell, true, upperOuterCell, true); + assertEquals(NUM_OF_CELLS, sub.size()); + Iterator iter = sub.values().iterator(); + for (int i = 0; i != ascCells.length; ++i) { + assertEquals(true, CellUtil.equals(iter.next(), ascCells[i])); + } + } /* Generic basic test for immutable CellSet */ private void testCellBlocks(CellSet cs) throws Exception { @@ -88,31 +150,31 @@ public class TestCellFlatSet extends TestCase { assertEquals(NUM_OF_CELLS, cs.size()); // check size assertFalse(cs.contains(outerCell)); // check outer cell - assertTrue(cs.contains(cells[0])); // check existence of the first + assertTrue(cs.contains(ascCells[0])); // check existence of the first Cell first = cs.first(); - assertTrue(cells[0].equals(first)); + assertTrue(ascCells[0].equals(first)); - assertTrue(cs.contains(cells[NUM_OF_CELLS - 1])); // check last + assertTrue(cs.contains(ascCells[NUM_OF_CELLS - 1])); // check last Cell last = cs.last(); - assertTrue(cells[NUM_OF_CELLS - 1].equals(last)); + assertTrue(ascCells[NUM_OF_CELLS - 1].equals(last)); - SortedSet tail = cs.tailSet(cells[1]); // check tail abd head sizes + SortedSet tail = cs.tailSet(ascCells[1]); // check tail abd head sizes assertEquals(NUM_OF_CELLS - 1, tail.size()); - SortedSet head = cs.headSet(cells[1]); + SortedSet head = cs.headSet(ascCells[1]); assertEquals(1, head.size()); SortedSet tailOuter = cs.tailSet(outerCell); // check tail starting from outer cell assertEquals(NUM_OF_CELLS - 1, tailOuter.size()); Cell tailFirst = tail.first(); - assertTrue(cells[1].equals(tailFirst)); + assertTrue(ascCells[1].equals(tailFirst)); Cell tailLast = tail.last(); - assertTrue(cells[NUM_OF_CELLS - 1].equals(tailLast)); + assertTrue(ascCells[NUM_OF_CELLS - 1].equals(tailLast)); Cell headFirst = head.first(); - assertTrue(cells[0].equals(headFirst)); + assertTrue(ascCells[0].equals(headFirst)); Cell headLast = head.last(); - assertTrue(cells[0].equals(headLast)); + assertTrue(ascCells[0].equals(headLast)); } /* Generic iterators test for immutable CellSet */ @@ -123,10 +185,10 @@ public class TestCellFlatSet extends TestCase { for (Cell kv: cs) { assertEquals("\n\n-------------------------------------------------------------------\n" + "Comparing iteration number " + (count + 1) + " the returned cell: " + kv - + ", the first Cell in the CellBlocksMap: " + cells[count] - + ", and the same transformed to String: " + cells[count].toString() + + ", the first Cell in the CellBlocksMap: " + ascCells[count] + + ", and the same transformed to String: " + ascCells[count].toString() + "\n-------------------------------------------------------------------\n", - cells[count], kv); + ascCells[count], kv); count++; } assertEquals(NUM_OF_CELLS, count); @@ -135,7 +197,7 @@ public class TestCellFlatSet extends TestCase { count = 0; for (Iterator i = cs.descendingIterator(); i.hasNext();) { Cell kv = i.next(); - assertEquals(cells[NUM_OF_CELLS - (count + 1)], kv); + assertEquals(ascCells[NUM_OF_CELLS - (count + 1)], kv); count++; } assertEquals(NUM_OF_CELLS, count);