Add serialization traits #819
Conversation
| &self, | ||
| _ctx: &RowSerializationContext<'_>, | ||
| out: &mut Vec<u8>, | ||
| ) -> Result<(), Arc<dyn Any + Send + Sync>> { |
There was a problem hiding this comment.
Please introduce an alias for Arc<dyn Any + Send + Sync>, e.g. SerializationError.
| use crate::{_macro_internal::Value, frame::response::result::ColumnType}; | ||
|
|
||
| pub trait SerializeCql { | ||
| fn preliminary_type_check(&self, typ: &ColumnType) -> bool; |
There was a problem hiding this comment.
I think the preliminary type check should return Result<(), SerializationError> so that it can return some information about the error.
| let err: Arc<dyn Any + Send + Sync> = Arc::new(err); | ||
| err |
There was a problem hiding this comment.
Nit: this can be written as Arc::new(err) as Arc<dyn Any + Send + Sync>, or Arc::new(err) as SerializationError
| @@ -0,0 +1,29 @@ | |||
| use std::{any::Any, sync::Arc}; | |||
|
|
|||
| use crate::{_macro_internal::Value, frame::response::result::ColumnType}; | |||
There was a problem hiding this comment.
Please avoid using the _macro_internal paths.
| pub mod serialize_row; | ||
| pub mod serialize_value; |
There was a problem hiding this comment.
The serialize_ part seems unnecessary, these already are in a module called serialize.
| pub fn column_by_idx(&self, i: usize) -> Option<&ColumnSpec> { | ||
| self.columns.get(i) | ||
| } |
There was a problem hiding this comment.
I'm not exactly a fan of this... This doesn't allow to easily get information about the number of columns, and makes iteration over the columns slightly awkward. How about exposing this field through a getter that returns &'a [ColumnSpec]?
| } | ||
|
|
||
| pub trait SerializeRow { | ||
| fn preliminary_type_check(&self, ctx: &RowSerializationContext<'_>) -> bool; |
There was a problem hiding this comment.
I thought that this was supposed to be an associated function, not a method?
There was a problem hiding this comment.
Ooops, yes - that's what I did in previous version but forgot about in this, sorry
Lorak-mmk
force-pushed
the
add_serialization_traits
branch
from
95d11bc
to
dee90ba
Compare
|
Adressed review comments |
| self.columns | ||
| } | ||
|
|
||
| // TODO: change RowSerializationContext to make this faster |
There was a problem hiding this comment.
Nit: what do you mean by this comment?
There was a problem hiding this comment.
Apart from list of clumns we can also keep a hashmap from name to column spec so that name lookups are in constant time instead of linear.
| } | ||
|
|
||
| impl<'a> RowSerializationContext<'a> { | ||
| pub fn columns(&self) -> &'a [ColumnSpec] { |
There was a problem hiding this comment.
Nit: this method and column_by_name could be made #[inline] - they are small and simple, and without it the compiler won't consider inlining them in when used in external crates.
This commit adds experimental version of new serialization traits. Blanket implementation are provided to support older traits.
Lorak-mmk
force-pushed
the
add_serialization_traits
branch
from
dee90ba
to
b58f804
Compare
This commit adds experimental version of new serialization traits. Blanket implementation are provided to support older traits.
For now there are no comments / other implementations with real type checking as the purpose of this PR is to unblock the work on macros
and other parts of refactor.
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.