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/HBASE-18410 Commit: 705b3fa98c97806c7eba63617a99f62d829400d1 Parents: fcaf71d Author: huzheng <open...@gmail.com> Authored: Tue Oct 24 15:30:55 2017 +0800 Committer: zhangduo <zhang...@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);
