-
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-25364 Redo the getMidPoint() in HFileWriterImpl to get rid of t… #2741
Conversation
…he double comparison process
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
} | ||
} | ||
return minMidpoint; | ||
//Note that left[diffIdx] can never be equal to 0xff since left < right |
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.
The logic here is different from original?
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.
The logic of composing midPoint is basically the same, with some small differences
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.
(diffByte + 1) < (ByteBufferUtils.toByte(right, rightOffset + diffIdx) & 0xff)
I think this logical branch is lost
byte[] minimumMidpointArray = new byte[minLength + 1]; | ||
ByteBufferUtils | ||
.copyFromBufferToArray(minimumMidpointArray, right, rightOffset, 0, minLength + 1); | ||
minimumMidpointArray[minLength] = 0x00; |
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.
Here is different from original?
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.
Yes, I made some adjustments.
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.
Skimmed. I like your addition of extra boundary checking test. LGTM.
@@ -366,8 +366,6 @@ private void finishBlock() throws IOException { | |||
*/ | |||
public static Cell getMidpoint(final CellComparator comparator, final Cell left, | |||
final Cell right) { | |||
// TODO: Redo so only a single pass over the arrays rather than one to |
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.
From HBASE-10800 ... By our Ramkrishna
@saintstack Thanks for reviewing |
@nyl3532016 You good w/ this change? If so, would like to add you as a sign-off. Thanks. |
@saintstack, I am good with the patch after @GeorryHuang amend a bit |
…he double comparison process (#2741) Signed-off-by: niuyulin <nyl353@163.com> Signed-off-by: stack <stack@apache.org>
…he double comparison process (apache#2741) Signed-off-by: niuyulin <nyl353@163.com> Signed-off-by: stack <stack@apache.org>
…he double comparison process (apache#2741) Signed-off-by: niuyulin <nyl353@163.com> Signed-off-by: stack <stack@apache.org>
…he double comparison process
There is a TODO like this "TODO: Redo so only a single pass over the arrays rather than one to compare and then a second composing midpoint." in getMidpoint() of class HFileWriteImpl
The old logic compares the left byte array and the right byte array twice:
My optimization combines them into one