From e464ea248b20633d227000b33f8f42ac426c283e Mon Sep 17 00:00:00 2001 From: huzheng Date: Tue, 14 Nov 2017 15:04:48 +0800 Subject: [PATCH] HBASE-19252 Move the transform logic of FilterList into transformCell() method to avoid extra ref to question cell --- .../org/apache/hadoop/hbase/filter/FilterList.java | 19 -------- .../apache/hadoop/hbase/filter/FilterListBase.java | 57 +++++++++------------- .../hadoop/hbase/filter/FilterListWithAND.java | 17 +++---- .../hadoop/hbase/filter/FilterListWithOR.java | 17 +++---- .../apache/hadoop/hbase/filter/TestFilterList.java | 49 +++++++++++++++++++ 5 files changed, 84 insertions(+), 75 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 4f9a8d8..3b6455e 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 @@ -165,25 +165,6 @@ final public class FilterList extends FilterBase { return filterListBase.transformCell(c); } - /** - * Internal implementation of {@link #filterCell(Cell)}. Compared to the - * {@link #filterCell(Cell)} method, this method accepts an additional parameter named - * transformedCell. This parameter indicates the initial value of transformed cell before this - * filter operation.
- * 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 internalFilterCell(Cell c, Cell transformedCell) throws IOException { - return this.filterListBase.internalFilterCell(c, transformedCell); - } - @Override @Deprecated public ReturnCode filterKeyValue(final Cell c) throws IOException { 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 4087437..19ae698 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 @@ -22,12 +22,11 @@ package org.apache.hadoop.hbase.filter; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; 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.KeyValueUtil; import org.apache.yetus.audience.InterfaceAudience; /** @@ -38,21 +37,17 @@ import org.apache.yetus.audience.InterfaceAudience; public abstract class FilterListBase extends FilterBase { private static final int MAX_LOG_FILTERS = 5; protected final ArrayList filters; - - /** Reference Cell used by {@link #transformCell(Cell)} for validation purpose. */ - protected Cell referenceCell = null; - /** - * When filtering a given Cell in {@link #filterCell(Cell)}, this stores the transformed Cell - * to be returned by {@link #transformCell(Cell)}. Individual filters transformation are applied - * only when the filter includes the Cell. Transformations are composed in the order specified by - * {@link #filters}. + * For each sub-filter in filter list, we save a boolean flag to indicate that whether the return + * code of filterCell(c) for sub-filter is INCLUDE* (INCLUDE, INCLUDE_AND_NEXT_COL, + * INCLUDE_AND_SEEK_NEXT_ROW) case. if true, we need to transform cell for the sub-filter. */ - protected Cell transformedCell = null; + protected final ArrayList returnCodeIncludeCase; public FilterListBase(List filters) { reversed = checkAndGetReversed(filters, reversed); this.filters = new ArrayList<>(filters); + this.returnCodeIncludeCase = new ArrayList<>(Collections.nCopies(filters.size(), false)); } protected static boolean isInReturnCodes(ReturnCode testRC, ReturnCode... returnCodes) { @@ -90,43 +85,35 @@ public abstract class FilterListBase extends FilterBase { return reversed ? -1 * cmp : cmp; } + /** + * 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. (HBASE-18879). + * @param c The cell in question. + * @return the transformed cell. + * @throws IOException + */ @Override public Cell transformCell(Cell c) throws IOException { if (isEmpty()) { return super.transformCell(c); } - if (!CellUtil.equals(c, referenceCell)) { - throw new IllegalStateException( - "Reference Cell: " + this.referenceCell + " does not match: " + c); + Cell transformed = c; + for (int i = 0, n = filters.size(); i < n; i++) { + if (returnCodeIncludeCase.get(i)) { + transformed = filters.get(i).transformCell(transformed); + } } - // Copy transformedCell into a new cell and reset transformedCell & referenceCell to null for - // Java GC optimization - Cell cell = KeyValueUtil.copyToNewKeyValue(this.transformedCell); - this.transformedCell = null; - this.referenceCell = null; - return cell; + return transformed; } - /** - * Internal implementation of {@link #filterCell(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#internalFilterCell(Cell, Cell) - */ - abstract ReturnCode internalFilterCell(Cell c, Cell transformedCell) throws IOException; - @Override public ReturnCode filterKeyValue(final Cell c) throws IOException { return filterCell(c); } - @Override - public ReturnCode filterCell(final Cell c) throws IOException { - return internalFilterCell(c, c); - } - /** * Filters that never filter by modifying the returned List of Cells can inherit this * implementation that does nothing. {@inheritDoc} 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 9d4e619..570ea63 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 @@ -24,6 +24,7 @@ import org.apache.yetus.audience.InterfaceAudience; import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; import java.util.List; /** @@ -45,6 +46,7 @@ public class FilterListWithAND extends FilterListBase { throw new IllegalArgumentException("Filters in the list must have the same reversed flag"); } this.filters.addAll(filters); + this.returnCodeIncludeCase.addAll(Collections.nCopies(filters.size(), false)); } @Override @@ -149,37 +151,31 @@ public class FilterListWithAND extends FilterListBase { } @Override - ReturnCode internalFilterCell(Cell c, Cell transformedCell) throws IOException { + public ReturnCode filterCell(Cell c) throws IOException { if (isEmpty()) { return ReturnCode.INCLUDE; } ReturnCode rc = ReturnCode.INCLUDE; - Cell transformed = transformedCell; - this.referenceCell = c; this.seekHintFilters.clear(); for (int i = 0, n = filters.size(); i < n; i++) { Filter filter = filters.get(i); + returnCodeIncludeCase.set(i, false); if (filter.filterAllRemaining()) { return ReturnCode.NEXT_ROW; } ReturnCode localRC; - if (filter instanceof FilterList) { - localRC = ((FilterList) filter).internalFilterCell(c, transformed); - } else { - localRC = filter.filterCell(c); - } + localRC = filter.filterCell(c); rc = mergeReturnCode(rc, localRC); // For INCLUDE* case, we need to update the transformed cell. if (isInReturnCodes(localRC, ReturnCode.INCLUDE, ReturnCode.INCLUDE_AND_NEXT_COL, ReturnCode.INCLUDE_AND_SEEK_NEXT_ROW)) { - transformed = filter.transformCell(transformed); + returnCodeIncludeCase.set(i, true); } if (localRC == ReturnCode.SEEK_NEXT_USING_HINT) { seekHintFilters.add(filter); } } - this.transformedCell = transformed; if (!seekHintFilters.isEmpty()) { return ReturnCode.SEEK_NEXT_USING_HINT; } @@ -190,6 +186,7 @@ public class FilterListWithAND extends FilterListBase { public void reset() throws IOException { for (int i = 0, n = filters.size(); i < n; i++) { filters.get(i).reset(); + returnCodeIncludeCase.set(i, false); } seekHintFilters.clear(); } 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 51886bc..55814da 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 @@ -56,6 +56,7 @@ public class FilterListWithOR extends FilterListBase { throw new IllegalArgumentException("Filters in the list must have the same reversed flag"); } this.filters.addAll(filters); + this.returnCodeIncludeCase.addAll(Collections.nCopies(filters.size(), false)); this.prevFilterRCList.addAll(Collections.nCopies(filters.size(), null)); this.prevCellList.addAll(Collections.nCopies(filters.size(), null)); } @@ -246,16 +247,15 @@ public class FilterListWithOR extends FilterListBase { } @Override - ReturnCode internalFilterCell(Cell c, Cell transformCell) throws IOException { + public ReturnCode filterCell(Cell c) throws IOException { if (isEmpty()) { return ReturnCode.INCLUDE; } ReturnCode rc = null; boolean everyFilterReturnHint = true; - Cell transformed = transformCell; - this.referenceCell = c; for (int i = 0, n = filters.size(); i < n; i++) { Filter filter = filters.get(i); + returnCodeIncludeCase.set(i, false); Cell prevCell = this.prevCellList.get(i); ReturnCode prevCode = this.prevFilterRCList.get(i); @@ -264,12 +264,7 @@ public class FilterListWithOR extends FilterListBase { continue; } - ReturnCode localRC; - if (filter instanceof FilterList) { - localRC = ((FilterList) filter).internalFilterCell(c, transformed); - } else { - localRC = filter.filterCell(c); - } + ReturnCode localRC = filter.filterCell(c); // Update previous return code and previous cell for filter[i]. updatePrevFilterRCList(i, localRC); @@ -284,11 +279,10 @@ public class FilterListWithOR extends FilterListBase { // For INCLUDE* case, we need to update the transformed cell. if (isInReturnCodes(localRC, ReturnCode.INCLUDE, ReturnCode.INCLUDE_AND_NEXT_COL, ReturnCode.INCLUDE_AND_SEEK_NEXT_ROW)) { - transformed = filter.transformCell(transformed); + returnCodeIncludeCase.set(i, true); } } - this.transformedCell = transformed; if (everyFilterReturnHint) { return ReturnCode.SEEK_NEXT_USING_HINT; } else if (rc == null) { @@ -303,6 +297,7 @@ public class FilterListWithOR extends FilterListBase { public void reset() throws IOException { for (int i = 0, n = filters.size(); i < n; i++) { filters.get(i).reset(); + returnCodeIncludeCase.set(i, false); prevFilterRCList.set(i, null); prevCellList.set(i, null); } 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 16a57fb..cba70c0 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 @@ -960,5 +960,54 @@ public class TestFilterList { filter.filterCell(kv2); assertEquals(2, mockNextRowFilter.getHitCount()); } + + private static class MockTransformFilter extends FilterBase { + private ReturnCode targetRetCode; + public boolean transformed = false; + + public MockTransformFilter(ReturnCode targetRetCode) { + this.targetRetCode = targetRetCode; + } + + @Override + public ReturnCode filterCell(final Cell v) throws IOException { + return targetRetCode; + } + + @Override + public Cell transformCell(Cell c) throws IOException { + transformed = true; + return super.transformCell(c); + } + } + + @Test + public void testTransformCell() throws IOException { + KeyValue kv = new KeyValue(Bytes.toBytes("row"), Bytes.toBytes("cf"), Bytes.toBytes("column1"), + 1, Bytes.toBytes("value")); + + // case MUST_PASS_ONE + MockTransformFilter filter1 = new MockTransformFilter(ReturnCode.INCLUDE); + MockTransformFilter filter2 = new MockTransformFilter(ReturnCode.NEXT_ROW); + MockTransformFilter filter3 = new MockTransformFilter(ReturnCode.SEEK_NEXT_USING_HINT); + FilterList filterList = new FilterList(Operator.MUST_PASS_ONE, filter1, filter2, filter3); + Assert.assertEquals(ReturnCode.INCLUDE, filterList.filterCell(kv)); + Assert.assertEquals(kv, filterList.transformCell(kv)); + Assert.assertEquals(true, filter1.transformed); + Assert.assertEquals(false, filter2.transformed); + Assert.assertEquals(false, filter3.transformed); + + // case MUST_PASS_ALL + filter1 = new MockTransformFilter(ReturnCode.INCLUDE); + filter2 = new MockTransformFilter(ReturnCode.INCLUDE_AND_SEEK_NEXT_ROW); + filter3 = new MockTransformFilter(ReturnCode.INCLUDE_AND_NEXT_COL); + filterList = new FilterList(Operator.MUST_PASS_ALL, filter1, filter2, filter3); + + Assert.assertEquals(ReturnCode.INCLUDE_AND_SEEK_NEXT_ROW, filterList.filterCell(kv)); + Assert.assertEquals(kv, filterList.transformCell(kv)); + Assert.assertEquals(true, filter1.transformed); + Assert.assertEquals(true, filter2.transformed); + Assert.assertEquals(true, filter3.transformed); + } } -- 2.3.2 (Apple Git-55)