HBASE-18368 FilterList with multiple FamilyFilters concatenated by OR does not work

Signed-off-by: Guanghao Zhang <zghao@apache.org>

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

@@ -172,8 +172,12 @@ public abstract class Filter {
      */
     NEXT_COL,
     /**
-     * Done with columns, skip to next row. Note that filterRow() will
+     * Seek to next row in current family. It may still pass a cell whose family is different but
-     * still be called.
+     * row is the same as previous cell to {@link #filterKeyValue(Cell)} , even if we get a NEXT_ROW
+     * returned for previous cell. For more details see HBASE-18368. <br>
+     * Once reset() method was invoked, then we switch to the next row for all family, and you can
+     * catch the event by invoking CellUtils.matchingRows(previousCell, currentCell). <br>
+     * Note that filterRow() will still be called. <br>
      */
     NEXT_ROW,
     /**
@@ -181,7 +185,7 @@ public abstract class Filter {
      */
     SEEK_NEXT_USING_HINT,
     /**
-     * Include KeyValue and done with row, seek to next.
+     * Include KeyValue and done with row, seek to next. See NEXT_ROW.
      */
     INCLUDE_AND_SEEK_NEXT_ROW,
 }

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

@@ -74,7 +74,12 @@ public class FilterListWithOR extends FilterListBase {
    * as the previous cell even if filter-A has NEXT_COL returned for the previous cell. So we should
    * save the previous cell and the return code list when checking previous cell for every filter in
    * filter list, and verify if currentCell fit the previous return code, if fit then pass the
-   * currentCell to the corresponding filter. (HBASE-17678)
+   * currentCell to the corresponding filter. (HBASE-17678) <br>
+   * Note that: In StoreScanner level, NEXT_ROW will skip to the next row in current family, and in
+   * RegionScanner level, NEXT_ROW will skip to the next row in current family and switch to the
+   * next family for RegionScanner, INCLUDE_AND_NEXT_ROW is the same. so we should pass current cell
+   * 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)
       throws IOException {
@@ -94,7 +99,8 @@ public class FilterListWithOR extends FilterListBase {
       return !CellUtil.matchingRowColumn(prevCell, currentCell);
     case NEXT_ROW:
     case INCLUDE_AND_SEEK_NEXT_ROW:
-      return !CellUtil.matchingRows(prevCell, currentCell);
+      return !CellUtil.matchingRows(prevCell, currentCell)
+          || !CellUtil.matchingFamily(prevCell, currentCell);
     default:
       throw new IllegalStateException("Received code is not valid.");
     }

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

@@ -936,5 +936,31 @@ public class TestFilterList {
     assertEquals(ReturnCode.INCLUDE, filterList.filterKeyValue(kv));
     assertEquals(kv, filterList.transformCell(kv));
   }
+
+  private static class MockNextRowFilter extends FilterBase {
+    private int hitCount = 0;
+
+    public ReturnCode filterKeyValue(Cell v) throws IOException {
+      hitCount++;
+      return ReturnCode.NEXT_ROW;
+    }
+
+    public int getHitCount() {
+      return hitCount;
+    }
+  }
+
+  @Test
+  public void testRowCountFilter() throws IOException {
+    KeyValue kv1 = new KeyValue(Bytes.toBytes("row"), Bytes.toBytes("fam1"), Bytes.toBytes("a"), 1,
+        Bytes.toBytes("value"));
+    KeyValue kv2 = new KeyValue(Bytes.toBytes("row"), Bytes.toBytes("fam2"), Bytes.toBytes("a"), 2,
+        Bytes.toBytes("value"));
+    MockNextRowFilter mockNextRowFilter = new MockNextRowFilter();
+    FilterList filter = new FilterList(Operator.MUST_PASS_ONE, mockNextRowFilter);
+    filter.filterKeyValue(kv1);
+    filter.filterKeyValue(kv2);
+    assertEquals(2, mockNextRowFilter.getHitCount());
+  }
 }
 

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

@@ -36,10 +36,10 @@ import org.junit.experimental.categories.Category;
 import org.junit.rules.TestName;
 
 /**
- * Tests filter Lists in ways that rely on a MiniCluster.
+ * Tests filter Lists in ways that rely on a MiniCluster. Where possible, favor tests in
- * Where possible, favor tests in TestFilterList and TestFilterFromRegionSide instead.
+ * TestFilterList and TestFilterFromRegionSide instead.
  */
-@Category({MediumTests.class, FilterTests.class})
+@Category({ MediumTests.class, FilterTests.class })
 public class TestFilterListOnMini {
 
   private static final Log LOG = LogFactory.getLog(TestFilterListOnMini.class);
@@ -58,7 +58,6 @@ public class TestFilterListOnMini {
     TEST_UTIL.shutdownMiniCluster();
   }
 
-  @Ignore("HBASE-18410 Should not merge without this test running.")
   @Test
   public void testFiltersWithOR() throws Exception {
     TableName tn = TableName.valueOf(name.getMethodName());