-
Notifications
You must be signed in to change notification settings - Fork 139
feat: add Snapshot::checkpoint() & Table::checkpoint() API
#797
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: add Snapshot::checkpoint() & Table::checkpoint() API
#797
Conversation
Table::checkpoint() APISnapshot::checkpoint() API
scovich
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.
Getting very very close, looking very nice
kernel/src/checkpoint/tests.rs
Outdated
| let snapshot = table.snapshot(&engine, None)?; | ||
| let mut writer = snapshot.checkpoint()?; |
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.
If we wanted to, we could still expose the old Table::checkpoint method, which takes an impl Into<Option<Version>>. Rationale: I would have directly checkpointed my snapshot if I had one handy, but I don't, so I have to call into the Table instead. So it's actually helpful for the method on Table to combine the snapshot+checkpoint operations.
These unit tests do a pretty good job of illustrating the scenario.
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.
👍 I'll expose the old Table::checkpoint method as well
| SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .map_err(|e| Error::generic(format!("Failed to calculate system time: {}", e)))?, |
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.
aside: This seems to come up more and more lately -- we really need a utility method that returns "now" in Delta timestamp format, that everyone can use instead of having to replicate the multi-step conversion process.
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.
Tracked 👍 #877
kernel/src/checkpoint/tests.rs
Outdated
| let result = | ||
| deleted_file_retention_timestamp_with_time(retention, Duration::from_secs(1000))?; |
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.
So this test is pretending that "now" is 1000 seconds [after what]?
tiny nit:
| let result = | |
| deleted_file_retention_timestamp_with_time(retention, Duration::from_secs(1000))?; | |
| let now = Duration::from_secs(1000); | |
| let result = deleted_file_retention_timestamp_with_time(retention, now)?; |
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.
Good question — in this test, now is a simulated timestamp representing 1000 seconds since the UNIX epoch It doesn’t matter in practice since the test just fixes now at 1000s past UNIX epoch to keep the math predictable. The logic under test only cares about relative time (i.e., now minus retention)
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.
After a second look, I cleaned up the test case to be more readable
Snapshot::checkpoint() APISnapshot::checkpoint() & Table::checkpoint() API
scovich
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.
Very nice!
zachschuermann
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.
LGTM pending last few questions/comments!
kernel/src/checkpoint/mod.rs
Outdated
|
|
||
| let version = self.snapshot.version().try_into().map_err(|e| { | ||
| Error::CheckpointWrite(format!( | ||
| "Failed to convert checkpoint version from u64 {} to i64: {}", |
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.
Also looks like this is only used below - should we just make self.create_checkpoint_metadata_batch only take engine? and internally just use self.version?
What changes are proposed in this pull request?
Motivation and Context
This is part of the parent-issue to provide checkpoint write support in
delta-kernel-rs#499. This PR introduces a new snapshot API for writing single-file checkpoints to Delta tables (#736), supporting single-file V1 and V2 spec classic-named checkpoint formats.Breaking Changes
Summary of Changes
Snapshot::checkpoint&Table::checkpointas the entry points for checkpoint creation, supporting both V1 and V2 checkpoint specs depending on table features.CheckpointWriter::finalize()#850).finalize()to persist checkpoint metadata - todo(feat: addCheckpointWriter::finalize()#850).Major Components and APIs
Error::CheckpointWrite(String)Snapshot::checkpoint()CheckpointWriterfor a snapshotTable::checkpoint()CheckpointWriterfor a version of a tableCheckpointWriterCheckpointWriter::checkpoint_path()CheckpointWriter::checkpoint_data()CheckpointDataIterator) to be written to the checkpoint fileCheckpointWriter::finalize()_last_checkpointfile after data is persistedCheckpointDataIteratorCheckpointBatch(private)CheckpointLogReplayProcessor, contains filtered actions and countsCheckpoint Types
CheckpointMetadataaction included.v2Checkpoints, includesCheckpointMetadataaction for enhanced metadata.Usage Workflow
CheckpointWriterviaSnapshot::checkpointorTable::checkpointCheckpointWriter::checkpoint_path()CheckpointWriter::checkpoint_data()CheckpointWriter.finalize()- todo(feat: addCheckpointWriter::finalize()#850)todo(#850): The
CheckpointWriter::finalize()API that was previously included in this PR has been split into a separate PR #851 for ease of review. Handle the finalization of the checkpointing process by writing the_last_checkpointfile on call to.finalize(). Note: we require the engine to write the entire checkpoint file to storage before calling.finalize(), otherwise the table may be corrupted. It will be hard for the engine not to do this since thefinalize()call takes theFileMetaof the checkpoint writeHow was this change tested?
Unit tests in
checkpoint/mod.rstest_deleted_file_retention_timestamp- tests file retention timestamp calculationstest_create_checkpoint_metadata_batchUnit tests in
checkpoint/tests.rstest_v1_checkpoint_latest_version_by_default: table that does not supportv2Checkpoint, no checkpoint version specifiedtest_v1_checkpoint_specific_version: table that does not supportv2Checkpoint, checkpointing at a specific versiontest_v2_checkpoint_supported_table: table that supportsv2Checkpoint& no version is specified