HBASE-29672 Handle runtime comparison failures during filtering gracefully

hbase-client/src/main/java/org/apache/hadoop/hbase/filter/ColumnValueFilter.java

@@ -121,12 +121,16 @@ public ReturnCode filterCell(Cell c) throws IOException {
    * @return true means cell should be filtered out, included otherwise.
    */
   private boolean compareValue(final CompareOperator op, final ByteArrayComparable comparator,
-    final Cell cell) {
+    final Cell cell) throws IOException {
     if (op == CompareOperator.NO_OP) {
       return true;
     }
-    int compareResult = PrivateCellUtil.compareValue(cell, comparator);
-    return CompareFilter.compare(op, compareResult);
+    try {
+      int compareResult = PrivateCellUtil.compareValue(cell, comparator);
+      return CompareFilter.compare(op, compareResult);
+    } catch (RuntimeException e) {
+      throw CompareFilter.wrapInHBaseIOException(e, comparator);
+    }
   }
 
   /**

hbase-client/src/main/java/org/apache/hadoop/hbase/filter/CompareFilter.java

@@ -22,6 +22,7 @@
 import java.util.Objects;
 import org.apache.hadoop.hbase.Cell;
 import org.apache.hadoop.hbase.CompareOperator;
+import org.apache.hadoop.hbase.HBaseIOException;
 import org.apache.hadoop.hbase.PrivateCellUtil;
 import org.apache.hadoop.hbase.util.Bytes;
 import org.apache.yetus.audience.InterfaceAudience;
@@ -80,41 +81,72 @@ public boolean filterRowKey(Cell cell) throws IOException {
     return false;
   }
 
+  /**
+   * RuntimeException when applying a comparator indicates a code bug or misconfigured
+   * filter/comparator, we wrap it in `HBaseIOException` to provide a clear exception message/stack
+   * trace and prevent propagating a runtime exception up the call stack (which would lead to
+   * unexpected throwable at RpcServer layer and a complicated unclear remote exception on the
+   * client)
+   */
+  public static HBaseIOException wrapInHBaseIOException(RuntimeException e,
+    ByteArrayComparable comparator) {
+    String msg =
+      String.format("Runtime exception occurred when applying comparator %s during filtering",
+        comparator.getClass().getSimpleName());
+    return new HBaseIOException(msg, e);
+  }
+
   protected boolean compareRow(final CompareOperator op, final ByteArrayComparable comparator,
-    final Cell cell) {
+    final Cell cell) throws IOException {
     if (op == CompareOperator.NO_OP) {
       return true;
     }
-    int compareResult = PrivateCellUtil.compareRow(cell, comparator);
-    return compare(op, compareResult);
+    try {
+      int compareResult = PrivateCellUtil.compareRow(cell, comparator);
+      return compare(op, compareResult);
+    } catch (RuntimeException e) {
+      throw wrapInHBaseIOException(e, comparator);
+    }
   }
 
   protected boolean compareFamily(final CompareOperator op, final ByteArrayComparable comparator,
-    final Cell cell) {
+    final Cell cell) throws IOException {
     if (op == CompareOperator.NO_OP) {
       return true;
     }
-    int compareResult = PrivateCellUtil.compareFamily(cell, comparator);
-    return compare(op, compareResult);
+    try {
+      int compareResult = PrivateCellUtil.compareFamily(cell, comparator);
+      return compare(op, compareResult);
+    } catch (RuntimeException e) {
+      throw wrapInHBaseIOException(e, comparator);
+    }
   }
 
   protected boolean compareQualifier(final CompareOperator op, final ByteArrayComparable comparator,
-    final Cell cell) {
+    final Cell cell) throws IOException {
     // We do not call through to the non-deprecated method for perf reasons.
     if (op == CompareOperator.NO_OP) {
       return true;
     }
-    int compareResult = PrivateCellUtil.compareQualifier(cell, comparator);
-    return compare(op, compareResult);
+    try {
+      int compareResult = PrivateCellUtil.compareQualifier(cell, comparator);
+      return compare(op, compareResult);
+    } catch (RuntimeException e) {
+      throw wrapInHBaseIOException(e, comparator);
+    }
   }
 
   protected boolean compareValue(final CompareOperator op, final ByteArrayComparable comparator,
-    final Cell cell) {
+    final Cell cell) throws IOException {
     if (op == CompareOperator.NO_OP) {
       return true;
     }
-    int compareResult = PrivateCellUtil.compareValue(cell, comparator);
-    return compare(op, compareResult);
+    try {
+      int compareResult = PrivateCellUtil.compareValue(cell, comparator);
+      return compare(op, compareResult);
+    } catch (RuntimeException e) {
+      throw wrapInHBaseIOException(e, comparator);
+    }
   }
 
   static boolean compare(final CompareOperator op, int compareResult) {

hbase-client/src/main/java/org/apache/hadoop/hbase/filter/DependentColumnFilter.java

@@ -117,7 +117,7 @@ public boolean filterAllRemaining() {
   }
 
   @Override
-  public ReturnCode filterCell(final Cell c) {
+  public ReturnCode filterCell(final Cell c) throws IOException {
     // Check if the column and qualifier match
     if (!CellUtil.matchingColumn(c, this.columnFamily, this.columnQualifier)) {
       // include non-matches for the time being, they'll be discarded afterwards

hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FamilyFilter.java

@@ -56,7 +56,7 @@ public FamilyFilter(final CompareOperator op, final ByteArrayComparable familyCo
   }
 
   @Override
-  public ReturnCode filterCell(final Cell c) {
+  public ReturnCode filterCell(final Cell c) throws IOException {
     int familyLength = c.getFamilyLength();
     if (familyLength > 0) {
       if (compareFamily(getCompareOperator(), this.comparator, c)) {

hbase-client/src/main/java/org/apache/hadoop/hbase/filter/QualifierFilter.java

@@ -53,7 +53,7 @@ public QualifierFilter(final CompareOperator op, final ByteArrayComparable quali
   }
 
   @Override
-  public ReturnCode filterCell(final Cell c) {
+  public ReturnCode filterCell(final Cell c) throws IOException {
     if (compareQualifier(getCompareOperator(), this.comparator, c)) {
       return ReturnCode.SKIP;
     }

hbase-client/src/main/java/org/apache/hadoop/hbase/filter/RowFilter.java

@@ -67,7 +67,7 @@ public ReturnCode filterCell(final Cell v) {
   }
 
   @Override
-  public boolean filterRowKey(Cell firstRowCell) {
+  public boolean filterRowKey(Cell firstRowCell) throws IOException {
     if (compareRow(getCompareOperator(), this.comparator, firstRowCell)) {
       this.filterOutRow = true;
     }

hbase-client/src/main/java/org/apache/hadoop/hbase/filter/SingleColumnValueFilter.java

@@ -147,7 +147,7 @@ public boolean filterRowKey(Cell cell) throws IOException {
   }
 
   @Override
-  public ReturnCode filterCell(final Cell c) {
+  public ReturnCode filterCell(final Cell c) throws IOException {
     // System.out.println("REMOVE KEY=" + keyValue.toString() + ", value=" +
     // Bytes.toString(keyValue.getValue()));
     if (this.matchedColumn) {
@@ -168,9 +168,13 @@ public ReturnCode filterCell(final Cell c) {
     return ReturnCode.INCLUDE;
   }
 
-  private boolean filterColumnValue(final Cell cell) {
-    int compareResult = PrivateCellUtil.compareValue(cell, this.comparator);
-    return CompareFilter.compare(this.op, compareResult);
+  private boolean filterColumnValue(final Cell cell) throws IOException {
+    try {
+      int compareResult = PrivateCellUtil.compareValue(cell, this.comparator);
+      return CompareFilter.compare(this.op, compareResult);
+    } catch (RuntimeException e) {
+      throw CompareFilter.wrapInHBaseIOException(e, this.comparator);
+    }
   }
 
   @Override

hbase-client/src/main/java/org/apache/hadoop/hbase/filter/ValueFilter.java

@@ -54,7 +54,7 @@ public ValueFilter(final CompareOperator valueCompareOp,
   }
 
   @Override
-  public ReturnCode filterCell(final Cell c) {
+  public ReturnCode filterCell(final Cell c) throws IOException {
     if (compareValue(getCompareOperator(), this.comparator, c)) {
       return ReturnCode.SKIP;
     }

hbase-client/src/test/java/org/apache/hadoop/hbase/filter/TestFiltersWithComparatorException.java

@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.filter;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+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.CompareOperator;
+import org.apache.hadoop.hbase.HBaseIOException;
+import org.apache.hadoop.hbase.KeyValue;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(SmallTests.class)
+public class TestFiltersWithComparatorException {
+
+  /**
+   * Tests that filters which take a ByteArrayComparable comparator handle runtime exceptions in the
+   * comparator layer, see HBASE-29672
+   */
+
+  byte[] cf = Bytes.toBytes("cf");
+  byte[] row = Bytes.toBytes("row1");
+  byte[] cq = Bytes.toBytes("q");
+  long ts = 12345L;
+  byte[] value = Bytes.toBytes("value");
+  Cell testCell = new KeyValue(row, cf, cq, ts, value);
+
+  @FunctionalInterface
+  interface FilterFunctionThrowable {
+    void run(Filter filter) throws IOException;
+  }
+
+  // Every filterX method that Filter implements to test
+  List<FilterFunctionThrowable> filterFunctionsToTest = Arrays.asList((Filter filter) -> {
+    filter.filterRowKey(testCell);
+  }, (Filter filter) -> {
+    filter.filterAllRemaining();
+  }, (Filter filter) -> {
+    filter.filterCell(testCell);
+  }, (Filter filter) -> {
+    filter.filterRowCells(new ArrayList<>(Collections.singletonList(testCell)));
+  }, (Filter filter) -> {
+    filter.filterRow();
+  });
+
+  /**
+   * Comparator which throws RuntimeException for every `compareTo` method that
+   * `ByteArrayComparable` implements + keeps a counter for number of compareTo invocations /
+   * RuntimeExceptions thrown
+   */
+  static class BadComparator extends ByteArrayComparable {
+
+    public int compareToInvokations = 0;
+
+    public BadComparator() {
+      super(new byte[1]);
+    }
+
+    @Override
+    public byte[] toByteArray() {
+      return new byte[1];
+    }
+
+    @Override
+    public int compareTo(byte[] value, int offset, int length) {
+      compareToInvokations++;
+      throw new RuntimeException("comparator runtime exception");
+    }
+
+    @Override
+    public int compareTo(ByteBuffer value, int offset, int length) {
+      compareToInvokations++;
+      throw new RuntimeException("comparator runtime exception");
+    }
+
+    @Override
+    public int compareTo(byte[] value) {
+      compareToInvokations++;
+      throw new RuntimeException("comparator runtime exception");
+    }
+
+  }
+
+  /**
+   * Verifies that a given {@link Filter} correctly triggers comparator logic and wraps any runtime
+   * exceptions happening at comparison time in {@link org.apache.hadoop.hbase.HBaseIOException}
+   * <p>
+   * This method runs a set of predefined filter functions against the provided filter instance and
+   * checks that if a comparator invocation occurs which throws a RuntimeException, it gets wrapped
+   * in a {@code HBaseIOException} by the filter implementation The test fails if:
+   * <ul>
+   * <li>The comparator is never invoked by any tested function, or</li>
+   * <li>A comparator invocation does not result in the expected exception.</li>
+   * </ul>
+   * @param filter        the filter instance under test
+   * @param badComparator the comparator the filter was constructed with
+   **/
+  private void testFilter(Filter filter, BadComparator badComparator) {
+    for (FilterFunctionThrowable filterFunction : filterFunctionsToTest) {
+      int invocationsBefore = badComparator.compareToInvokations;
+      boolean ioExceptionThrown = false;
+      try {
+        filterFunction.run(filter);
+      } catch (HBaseIOException e) {
+        ioExceptionThrown = true;
+      } catch (IOException ignored) {
+      }
+      if (invocationsBefore != badComparator.compareToInvokations) {
+        Assert.assertTrue("IOException should have been thrown", ioExceptionThrown);
+      }
+    }
+    if (badComparator.compareToInvokations == 0) {
+      Assert
+        .fail(String.format(
+          "Filter %s never invoked the comparator for any of the functions tested - "
+            + "intended behavior was not tested, this is not expected",
+          filter.getClass().getName()));
+    }
+  }
+
+  @Test
+  public void testColumnValueFilter() {
+    BadComparator comparator = new BadComparator();
+    ColumnValueFilter columnValueFilter =
+      new ColumnValueFilter(cf, cq, CompareOperator.EQUAL, comparator);
+    testFilter(columnValueFilter, comparator);
+  }
+
+  @Test
+  public void testRowFilter() {
+    BadComparator comparator = new BadComparator();
+    RowFilter rowFilter = new RowFilter(CompareOperator.EQUAL, comparator);
+    testFilter(rowFilter, comparator);
+  }
+
+  @Test
+  public void testDependentColumnFilter() {
+    BadComparator comparator = new BadComparator();
+    DependentColumnFilter filter =
+      new DependentColumnFilter(cf, cq, false, CompareOperator.EQUAL, comparator);
+    testFilter(filter, comparator);
+  }
+
+  @Test
+  public void testFamilyFilter() {
+    BadComparator comparator = new BadComparator();
+    FamilyFilter filter = new FamilyFilter(CompareOperator.EQUAL, comparator);
+    testFilter(filter, comparator);
+  }
+
+  @Test
+  public void testQualifierFilter() {
+    BadComparator comparator = new BadComparator();
+    QualifierFilter filter = new QualifierFilter(CompareOperator.EQUAL, comparator);
+    testFilter(filter, comparator);
+  }
+
+  @Test
+  public void testSingleColumnValueExcludeFilter() {
+    BadComparator comparator = new BadComparator();
+    SingleColumnValueExcludeFilter filter =
+      new SingleColumnValueExcludeFilter(cf, cq, CompareOperator.EQUAL, comparator);
+    testFilter(filter, comparator);
+  }
+
+  @Test
+  public void testSingleColumnValueFilter() {
+    BadComparator comparator = new BadComparator();
+    SingleColumnValueFilter filter =
+      new SingleColumnValueFilter(cf, cq, CompareOperator.EQUAL, comparator);
+    testFilter(filter, comparator);
+  }
+
+  @Test
+  public void testValueFilter() {
+    BadComparator comparator = new BadComparator();
+    ValueFilter filter = new ValueFilter(CompareOperator.EQUAL, comparator);
+    testFilter(filter, comparator);
+  }
+
+}