feat: support global tag retrieval and improve tag api

[majin1102]

When I was planning the work #5073 . I found some uncompleted work:

1. Tag retrieval doesn't contain branch field both in Java and Python.
2. The reference parameter is not well used in scenarios including createTag, updateTag. And createBranch both in Java and Python.
3. For python, Tuple[str, int] is not the best type and lead to the api of checkout_branch. Use Tuple[Optional[str], Optional[int]] instead for more syntax expressions and remove checkout_branch.
4. Some necessary refactoring like removing all ref_

[chatgpt-codex-connector]

💡 Codex Review

https://github.com/lancedb/lance/blob/82553ab6e87dd39ae60327920789a50db86caba7/java/lance-jni/src/blocking_dataset.rs#L1654-L1665
JNI tag APIs ignore tag references

The new Java overloads allow Tags.create("tag", Ref) and Tags.update("tag", Ref) where Ref may refer to another tag. However, the native implementations only read getVersionNumber() and getBranchName() from the Ref and call create_tag/update_tag with those optional values, dropping getTagName(). When a caller passes Ref.ofTag("v1"), both extracted fields are None and the JNI layer tags whatever happens to be the current dataset version instead of the referenced tag’s version. Python bindings resolve tag references to concrete branch/version before creating/updating tags, so Java callers now get inconsistent behaviour. These helpers should resolve the tag name to its version (and branch) before invoking the Rust API, similar to the Python path.

ℹ️ 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".

[majin1102]

There's another issue that I don't feel right but might need some discussion.

For now we use None to present the main branch. For example, we are in branch A:10, if we want to checkout A:9, we need to use checkout_version(('A', 9)). This is all right if user knows he is on branch A, however, if he load the dataset by a branch uri, he just treats the uri as an independent dataset(at least the experience). He might just use checkout_version(9) or checkout((None, 9)). But the actual result is he checked out the main.

I think most cases when people see None, maybe they are expecting no changes. So I think we might should to present None as the current branch instead of the main. On the other hand, we could use a constant string 'main' to present the main branch since we have reserved it. Although We store None for the main in the spec, we could provide better API for this.

How do you feel about this? @jackye1995 If you agree I could do this in this PR

[jackye1995]

Looks like there are 2 things here,

(1) feels like from what you are describing that putting "None" is confusing, maybe we should just reserve the name main to mean the main branch, so user can use that to reference the main branch. (they can continue to use None to also mean main branch that is fine)

(2) I agree that checkout_version(version_number) should better be against the current branch, we can improve that experience.

[majin1102]

Looks like there are 2 things here,

(1) feels like from what you are describing that putting "None" is confusing, maybe we should just reserve the name main to mean the main branch, so user can use that to reference the main branch. (they can continue to use None to also mean main branch that is fine)

(2) I agree that checkout_version(version_number) should better be against the current branch, we can improve that experience.

Sorry I think I didn't make myself clear enough.

If we use checkout_version(version_number) the version_number would be a impl Into<Ref> and that would be (None, Some(version_number)) pointing to the main branch. So I guess 1) and 2) are somehow conflicted:

    pub async fn checkout_version(&self, version: impl Into<refs::Ref>) -> Result<Self> {
        let reference: refs::Ref = version.into()
        ............

We can transform an u64 to (current_branch, Some(version_number)), but will make the interface complicated(the Into trait doesn't have current branch, so we might need to split the api). If we make (None, Some(version_number)) just point to the current branch, it will make sense to the impl Into<Ref>

This is just an impl issue, but I think we better make these all matched and clear. WDYT? @jackye1995

[codecov-commenter]

Codecov Report

❌ Patch coverage is 85.39326% with 26 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
rust/lance/src/dataset/refs.rs	85.92%	13 Missing and 6 partials ⚠️
rust/lance/src/dataset.rs	82.60%	2 Missing and 2 partials ⚠️
rust/lance/src/dataset/builder.rs	25.00%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[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".

[jackye1995]

mostly looks good to me, pending rebase

[majin1102]

Hi, @jackye1995

I think there's a bug for current branch datasets. As we know we now use (str, u64) to identify a global version. if we are working on a branch and use interfaces like this:

    pub async fn read_transaction_by_version(&self, version: u64) -> Result<Option<Transaction>> {
        let dataset_version = self.checkout_version(version).await?;
        dataset_version.read_transaction().await
    }

We will redirect to the main branch and get the transaction in that version line. On the other hand, everytime we use checkout_verson by a version_number, we are checking out the main branch which might be probably unexpected.

I'm solving this issue by introducing a VersionNumber enumation which is binded to the current branch context. In the meantime, we keep None as the main branch.

Please take a look on the newsest code when you have time

[yanghua]

Left some comments.

[majin1102]

Comments addressed, please take a look @yanghua

[yanghua]

+1

[majin1102]

@jackye1995 Please let me know if you have any further comments

[jackye1995]

Looks good to me!