HBASE-28622 FilterListWithAND can swallow SEEK_NEXT_USING_HINT #5955
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
I am open to suggestions for a better name for the HintingFilter marker interface. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
May 31, 2024
hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithAND.java
Show resolved
Hide resolved
| @@ -184,9 +205,20 @@ public ReturnCode filterCell(Cell c) throws IOException { | |||
| // otherwise we may mess up the global state (such as offset, count..) in the following | |||
| // sub-filters. (HBASE-20565) | |||
| if (!isIncludeRelatedReturnCode(rc)) { | |||
| return rc; | |||
| i++; | |||
There was a problem hiding this comment.
Ditto.
I think the intention here is that, usually SEEK_NEXT_USING_HINT can skip more rows, so even if we decide to skip the current cell, we'd better still looking to remaining filters to see if we can get a SEEK_NEXT_USING_HINT.
There was a problem hiding this comment.
will add more comments.
There was a problem hiding this comment.
Let's speak more here? It is not easy for a developer without the context of this issue here to understand, and please also mention the issue number.
hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithAND.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Added more comments, @Apache9 . |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Jun 1, 2024
| retVal = true; | ||
| anyFiltered = true; | ||
| } else if (hintingFilters[i]) { | ||
| // If any of the hinting filters has returned false, then we must not filter this rowkey. |
There was a problem hiding this comment.
I think hintingFilters being true only means it is a HintingFilter, does not mean it has returned false?
There was a problem hiding this comment.
That's right, hintingFilters is just caching if a filter is a HintingFilter, it's just a performance aid to reduce instanceof calls.
We know that filterRowKey() has returned false, becasue we are in the else clause of the if clause which checks the return value.
We do want the case where filterRowKey() has returned false.
if filterRowKey() returns true, then we skip the whole rowkey.
If it returns false, then the we call filterCell() on the individual cells (which is what we want in this case)
However, I realize that the patch regresses the filterAllRemaining() case performance.
If any of the filters returns true for filterAllRemaining, then we can just skip the row. I will update the patch.
| @@ -184,9 +205,20 @@ public ReturnCode filterCell(Cell c) throws IOException { | |||
| // otherwise we may mess up the global state (such as offset, count..) in the following | |||
| // sub-filters. (HBASE-20565) | |||
| if (!isIncludeRelatedReturnCode(rc)) { | |||
| return rc; | |||
| i++; | |||
There was a problem hiding this comment.
Let's speak more here? It is not easy for a developer without the context of this issue here to understand, and please also mention the issue number.
|
I have fixed two bugs in the patch:
|
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Jun 3, 2024
| break; | ||
| } | ||
| } | ||
| // We have the preliminary return code. However, if there are remaining uncalled hintingFilters, |
| for (int i = 0, n = filters.size(); i < n; i++) { | ||
| Filter filter = filters.get(i); | ||
| if (filter.filterAllRemaining() || filter.filterRowKey(firstRowCell)) { | ||
| if (filter.filterAllRemaining()) { |
There was a problem hiding this comment.
Do we need to check this here? We do have a filterAllRemaining method below?
There was a problem hiding this comment.
Technically no, but we do it anyway everywhere, so I didn't want to remove it just here.
There is also an MR job that possibly requires this.
I have opened HBASE-28633 for this.
There was a problem hiding this comment.
I realize that I referred a wrong ticket in the comment.
Fixing.
hbase-client/src/main/java/org/apache/hadoop/hbase/filter/FilterListWithAND.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Jun 4, 2024
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
stoty
added a commit
that referenced
this pull request
Jun 4, 2024
Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit c0fb41f)
stoty
added a commit
that referenced
this pull request
Jun 4, 2024
Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit c0fb41f)
stoty
added a commit
that referenced
this pull request
Jun 4, 2024
Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit c0fb41f)
stoty
added a commit
that referenced
this pull request
Jun 4, 2024
Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit c0fb41f)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.