-
Notifications
You must be signed in to change notification settings - Fork 94
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 User Defined Types (Issue #98, #105) #113
Conversation
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.
Looks good, left some small nits. It's sad that serializing values involves so much copies and allocations now, but I agree that improving this situation is a task for another PR.
scylla/src/frame/value.rs
Outdated
fn try_into_value(self) -> Result<Value, ValueTooBig>; | ||
} | ||
|
||
pub struct ValueTooBig {} |
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.
Nit: let's change it to a unit struct:
pub struct ValueTooBig;
This way you don't have to type curly braces after the type name when you want to instantiate it:
let err = ValueTooBig;
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.
Done 👍
Cool I didn't know that was possible
}; | ||
|
||
TokenStream::from(generated) | ||
} |
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.
In the future, please put refactors such as moving parts of code into a separate method into separate commits. It makes reviewing easier.
Okay done |
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.
Sorry for the delay. LGTM.
This PR implements support for using User Defined Types that can be used like this:
Fixes #105
Fixes #98
The main challenge introduced by UDT is that creating a
Value
from user struct can fail if the resulting value would be bigger than 2GiB. This PR deals with this by implementingTryIntoValue
for user structs with correspondingtry_values!
.Maybe the
values!
API could be kept if we changed public query APIs to takeValueIn
instead ofValue
whereValueIn
would be something like:and we would return an error when sending a request with
Invalid
values.But that would introduce more complexity in query code, maybe we could wait with such refactor until implementing named values as they are gonna require a redesign anyway?
Another issue is that currently serializing struct into value is not very efficient - fields get recursively converted into values with one allocation for every field. This could be avoided by adding a serialization trait that would write each type into
BufMut
as binaryValue
. But adding such trait would make current Value struct not needed - we could just haveBytes
and value would be justBytes
created using serialization trait.This PR implements UDTs using current Value api, I'm not sure if full redesign of Value api should be included in this, maybe we could create a separate issue for this? Also there might be additional issues that are gonna come to light when implementing the rest of CQL types, so maybe we could wait to get the full picture and then start refactoring.
EDIT:
I have moved refactoring
Value
api into #114