Repository: hbase Updated Branches: refs/heads/branch-2 8b30adb83 -> 57291108e
HBASE-19252 Move the transform logic of FilterList into transformCell() method to avoid extra ref to question cell Project: http://git-wip-us.apache.org/repos/asf/hbase/repo Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/57291108 Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/57291108 Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/57291108 Branch: refs/heads/branch-2 Commit: 57291108ed44568fe9e39ab4702d8e158c87273e Parents: 8b30adb Author: huzheng <open...@gmail.com> Authored: Tue Nov 14 15:04:48 2017 +0800 Committer: huzheng <open...@gmail.com> Committed: Fri Nov 17 10:42:58 2017 +0800 ---------------------------------------------------------------------- .../apache/hadoop/hbase/filter/FilterList.java | 19 ------- .../hadoop/hbase/filter/FilterListBase.java | 55 +++++++------------- .../hadoop/hbase/filter/FilterListWithAND.java | 22 +++----- .../hadoop/hbase/filter/FilterListWithOR.java | 18 +++---- .../hadoop/hbase/filter/TestFilterList.java | 54 +++++++++++++++++++ 5 files changed, 89 insertions(+), 79 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hbase/blob/57291108/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 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. <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 internalFilterCell(Cell c, Cell transformedCell) throws IOException { - return this.filterListBase.internalFilterCell(c, transformedCell); - } - @Override @Deprecated public ReturnCode filterKeyValue(final Cell c) throws IOException { http://git-wip-us.apache.org/repos/asf/hbase/blob/57291108/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 4087437..e02f7e2 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 @@ -26,8 +26,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.KeyValueUtil; import org.apache.yetus.audience.InterfaceAudience; /** @@ -38,17 +36,12 @@ import org.apache.yetus.audience.InterfaceAudience; public abstract class FilterListBase extends FilterBase { private static final int MAX_LOG_FILTERS = 5; protected final ArrayList<Filter> 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 ArrayList<Boolean> subFiltersIncludedCell; public FilterListBase(List<Filter> filters) { reversed = checkAndGetReversed(filters, reversed); @@ -90,43 +83,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 (subFiltersIncludedCell.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} http://git-wip-us.apache.org/repos/asf/hbase/blob/57291108/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 9d4e619..9f2ca21 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; /** @@ -37,6 +38,10 @@ public class FilterListWithAND extends FilterListBase { public FilterListWithAND(List<Filter> filters) { super(filters); + // For FilterList with AND, when call FL's transformCell(), we should transform cell for all + // sub-filters (because all sub-filters return INCLUDE*). So here, fill this array with true. we + // keep this in FilterListWithAND for abstracting the transformCell() in FilterListBase. + subFiltersIncludedCell = new ArrayList<>(Collections.nCopies(filters.size(), true)); } @Override @@ -45,6 +50,7 @@ public class FilterListWithAND extends FilterListBase { throw new IllegalArgumentException("Filters in the list must have the same reversed flag"); } this.filters.addAll(filters); + this.subFiltersIncludedCell.addAll(Collections.nCopies(filters.size(), true)); } @Override @@ -149,13 +155,11 @@ 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); @@ -163,23 +167,13 @@ public class FilterListWithAND extends FilterListBase { 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); - } if (localRC == ReturnCode.SEEK_NEXT_USING_HINT) { seekHintFilters.add(filter); } } - this.transformedCell = transformed; if (!seekHintFilters.isEmpty()) { return ReturnCode.SEEK_NEXT_USING_HINT; } http://git-wip-us.apache.org/repos/asf/hbase/blob/57291108/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 51886bc..064dd83 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 @@ -48,6 +48,7 @@ public class FilterListWithOR extends FilterListBase { super(filters); prevFilterRCList = new ArrayList<>(Collections.nCopies(filters.size(), null)); prevCellList = new ArrayList<>(Collections.nCopies(filters.size(), null)); + subFiltersIncludedCell = new ArrayList<>(Collections.nCopies(filters.size(), false)); } @Override @@ -56,6 +57,7 @@ public class FilterListWithOR extends FilterListBase { throw new IllegalArgumentException("Filters in the list must have the same reversed flag"); } this.filters.addAll(filters); + this.subFiltersIncludedCell.addAll(Collections.nCopies(filters.size(), false)); this.prevFilterRCList.addAll(Collections.nCopies(filters.size(), null)); this.prevCellList.addAll(Collections.nCopies(filters.size(), null)); } @@ -246,16 +248,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); + subFiltersIncludedCell.set(i, false); Cell prevCell = this.prevCellList.get(i); ReturnCode prevCode = this.prevFilterRCList.get(i); @@ -264,12 +265,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 +280,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); + subFiltersIncludedCell.set(i, true); } } - this.transformedCell = transformed; if (everyFilterReturnHint) { return ReturnCode.SEEK_NEXT_USING_HINT; } else if (rc == null) { @@ -303,6 +298,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(); + subFiltersIncludedCell.set(i, false); prevFilterRCList.set(i, null); prevCellList.set(i, null); } http://git-wip-us.apache.org/repos/asf/hbase/blob/57291108/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 16a57fb..fa5981e 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,59 @@ public class TestFilterList { filter.filterCell(kv2); assertEquals(2, mockNextRowFilter.getHitCount()); } + + private static class TransformFilter extends FilterBase { + private ReturnCode targetRetCode; + private boolean transformed = false; + + public TransformFilter(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); + } + + public boolean getTransformed() { + return this.transformed; + } + } + + @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 + TransformFilter filter1 = new TransformFilter(ReturnCode.INCLUDE); + TransformFilter filter2 = new TransformFilter(ReturnCode.NEXT_ROW); + TransformFilter filter3 = new TransformFilter(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.getTransformed()); + Assert.assertEquals(false, filter2.getTransformed()); + Assert.assertEquals(false, filter3.getTransformed()); + + // case MUST_PASS_ALL + filter1 = new TransformFilter(ReturnCode.INCLUDE); + filter2 = new TransformFilter(ReturnCode.INCLUDE_AND_SEEK_NEXT_ROW); + filter3 = new TransformFilter(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.getTransformed()); + Assert.assertEquals(true, filter2.getTransformed()); + Assert.assertEquals(true, filter3.getTransformed()); + } }