-
Notifications
You must be signed in to change notification settings - Fork 76
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
fix: prevent batch_index overflow in raw_curp #704
Conversation
0d73ba7
to
a80897e
Compare
8b788c1
to
a7b56d8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #704 +/- ##
==========================================
+ Coverage 75.55% 75.68% +0.13%
==========================================
Files 180 187 +7
Lines 26938 27788 +850
Branches 26938 27788 +850
==========================================
+ Hits 20353 21032 +679
- Misses 5366 5467 +101
- Partials 1219 1289 +70 ☔ View full report in Codecov by Sentry. |
This implementation is too complex compare to the original prefix sum version. I suggest making some modifications to the original algorithm. Here's the pseudocode. The interval representation is #[derive(Default)]
struct Log {
right: Vec<usize>,
sizes: Vec<u64>,
limit: u64,
p: usize,
sum: u64,
}
impl Log {
fn push(&mut self, size: u64) {
self.sizes.push(size);
self.right.push(0);
self.sum += size;
while self.sum > self.limit {
self.right[self.p] = self.sizes.len() - 1;
self.sum -= self.sizes[self.p];
self.p += 1;
}
}
fn get(&self, left: usize) -> usize {
self.right[left]
}
} |
1574f83
to
7a2046d
Compare
@GFX9 Convert your pr to draft since CI failed |
@GFX9 Convert your pr to draft since CI failed |
@GFX9 Convert your pr to draft since CI failed |
The current implementation uses relevant offset to handle the right index of the |
@Mergifyio rebase |
72f76c2
to
55442ec
Compare
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
f5ff5aa
to
1451a42
Compare
@GFX9 Your PR is in conflict and cannot be merged. |
Closes: xline-kv#368, xline-kv#800 Signed-off-by: Phoeniix Zhao <[email protected]>
Signed-off-by: Phoeniix Zhao <[email protected]>
Signed-off-by: Phoeniix Zhao <[email protected]>
what problem are you trying to solve? (or if there's no problem, what's the motivation for this change?)
-> Fixes Bug: batch_index in raw_curp will overflow eventually #368
what changes does this pull request make?
-> Use a scrolling array to replace the previous prefix array to prevent
batch_index
overflow, which will be truncated when compact. Besides, the scrolling array reduces the operation time complexity of obtaining batch log to O(1).are there any non-obvious implications of these changes? (does it break compatibility with previous versions, etc)
-> No