From 8385bdc333478686878a1ae7b6a1590bf0f2621f Mon Sep 17 00:00:00 2001
From: Peter Somogyi <psomogyi@cloudera.com>
Date: Wed, 26 Jul 2017 16:15:01 +0200
Subject: [PATCH 1/2] HACK! Revert "HBASE-17678 FilterList with MUST_PASS_ONE
 lead to redundancy cells returned - addendum"

This commit caused an issue upstream which led to a failing pig smoke test.
Family filters with OR does not work. HBASE-18410
Ref: CDH-56405

This reverts commit 347bef8d336377ae38bb6c357da9d033ccba155c.
---
 .../org/apache/hadoop/hbase/filter/FilterList.java   | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java
index 7433cca40f94..985cb16612dc 100644
--- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java
+++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java
@@ -27,7 +27,6 @@
 import org.apache.hadoop.hbase.Cell;
 import org.apache.hadoop.hbase.CellComparator;
 import org.apache.hadoop.hbase.CellUtil;
-import org.apache.hadoop.hbase.KeyValueUtil;
 import org.apache.hadoop.hbase.classification.InterfaceAudience;
 import org.apache.hadoop.hbase.exceptions.DeserializationException;
 import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
@@ -145,7 +144,7 @@ public FilterList(final Operator operator, final Filter... rowFilters) {
 
   public void initPrevListForMustPassOne(int size) {
     if (operator == Operator.MUST_PASS_ONE) {
-      if (this.prevFilterRCList == null) {
+      if (this.prevCellList == null) {
         prevFilterRCList = new ArrayList<>(Collections.nCopies(size, null));
       }
       if (this.prevCellList == null) {
@@ -404,14 +403,7 @@ public ReturnCode filterKeyValue(Cell c) throws IOException {
         ReturnCode localRC = filter.filterKeyValue(c);
         // Update previous cell and return code we encountered.
         prevFilterRCList.set(i, localRC);
-        if (c == null || localRC == ReturnCode.INCLUDE || localRC == ReturnCode.SKIP) {
-          // If previous return code is INCLUDE or SKIP, we should always pass the next cell to the
-          // corresponding sub-filter(need not test shouldPassCurrentCellToFilter() method), So we
-          // need not save current cell to prevCellList for saving heap memory.
-          prevCellList.set(i, null);
-        } else {
-          prevCellList.set(i, KeyValueUtil.toNewKeyCell(c));
-        }
+        prevCellList.set(i, c);
 
         if (localRC != ReturnCode.SEEK_NEXT_USING_HINT) {
           seenNonHintReturnCode = true;

From 7757f818677f0bb75c3982a6ffc1ccf49769785b Mon Sep 17 00:00:00 2001
From: Peter Somogyi <psomogyi@cloudera.com>
Date: Wed, 26 Jul 2017 16:15:34 +0200
Subject: [PATCH 2/2] HACK! Revert "HBASE-17678 FilterList with MUST_PASS_ONE
 may lead to redundant cells returned"

This commit caused an issue upstream which led to a failing pig smoke test.
Family filters with OR does not work. HBASE-18410
Ref: CDH-56405

This reverts commit 0d0c330401ade938bf934aafd79ec23705edcc60.
---
 .../hadoop/hbase/filter/FilterList.java       |  76 +-----------
 .../hadoop/hbase/filter/TestFilterList.java   | 117 ------------------
 2 files changed, 2 insertions(+), 191 deletions(-)

diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java
index 985cb16612dc..0742b225f117 100644
--- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java
+++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java
@@ -66,14 +66,6 @@ public static enum Operator {
   private final List<Filter> filters;
   private Filter seekHintFilter = null;
 
-  /**
-   * Save previous return code and previous cell for every filter in filter list. For MUST_PASS_ONE,
-   * we use the previous return code to decide whether we should pass current cell encountered to
-   * the filter. For MUST_PASS_ALL, the two list are meaningless.
-   */
-  private List<ReturnCode> prevFilterRCList = null;
-  private List<Cell> prevCellList = null;
-
   /** Reference Cell used by {@link #transformCell(Cell)} for validation purpose. */
   private Cell referenceCell = null;
 
@@ -95,7 +87,6 @@ public static enum Operator {
   public FilterList(final List<Filter> rowFilters) {
     reversed = getReversed(rowFilters, reversed);
     this.filters = new ArrayList<>(rowFilters);
-    initPrevListForMustPassOne(rowFilters.size());
   }
 
   /**
@@ -115,7 +106,6 @@ public FilterList(final Filter... rowFilters) {
   public FilterList(final Operator operator) {
     this.operator = operator;
     this.filters = new ArrayList<>();
-    initPrevListForMustPassOne(filters.size());
   }
 
   /**
@@ -127,7 +117,6 @@ public FilterList(final Operator operator) {
   public FilterList(final Operator operator, final List<Filter> rowFilters) {
     this(rowFilters);
     this.operator = operator;
-    initPrevListForMustPassOne(rowFilters.size());
   }
 
   /**
@@ -139,21 +128,8 @@ public FilterList(final Operator operator, final List<Filter> rowFilters) {
   public FilterList(final Operator operator, final Filter... rowFilters) {
     this(rowFilters);
     this.operator = operator;
-    initPrevListForMustPassOne(rowFilters.length);
-  }
-
-  public void initPrevListForMustPassOne(int size) {
-    if (operator == Operator.MUST_PASS_ONE) {
-      if (this.prevCellList == null) {
-        prevFilterRCList = new ArrayList<>(Collections.nCopies(size, null));
-      }
-      if (this.prevCellList == null) {
-        prevCellList = new ArrayList<>(Collections.nCopies(size, null));
-      }
-    }
   }
 
-
   /**
    * Get the operator.
    *
@@ -208,10 +184,6 @@ private static void checkReversed(List<Filter> rowFilters, boolean expected) {
   public void addFilter(List<Filter> filters) {
     checkReversed(filters, isReversed());
     this.filters.addAll(filters);
-    if (operator == Operator.MUST_PASS_ONE) {
-      this.prevFilterRCList.addAll(Collections.nCopies(filters.size(), null));
-      this.prevCellList.addAll(Collections.nCopies(filters.size(), null));
-    }
   }
 
   /**
@@ -228,10 +200,6 @@ public void reset() throws IOException {
     int listize = filters.size();
     for (int i = 0; i < listize; i++) {
       filters.get(i).reset();
-      if (operator == Operator.MUST_PASS_ONE) {
-        prevFilterRCList.set(i, null);
-        prevCellList.set(i, null);
-      }
     }
     seekHintFilter = null;
   }
@@ -314,41 +282,6 @@ public Cell transformCell(Cell c) throws IOException {
     return this.transformedCell;
   }
 
-  /**
-   * For MUST_PASS_ONE, we cannot make sure that when filter-A in filter list return NEXT_COL then
-   * the next cell passing to filterList will be the first cell in next column, because if filter-B
-   * in filter list return SKIP, then the filter list will return SKIP. In this case, we should pass
-   * the cell following the previous cell, and it's possible that the next cell has the same column
-   * 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)
-   */
-  private boolean shouldPassCurrentCellToFilter(Cell prevCell, Cell currentCell, int filterIdx)
-      throws IOException {
-    ReturnCode prevCode = this.prevFilterRCList.get(filterIdx);
-    if (prevCell == null || prevCode == null) {
-      return true;
-    }
-    switch (prevCode) {
-    case INCLUDE:
-    case SKIP:
-      return true;
-    case SEEK_NEXT_USING_HINT:
-      Cell nextHintCell = getNextCellHint(prevCell);
-      return nextHintCell == null
-          || CellComparator.COMPARATOR.compare(currentCell, nextHintCell) >= 0;
-    case NEXT_COL:
-    case INCLUDE_AND_NEXT_COL:
-      return !CellUtil.matchingRowColumn(prevCell, currentCell);
-    case NEXT_ROW:
-    case INCLUDE_AND_SEEK_NEXT_ROW:
-      return !CellUtil.matchingRows(prevCell, currentCell);
-    default:
-      throw new IllegalStateException("Received code is not valid.");
-    }
-  }
-
   @Override
   @edu.umd.cs.findbugs.annotations.SuppressWarnings(value="SF_SWITCH_FALLTHROUGH",
     justification="Intentional")
@@ -394,17 +327,12 @@ public ReturnCode filterKeyValue(Cell c) throws IOException {
           return code;
         }
       } else if (operator == Operator.MUST_PASS_ONE) {
-        Cell prevCell = this.prevCellList.get(i);
-        if (filter.filterAllRemaining() || !shouldPassCurrentCellToFilter(prevCell, c, i)) {
+        if (filter.filterAllRemaining()) {
           seenNonHintReturnCode = true;
           continue;
         }
 
         ReturnCode localRC = filter.filterKeyValue(c);
-        // Update previous cell and return code we encountered.
-        prevFilterRCList.set(i, localRC);
-        prevCellList.set(i, c);
-
         if (localRC != ReturnCode.SEEK_NEXT_USING_HINT) {
           seenNonHintReturnCode = true;
         }
@@ -557,7 +485,7 @@ public Cell getNextCellHint(Cell currentCell) throws IOException {
     }
     Cell keyHint = null;
     if (operator == Operator.MUST_PASS_ALL) {
-      if (seekHintFilter != null) keyHint = seekHintFilter.getNextCellHint(currentCell);
+      keyHint = seekHintFilter.getNextCellHint(currentCell);
       return keyHint;
     }
 
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilterList.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilterList.java
index cf8a0a0ca60f..ad71fcc85020 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilterList.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilterList.java
@@ -25,7 +25,6 @@
 import java.util.List;
 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.KeyValueUtil;
 import org.apache.hadoop.hbase.exceptions.DeserializationException;
@@ -594,121 +593,5 @@ public void testTransformMPO() throws Exception {
     assertEquals(Filter.ReturnCode.SKIP, flist.filterKeyValue(kvQual3));
   }
 
-  @Test
-  public void testWithMultiVersionsInSameRow() throws Exception {
-    FilterList filterList01 =
-        new FilterList(Operator.MUST_PASS_ONE, new ColumnPaginationFilter(1, 0));
-
-    KeyValue kv1 = new KeyValue(Bytes.toBytes("row"), Bytes.toBytes("fam"), Bytes.toBytes("qual"),
-        1, Bytes.toBytes("value"));
-    KeyValue kv2 = new KeyValue(Bytes.toBytes("row"), Bytes.toBytes("fam"), Bytes.toBytes("qual"),
-        2, Bytes.toBytes("value"));
-    KeyValue kv3 = new KeyValue(Bytes.toBytes("row"), Bytes.toBytes("fam"), Bytes.toBytes("qual"),
-        3, Bytes.toBytes("value"));
-
-    assertEquals(filterList01.filterKeyValue(kv1), ReturnCode.INCLUDE_AND_NEXT_COL);
-    assertEquals(filterList01.filterKeyValue(kv2), ReturnCode.SKIP);
-    assertEquals(filterList01.filterKeyValue(kv3), ReturnCode.SKIP);
-
-    FilterList filterList11 =
-        new FilterList(Operator.MUST_PASS_ONE, new ColumnPaginationFilter(1, 1));
-
-    assertEquals(filterList11.filterKeyValue(kv1), ReturnCode.SKIP);
-    assertEquals(filterList11.filterKeyValue(kv2), ReturnCode.SKIP);
-    assertEquals(filterList11.filterKeyValue(kv3), ReturnCode.SKIP);
-  }
-
-  @Test
-  public void testMPONEWithSeekNextUsingHint() throws Exception {
-    byte[] col = Bytes.toBytes("c");
-    FilterList filterList =
-        new FilterList(Operator.MUST_PASS_ONE, new ColumnPaginationFilter(1, col));
-
-    KeyValue kv1 = new KeyValue(Bytes.toBytes("row"), Bytes.toBytes("fam"), Bytes.toBytes("a"), 1,
-        Bytes.toBytes("value"));
-    KeyValue kv2 = new KeyValue(Bytes.toBytes("row"), Bytes.toBytes("fam"), Bytes.toBytes("b"), 2,
-        Bytes.toBytes("value"));
-    KeyValue kv3 = new KeyValue(Bytes.toBytes("row"), Bytes.toBytes("fam"), Bytes.toBytes("c"), 3,
-        Bytes.toBytes("value"));
-    KeyValue kv4 = new KeyValue(Bytes.toBytes("row"), Bytes.toBytes("fam"), Bytes.toBytes("c"), 4,
-        Bytes.toBytes("value"));
-
-    assertEquals(filterList.filterKeyValue(kv1), ReturnCode.SEEK_NEXT_USING_HINT);
-    assertEquals(filterList.filterKeyValue(kv2), ReturnCode.SKIP);
-    assertEquals(filterList.filterKeyValue(kv3), ReturnCode.INCLUDE_AND_NEXT_COL);
-    assertEquals(filterList.filterKeyValue(kv4), ReturnCode.SKIP);
-  }
-
-  private static class MockFilter extends FilterBase {
-    private ReturnCode targetRetCode;
-    public boolean didCellPassToTheFilter = false;
-
-    public MockFilter(ReturnCode targetRetCode) {
-      this.targetRetCode = targetRetCode;
-    }
-
-    @Override
-    public ReturnCode filterKeyValue(Cell v) throws IOException {
-      this.didCellPassToTheFilter = true;
-      return targetRetCode;
-    }
-  }
-
-  @Test
-  public void testShouldPassCurrentCellToFilter() throws IOException {
-    KeyValue kv1 = new KeyValue(Bytes.toBytes("row"), Bytes.toBytes("fam"), Bytes.toBytes("a"), 1,
-        Bytes.toBytes("value"));
-    KeyValue kv2 = new KeyValue(Bytes.toBytes("row"), Bytes.toBytes("fam"), Bytes.toBytes("a"), 2,
-        Bytes.toBytes("value"));
-    KeyValue kv3 = new KeyValue(Bytes.toBytes("row"), Bytes.toBytes("fam"), Bytes.toBytes("b"), 3,
-        Bytes.toBytes("value"));
-    KeyValue kv4 = new KeyValue(Bytes.toBytes("row1"), Bytes.toBytes("fam"), Bytes.toBytes("c"), 4,
-        Bytes.toBytes("value"));
-
-    MockFilter mockFilter = new MockFilter(ReturnCode.NEXT_COL);
-    FilterList filter = new FilterList(Operator.MUST_PASS_ONE, mockFilter);
-
-    filter.filterKeyValue(kv1);
-    assertTrue(mockFilter.didCellPassToTheFilter);
-
-    mockFilter.didCellPassToTheFilter = false;
-    filter.filterKeyValue(kv2);
-    assertFalse(mockFilter.didCellPassToTheFilter);
-
-    mockFilter.didCellPassToTheFilter = false;
-    filter.filterKeyValue(kv3);
-    assertTrue(mockFilter.didCellPassToTheFilter);
-
-    mockFilter = new MockFilter(ReturnCode.INCLUDE_AND_NEXT_COL);
-    filter = new FilterList(Operator.MUST_PASS_ONE, mockFilter);
-
-    filter.filterKeyValue(kv1);
-    assertTrue(mockFilter.didCellPassToTheFilter);
-
-    mockFilter.didCellPassToTheFilter = false;
-    filter.filterKeyValue(kv2);
-    assertFalse(mockFilter.didCellPassToTheFilter);
-
-    mockFilter.didCellPassToTheFilter = false;
-    filter.filterKeyValue(kv3);
-    assertTrue(mockFilter.didCellPassToTheFilter);
-
-    mockFilter = new MockFilter(ReturnCode.NEXT_ROW);
-    filter = new FilterList(Operator.MUST_PASS_ONE, mockFilter);
-    filter.filterKeyValue(kv1);
-    assertTrue(mockFilter.didCellPassToTheFilter);
-
-    mockFilter.didCellPassToTheFilter = false;
-    filter.filterKeyValue(kv2);
-    assertFalse(mockFilter.didCellPassToTheFilter);
-
-    mockFilter.didCellPassToTheFilter = false;
-    filter.filterKeyValue(kv3);
-    assertFalse(mockFilter.didCellPassToTheFilter);
-
-    mockFilter.didCellPassToTheFilter = false;
-    filter.filterKeyValue(kv4);
-    assertTrue(mockFilter.didCellPassToTheFilter);
-  }
 }