Copilot commented on code in PR #8360:
URL: https://github.com/apache/hbase/pull/8360#discussion_r3426743288
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FuzzyRowFilter.java:
##########
@@ -216,9 +216,14 @@ public ReturnCode filterCell(final Cell c) {
return ReturnCode.INCLUDE;
}
}
- // NOT FOUND -> seek next using hint
+ // NOT FOUND -> seek next using hint or skip the current row.
lastFoundIndex = -1;
filterRow = true;
+ // For reverse scans, a non-matching row can recreate itself as the next
hint. Since fuzzy
+ // matching is row-key based, skip the whole non-matching row instead of
seeking to it again.
+ if (isReversed() && tracker.updateTracker(c) &&
tracker.isNextRowSameAs(c)) {
+ return ReturnCode.NEXT_ROW;
+ }
return ReturnCode.SEEK_NEXT_USING_HINT;
Review Comment:
`tracker.updateTracker(...)` is now invoked in `filterCell` (reverse
non-match path) and will be invoked again in `getNextCellHint` for the common
`SEEK_NEXT_USING_HINT` flow. Since `updateTracker` has side effects and can be
non-trivial, consider avoiding double-updating per cell by caching/memoizing
the tracker update result for the current row/cell (e.g., track the last row
processed and whether the queue is already updated) or by refactoring so only
`getNextCellHint` performs the update.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FuzzyRowFilter.java:
##########
@@ -231,6 +236,12 @@ public Cell getNextCellHint(Cell currentCell) {
return null;
}
byte[] nextRowKey = tracker.nextRow();
+ if (isReversed() && !tracker.lessThan(currentCell, nextRowKey)) {
+ // filterCell normally handles same-row reverse hints with NEXT_ROW. If
a non-progressing
+ // hint still reaches here, keep the current-row boundary to avoid
skipping matching rows
+ // under it, but return a non-seeking hint so StoreScanner advances
normally.
+ return PrivateCellUtil.createLastOnRow(currentCell);
Review Comment:
The comment says “return a non-seeking hint”, but `SEEK_NEXT_USING_HINT`
still causes the scanner to reseek—this hint is just constrained to the current
row (last cell) to force progress. Suggest rewording to avoid ambiguity (e.g.,
“return a hint within the current row (last cell) so the scanner can advance
past the row”).
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFuzzyRowFilterEndToEnd.java:
##########
@@ -330,6 +331,103 @@ public void testHBASE26967(TestInfo testInfo) throws
IOException {
TEST_UTIL.deleteTable(TableName.valueOf(name));
}
+ @Test
+ public void testReverseScanMovesPastSameRowFuzzyHint(TestInfo testInfo)
throws IOException {
+ final String cf = "f";
+ final String cq = "q";
+
+ String name =
testInfo.getTestMethod().orElseThrow(AssertionError::new).getName();
+ try (Table ht = TEST_UTIL.createTable(TableName.valueOf(name),
Bytes.toBytes(cf))) {
+ List<Put> puts = Lists.newArrayList();
+ puts.add(new Put(Bytes.toBytes("aaa")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ puts.add(new Put(Bytes.toBytes("aba")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ puts.add(new Put(Bytes.toBytes("abaa")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ puts.add(new Put(Bytes.toBytes("abb")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ puts.add(new Put(Bytes.toBytes("abc")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ ht.put(puts);
+
+ TEST_UTIL.flush();
+
+ List<Pair<byte[], byte[]>> fuzzyList = new LinkedList<>();
+ fuzzyList.add(new Pair<>(Bytes.toBytes("aaa"), new byte[] { 0, 1, 0 }));
+
+ Scan scan = new Scan();
+ scan.setReversed(true);
+ scan.setFilter(new FuzzyRowFilter(fuzzyList));
+
+ try (ResultScanner scanner = ht.getScanner(scan)) {
+ Result result = scanner.next();
+ assertNotNull(result);
+ assertEquals("abaa", Bytes.toString(result.getRow()));
+ result = scanner.next();
+ assertNotNull(result);
+ assertEquals("aba", Bytes.toString(result.getRow()));
+ result = scanner.next();
+ assertNotNull(result);
+ assertEquals("aaa", Bytes.toString(result.getRow()));
+ assertNull(scanner.next());
+ }
+ }
+
+ TEST_UTIL.deleteTable(TableName.valueOf(name));
Review Comment:
`deleteTable(...)` is executed after the try-with-resources block, so it
won’t run if an assertion or scanner operation throws inside the test. To
ensure cleanup even on failure (and reduce potential cascading
failures/resource buildup in the mini-cluster), wrap `deleteTable` in a
`finally` or use a test rule/extension that guarantees table cleanup.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFuzzyRowFilterEndToEnd.java:
##########
@@ -330,6 +331,103 @@ public void testHBASE26967(TestInfo testInfo) throws
IOException {
TEST_UTIL.deleteTable(TableName.valueOf(name));
}
+ @Test
+ public void testReverseScanMovesPastSameRowFuzzyHint(TestInfo testInfo)
throws IOException {
+ final String cf = "f";
+ final String cq = "q";
+
+ String name =
testInfo.getTestMethod().orElseThrow(AssertionError::new).getName();
+ try (Table ht = TEST_UTIL.createTable(TableName.valueOf(name),
Bytes.toBytes(cf))) {
Review Comment:
`deleteTable(...)` is executed after the try-with-resources block, so it
won’t run if an assertion or scanner operation throws inside the test. To
ensure cleanup even on failure (and reduce potential cascading
failures/resource buildup in the mini-cluster), wrap `deleteTable` in a
`finally` or use a test rule/extension that guarantees table cleanup.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFuzzyRowFilterEndToEnd.java:
##########
@@ -330,6 +331,103 @@ public void testHBASE26967(TestInfo testInfo) throws
IOException {
TEST_UTIL.deleteTable(TableName.valueOf(name));
}
+ @Test
+ public void testReverseScanMovesPastSameRowFuzzyHint(TestInfo testInfo)
throws IOException {
+ final String cf = "f";
+ final String cq = "q";
+
+ String name =
testInfo.getTestMethod().orElseThrow(AssertionError::new).getName();
+ try (Table ht = TEST_UTIL.createTable(TableName.valueOf(name),
Bytes.toBytes(cf))) {
+ List<Put> puts = Lists.newArrayList();
+ puts.add(new Put(Bytes.toBytes("aaa")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ puts.add(new Put(Bytes.toBytes("aba")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ puts.add(new Put(Bytes.toBytes("abaa")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ puts.add(new Put(Bytes.toBytes("abb")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ puts.add(new Put(Bytes.toBytes("abc")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ ht.put(puts);
+
+ TEST_UTIL.flush();
+
+ List<Pair<byte[], byte[]>> fuzzyList = new LinkedList<>();
+ fuzzyList.add(new Pair<>(Bytes.toBytes("aaa"), new byte[] { 0, 1, 0 }));
+
+ Scan scan = new Scan();
+ scan.setReversed(true);
+ scan.setFilter(new FuzzyRowFilter(fuzzyList));
+
+ try (ResultScanner scanner = ht.getScanner(scan)) {
+ Result result = scanner.next();
+ assertNotNull(result);
+ assertEquals("abaa", Bytes.toString(result.getRow()));
+ result = scanner.next();
+ assertNotNull(result);
+ assertEquals("aba", Bytes.toString(result.getRow()));
+ result = scanner.next();
+ assertNotNull(result);
+ assertEquals("aaa", Bytes.toString(result.getRow()));
+ assertNull(scanner.next());
+ }
+ }
+
+ TEST_UTIL.deleteTable(TableName.valueOf(name));
+ }
+
+ @Test
+ public void
testReverseScanMovesPastSameRowFuzzyHintAcrossMultipleCells(TestInfo testInfo)
+ throws IOException {
+ final String cf = "f";
+
+ String name =
testInfo.getTestMethod().orElseThrow(AssertionError::new).getName();
+ try (Table ht = TEST_UTIL.createTable(TableName.valueOf(name),
Bytes.toBytes(cf))) {
+ List<Put> puts = Lists.newArrayList();
+ puts.add(new Put(Bytes.toBytes("aaa")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes("q1"),
+ Bytes.toBytes("v")));
+ puts.add(new Put(Bytes.toBytes("aba")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes("q1"),
+ Bytes.toBytes("v")));
+ puts.add(new Put(Bytes.toBytes("abaa")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes("q1"),
+ Bytes.toBytes("v")));
+ // The same-row reverse hint can be recreated for each cell on this
non-matching row.
+ // Before the fix, the first cell could loop inside RowTracker and never
return to
+ // StoreScanner. After the fix, NEXT_ROW skips the whole row without
skipping abaa.
+ puts.add(new Put(Bytes.toBytes("abb"))
+ .addColumn(Bytes.toBytes(cf), Bytes.toBytes("q1"), Bytes.toBytes("v1"))
+ .addColumn(Bytes.toBytes(cf), Bytes.toBytes("q2"), Bytes.toBytes("v2"))
+ .addColumn(Bytes.toBytes(cf), Bytes.toBytes("q3"),
Bytes.toBytes("v3")));
+ puts.add(new Put(Bytes.toBytes("abc")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes("q1"),
+ Bytes.toBytes("v")));
+ ht.put(puts);
+
+ TEST_UTIL.flush();
+
+ List<Pair<byte[], byte[]>> fuzzyList = new LinkedList<>();
+ fuzzyList.add(new Pair<>(Bytes.toBytes("aaa"), new byte[] { 0, 1, 0 }));
+
+ Scan scan = new Scan();
+ scan.setReversed(true);
+ scan.setFilter(new FuzzyRowFilter(fuzzyList));
+
+ try (ResultScanner scanner = ht.getScanner(scan)) {
+ Result result = scanner.next();
+ assertNotNull(result);
+ assertEquals("abaa", Bytes.toString(result.getRow()));
+ result = scanner.next();
+ assertNotNull(result);
+ assertEquals("aba", Bytes.toString(result.getRow()));
+ result = scanner.next();
+ assertNotNull(result);
+ assertEquals("aaa", Bytes.toString(result.getRow()));
+ assertNull(scanner.next());
+ }
+ }
+
+ TEST_UTIL.deleteTable(TableName.valueOf(name));
Review Comment:
`deleteTable(...)` is executed after the try-with-resources block, so it
won’t run if an assertion or scanner operation throws inside the test. To
ensure cleanup even on failure (and reduce potential cascading
failures/resource buildup in the mini-cluster), wrap `deleteTable` in a
`finally` or use a test rule/extension that guarantees table cleanup.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFuzzyRowFilterEndToEnd.java:
##########
@@ -330,6 +331,103 @@ public void testHBASE26967(TestInfo testInfo) throws
IOException {
TEST_UTIL.deleteTable(TableName.valueOf(name));
}
+ @Test
+ public void testReverseScanMovesPastSameRowFuzzyHint(TestInfo testInfo)
throws IOException {
+ final String cf = "f";
+ final String cq = "q";
+
+ String name =
testInfo.getTestMethod().orElseThrow(AssertionError::new).getName();
+ try (Table ht = TEST_UTIL.createTable(TableName.valueOf(name),
Bytes.toBytes(cf))) {
+ List<Put> puts = Lists.newArrayList();
+ puts.add(new Put(Bytes.toBytes("aaa")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ puts.add(new Put(Bytes.toBytes("aba")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ puts.add(new Put(Bytes.toBytes("abaa")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ puts.add(new Put(Bytes.toBytes("abb")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ puts.add(new Put(Bytes.toBytes("abc")).addColumn(Bytes.toBytes(cf),
Bytes.toBytes(cq),
+ Bytes.toBytes("v")));
+ ht.put(puts);
+
+ TEST_UTIL.flush();
+
+ List<Pair<byte[], byte[]>> fuzzyList = new LinkedList<>();
+ fuzzyList.add(new Pair<>(Bytes.toBytes("aaa"), new byte[] { 0, 1, 0 }));
+
+ Scan scan = new Scan();
+ scan.setReversed(true);
+ scan.setFilter(new FuzzyRowFilter(fuzzyList));
+
+ try (ResultScanner scanner = ht.getScanner(scan)) {
+ Result result = scanner.next();
+ assertNotNull(result);
+ assertEquals("abaa", Bytes.toString(result.getRow()));
+ result = scanner.next();
+ assertNotNull(result);
+ assertEquals("aba", Bytes.toString(result.getRow()));
+ result = scanner.next();
+ assertNotNull(result);
+ assertEquals("aaa", Bytes.toString(result.getRow()));
+ assertNull(scanner.next());
+ }
+ }
+
+ TEST_UTIL.deleteTable(TableName.valueOf(name));
+ }
+
+ @Test
+ public void
testReverseScanMovesPastSameRowFuzzyHintAcrossMultipleCells(TestInfo testInfo)
+ throws IOException {
+ final String cf = "f";
+
+ String name =
testInfo.getTestMethod().orElseThrow(AssertionError::new).getName();
+ try (Table ht = TEST_UTIL.createTable(TableName.valueOf(name),
Bytes.toBytes(cf))) {
Review Comment:
`deleteTable(...)` is executed after the try-with-resources block, so it
won’t run if an assertion or scanner operation throws inside the test. To
ensure cleanup even on failure (and reduce potential cascading
failures/resource buildup in the mini-cluster), wrap `deleteTable` in a
`finally` or use a test rule/extension that guarantees table cleanup.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestFuzzyRowFilter.java:
##########
@@ -302,6 +307,24 @@ public void testGetNextForFuzzyRuleReverse() {
new byte[] { 2, 1, 1, 1, 0 }, // row to check
new byte[] { 1, 2, (byte) 255, 4 }); // expected next
+ // no before cell than current which satisfies the fuzzy row -> null
+ assertNull(FuzzyRowFilter.getNextForFuzzyRule(true, new byte[] { 1, 1, 1,
3, 0 },
+ new byte[] { 1, 2, 0, 3 }, new byte[] { -1, -1, 0, -1 }));
Review Comment:
Grammar in the comment is off; consider adjusting for clarity (e.g., “no
cell before the current one satisfies the fuzzy row -> null”).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
