feat(java): support list versions and checkout version in Dataset#3945
feat(java): support list versions and checkout version in Dataset#3945jackye1995 merged 3 commits intolance-format:mainfrom
Conversation
| throw new IllegalArgumentException("Version must be greater than 0"); | ||
| } | ||
|
|
||
| public Dataset checkout(long version) { |
There was a problem hiding this comment.
Why change this name? The original name seems fine to me
There was a problem hiding this comment.
I have discuessed with @yanghua earlier
There will be
Dataset checkout(String tag)
Dataset checkout(long timestamp) // or dataTime
We both feel overload checkout to both tag and version more elegent and concise
There was a problem hiding this comment.
If you really want to go with this, then let's follow the breaking change policy and adjust the PR title and description: https://github.com/lancedb/lance/blob/main/release_process.md#breaking-change-policy
To me, I think we can just go with checkoutVersion, checkoutTag, checkoutTimestamp, but I am not super opinionated about this.
btw, if you plan to use long for timestamp, shouldn't you also use long for private final ZonedDateTime dataTime?
There was a problem hiding this comment.
btw, if you plan to use long for timestamp, shouldn't you also use long for private final ZonedDateTime dataTime?
I'm not quite sure about using timestamp as the parameter. This is only for discussion. I checked rust and python there is no similar api. Using ZonedDateTime is because rust code using DataTime<Utc>
I'm OK with checkoutVersion as well as checkoutTag.
PTAL when you have time
There was a problem hiding this comment.
I see, then let's just stick with ZonedDateTime then, although it's always UTC so a bit redundant but that's fine I guess.
| try (LockManager.ReadLock readLock = lockManager.acquireReadLock()) { | ||
| Preconditions.checkArgument(nativeDatasetHandle != 0, "Dataset is closed"); | ||
|
|
||
| if (this.version() == version) { |
There was a problem hiding this comment.
What is wrong with this logic?
There was a problem hiding this comment.
This logic is inconsistency and redundant with rust code here(underlying rust checkout):
2 participants
close #3944