scylladb / cpp-rust-driver

API-compatible rewrite of https://github.com/scylladb/cpp-driver as a wrapper for Rust driver.
GNU Lesser General Public License v2.1
11 stars 11 forks source link

New deserialization api #112

Closed Gor027 closed 2 months ago

Gor027 commented 1 year ago

Pre-review checklist

Fixes #94

This PR Integrates the new deserialization API from the Rust driver which is a WIP. It is dependent upon the execution profiles' PR #107 and can be merged only after the latter is already done. The lazy deserialization allows to also implement binder makers for decimal and duration types. The test_cassandra_types tests for these types are also enabled.

Gor027 commented 1 year ago

@wprzytula @Lorak-mmk Ping for review.

Gor027 commented 1 year ago

A general idea to make the logic more type-safe (which Rust promotes): replace groups of Options with custom types that encode the logic inside themselves (their names, variants, etc.). E.g. in case of CassResultIterator, position is None iff row is None. This dependency could be represented as an enum:

pub struct CassResultIterator {
    result: Arc<CassResult>,
    curr_row_info: CassResultIteratorCurrentRowInfo,
}

enum CassResultIteratorCurrentRowInfo {
    NoRows,
    Rows {
        current_row: CassRow,
        position: usize,
    }
}

I like the idea to express the state of the fields of iterators through enums, but that will make this PR a lot more complicated. Do you mind doing this refactor in another PR? I will open an issue to not forget it.

wprzytula commented 1 year ago

A general idea to make the logic more type-safe (which Rust promotes): replace groups of Options with custom types that encode the logic inside themselves (their names, variants, etc.). E.g. in case of CassResultIterator, position is None iff row is None. This dependency could be represented as an enum:

pub struct CassResultIterator {
    result: Arc<CassResult>,
    curr_row_info: CassResultIteratorCurrentRowInfo,
}

enum CassResultIteratorCurrentRowInfo {
    NoRows,
    Rows {
        current_row: CassRow,
        position: usize,
    }
}

I like the idea to express the state of the fields of iterators through enums, but that will make this PR a lot more complicated. Do you mind doing this refactor in another PR? I will open an issue to not forget it.

I'd prefer to have the refactor done in this PR. You can put it in a separate commit.

Gor027 commented 1 year ago

A general idea to make the logic more type-safe (which Rust promotes): replace groups of Options with custom types that encode the logic inside themselves (their names, variants, etc.). E.g. in case of CassResultIterator, position is None iff row is None. This dependency could be represented as an enum:

pub struct CassResultIterator {
    result: Arc<CassResult>,
    curr_row_info: CassResultIteratorCurrentRowInfo,
}

enum CassResultIteratorCurrentRowInfo {
    NoRows,
    Rows {
        current_row: CassRow,
        position: usize,
    }
}

I have applied the idea in separate commits. Perhaps this approach is more expressive and makes it easy to understand the iterator's state, although it looks overhead to me.

Gor027 commented 1 year ago

Looking at the maze of ifs, I'm convinced that a set of unit tests for deserialization is absolutely required.

I added some relevant tests for collection iterators as Cassandra tests may not cover everything.

Lorak-mmk commented 6 months ago

I think we can safely close this.

Lorak-mmk commented 5 months ago

Oops, wrong PR

Lorak-mmk commented 2 months ago

After all I think we can close this - applying new deserialization to cpp-rust-driver will require at least some changes to this PR, so it will have to be done by someone else.