feat: dataset supports deep_clone by majin1102 · Pull Request #5250 · lance-format/lance

majin1102 · 2025-11-17T04:48:57Z

Close #5249

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

codecov · 2025-12-03T10:11:50Z

Codecov Report

❌ Patch coverage is 75.36232% with 51 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
rust/lance/src/dataset.rs	70.00%	30 Missing and 12 partials ⚠️
rust/lance/src/io/commit.rs	84.74%	1 Missing and 8 partials ⚠️

📢 Thoughts on this report? Let us know!

chatgpt-codex-connector · 2025-12-03T12:26:36Z

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

majin1102 · 2025-12-04T03:13:55Z

This PR is ready for review @jackye1995

Please take a look if you have time. Thanks

jackye1995

thanks for working on this, added some comments, and can we also add some tests?

jackye1995 · 2025-12-08T06:14:34Z

+
+        // Resolve source dataset and its manifest using checkout_version
+        let src_ds = self.checkout_version(version).await?;
+        let path_specs = self.collect_paths().await?;


this should use src_ds?

jackye1995 · 2025-12-08T06:15:51Z

+/// A file path wrapper that can be used to represent a file in a dataset.
+/// This wrapper is used for changing the base_path like deep_clone
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct FilePath {


can we just not have this for now? I agree we probably need some refactoring to have this, but it should be in an independent PR that covers all cases.

jackye1995 · 2025-12-08T06:19:52Z

-            vec![]
-        };
-        (new_manifest, updated_indices)
+            // Deep clone: build a manifest that references local files (no external bases)


I just realized one thing, we need to also set base_id for ExternalFile, so those files can be handled for shallow clone case, and also not inherit base_id for deep clone case.

I did realize that. But I searched the code and asked @yanghua he told me this has not been used in any write paths yet. And I don't know what the path looks like and how it shoud be copied. So I wrote a check:

https://github.com/majin1102/lance/blob/3823f466a5daa6d9e4eed1ddaa00bb4cc95bd705/rust/lance/src/dataset.rs#L2273

Maybe you have more context to give me?

Another place I can think of is https://github.com/lance-format/lance/blob/main/rust/lance/src/index/frag_reuse.rs#L162

For stable tow id, we are not doing it today but from spec correctness perspective I think we should probably handle it here. Can be a separated PR though I'm fine with that

Yeah, I think we can handle external file in the further PRs.

jackye1995 · 2025-12-08T06:24:05Z

+        }
+
+        let io_parallelism = self.object_store.io_parallelism();
+        let copy_futures = path_specs


I guess this is the best we can do now, but all cloud storage have batch copy, we should ideally leverage that, but that requires upstream support. Maybe create a github issue to track that and add a TODO here.

majin1102 · 2025-12-08T14:10:27Z

thanks for working on this, added some comments, and can we also add some tests?

Oops!
The dataset tests were relocated and I forgot to merge this test_deep_clone when I rebased. Sorry for that!

majin1102 · 2025-12-09T16:52:02Z

Ready for another look @jackye1995

jackye1995

Overall looking good to me, some follow ups I think we can have followup PRs

github-actions · 2025-12-22T07:43:42Z

Code Review: feat: dataset supports deep_clone

Summary

This PR adds a deep_clone method to perform a full server-side copy of all dataset files to a new location. The implementation reuses the existing Operation::Clone infrastructure with an is_shallow: false flag.

P0/P1 Issues

P0: Potential data loss on partial copy failure

In deep_clone(), if any file copy fails midway through the parallel copy stream, partial files may be left at the target location with no cleanup. The error is returned but orphan files remain.

Recommendation: Consider adding cleanup logic on failure, or document this behavior clearly so users know to clean up failed clones manually.

P1: &mut self signature may be overly restrictive

The deep_clone method takes &mut self but does not appear to mutate self - it calls self.checkout_version() which only requires &self. This prevents concurrent deep_clone operations unnecessarily.

P1: External row_id_meta returns error rather than being unsupported gracefully

The collect_paths method returns an internal error for external row_id files. This should probably be Error::NotSupported rather than Error::Internal since it is a user-facing limitation, not an internal invariant violation.

Minor Observations (non-blocking)

