-
Notifications
You must be signed in to change notification settings - Fork 240
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
Add support for UUIDv7 #4877
Add support for UUIDv7 #4877
Conversation
|
CodSpeed Performance ReportMerging #4877 will not alter performanceComparing Summary
|
f5d661e
to
46047f6
Compare
FYI: after this gets an approval, I'll rebase + squash the fixup commits |
@Weakky Gentle ping. Is anything missing in this PR? |
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.
Apologies for the delayed review and thanks for the quality work @mcuelenaere. Would you mind adding a query-engine test that uses uuid(7)
and:
- create a row with it
- reads it from findMany
- reads it from findUnique
You could add this test in this file: query-engine/connector-test-kit-rs/query-engine-tests/tests/writes/ids/uuid_create_graphql.rs
@mcuelenaere Thanks for the quick review fix. I have run the tests. Would you mind having a look at the failures? |
@Weakky I am able to run the tests for sqlite locally, but they seem to fail in CI for the driver adapters. However, I can't seem to easily run these locally + the output of the CI does not give any indication as to what's going wrong (just that it's failing). Any help would be appreciated. |
follow |
81642e5
to
7c65719
Compare
I've rebased on current |
7c65719
to
7258152
Compare
I've also updated |
@Weakky Is there anything missing? |
@mcuelenaere I have investigated why tests were failing. Turns out Fortunately, https://github.com/uuid-rs/uuid/blob/main/src/timestamp.rs#L302-L327 Could you please add the Thanks |
6a39634
to
9d7a3ce
Compare
Aha, good catch! I've pushed a fixup commit for that. I assume just straight-up adding the feature to |
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.
Apologies again for the long delay to get this point. This looks good now. Great work, thanks a lot for the contribution. LGTM 🙏
@@ -52,7 +52,7 @@ tokio = { version = "1.25", features = [ | |||
] } | |||
chrono = { version = "0.4.38", features = ["serde"] } | |||
user-facing-errors = { path = "./libs/user-facing-errors" } | |||
uuid = { version = "1", features = ["serde", "v4"] } | |||
uuid = { version = "1", features = ["serde", "v4", "v7", "js"] } |
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.
@mcuelenaere Yes, that's fine this way.
This implements prisma/prisma#24079. The change is pretty minimal, widening the support from
@default(uuid())
to@default(uuid(7))
.It leverages the existing
uuid
crate instead of adding a new one (uuid7
).Right now this code is mostly untested (apart from the tests I added toCI runs finepsl
), because I couldn't get the tests running locally. I'd appreciate some guidance in this area.