feat: allow multiple incremental commits in optimize #1621
Conversation
kvap
requested review from
houqp,
xianwill,
wjones127,
fvaleye,
roeap,
rtyler and
mosyp
as code owners
|
ACTION NEEDED delta-rs follows the Conventional Commits specification for release automation. The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
kvap
changed the title
kvap
force-pushed
the
optimize-in-multiple-commits
branch
from
66c5045
to
acca200
Compare
| let now = Instant::now(); | ||
| if !actions.is_empty() && (self.min_commit_interval.map_or(false, |i| now.duration_since(last_commit) > i) || end) { | ||
| let actions = std::mem::take(&mut actions); | ||
| last_commit = now; |
There was a problem hiding this comment.
Does it matter if this is updated before the commit success?
There was a problem hiding this comment.
I don't think it matters much, because min_commit_interval is supposed to be much larger than the time it takes to do a commit.
|
Should we move those out of |
kvap
force-pushed
the
optimize-in-multiple-commits
branch
2 times, most recently
from
39a12bb
to
236c86a
Compare
Yeah that sounds like a good idea 👍 |
kvap
force-pushed
the
optimize-in-multiple-commits
branch
from
236c86a
to
4c09037
Compare
Done. |
|
@kvap is this option also exposed to the python bindings? |
There was a problem hiding this comment.
I'd like to see one more change to the tests before merging.
@ion-elgreco I've created a follow up issue for the Python bindings: #1640
| max_concurrent_tasks: usize, | ||
| #[allow(unused_variables)] // used behind a feature flag | ||
| max_spill_size: usize, | ||
| min_commit_interval: Option<Duration>, |
There was a problem hiding this comment.
This is fine for now, but eventually we might want to put these execution settings in a struct.
| let maybe_metrics = plan | ||
| .execute(dt.object_store(), &dt.state, 1, 20, None) | ||
| .await; |
There was a problem hiding this comment.
Could you add at least one test where we pass in a commit interval? even if it doesn't make an intermediate commit, it would be good to know those code paths run through.
There was a problem hiding this comment.
Added a test where an intermediate commit is actually created.
Currently "optimize" executes the whole plan in one commit, which might fail. The larger the table, the more likely it is to fail and the more expensive the failure is. Add an option in OptimizeBuilder that allows specifying a commit interval. If that is provided, the plan executor will periodically commit the accumulated actions.
kvap
force-pushed
the
optimize-in-multiple-commits
branch
from
4c09037
to
15892d9
Compare
Currently "optimize" executes the whole plan in one commit, which might fail. The larger the table, the more likely it is to fail and the more expensive the failure is.
Add an option in OptimizeBuilder that allows specifying a commit interval. If that is provided, the plan executor will periodically commit the accumulated actions.