HBASE-19057 Fix other code review comments about FilterList improvement

hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java

@@ -21,17 +21,12 @@ package org.apache.hadoop.hbase.filter;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 
 import org.apache.hadoop.hbase.Cell;
-import org.apache.hadoop.hbase.CellComparatorImpl;
-import org.apache.hadoop.hbase.CellUtil;
-import org.apache.hadoop.hbase.KeyValueUtil;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.apache.hadoop.hbase.exceptions.DeserializationException;
-import org.apache.yetus.audience.InterfaceAudience;
 
 import org.apache.hadoop.hbase.shaded.com.google.protobuf.InvalidProtocolBufferException;
 import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
@@ -170,8 +165,23 @@ final public class FilterList extends FilterBase {
     return filterListBase.transformCell(c);
   }
 
-  ReturnCode internalFilterKeyValue(Cell c, Cell currentTransformedCell) throws IOException {
+  /**
-    return this.filterListBase.internalFilterKeyValue(c, currentTransformedCell);
+   * Internal implementation of {@link #filterKeyValue(Cell)}. Compared to the
+   * {@link #filterKeyValue(Cell)} method, this method accepts an additional parameter named
+   * transformedCell. This parameter indicates the initial value of transformed cell before this
+   * filter operation. <br/>
+   * For FilterList, we can consider a filter list as a node in a tree. sub-filters of the filter
+   * list are children of the relative node. The logic of transforming cell of a filter list, well,
+   * we can consider it as the process of post-order tree traverse. For a node , Before we traverse
+   * the current child, we should set the traverse result (transformed cell) of previous node(s) as
+   * the initial value. so the additional currentTransformedCell parameter is needed (HBASE-18879).
+   * @param c The cell in question.
+   * @param transformedCell The transformed cell of previous filter(s)
+   * @return ReturnCode of this filter operation.
+   * @throws IOException
+   */
+  ReturnCode internalFilterKeyValue(Cell c, Cell transformedCell) throws IOException {
+    return this.filterListBase.internalFilterKeyValue(c, transformedCell);
   }
 
   @Override

hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListBase.java

@@ -107,8 +107,20 @@ public abstract class FilterListBase extends FilterBase {
     return cell;
   }
 
-  abstract ReturnCode internalFilterKeyValue(Cell c, Cell currentTransformedCell)
+  /**
-      throws IOException;
+   * Internal implementation of {@link #filterKeyValue(Cell)}
+   * @param c The cell in question.
+   * @param transformedCell The transformed cell of previous filter(s)
+   * @return ReturnCode of this filter operation.
+   * @throws IOException
+   * @see org.apache.hadoop.hbase.filter.FilterList#internalFilterKeyValue(Cell, Cell)
+   */
+  abstract ReturnCode internalFilterKeyValue(Cell c, Cell transformedCell) throws IOException;
+
+  @Override
+  public ReturnCode filterKeyValue(Cell c) throws IOException {
+    return internalFilterKeyValue(c, c);
+  }
 
   /**
    * Filters that never filter by modifying the returned List of Cells can inherit this

hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithAND.java

@@ -148,12 +148,12 @@ public class FilterListWithAND extends FilterListBase {
   }
 
   @Override
-  ReturnCode internalFilterKeyValue(Cell c, Cell currentTransformedCell) throws IOException {
+  ReturnCode internalFilterKeyValue(Cell c, Cell transformedCell) throws IOException {
     if (isEmpty()) {
       return ReturnCode.INCLUDE;
     }
     ReturnCode rc = ReturnCode.INCLUDE;
-    Cell transformed = currentTransformedCell;
+    Cell transformed = transformedCell;
     this.referenceCell = c;
     this.seekHintFilter.clear();
     for (int i = 0, n = filters.size(); i < n; i++) {
@@ -185,11 +185,6 @@ public class FilterListWithAND extends FilterListBase {
     return rc;
   }
 
-  @Override
-  public ReturnCode filterKeyValue(Cell c) throws IOException {
-    return internalFilterKeyValue(c, c);
-  }
-
   @Override
   public void reset() throws IOException {
     for (int i = 0, n = filters.size(); i < n; i++) {
@@ -222,6 +217,9 @@ public class FilterListWithAND extends FilterListBase {
     for (int i = 0, n = filters.size(); i < n; i++) {
       Filter filter = filters.get(i);
       if (filter.filterAllRemaining() || filter.filterRowKey(firstRowCell)) {
+        // Can't just return true here, because there are some filters (such as PrefixFilter) which
+        // will catch the row changed event by filterRowKey(). If we return early here, those
+        // filters will have no chance to update their row state.
         retVal = true;
       }
     }

hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithOR.java

@@ -81,9 +81,8 @@ public class FilterListWithOR extends FilterListBase {
    * to the filter, if row mismatch or row match but column family mismatch. (HBASE-18368)
    * @see org.apache.hadoop.hbase.filter.Filter.ReturnCode
    */
-  private boolean shouldPassCurrentCellToFilter(Cell prevCell, Cell currentCell, int filterIdx)
+  private boolean shouldPassCurrentCellToFilter(Cell prevCell, Cell currentCell,
-      throws IOException {
+      ReturnCode prevCode) throws IOException {
-    ReturnCode prevCode = this.prevFilterRCList.get(filterIdx);
     if (prevCell == null || prevCode == null) {
       return true;
     }
@@ -96,11 +95,13 @@ public class FilterListWithOR extends FilterListBase {
       return nextHintCell == null || this.compareCell(currentCell, nextHintCell) >= 0;
     case NEXT_COL:
     case INCLUDE_AND_NEXT_COL:
-      return !CellUtil.matchingRowColumn(prevCell, currentCell);
+      // Once row changed, reset() will clear prevCells, so we need not to compare their rows
+      // because rows are the same here.
+      return !CellUtil.matchingColumn(prevCell, currentCell);
     case NEXT_ROW:
     case INCLUDE_AND_SEEK_NEXT_ROW:
-      return !CellUtil.matchingRows(prevCell, currentCell)
+      // As described above, rows are definitely the same, so we only compare the family.
-          || !CellUtil.matchingFamily(prevCell, currentCell);
+      return !CellUtil.matchingFamily(prevCell, currentCell);
     default:
       throw new IllegalStateException("Received code is not valid.");
     }
@@ -181,6 +182,9 @@ public class FilterListWithOR extends FilterListBase {
       if (isInReturnCodes(rc, ReturnCode.INCLUDE)) {
         return ReturnCode.INCLUDE;
       }
+      if (isInReturnCodes(rc, ReturnCode.NEXT_COL, ReturnCode.NEXT_ROW)) {
+        return ReturnCode.NEXT_COL;
+      }
       if (isInReturnCodes(rc, ReturnCode.INCLUDE_AND_NEXT_COL,
         ReturnCode.INCLUDE_AND_SEEK_NEXT_ROW)) {
         return ReturnCode.INCLUDE_AND_NEXT_COL;
@@ -242,19 +246,20 @@ public class FilterListWithOR extends FilterListBase {
   }
 
   @Override
-  ReturnCode internalFilterKeyValue(Cell c, Cell currentTransformCell) throws IOException {
+  ReturnCode internalFilterKeyValue(Cell c, Cell transformCell) throws IOException {
     if (isEmpty()) {
       return ReturnCode.INCLUDE;
     }
     ReturnCode rc = null;
     boolean everyFilterReturnHint = true;
-    Cell transformed = currentTransformCell;
+    Cell transformed = transformCell;
     this.referenceCell = c;
     for (int i = 0, n = filters.size(); i < n; i++) {
       Filter filter = filters.get(i);
 
       Cell prevCell = this.prevCellList.get(i);
-      if (filter.filterAllRemaining() || !shouldPassCurrentCellToFilter(prevCell, c, i)) {
+      ReturnCode prevCode = this.prevFilterRCList.get(i);
+      if (filter.filterAllRemaining() || !shouldPassCurrentCellToFilter(prevCell, c, prevCode)) {
         everyFilterReturnHint = false;
         continue;
       }
@@ -294,11 +299,6 @@ public class FilterListWithOR extends FilterListBase {
     }
   }
 
-  @Override
-  public ReturnCode filterKeyValue(Cell c) throws IOException {
-    return internalFilterKeyValue(c, c);
-  }
-
   @Override
   public void reset() throws IOException {
     for (int i = 0, n = filters.size(); i < n; i++) {
@@ -332,6 +332,9 @@ public class FilterListWithOR extends FilterListBase {
     for (int i = 0, n = filters.size(); i < n; i++) {
       Filter filter = filters.get(i);
       if (!filter.filterAllRemaining() && !filter.filterRowKey(firstRowCell)) {
+        // Can't just return false here, because there are some filters (such as PrefixFilter) which
+        // will catch the row changed event by filterRowKey(). If we return early here, those
+        // filters will have no chance to update their row state.
         retVal = false;
       }
     }

hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilterList.java

@@ -330,7 +330,7 @@ public class TestFilterList {
    * @throws Exception
    */
   @Test
-  public void testFilterListWithInclusiveStopFilteMustPassOne() throws Exception {
+  public void testFilterListWithInclusiveStopFilterMustPassOne() throws Exception {
     byte[] r1 = Bytes.toBytes("Row1");
     byte[] r11 = Bytes.toBytes("Row11");
     byte[] r2 = Bytes.toBytes("Row2");
@@ -351,11 +351,13 @@ public class TestFilterList {
     public AlwaysNextColFilter() {
       super();
     }
+
     @Override
     public ReturnCode filterKeyValue(Cell v) {
       return ReturnCode.NEXT_COL;
     }
-    public static AlwaysNextColFilter parseFrom(final byte [] pbBytes)
+
+    public static AlwaysNextColFilter parseFrom(final byte[] pbBytes)
         throws DeserializationException {
       return new AlwaysNextColFilter();
     }
@@ -701,6 +703,7 @@ public class TestFilterList {
     filter.filterKeyValue(kv3);
     assertFalse(mockFilter.didCellPassToTheFilter);
 
+    filter.reset();
     mockFilter.didCellPassToTheFilter = false;
     filter.filterKeyValue(kv4);
     assertTrue(mockFilter.didCellPassToTheFilter);
@@ -718,6 +721,7 @@ public class TestFilterList {
     filter.filterKeyValue(kv3);
     assertFalse(mockFilter.didCellPassToTheFilter);
 
+    filter.reset();
     mockFilter.didCellPassToTheFilter = false;
     filter.filterKeyValue(kv4);
     assertTrue(mockFilter.didCellPassToTheFilter);