Introduce CqlTimeuuid type

[muzarski]

Fixes: #749

Changes

• Introduced CqlTimeuuid type as a wrapper for version 1 uuids
• Implemented uuid::Uuid delegate methods for CqlTimeuuid
• Implemented strong ordering on CqlTimeuuid type so it has the same semantics as Scylla/Cassandra
• CqlValue::Timeuuid now wraps CqlTimeuuid
• Implemented serialization and deserialization logic for CqlTimeuuid
• Got rid of timeuuid <-> uuid::Uuid mapping

Pre-review checklist

• I have split my patch into logically separate commits.
• All commit messages clearly explain what they change and why.
• I added relevant tests for new features and bug fixes.
• All commits compile, pass static checks and pass test.
• PR description sums up the changes and reasons why they should be introduced.
• I have provided docstrings for the public items that I want to introduce.
• I have adjusted the documentation in ./docs/source/.
• I added appropriate Fixes: annotations to PR description.

[piodul]

Please verify whether Scylla (and Cassandra) validates timeuuids like this, i.e. whether it allows inserting uuids that are not necessarily timeuuids into timeuuid column. If they are then perhaps we can do this validation here as well, otherwise I would abstain from it so that users don't run into a situation where they cannot read data they inserted earlier or via another driver.

Perhaps it is safer not to validate here anyway - even if the DBs properly validate right now, just in case if validation breaks in Scylla/Cassandra at some future release.

[muzarski]

Please verify whether Scylla (and Cassandra) validates timeuuids like this, i.e. whether it allows inserting uuids that are not necessarily timeuuids into timeuuid column.

A simple test case shows that Scylla validates whether uuid inserted to timeuuid column is a valid timeuuid.

#[tokio::test]
async fn test_invalid_timeuuid() {
    let session: Session = create_new_session_builder().build().await.unwrap();
    let ks = unique_keyspace_name();

    session
        .query(
            format!(
                "CREATE KEYSPACE IF NOT EXISTS {} WITH REPLICATION = \
            {{'class' : 'NetworkTopologyStrategy', 'replication_factor' : 1}}",
                ks
            ),
            &[],
        )
        .await
        .unwrap();
    session.use_keyspace(ks, false).await.unwrap();

    session
        .query(
            "CREATE TABLE tab (p int, t timeuuid, PRIMARY KEY (p, t))",
            (),
        )
        .await
        .unwrap();

    // Invalid timeuuid (version 2 uuid)
    let invalid_timeuuid = uuid::uuid!("00000000-0000-2000-8080-808080808080");

    session
        .query("INSERT INTO tab (p, t) VALUES (0, ?)", (invalid_timeuuid,))
        .await
        .unwrap();
}

The result:

thread 'transport::cql_types_test::test_invalid_timeuuid' panicked at scylla/src/transport/cql_types_test.rs:1731:10:
called `Result::unwrap()` on an `Err` value: DbError(Invalid, "Exception while binding column t: marshaling error: Unsupported UUID version (2)")

Perhaps it is safer not to validate here anyway - even if the DBs properly validate right now, just in case if validation breaks in Scylla/Cassandra at some future release.

But I agree, I'll get rid of the validation part in CqlTimeuuid constructors i.e. replace

impl TryFrom<Uuid> for CqlTimeuuid {
    type Error = InvalidTimeuuidVersion;

    fn try_from(value: Uuid) -> Result<Self, Self::Error> {
        let version = value.get_version_num();
        if version != 1 {
            return Err(InvalidTimeuuidVersion(version));
        }

        Ok(Self(value))
    }
}

with

impl From<Uuid> for CqlTimeuuid { ... }

[muzarski]

v2:

• rebased on top of master
• fixed review comments

[Lorak-mmk]

Again, if we test sorting, it would be good to have unsorted data. You can use https://docs.rs/rand/latest/rand/seq/trait.SliceRandom.html#tymethod.shuffle to avoid having 2 literals in the code.

[piodul]

This makes the test non-deterministic... If one of the 362880 possible orderings results in an incorrectly sorted sequence then you won't know which one failed. I think we should either use a fixed seed or use a random one but print it during the test so that we can reproduce the results.

I also think that we should test multiple orderings in one run with sort() - let's say several thousand times, so that this part of the test doesn't run longer than a couple milliseconds. Of course, it's unpractical to insert this data to ScyllaDB for each of the orderings, but I don't think it matters because we can assume that ScyllaDB's ordering works correctly - inserting this data in any order only once and then verifying that it matches our expected order should be enough.

[piodul]

We should also use a random generator with known algorithm - std::thread_rng uses an unspecified algorithm which can probably change in the future. We already have tests with fixed pseudorandomness - for example, see partitioners_output_same_result_no_matter_how_input_is_partitioned.

[piodul]

Also, 362880 is not a huge number. Perhaps checking all the permutations would not take much time (let's aim under 100ms)? We can iterate over permutations using Itertools::permutations().

[piodul]

I think it would be great to have this information in the docstring for CqlTimeuuid. The custom behaviors of CqlTimeuuid make it something more than a mere wrapper over Uuid, so I think it needs to be properly explained in the documentation.

[piodul]

This makes the test non-deterministic... If one of the 362880 possible orderings results in an incorrectly sorted sequence then you won't know which one failed. I think we should either use a fixed seed or use a random one but print it during the test so that we can reproduce the results.

I also think that we should test multiple orderings in one run with sort() - let's say several thousand times, so that this part of the test doesn't run longer than a couple milliseconds. Of course, it's unpractical to insert this data to ScyllaDB for each of the orderings, but I don't think it matters because we can assume that ScyllaDB's ordering works correctly - inserting this data in any order only once and then verifying that it matches our expected order should be enough.

[muzarski]

v3:

• rebased on top of master
• addressed review comments

[piodul]

Left some nitpicks, but they aren't blocking the merge - can be fixed later. LGTM.

[piodul]

Nit: the lsb_signed() and msb() do some unnecessary work - the former performs a XOR, and the latter shuffles bytes around. It would be sufficient to take the bytes as they are, AND one of the bytes (like msb() does now) and compare. Same goes for the PartialEq implementation.

These are micro-optimizations with dubious performance impact, so it's fine to implement them in a followup (or never).

[piodul]

Nit: Itertools is an extention trait for iterators. While what you wrote works, you are supposed to use it on the iterators directly:

let perms = sorted_timeuuid_vals
    .iter()
    .permutations(sorted_timeuuid_vals.len())
    .collect::<Vec<_>>();

[piodul]

Nit: the perms are going to take a few tens of megabytes of memory: an allocation of 362880 * 3 * 8 = ~8.3MB of contiguous memory for the direct storage of perms alone (but may be up to twice as much due to vec exponential reallocation) and 362880 * 9 * 16 = ~50MB for the timeuuids alone (spread over 362880 individual allocations).

While this is not very much for today's standards, you need to take into account that many test cases can run in parallel, so this memory usage adds up - and it's a good practice in general to be mindful of those allocations.

In this case, it could have been easily and completely avoided by not collecting the permutations iterator but rather consuming it in a loop.

Again, not a huge problem and not blocking the merge in my opinion. Can be fixed in a followup.