-
Notifications
You must be signed in to change notification settings - Fork 220
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
feat(storage): add support composite sort key in MergeIterator #622
feat(storage): add support composite sort key in MergeIterator #622
Conversation
Signed-off-by: BaymaxHWY <854541526@qq.com>
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.
LGTM, good work! The implementation is so precise and concise :)
// TODO: handle can-not-compare | ||
res = left_data.partial_cmp(&right_data).unwrap(); | ||
if Ordering::Equal != res { | ||
break; |
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.
just return res here? so that res
can simply be a local variable
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.
Ok
for sort_key_idx in &self.sort_key_idx { | ||
let left_data = self.chunk_buffer[*left_id] | ||
.as_ref() | ||
.unwrap() |
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.
can we move this unwrap to the outer loop, so that we can do less conditional branch?
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.
Get it!
Signed-off-by: BaymaxHWY <854541526@qq.com>
Signed-off-by: BaymaxHWY 854541526@qq.com
close issue: #588