-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-28621 PrefixFilter should use SEEK_NEXT_USING_HINT #6361
base: master
Are you sure you want to change the base?
HBASE-28621 PrefixFilter should use SEEK_NEXT_USING_HINT #6361
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hbase-client/src/main/java/org/apache/hadoop/hbase/filter/PrefixFilter.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
if (firstRowCell == null || this.prefix == null) return true; | ||
if (filterAllRemaining()) return true; | ||
if (firstRowCell == null || this.prefix == null) { | ||
return true; | ||
} | ||
if (filterAllRemaining()) { | ||
return true; | ||
} | ||
int length = firstRowCell.getRowLength(); | ||
if (length < prefix.length) return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are actually not strictly required to resolve the ticket. Just the checkstyle report contained these warnings:
(blocks) NeedBraces: 'if' construct must use '{}'s.
(blocks) NeedBraces: 'if' construct must use '{}'s.
(blocks) NeedBraces: 'if' construct must use '{}'s.
(blocks) NeedBraces: 'if' construct must use '{}'s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this if looks buggy.
If the firstRowCell is lexicographically smaller than the prefix, then we still want to provide the hint, regardless of the key length.
We can probably just drop this, I believe that the compare later will also handle this case.
This was supposed to be an optization, but it is not relevant when we provide hints, and in fact hurts performance.
Please fix and also add test cases for this.
@@ -40,7 +43,6 @@ public class TestPrefixFilter { | |||
static final char FIRST_CHAR = 'a'; | |||
static final char LAST_CHAR = 'e'; | |||
static final String HOST_PREFIX = "org.apache.site-"; | |||
static final byte[] GOOD_BYTES = Bytes.toBytes("abc"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a bit unrelated - an unused constant field.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
please remove the colon from the commit message / PR description |
(blocks) NeedBraces: 'if' construct must use '{}'s.
09fc843
to
02ed23a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
if (firstRowCell == null || this.prefix == null) return true; | ||
if (filterAllRemaining()) return true; | ||
if (firstRowCell == null || this.prefix == null) { | ||
return true; | ||
} | ||
if (filterAllRemaining()) { | ||
return true; | ||
} | ||
int length = firstRowCell.getRowLength(); | ||
if (length < prefix.length) return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this if looks buggy.
If the firstRowCell is lexicographically smaller than the prefix, then we still want to provide the hint, regardless of the key length.
We can probably just drop this, I believe that the compare later will also handle this case.
This was supposed to be an optization, but it is not relevant when we provide hints, and in fact hurts performance.
Please fix and also add test cases for this.
This comment has been minimized.
This comment has been minimized.
@@ -77,7 +77,7 @@ public boolean filterRowKey(Cell firstRowCell) { | |||
if ((!isReversed() && cmp > 0) || (isReversed() && cmp < 0)) { | |||
passedPrefix = true; | |||
} | |||
filterRow = (!isReversed() && cmp > 0) || (isReversed() && cmp < 0); | |||
filterRow = (!isReversed() && cmp > 0) || (isReversed() && cmp != 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks fishy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely agree. But this fixes the previously failing TestFilter.testPrefixFilterWithReverseScan unit test. 😅
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
No description provided.