Skip to content

Conversation

@murali-db
Copy link
Collaborator

@murali-db murali-db commented Sep 16, 2025

What changes are proposed in this pull request?

introduce SnapshotRef type alias for Arc<Snapshot>

How was this change tested?

Existing tests.

@murali-db murali-db changed the title -refactor PR: introduce SnapshotRef type alias refactor PR: introduce SnapshotRef type alias Sep 16, 2025
@murali-db murali-db changed the title refactor PR: introduce SnapshotRef type alias refactor: introduce SnapshotRef type alias Sep 16, 2025
@codecov
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.96%. Comparing base (e789132) to head (c0bf53d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
ffi/src/scan.rs 0.00% 1 Missing ⚠️
kernel/src/scan/mod.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1299      +/-   ##
==========================================
- Coverage   83.97%   83.96%   -0.02%     
==========================================
  Files         111      111              
  Lines       26250    26250              
  Branches    26250    26250              
==========================================
- Hits        22044    22041       -3     
- Misses       3109     3110       +1     
- Partials     1097     1099       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@zachschuermann zachschuermann left a comment

Choose a reason for hiding this comment

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

awesome, LGTM! just a quick fix in docstrings

//!
//! ```no_run
//! # use std::sync::Arc;
//! # use delta_kernel::checkpoint::CheckpointDataIterator;
Copy link
Member

Choose a reason for hiding this comment

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

ah looks like some of the docs are broken due to no longer pulling in Snapshot. and btw you can test with cargo doc and/or cargo doc --open (latter is just a nice shortcut to pull up the docs we build after they are compiled)

we could just do something like add this at the bottom:

[`Snapshot::checkpoint`]: crate::Snapshot::checkpoint

@zachschuermann
Copy link
Member

oh and I think need to peek at tests as well

Copy link
Collaborator

@nicklan nicklan left a comment

Choose a reason for hiding this comment

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

Looks good, but I think you missed a few spots:

kernel/examples/common/src/lib.rs
65:pub fn get_scan(snapshot: Arc<Snapshot>, args: &ScanArgs) -> DeltaResult<Option<Scan>> {

kernel/examples/write-table/src/main.rs
126:) -> DeltaResult<Arc<Snapshot>> {

kernel/src/log_compaction/tests.rs
8:fn create_mock_snapshot() -> Arc<Snapshot> {
18:fn create_multi_version_snapshot() -> Arc<Snapshot> {

kernel/src/log_compaction/writer.rs
33:    snapshot: Arc<Snapshot>,
48:        snapshot: Arc<Snapshot>,

kernel/src/snapshot/builder.rs
48:    pub(crate) fn new_from(existing_snapshot: Arc<Snapshot>) -> Self {
63:    /// Create a new [`Snapshot`]. This returns a [`SnapshotRef`] (`Arc<Snapshot>`), perhaps
70:    pub fn build(self, engine: &dyn Engine) -> DeltaResult<Arc<Snapshot>> {

@murali-db murali-db requested a review from nicklan September 17, 2025 17:21
@murali-db
Copy link
Collaborator Author

Looks good, but I think you missed a few spots:

kernel/examples/common/src/lib.rs
65:pub fn get_scan(snapshot: Arc<Snapshot>, args: &ScanArgs) -> DeltaResult<Option<Scan>> {

kernel/examples/write-table/src/main.rs
126:) -> DeltaResult<Arc<Snapshot>> {

kernel/src/log_compaction/tests.rs
8:fn create_mock_snapshot() -> Arc<Snapshot> {
18:fn create_multi_version_snapshot() -> Arc<Snapshot> {

kernel/src/log_compaction/writer.rs
33:    snapshot: Arc<Snapshot>,
48:        snapshot: Arc<Snapshot>,

kernel/src/snapshot/builder.rs
48:    pub(crate) fn new_from(existing_snapshot: Arc<Snapshot>) -> Self {
63:    /// Create a new [`Snapshot`]. This returns a [`SnapshotRef`] (`Arc<Snapshot>`), perhaps
70:    pub fn build(self, engine: &dyn Engine) -> DeltaResult<Arc<Snapshot>> {

looks like they were introduced during the rebase. got to them now @nicklan

@github-actions github-actions bot added the breaking-change Change that require a major version bump label Sep 17, 2025
- Add SnapshotRef type alias
- Update imports and documentation
@murali-db murali-db force-pushed the murali-db/type-alias branch 2 times, most recently from 058adb1 to 844b024 Compare September 18, 2025 00:24
@murali-db murali-db merged commit 63d829c into delta-io:main Sep 18, 2025
19 of 21 checks passed
@zachschuermann zachschuermann removed the breaking-change Change that require a major version bump label Sep 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

introduce SnapshotRef type alias for Arc<Snapshot>

4 participants