Skip to content
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: implement restore operation #1502

Merged
merged 6 commits into from
Jul 4, 2023
Merged

feat: implement restore operation #1502

merged 6 commits into from
Jul 4, 2023

Conversation

loleek
Copy link
Contributor

@loleek loleek commented Jun 29, 2023

Description

This is a implementation of the Restore Command.

Related Issue(s)

#837

@github-actions github-actions bot added binding/rust Issues for the Rust crate rust labels Jun 29, 2023
@loleek loleek changed the title feat: implement resotre operation feat: implement restore operation Jun 29, 2023
@loleek
Copy link
Contributor Author

loleek commented Jun 29, 2023

I will file python binding implement in another PR. Is that OK?

@loleek
Copy link
Contributor Author

loleek commented Jul 3, 2023

I saw integration test failed. But it looks like not caused by my PR.
image

Copy link
Collaborator

@roeap roeap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thaks for taking care of this @loleek - left a minor comment re naming, otherwise looking great!

rust/src/operations/restore.rs Outdated Show resolved Hide resolved
Comment on lines +191 to +193
if !ignore_missing_files {
check_files_available(object_store.as_ref(), &files_to_add).await?;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting - would not have thought that it is useful, to delivberately create a state with missing files, but it seems the reference implementation also supports this :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data file may missing for many reason, deleted unintentionally or by vacuum operation.This will cause data missing if always allow to restore, so we must make users aware of the problem.

Comment on lines +233 to +242
match try_commit_transaction(object_store.as_ref(), &commit, commit_version).await {
Ok(_) => {}
Err(err @ TransactionError::VersionAlreadyExists(_)) => {
return Err(err.into());
}
Err(err) => {
object_store.delete(&commit).await?;
return Err(err.into());
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just making sure I understand. For restore we always want to fail, if any other operation was performed on the table?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if any other operation was performed, the restore is invalid because the ADD and REMOVE files are computed based on latest snapshot state and state of restored version.

@roeap
Copy link
Collaborator

roeap commented Jul 3, 2023

I saw integration test failed. But it looks like not caused by my PR.

Indeed it is not, unfortunately we have some flakiness in the tests due to memory issues.

Copy link
Collaborator

@roeap roeap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the great contribution, @loleek!

@roeap roeap merged commit 6650bd2 into delta-io:main Jul 4, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/rust Issues for the Rust crate rust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants