-
Notifications
You must be signed in to change notification settings - Fork 492
Persist: plumb incremental apply APIs through #33045
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
Conversation
6f74d0b to
c60199e
Compare
f035a90 to
23e21c8
Compare
c60199e to
5779b4d
Compare
23e21c8 to
f497c4a
Compare
5779b4d to
9f945f4
Compare
f497c4a to
ae7312a
Compare
b75952b to
0d9a612
Compare
2bb9775 to
a15c651
Compare
0d9a612 to
43e3595
Compare
a15c651 to
85910d2
Compare
ac52cb3 to
1ddef6e
Compare
85910d2 to
0274b1f
Compare
1ddef6e to
b4e20a7
Compare
ee85994 to
3c38325
Compare
2ac14ef to
0188a39
Compare
a97ff26 to
a946d3c
Compare
a9fad35 to
955a883
Compare
a946d3c to
aee2ad8
Compare
955a883 to
e7388bb
Compare
|
Nightly with the flag enabled seems pretty red: https://buildkite.com/materialize/nightly/builds/12728 |
71825eb to
efa44b0
Compare
2124c7f to
6d76e6d
Compare
| let (batch_ids, run_ids, descriptions): (BTreeSet<_>, BTreeSet<_>, Vec<_>) = runs.iter() | ||
| .map(|(run_id, desc, _, _)| (run_id.0, run_id.1.expect("run_id must be present"), *desc)) | ||
| .multiunzip(); | ||
|
|
||
| let input = if incremental_enabled { | ||
| match batch_ids.iter().exactly_one().ok() { |
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.
Every day I learn a new useful itertools thing
|
In the latest run: https://buildkite.com/materialize/test/builds/107434#01988262-f51c-46c1-9683-5103676fb135 |
bd3ff86 to
5c8d2de
Compare
5c8d2de to
f64d01d
Compare
bkirwi
left a comment
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.
Cool! Thank you for the followup!
Left a few nits. We've also discussed some possible cleanups that would make sense as a followup, but this seems overall in good shape...
| } | ||
|
|
||
| /// A location in a spine, used to identify the Run that a batch belongs to. | ||
| /// If the Run is not known, the RunId is None. |
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.
✨ - thanks!
| run_reserved_memory_bytes, | ||
| ); | ||
| let total_chunked_runs = chunked_runs.len(); | ||
| let mut applied = 0; |
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.
Mild preference for using chunked_runs.into_iter().enumerate() instead of the counter here... that way I don't have to check the control flow to see if it's incremented every iteration.
3bdf7d7 to
f64d01d
Compare
Depends on #33044
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.