-
Notifications
You must be signed in to change notification settings - Fork 0
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
allow for optimize without commits, surface commit API to python client for deferred committing #1
Conversation
7677d64
to
cc51d41
Compare
python/src/lib.rs
Outdated
|
||
let commit = match &self._commit { | ||
Some(c) => c, | ||
None => panic!("No commit found from previous optimize, cannot commit."), |
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.
should throw a python exception instead.
3dc4670
to
3ecc344
Compare
fix: clean up unnecessay mut and dead code warnings
e549858
to
f5f70ed
Compare
self, | ||
commit_info: CommitContext | ||
) -> DeltaResult<DeltaTable> { | ||
let _ = commit( |
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.
what's the reason for let _ =
?
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.
Updated to use ?
and propagate the error
f5f70ed
to
444d4d6
Compare
|
||
rt()?.block_on( | ||
cmd.commit_writes( | ||
self._commit.take().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.
@houqp is this the best way to handle this case? .take().unwrap()
?
Some(CommitContext{ | ||
actions: total_actions, | ||
app_metadata, | ||
snapshot: Some(snapshot.clone()), // using original table snapshot as all commits are added at once |
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.
Not 100% sure if this is correct -- maybe we need to use table.snapshot()
if there were other writes?
self, | ||
commit_info: CommitContext | ||
) -> DeltaResult<DeltaTable> { | ||
let _ = commit( |
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.
Updated to use ?
and propagate the error
Description
The description of the main changes of your pull request
Related Issue(s)
Documentation