remove incorrect early return of MatchCode.SKIP

Author: terence-yoo

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/NormalUserScanQueryMatcher.java

@@ -82,11 +82,6 @@ public MatchCode match(ExtendedCell cell, ExtendedCell prevCell, boolean visibil
       if (includeDeleteMarker) {
         this.deletes.add(cell);
       }
-      // optimization for delete markers
-      if ((returnCode = checkCanSeekNextCol(cell, prevCell, visibilityLabelEnabled)) != null) {
-        return returnCode;
-      }
-      return MatchCode.SKIP;
     }
     // optimization when prevCell is Delete or DeleteFamilyVersion
     if ((returnCode = checkDeletedEffectively(cell, prevCell, visibilityLabelEnabled)) != null) {
@@ -98,28 +93,15 @@ public MatchCode match(ExtendedCell cell, ExtendedCell prevCell, boolean visibil
     return matchColumn(cell, timestamp, typeByte);
   }
 
-  private MatchCode checkCanSeekNextCol(ExtendedCell cell, ExtendedCell prevCell,
-    boolean visibilityLabelEnabled) {
-    // optimization for DeleteFamily and DeleteColumn(only for empty qualifier)
-    if (
-      canOptimizeReadDeleteMarkers(visibilityLabelEnabled) && (PrivateCellUtil.isDeleteFamily(cell)
-        || PrivateCellUtil.isDeleteColumns(cell) && cell.getQualifierLength() > 0)
-    ) {
-      return MatchCode.SEEK_NEXT_COL;
-    }
-    // optimization for duplicate Delete and DeleteFamilyVersion
-    return checkDeletedEffectively(cell, prevCell, visibilityLabelEnabled);
-  }
-
-  // If prevCell is a delete marker and cell is a Put or delete marker,
+  // If prevCell is a delete marker and cell is a delete marked Put or delete marker,
   // it means the cell is deleted effectively.
   // And we can do SEEK_NEXT_COL.
   private MatchCode checkDeletedEffectively(ExtendedCell cell, ExtendedCell prevCell,
     boolean visibilityLabelEnabled) {
     if (
       prevCell != null && canOptimizeReadDeleteMarkers(visibilityLabelEnabled)
         && CellUtil.matchingRowColumn(prevCell, cell) && CellUtil.matchingTimestamp(prevCell, cell)
-        && (PrivateCellUtil.isDeleteType(prevCell) && cell.getQualifierLength() > 0
+        && (PrivateCellUtil.isDeleteType(prevCell)
           || PrivateCellUtil.isDeleteFamilyVersion(prevCell))
     ) {
       return MatchCode.SEEK_NEXT_COL;

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScannerDeleteMarkerOptimization.java

@@ -224,13 +224,13 @@ public void testKeepDeletedCells() throws IOException {
     kvs.add(create("r", CF, "q", 1, Type.Put, "v"));
     kvs.add(create("r", CF, "q1", 1, Type.Put, "v"));
 
-    // optimization works only for KeepDeletedCells.FALSE
+    // optimization does not work. optimization works only for KeepDeletedCells.FALSE
     result = scanAll(KeepDeletedCells.TRUE, 1);
     assertEquals(1, result.getFirst().size());
     assertEquals(kvs.get(4), result.getFirst().get(0));
     assertEquals(kvs.size(), result.getSecond().longValue());
 
-    // optimization works only for KeepDeletedCells.FALSE
+    // optimization does not work. optimization works only for KeepDeletedCells.FALSE
     result = scanAll(KeepDeletedCells.TTL, 1);
     assertEquals(1, result.getFirst().size());
     assertEquals(kvs.get(4), result.getFirst().get(0));
@@ -239,16 +239,16 @@ public void testKeepDeletedCells() throws IOException {
 
   @Test
   public void testScanMaxVersions() throws IOException {
-    kvs.add(create("r", CF, "q", 4, Type.DeleteColumn, ""));
-    kvs.add(create("r", CF, "q", 3, Type.DeleteColumn, ""));
-    kvs.add(create("r", CF, "q", 2, Type.DeleteColumn, ""));
+    kvs.add(create("r", CF, "q", 2, Type.Delete, ""));
+    kvs.add(create("r", CF, "q", 2, Type.Put, "v"));
+    kvs.add(create("r", CF, "q", 1, Type.Delete, ""));
     kvs.add(create("r", CF, "q", 1, Type.Put, "v"));
 
     // optimization works only for maxVersions = 1
     result = scanAll(KeepDeletedCells.FALSE, 1);
     assertEquals(0, result.getFirst().size());
-    // kvs0
-    assertEquals(1, result.getSecond().longValue());
+    // kvs0, kvs1
+    assertEquals(2, result.getSecond().longValue());
 
     // optimization does not work
     result = scanAll(KeepDeletedCells.FALSE, 2);

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/querymatcher/TestUserScanQueryMatcherDeleteMarkerOptimization.java

@@ -108,6 +108,8 @@ public void testNotDuplicatDelete() throws IOException {
 
   @Test
   public void testEffectiveDelete() throws IOException {
+    pairs.add(new Pair<>(createKV(null, 2, Type.Delete), MatchCode.SKIP));
+    pairs.add(new Pair<>(createKV(null, 2, Type.Put), MatchCode.SEEK_NEXT_COL));
     pairs.add(new Pair<>(createKV(col1, 2, Type.Delete), MatchCode.SKIP));
     pairs.add(new Pair<>(createKV(col1, 2, Type.Put), MatchCode.SEEK_NEXT_COL));
     verify(pairs);
@@ -213,20 +215,6 @@ public void testDeleteFamilyAfterPut() throws IOException {
     verify(pairs);
   }
 
-  @Test
-  public void testEmptyQualifierDeleteShouldNotSeekNextColumn() throws IOException {
-    // The empty qualifier is used for DeleteFamily and DeleteFamilyVersion markers only.
-    // So we should not seek to next column for any other type.
-    pairs.add(new Pair<>(createKV(null, 4, Type.Delete), MatchCode.SKIP));
-    pairs.add(new Pair<>(createKV(null, 4, Type.Delete), MatchCode.SKIP));
-    pairs.add(new Pair<>(createKV(null, 3, Type.DeleteColumn), MatchCode.SKIP));
-    pairs.add(new Pair<>(createKV(null, 3, Type.DeleteColumn), MatchCode.SKIP));
-    pairs.add(new Pair<>(createKV(null, 2, Type.DeleteFamilyVersion), MatchCode.SKIP));
-    pairs.add(new Pair<>(createKV(null, 2, Type.DeleteFamilyVersion), MatchCode.SEEK_NEXT_COL));
-    pairs.add(new Pair<>(createKV(null, 1, Type.DeleteFamily), MatchCode.SEEK_NEXT_COL));
-    verify(pairs);
-  }
-
   @Test
   public void testKeyForNextColumnForDeleteFamily() throws IOException {
     long now = EnvironmentEdgeManager.currentTime();
@@ -262,18 +250,22 @@ public void testNoDelete() throws IOException {
   }
 
   @Test
-  public void testDeleteFamilyWithVisibilityLabelEnabled() throws IOException {
+  public void testVisibilityLabelEnabled() throws IOException {
     Configuration conf = HBaseConfiguration.create();
     enableVisiblityLabels(conf);
-    pairs.add(new Pair<>(createKV(null, 2, Type.DeleteFamily), MatchCode.SKIP));
+    pairs.add(new Pair<>(createKV(col1, 3, Type.Delete), MatchCode.SKIP));
+    pairs.add(new Pair<>(createKV(col1, 3, Type.Put), MatchCode.SKIP));
+    pairs.add(new Pair<>(createKV(null, 2, Type.DeleteFamily), MatchCode.SEEK_NEXT_COL));
     pairs.add(new Pair<>(createKV(col1, 1, Type.Put), MatchCode.SEEK_NEXT_COL));
     verify(pairs, 1, VisibilityUtils.isVisibilityLabelEnabled(conf));
   }
 
   @Test
   public void testScanWithFilter() throws IOException {
     scan.setFilter(new FilterList());
-    pairs.add(new Pair<>(createKV(null, 2, Type.DeleteFamily), MatchCode.SKIP));
+    pairs.add(new Pair<>(createKV(col1, 3, Type.Delete), MatchCode.SKIP));
+    pairs.add(new Pair<>(createKV(col1, 3, Type.Put), MatchCode.SKIP));
+    pairs.add(new Pair<>(createKV(null, 2, Type.DeleteFamily), MatchCode.SEEK_NEXT_COL));
     pairs.add(new Pair<>(createKV(col1, 1, Type.Put), MatchCode.SEEK_NEXT_COL));
     verify(pairs);
   }