feat: expose branch_identifier in python and java bindings#6360
feat: expose branch_identifier in python and java bindings#6360
Conversation
|
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 681c769755
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public Branch( | ||
| String name, String parentBranch, long parentVersion, long createAt, int manifestSize) { | ||
| String name, | ||
| String parentBranch, | ||
| List<BranchVersionMapping> branchIdentifier, | ||
| long parentVersion, |
There was a problem hiding this comment.
Preserve the previous Branch constructor overload
This change removes the only public Branch(String, String, long, long, int) constructor and replaces it with a new signature that requires List<BranchVersionMapping>, which is a source/binary incompatible API break for downstream Java users who instantiate Branch directly. After upgrading, existing callers will fail to compile (or hit NoSuchMethodError when artifacts are mixed). Please keep the old constructor as an overload and delegate to the new one with a default lineage list to maintain compatibility.
Useful? React with 👍 / 👎.
| && Objects.equals(name, branch.name) | ||
| && Objects.equals(parentBranch, branch.parentBranch); | ||
| && Objects.equals(parentBranch, branch.parentBranch) | ||
| && Objects.equals(branchIdentifier, branch.branchIdentifier); |
There was a problem hiding this comment.
Avoid unstable lineage IDs in Branch equality
Including branchIdentifier in equals makes equality unstable for legacy branches whose refs JSON lacks identifier: Rust fills that with BranchIdentifier::none(), which generates a fresh UUID during deserialization, so the same logical branch can compare unequal across separate branches().list() calls. This can break client-side map/set lookups and cache keys for older datasets; consider excluding this field from equality (or ensuring legacy identifiers are deterministic before exposing them).
Useful? React with 👍 / 👎.
Motivation
BranchIdentifierinto higher-level bindings so clients can inspect a branch's lineage chain.Description
java/lance-jni/src/blocking_dataset.rs) to construct and pass aList<BranchVersionMapping>when creating the JavaBranchobject.java/src/main/java/org/lance/Branch.java) with an innerBranchVersionMappingclass, add abranchIdentifierfield and getter, update constructor/signature, and include it intoString,equals, andhashCode.java/src/test/java/org/lance/DatasetTest.java) to assert the presence and contents ofbranchIdentifierfor created branches.branch_identifier: List[Tuple[int, str]]to the PythonBranchTypedDict(python/python/lance/dataset.py) and update the Python Rust bridge (python/src/dataset.rs) to includemeta.identifier.version_mappingin the returned branch dictionaries.python/python/tests/test_dataset.py) to validate thebranch_identifiercontents inbranches.list()responses.Testing
DatasetTest::test_brancheswhich exercises branch creation, listing and metadata assertions, and it passed.test_brancheswithpytestwhich verifiesbranch_identifierinbranches.list()and it passed.