Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions icechunk-python/python/icechunk/_icechunk_python.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -1567,6 +1567,8 @@ class BranchResetUpdate(UpdateType):
class NewCommitUpdate(UpdateType):
@property
def branch(self) -> str: ...
@property
def snap_id(self) -> str: ...

class CommitAmendedUpdate(UpdateType):
@property
Expand Down
11 changes: 8 additions & 3 deletions icechunk-python/src/repository.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,8 @@ pub(crate) struct PyBranchResetUpdate {
pub(crate) struct PyNewCommitUpdate {
#[pyo3(get)]
branch: String,
#[pyo3(get)]
snap_id: String,
}

#[pyclass(name = "CommitAmendedUpdate", eq, extends=PyUpdateType)]
Expand Down Expand Up @@ -605,7 +607,7 @@ impl PyBranchResetUpdate {
#[pymethods]
impl PyNewCommitUpdate {
fn __repr__(&self) -> PyResult<String> {
Ok(format!("NewCommitUpdate(branch={})", self.branch))
Ok(format!("NewCommitUpdate(branch={}, snap_id={})", self.branch, self.snap_id))
}
}

Expand Down Expand Up @@ -734,10 +736,13 @@ fn mk_update_type(
)?
.into_any()
.unbind(),
UpdateType::NewCommitUpdate { branch } => Bound::new(
UpdateType::NewCommitUpdate { branch, snap_id } => Bound::new(
py,
(
PyNewCommitUpdate { branch: branch.clone() },
PyNewCommitUpdate {
branch: branch.clone(),
snap_id: snap_id.to_string(),
},
PyUpdateType { updated_at, backup_path },
),
)?
Expand Down
Binary file modified icechunk-python/tests/data/split-repo-v2-migrated/repo
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file modified icechunk-python/tests/data/split-repo-v2/repo
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file modified icechunk-python/tests/data/test-repo-v2-migrated/repo
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file modified icechunk-python/tests/data/test-repo-v2/repo
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
18 changes: 18 additions & 0 deletions icechunk-python/tests/test_timetravel.py
Original file line number Diff line number Diff line change
Expand Up @@ -1070,3 +1070,21 @@ async def test_long_ops_log(spec_version: int | None) -> None:
assert t == ic.BranchCreatedUpdate

# TODO: add check for next updates page path


def test_ops_log_commit_snap_id() -> None:
repo = ic.Repository.create(
storage=ic.in_memory_storage(),
)

session = repo.writable_session("main")
store = session.store

group = zarr.group(store=store, overwrite=True)
array = group.create_array("array", shape=(10,), chunks=(5,), dtype="i4")
array[:] = 42
snap_id = session.commit("commit 1")

change = next(repo.ops_log())
assert isinstance(change, ic.NewCommitUpdate)
assert change.snap_id == snap_id
1 change: 1 addition & 0 deletions icechunk/flatbuffers/repo.fbs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ table BranchResetUpdate {
}
table NewCommitUpdate {
branch: string (required);
snap_id: ObjectId12 (required);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will touch almost every change in this PR, but... is snapshot_id too long? 😅

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

we have snap_id in several places. We should change them all at once in a separate PR

}
table CommitAmendedUpdate {
branch: string (required);
Expand Down
27 changes: 25 additions & 2 deletions icechunk/src/format/flatbuffers/all_generated.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5124,6 +5124,7 @@ pub mod generated {

impl<'a> NewCommitUpdate<'a> {
pub const VT_BRANCH: flatbuffers::VOffsetT = 4;
pub const VT_SNAP_ID: flatbuffers::VOffsetT = 6;

#[inline]
pub unsafe fn init_from_table(table: flatbuffers::Table<'a>) -> Self {
Expand All @@ -5140,6 +5141,9 @@ pub mod generated {
args: &'args NewCommitUpdateArgs<'args>,
) -> flatbuffers::WIPOffset<NewCommitUpdate<'bldr>> {
let mut builder = NewCommitUpdateBuilder::new(_fbb);
if let Some(x) = args.snap_id {
builder.add_snap_id(x);
}
if let Some(x) = args.branch {
builder.add_branch(x);
}
Expand All @@ -5160,6 +5164,15 @@ pub mod generated {
.unwrap()
}
}
#[inline]
pub fn snap_id(&self) -> &'a ObjectId12 {
// Safety:
// Created from valid Table for this object
// which contains a valid value in this slot
unsafe {
self._tab.get::<ObjectId12>(NewCommitUpdate::VT_SNAP_ID, None).unwrap()
}
}
}

impl flatbuffers::Verifiable for NewCommitUpdate<'_> {
Expand All @@ -5175,19 +5188,22 @@ pub mod generated {
Self::VT_BRANCH,
true,
)?
.visit_field::<ObjectId12>("snap_id", Self::VT_SNAP_ID, true)?
.finish();
Ok(())
}
}
pub struct NewCommitUpdateArgs<'a> {
pub branch: Option<flatbuffers::WIPOffset<&'a str>>,
pub snap_id: Option<&'a ObjectId12>,
}
impl<'a> Default for NewCommitUpdateArgs<'a> {
#[inline]
fn default() -> Self {
NewCommitUpdateArgs {
branch: None, // required field
}
branch: None, // required field
snap_id: None, // required field
}
}
}

