[HUDI-4284] Implement bloom lookup tree as red-black tree #5978
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
yabola
changed the title
yabola
changed the title
yabola
force-pushed
the
HUDI-4284
branch
2 times, most recently
from
53e67f6 to
a8f041f
Compare
yabola
changed the title
XuQianJin-Stars
requested review from
nsivabalan,
xushiyan,
danny0405 and
codope
and removed request for
nsivabalan
|
@codope @nsivabalan Please take a look~ |
|
@vinothchandar Can you help review this PR, thanks. |
|
@yabola Thanks for putting up this PR. Sorry for the delay as I was occupied with Hudi release work. I am going to review it towards the end of week. Expect feedback by Monday. |
yihua
added
priority:major
codope
reviewed
Oct 26, 2022
| // Note that the interval tree implementation doesn't have auto-balancing to ensure logN search time. | ||
| // So, we are shuffling the input here hoping the tree will not have any skewness. If not, the tree could be skewed | ||
| // which could result in N search time instead of NlogN. | ||
| Collections.shuffle(allIndexFiles); |
There was a problem hiding this comment.
@yabola do you have some micro-benchmark as to how much improvement this change brings?
There was a problem hiding this comment.
Sorry, actually I don't have benchmark on it. I think red-black tree is a general optimization strategy like hashmap
| * @param key the numeric value of the key | ||
| * @return result of aligned numbers. For example, `1` -> `00001`. | ||
| */ | ||
| private String alignedNumber(long key) { |
| * 4. Every path from a node (including root) to any of its descendants NULL nodes has the same number of black nodes. | ||
| */ | ||
| @SuppressWarnings({"unchecked", "rawtypes"}) | ||
| public class RedBlackTree<T extends RedBlackTreeNode<K>, K extends Comparable<K>> implements Serializable { |
There was a problem hiding this comment.
Have you considered thread safety of operations on the tree?
There was a problem hiding this comment.
I can make this tree thread safety later if need
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.
What is the purpose of the pull request
The existing KeyRangeLookupTree implementation is Binary Sorting Tree.
Although it is shuffled before insertion, it may still cause uneven distribution. This PR implement it as a Red Black Tree.
Brief change log
Added abstract implementation of red-black tree
RedBlackTreeand implement theKeyRangeLookupTreeas red-black tree.Verify this pull request
Added new unit test
org.apache.hudi.common.util.rbtree.TestRedBlackTreeCommitter checklist
Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.