Skip to content
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

Serialization refactor: add new serialization traits #801

Closed
2 tasks done
Tracked by #463
piodul opened this issue Aug 25, 2023 · 3 comments
Closed
2 tasks done
Tracked by #463

Serialization refactor: add new serialization traits #801

piodul opened this issue Aug 25, 2023 · 3 comments
Assignees
Milestone

Comments

@piodul
Copy link
Collaborator

piodul commented Aug 25, 2023

Sub-task of #463.

Let's use the convention introduced in the not-yet-done deserialization refactor and let's name the new traits SerializeCql and SerializeRow:

pub trait SerializeRow {
    fn serialize(&self, ctx: &RowSerializationContext<'_>, out: &mut impl std::io::Write) -> Result<(), std::io::Error>;
}

pub trait SerializeCql {
    fn serialize(&self, typ: &CqlType, out: &mut impl std::io::Write) -> Result<(), std::io::Error>;
}

Instead of std::io::Write, perhaps we could use a different trait that abstracts away appending data to a buffer (bytes::BufMut?). Using a trait instead of e.g. &mut Vec<u8> will let us to very easily implement size hints like here: #582 based on the existing serialize() method.

The RowSerializationContext should contain information about the bind markers: column names and their types. Later, we may add more stuff there.

As this change will affect all Session::query and Session::execute calls which should be ubiquitous in the user codebase, we should make it as easy as possible to migrate to the new API, preferably step by step.

Current serialization API is heavily based on SerializedValues type which is an untyped container for serialized values. Session::{execute,query} receive the argument list as an impl ValueList type - which is just a trait that allows to convert the type to SerializedValues. In general, we should implement SerializeRow for each type that implements ValueList, which also includes SerializedValues.

  • Calls like session.execute(&prepared, (1, 2, 3)) should work automatically out of the box as we will implement SerializeRow for types that implement SerializedValues, and SerializeCql for types that implement Value. This should be the most common case.
  • In a generic context like session.execute(&prepared, generic_list) where generic_list: impl ValueList, the piece of code should be adjusted so that generic_list.serialized()? is called instead - which will convert to SerializedValues which does implement the new SerializedRow.
  • For the case of user impls of ValueList and Value, we can provide macros (not necessarily procedural) that generate a SerializedRow/SerializedCql implementation based on the existing ValueList/Value.

It will be necessary to update some examples and documentation, but I don't expect it to be much work compared to the deserialization refactor.

Tasks

Preview Give feedback
  1. piodul
  2. Lorak-mmk piodul
@piodul
Copy link
Collaborator Author

piodul commented Aug 25, 2023

The issue #802 depends on the SerializeRow and SerializeCql traits introduced by this task. I think just those traits + impls of 2-3 trivial types should be added first, in a separate PR in order to unblock it.

@piodul piodul added this to the 0.10.0 milestone Aug 25, 2023
@piodul
Copy link
Collaborator Author

piodul commented Oct 6, 2023

Definitions of the traits have been merged to main (#819). To complete the task, we further need to:

  • Implement the new traits for the existing types that support Value and ValueList (except for macros for UDTs, those are covered in Serialization refactor: macros for the new traits #802) - can be split over several PRs if needed,
  • Adjust the codebase to use the new serialization traits instead of the old ones (Session::query, Session::execute, _iter versions, etc.).

@Lorak-mmk
Copy link
Collaborator

This was done, in many PRs:
#887
#881
#880
#878
#871
#870 (succeeded by #881)
#869
#862
#858
#855
#851
#819

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants