Value support for Cassandra types

[conorbros]

Pulling some changes out of #404

Done

• Support all native and collection Cassandra types in Value
• Add todos for conversions not yet implemented.

[rukai]

I think the questions we really need to answer before trying to resolve these issues is:

• Does the data represented in Value need to be able to read without losing precision/accuracy from the original message?
• And a stronger requirement: Do we need to be able to roundtrip all data through Value?

[benbromhead]

To answer @rukai questions. Yes to both.

We should be able to losslesly go from arbitrary Redis <=> Value and Cassandra <=> Value (thus Redis <=> Cassandra)

[benbromhead]

This will result in some "less than optimal" decisions. E.g. storing certain types that use more memory than needed. E.g. if one datastores Int type is 32 bits and another is 64 bits.

For the moment, I would just continue down the path of conversion to a common Value type and using that as common intermediary. For some context, however, @Claude-at-Instaclustr and I have also had some discussions about a better way of representing internal types and presenting a nicer way to reason about them. Mainly using a resource descriptor framework (RDF triple). An RDF won't solve the above two questions, but it will provide a better way of reasoning about types... potentially.

See https://ieeexplore.ieee.org/document/7226706 for some background reading.

[Claude-at-Instaclustr]

A better link to the paper: https://arxiv.org/abs/1410.4977

…

On Wed, 8 Dec 2021 at 06:50, Ben Bromhead ***@***.***> wrote: This will result in some "less than optimal" decisions. E.g. storing certain types that use more memory than needed. E.g. if one datastores Int type is 32 bits and another is 64 bits. For the moment, I would just continue down the path of conversion to a common Value type and using that as common intermediary. For some context, however, @Claude-at-Instaclustr <https://github.com/Claude-at-Instaclustr> and I have also had some discussions about a better way of representing internal types and presenting a nicer way to reason about them. Mainly using a resource descriptor framework (RDF triple). An RDF won't solve the above two questions, but it will provide a better way of reasoning about types... potentially. See https://ieeexplore.ieee.org/document/7226706 for some background reading. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#409 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AVM5ARL4JRBT2YYJUZWYQETUP354BANCNFSM5JS22UNQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[rukai]

Some initial feedback, I'll look into the BigInt and BigDecimal stuff deeper tomorrow

[rukai]

Once all the feedback is addressed we can land this.

As you've said, in a follow up PR you can add tests for all functions here (Dont bother with into_str_bytes though as its implementation is currently nonsense)

[conorbros]

I'll implement the collections stuff in this PR too because that's blocking my other Cassandra integration test PRs

[conorbros]

Updates:

• added support for parsing collections into Value
• changes to cdrs-tokio: conorbros/cdrs-tokio@0353d89

[conorbros]

While rebasing I accidentally added into_str_bytes back so I have removed it in the most recent commit.

[conorbros]

Addressed all of the feedback. The changes to cdrs-tokio can be found here

[rukai]

This looks fine to me.
I can approve once the cassandra-protocol changes land upstream.

One thing I did notice: for some reason #[allow(clippy::mutable_key_type)] isnt needed if we dont store values as an intermediate variable.
Up to you if you want to make use of that.

[conorbros]

That is strange, I've removed it so we don't have an unnecessary #[allow].

Opened a PR for the changes to cdrs-tokio: krojew/cdrs-tokio#74

[rukai]

LGTM