Expand All @@ -5204,6 +5220,11 @@ pub mod generated {
);
}
#[inline]
pub fn add_snap_id(&mut self, snap_id: &ObjectId12) {
self.fbb_
.push_slot_always::<&ObjectId12>(NewCommitUpdate::VT_SNAP_ID, snap_id);
}
#[inline]
pub fn new(
_fbb: &'b mut flatbuffers::FlatBufferBuilder<'a, A>,
) -> NewCommitUpdateBuilder<'a, 'b, A> {
Expand All @@ -5214,6 +5235,7 @@ pub mod generated {
pub fn finish(self) -> flatbuffers::WIPOffset<NewCommitUpdate<'a>> {
let o = self.fbb_.end_table(self.start_);
self.fbb_.required(o, NewCommitUpdate::VT_BRANCH, "branch");
self.fbb_.required(o, NewCommitUpdate::VT_SNAP_ID, "snap_id");
flatbuffers::WIPOffset::new(o.value())
}
}
Expand All @@ -5222,6 +5244,7 @@ pub mod generated {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
let mut ds = f.debug_struct("NewCommitUpdate");
ds.field("branch", &self.branch());
ds.field("snap_id", &self.snap_id());
ds.finish()
}
}
Expand Down
31 changes: 23 additions & 8 deletions icechunk/src/format/repo_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ pub enum UpdateType {
BranchCreatedUpdate { name: String },
BranchDeletedUpdate { name: String, previous_snap_id: SnapshotId },
BranchResetUpdate { name: String, previous_snap_id: SnapshotId },
NewCommitUpdate { branch: String },
NewCommitUpdate { branch: String, snap_id: SnapshotId },
CommitAmendedUpdate { branch: String, previous_snap_id: SnapshotId },
NewDetachedSnapshotUpdate { new_snap_id: SnapshotId },
GCRanUpdate,
Expand Down Expand Up @@ -872,7 +872,11 @@ impl RepoInfo {
}
generated::UpdateType::NewCommitUpdate => {
let up = update.update_type_as_new_commit_update().unwrap();
Ok(UpdateType::NewCommitUpdate { branch: up.branch().to_string() })
let snap_id = SnapshotId::new(up.snap_id().0);
Ok(UpdateType::NewCommitUpdate {
branch: up.branch().to_string(),
snap_id,
})
}
generated::UpdateType::CommitAmendedUpdate => {
let up = update.update_type_as_commit_amended_update().unwrap();
Expand Down Expand Up @@ -1126,13 +1130,15 @@ fn update_type_to_fb<'bldr>(
.as_union_value(),
))
}
UpdateType::NewCommitUpdate { branch } => {
UpdateType::NewCommitUpdate { branch, snap_id } => {
let branch = Some(builder.create_string(branch));
let object_id12 = generated::ObjectId12::new(&snap_id.0);
let snap_id = Some(&object_id12);
Ok((
generated::UpdateType::NewCommitUpdate,
generated::NewCommitUpdate::create(
builder,
&generated::NewCommitUpdateArgs { branch },
&generated::NewCommitUpdateArgs { branch, snap_id },
)
.as_union_value(),
))
Expand Down Expand Up @@ -1210,7 +1216,10 @@ mod tests {
SpecVersionBin::current(),
snap2.clone(),
Some("main"),
UpdateType::NewCommitUpdate { branch: "main".to_string() },
UpdateType::NewCommitUpdate {
branch: "main".to_string(),
snap_id: snap2.id.clone(),
},
"foo/bar",
)?;
assert_eq!(&repo.resolve_branch("main")?, &snap2.id);
Expand Down Expand Up @@ -1238,7 +1247,10 @@ mod tests {
SpecVersionBin::current(),
snap3.clone(),
Some("main"),
UpdateType::NewCommitUpdate { branch: "main".to_string() },
UpdateType::NewCommitUpdate {
branch: "main".to_string(),
snap_id: snap3.id.clone(),
},
"foo",
)?;
assert_eq!(&repo.resolve_branch("main")?, &snap3.id);
Expand Down Expand Up @@ -1308,9 +1320,12 @@ mod tests {
};
let repo = repo.add_snapshot(
SpecVersionBin::current(),
snap2,
snap2.clone(),
Some("main"),
UpdateType::NewCommitUpdate { branch: "main".to_string() },
UpdateType::NewCommitUpdate {
branch: "main".to_string(),
snap_id: snap2.id.clone(),
},
"foo",
)?;
let repo = repo.add_branch(SpecVersionBin::current(), "baz", &id2, "/foo/bar")?;
Expand Down
20 changes: 12 additions & 8 deletions icechunk/src/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2617,9 +2617,10 @@ async fn do_commit_v2(
};

let update_type = match commit_method {
CommitMethod::NewCommit => {
UpdateType::NewCommitUpdate { branch: branch_name.to_string() }
}
CommitMethod::NewCommit => UpdateType::NewCommitUpdate {
branch: branch_name.to_string(),
snap_id: new_snapshot_id.clone(),
},
CommitMethod::Amend => UpdateType::CommitAmendedUpdate {
branch: branch_name.to_string(),
previous_snap_id: parent_snapshot.id.clone(),
Expand Down Expand Up @@ -3221,7 +3222,10 @@ mod tests {
SpecVersionBin::current(),
snapshot.as_ref().try_into()?,
Some("main"),
UpdateType::NewCommitUpdate { branch: "main".to_string() },
UpdateType::NewCommitUpdate {
branch: "main".to_string(),
snap_id: snapshot.id().clone(),
},
"backup_path",
)?;
asset_manager.create_repo_info(Arc::new(repo_info)).await?;
Expand Down Expand Up @@ -4186,7 +4190,7 @@ mod tests {
let repo = create_memory_store_repository().await;
let mut session = repo.writable_session("main").await?;
session.add_group(Path::root(), Bytes::copy_from_slice(b"")).await?;
session.commit("make root", None).await?;
let snap1 = session.commit("make root", None).await?;

let mut session = repo.writable_session("main").await?;
session.add_group("/a".try_into().unwrap(), Bytes::copy_from_slice(b"")).await?;
Expand Down Expand Up @@ -4258,10 +4262,10 @@ mod tests {
TagCreatedUpdate { name: "tag".to_string() },
CommitAmendedUpdate {
branch: "main".to_string(),
previous_snap_id: before_amend1
previous_snap_id: before_amend1.clone(),
},
NewCommitUpdate { branch: "main".to_string() },
NewCommitUpdate { branch: "main".to_string() },
NewCommitUpdate { branch: "main".to_string(), snap_id: before_amend1 },
NewCommitUpdate { branch: "main".to_string(), snap_id: snap1 },
RepoInitializedUpdate,
]
);
Expand Down
Loading