hbase-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From zhang...@apache.org
Subject [01/10] hbase git commit: HBASE-19057 Fix other code review comments about FilterList improvement
Date Wed, 25 Oct 2017 12:38:36 GMT
Repository: hbase
Updated Branches:
  refs/heads/master 43a8ac001 -> 705b3fa98


HBASE-19057 Fix other code review comments about FilterList improvement


Project: http://git-wip-us.apache.org/repos/asf/hbase/repo
Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/705b3fa9
Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/705b3fa9
Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/705b3fa9

Branch: refs/heads/master
Commit: 705b3fa98c97806c7eba63617a99f62d829400d1
Parents: fcaf71d
Author: huzheng <openinx@gmail.com>
Authored: Tue Oct 24 15:30:55 2017 +0800
Committer: zhangduo <zhangduo@apache.org>
Committed: Wed Oct 25 20:36:19 2017 +0800

----------------------------------------------------------------------
 .../apache/hadoop/hbase/filter/FilterList.java  | 24 ++++++++++-----
 .../hadoop/hbase/filter/FilterListBase.java     | 16 ++++++++--
 .../hadoop/hbase/filter/FilterListWithAND.java  | 12 ++++----
 .../hadoop/hbase/filter/FilterListWithOR.java   | 31 +++++++++++---------
 .../hadoop/hbase/filter/TestFilterList.java     |  8 +++--
 5 files changed, 59 insertions(+), 32 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/hbase/blob/705b3fa9/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java
----------------------------------------------------------------------
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 e87f1b3..d4242ae 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
@@ -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

http://git-wip-us.apache.org/repos/asf/hbase/blob/705b3fa9/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListBase.java
----------------------------------------------------------------------
diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListBase.java
b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListBase.java
index 60b0dc1..f92d2e7 100644
--- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListBase.java
+++ b/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

http://git-wip-us.apache.org/repos/asf/hbase/blob/705b3fa9/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithAND.java
----------------------------------------------------------------------
diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithAND.java
b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithAND.java
index 4909dfd..9217ff9 100644
--- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithAND.java
+++ b/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++) {
@@ -186,11 +186,6 @@ public class FilterListWithAND 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++) {
       filters.get(i).reset();
@@ -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;
       }
     }

http://git-wip-us.apache.org/repos/asf/hbase/blob/705b3fa9/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithOR.java
----------------------------------------------------------------------
diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithOR.java
b/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithOR.java
index 31e2a55..e51915b 100644
--- a/hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithOR.java
+++ b/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)
-      throws IOException {
-    ReturnCode prevCode = this.prevFilterRCList.get(filterIdx);
+  private boolean shouldPassCurrentCellToFilter(Cell prevCell, Cell currentCell,
+      ReturnCode prevCode) throws IOException {
     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)
-          || !CellUtil.matchingFamily(prevCell, currentCell);
+      // As described above, rows are definitely the same, so we only compare the family.
+      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;
       }
@@ -295,11 +300,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++) {
       filters.get(i).reset();
@@ -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;
       }
     }

http://git-wip-us.apache.org/repos/asf/hbase/blob/705b3fa9/hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilterList.java
----------------------------------------------------------------------
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 d420b02..8c83cf6 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
@@ -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);


Mime
View raw message