Skip to content

Commit

Permalink
HACK! Revert "HBASE-17678 FilterList with MUST_PASS_ONE may lead to r…
Browse files Browse the repository at this point in the history 
…edundant 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 0d0c330.
  • Loading branch information
@petersomogyi
petersomogyi committed Jul 26, 2017
1 parent 8385bdc commit 7757f81
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 191 deletions.
76 changes: 2 additions & 74 deletions hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterList.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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());
}

/**
Expand All @@ -115,7 +106,6 @@ public FilterList(final Filter... rowFilters) {
public FilterList(final Operator operator) {
this.operator = operator;
this.filters = new ArrayList<>();
initPrevListForMustPassOne(filters.size());
}

/**
Expand All @@ -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());
}

/**
Expand All @@ -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.
*
Expand Down Expand Up @@ -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));
}
}

/**
Expand All @@ -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;
}
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}

Expand Down
117 changes: 0 additions & 117 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFilterList.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}

0 comments on commit 7757f81

Please sign in to comment.