-
Notifications
You must be signed in to change notification settings - Fork 513
feat: provide feature flag for shallow cloning #4552
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
Conversation
|
Is this related to our storage version? Do we need to add it in 2.2 instead of 2.1? I ask this question since we are very close to stabilize format 2.1 and we don't want to introduce any breaking changes after that. |
From my perspective, I recommend including the shallow_clone feature in v2.1 for the following reasons:
However, if there are broader compatibility or dependency concerns I might have missed, I’m open to deferring it to v2.2. Would appreciate your thoughts! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4552 +/- ##
==========================================
- Coverage 81.06% 81.06% -0.01%
==========================================
Files 308 308
Lines 114382 114421 +39
Branches 114382 114421 +39
==========================================
+ Hits 92727 92758 +31
- Misses 18385 18392 +7
- Partials 3270 3271 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@jackye1995 This PR is ready for review |
I have not been following the shallow clone feature very closely but I think it is a table format feature and not a file format feature. The 2.1 stabilization effort is for the file format. We have not yet had a need to version the table format. We require all changes are backwards compatible and have gone to some substantial effort to avoid any kind of breaking changes. Not all changes are forwards compatible however. |
westonpace
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.
This looks good to me. An attempt to open a dataset that has shallow clones should fail if the Lance version is too old to understand shallow clones.
I don't think we have any "forwards compatibility" testing framework (e.g. no way for a CI test to confirm that an old reader will fail to read a cloned dataset with the right error message). Did you confirm this manually?
jackye1995
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.
Overall looks good to me.
We require all changes are backwards compatible and have gone to some substantial effort to avoid any kind of breaking changes. Not all changes are forwards compatible however.
Yeah I think this is more of a forward compatibility problem of the table format.
Context:
#4257 (comment)
This is simpler than I thought...