chore: use Uuid type for Transaction.uuid instead of String#6350
Draft
wjones127 wants to merge 4 commits intolance-format:mainfrom
Draft
chore: use Uuid type for Transaction.uuid instead of String#6350wjones127 wants to merge 4 commits intolance-format:mainfrom
wjones127 wants to merge 4 commits intolance-format:mainfrom
Conversation
Previously, Transaction.uuid was stored as String, requiring Uuid::new_v4().to_string() at creation and .clone() everywhere else. Change it to Uuid, parsing from the proto string in TryFrom and formatting with .hyphenated().to_string() at the proto serialization boundary. Update TransactionBuilder::uuid to accept Uuid directly. Closes lance-format#6346
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Extract uuid as String from Python then parse to Uuid in FromPyObject, and convert Uuid to hyphenated String in IntoPyObject.
- Drop .hyphenated() since Display for Uuid already uses hyphenated format - In dir.rs tests, parse the API-returned string to Uuid for comparison rather than converting Uuid to String — this is semantically cleaner and removes noise from the diff - Add uuid as dev-dependency in lance-namespace-impls for test use
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously,
Transaction.uuidwas typed asStringdespite always holding a UUID value. This requiredUuid::new_v4().to_string()at creation,.clone()at every use site, and no type-level guarantee about the format.Change the field to
Uuid:stringfield inTryFrom<pb::Transaction>withUuid::parse_str.hyphenated().to_string()inFrom<&Transaction>TransactionBuilder::uuidnow acceptsUuiddirectlyDeepSizeOfis now implemented manually sinceuuid::Uuiddoesn't implement that trait (it's fixed-size, sodeep_size_of_childrenis 0 for that field)lance-namespace-implsthat exposed the uuid as a string via the APICloses #6346