The test correctly validates index copying, deletion files, and error cases. Good coverage.
The TODO comment about bulk copy APIs (issue Leverage object store bulk copy to efficiently deep_clone dataset #5435) is appropriate - this is the right place to optimize in the future.
The manifest handling in do_commit_new_dataset properly clears base_paths, branch, tag, and resets base_id values for deep clone, ensuring the cloned dataset is truly independent.

github-actions · 2025-12-22T10:07:00Z

PR Review: feat: dataset supports deep_clone

Summary

This PR adds deep_clone functionality to copy all dataset files (data, indices, deletions) to a new location, creating a fully independent dataset. The implementation leverages the existing Operation::Clone infrastructure with is_shallow: false.

P0 Issues

1. No cleanup on partial copy failure (data corruption risk)

In deep_clone() (rust/lance/src/dataset.rs:2196-2213), if any file copy fails mid-operation, the target location will contain partially copied files but the commit will not proceed. However, if a subsequent deep_clone is attempted to the same location, it will fail with DatasetAlreadyExists (line 2188) because some files exist, even though no manifest was committed. This creates an orphaned, inconsistent state.

More critically, if the copy phase succeeds but the builder.execute(txn) fails (e.g., due to a transient error), the target will have all data files but no valid manifest, leaving an unusable dataset that cannot be recovered or cleaned up.

Suggested fix: Either (a) implement cleanup on failure, or (b) check for file existence rather than manifest existence when detecting existing targets, or (c) document this behavior and provide a cleanup utility.

P1 Issues

2. Missing test for cross-storage deep_clone

The test only covers local filesystem cloning. Since deep_clone takes store_params for the target and relies on store.copy() which may behave differently across object stores (S3, GCS, Azure), integration tests for cross-storage scenarios would increase confidence.

3. &mut self signature is unexpected for read-like operation

pub async fn deep_clone(
    &mut self,  // <-- This is surprising
    target_path: &str,
    ...

The method only reads from self (via checkout_version) and doesn't mutate the source dataset. Taking &mut self unnecessarily restricts usage. Consider changing to &self.

Minor Observations (non-blocking)

The TODO comment at line 2206 referencing issue Leverage object store bulk copy to efficiently deep_clone dataset #5435 is good documentation.
The collect_paths method correctly handles base_id lookups and rejects unsupported external row_id files with a clear error message.

Close lance-format#5249

github-actions Bot added the enhancement New feature or request label Nov 17, 2025

majin1102 marked this pull request as draft November 17, 2025 04:54

chatgpt-codex-connector Bot reviewed Nov 17, 2025

View reviewed changes

Comment thread rust/lance-table/src/format/fragment.rs Outdated

majin1102 force-pushed the deep_clone branch from be7b215 to 2ff4807 Compare November 17, 2025 10:22

init

c0bc1c5

majin1102 force-pushed the deep_clone branch from 3d54929 to c0bc1c5 Compare December 3, 2025 09:43

majin1102 added 2 commits December 3, 2025 18:25

fmt

38db690

Use Path::parse instead of Path::from

1a9d3b5

majin1102 marked this pull request as ready for review December 3, 2025 12:26

Merge branch 'main' into deep_clone

c84719c

majin1102 added 3 commits December 4, 2025 14:58

Merge branch 'main' into deep_clone

bad5e7b

Merge branch 'main' into deep_clone

21e5071

fix clippy after rebasing

93b5c41

jackye1995 reviewed Dec 8, 2025

View reviewed changes

majin1102 mentioned this pull request Dec 8, 2025

Leverage object store bulk copy to efficiently deep_clone dataset #5435

Open

address comments

91c2fc2

majin1102 force-pushed the deep_clone branch from 3823f46 to 91c2fc2 Compare December 8, 2025 15:10

majin1102 added 2 commits December 9, 2025 13:41

fix clippy

d8a73c4

fix ci

3ce420b

jackye1995 approved these changes Dec 20, 2025

View reviewed changes

Merge branch 'main' into deep_clone

ad4a6f8

fix rebase issue

5216f58

majin1102 merged commit a7741f9 into lance-format:main Dec 22, 2025
29 checks passed

wjones127 pushed a commit to wjones127/lance that referenced this pull request Dec 30, 2025

feat: dataset supports deep_clone (lance-format#5250)

910c7b4

Close lance-format#5249

jackye1995 pushed a commit to jackye1995/lance that referenced this pull request Jan 21, 2026

feat: dataset supports deep_clone (lance-format#5250)

8e3dd75

Close lance-format#5249

andrea-reale mentioned this pull request Mar 30, 2026

emilk/fix write starvation rerun-io/lance#12

Closed

hushengquan mentioned this pull request Apr 17, 2026

feat(python): support deep_clone in python api #6559

Open

Conversation

majin1102 commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

codecov Bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chatgpt-codex-connector Bot commented Dec 3, 2025

Uh oh!

majin1102 commented Dec 4, 2025

Uh oh!

jackye1995 left a comment

Choose a reason for hiding this comment

Uh oh!

jackye1995 Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

jackye1995 Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

jackye1995 Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

majin1102 Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackye1995 Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

yanghua Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

jackye1995 Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

majin1102 commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

majin1102 commented Dec 9, 2025

Uh oh!

jackye1995 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Dec 22, 2025

Code Review: feat: dataset supports deep_clone

Summary

P0/P1 Issues

Minor Observations (non-blocking)

Uh oh!

github-actions Bot commented Dec 22, 2025

PR Review: feat: dataset supports deep_clone

Summary

P0 Issues

P1 Issues

Minor Observations (non-blocking)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

majin1102 commented Nov 17, 2025 •

edited

Loading

codecov Bot commented Dec 3, 2025 •

edited

Loading

majin1102 Dec 8, 2025 •

edited

Loading

majin1102 commented Dec 8, 2025 •

edited

Loading