Hide public usages of `num_bigint::BigInt` behind a feature flag #902

muzarski · 2024-01-03T12:03:36Z

Related issue: #771 (not fixed yet, as bigdecimal::BigDecimal is an unstable type as well).

Fixes: #679

Changes:

added CqlVarint type which is a simple native representation of cql varint type
wrapped CqlVarint with CqlValue::Varint
hidden public usages of unstable types (num_bigint - v0.3 and v0.4) behind feature flags. Notice, however, that num_bigint v0.3 is still used within driver's code even without any feature flags. We make use of some library utilities such as pretty printing, or normalization of value's binary representation (e.g. getting rid of leading zeros).

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.

scylla-cql/src/frame/value.rs

scylla-cql/src/frame/response/cql_to_rust.rs

scylla/src/utils/pretty.rs

scylla-cql/Cargo.toml

scylla-cql/src/frame/response/result.rs

scylla-cql/src/frame/value_tests.rs

scylla/src/transport/cql_types_test.rs

scylla-cql/src/frame/value.rs

muzarski · 2024-01-11T16:48:00Z

v2: fixed review comments

muzarski · 2024-01-12T10:52:45Z

v3: adjusted Cargo.lock.msrv file.

muzarski · 2024-01-12T11:25:38Z

v4:
implemented From<Vec<u8>> for CqlVarint and From<CqlVarint> for Vec<u8>

scylla-cql/src/frame/value.rs

scylla/src/transport/cql_types_test.rs

scylla/Cargo.toml

muzarski · 2024-01-12T14:04:41Z

v5: fixes based on review comments

muzarski · 2024-01-12T14:06:29Z

v6: removed derive(Eq) from CqlVarint type

scylla/src/transport/cql_types_test.rs

Lorak-mmk · 2024-01-16T16:28:06Z

scylla/src/transport/cql_types_test.rs

+    let table_name = "cql_varint_tests";
+    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(
+            format!(
+                "CREATE TABLE IF NOT EXISTS {} (id int PRIMARY KEY, val varint)",
+                table_name
+            ),
+            &[],
+        )
+        .await
+        .unwrap();
+
+    let prepared_insert = session
+        .prepare(format!(
+            "INSERT INTO {} (id, val) VALUES (0, ?)",
+            table_name
+        ))
+        .await
+        .unwrap();
+    let prepared_select = session
+        .prepare(format!("SELECT val FROM {} WHERE id = 0", table_name))
+        .await
+        .unwrap();
+
+    for test in tests {
+        let cql_varint = CqlVarint::from_signed_bytes_be_slice(&test);
+        session
+            .execute(&prepared_insert, (&cql_varint,))
+            .await
+            .unwrap();
+
+        let read_values: Vec<CqlVarint> = session
+            .execute(&prepared_select, &[])
+            .await
+            .unwrap()
+            .rows
+            .unwrap()
+            .into_typed::<(CqlVarint,)>()
+            .map(Result::unwrap)
+            .map(|row| row.0)
+            .collect::<Vec<_>>();
+
+        assert_eq!(read_values, vec![cql_varint])
+    }


Why it can't be expressed as a call to run_tests?

As run_tests requires that the type implements std::str::FromStr. It's not trivial to implement it without the use of the num_bigint crate.

CQL accepts varint string literals represented in decimal/octal. Implementing FromStr for hexadecimal numbers wouldn't be hard but the hexadecimal strings provided to run_tests couldn't be contained as literals in queries.

As run_tests requires that the type implements std::str::FromStr. It's not trivial to implement it without the use of the num_bigint crate.

CQL accepts varint string literals represented in decimal/octal. Implementing FromStr for hexadecimal numbers wouldn't be hard but the hexadecimal strings provided to run_tests couldn't be contained as literals in queries.

I don't understand the last sentence. Why couldn't they?

Because you can't put a hexadecimal literal for a value of type varint in CQL query:

cqlsh> CREATE TABLE ks.t (a int PRIMARY KEY, b varint); cqlsh> INSERT INTO ks.t (a, b) VALUES (0, 0x1); InvalidRequest: Error from server: code=2200 [Invalid query] message="Invalid HEX constant (0x1) for "b" of type varint"

I just found out that you actually can, with blobAsVarint built-in CQL function:

cqlsh> create table ks.t (pk int, v varint, primary key (pk)); cqlsh> insert into ks.t (pk, v) values (2, blobasvarint(0x123421934815718293482108421905702184921834021943)); cqlsh> select pk, v from ks.t; pk | v ----+----------------------------------------------------------- 2 | 446351888295288528376725140517290295205369310673010563395 (1 rows)

Perhaps later we can adjust run_tests to take a list of (Cql?)Values instead of strings, use CqlValueDisplayer to print them when putting them into the query and finally compare the values fetched from the database.

Can we do it in a separate PR? I can create an issue regarding this.

Can we do it in a separate PR?

Yes, that's what I meant by "later".

scylla/Cargo.toml

muzarski · 2024-01-19T04:03:05Z

v7: added test cases for varints that can't be contained in any primitive type

scylla-cql/src/frame/value.rs

scylla/Cargo.toml

Lorak-mmk

.

scylla/src/utils/pretty.rs

scylla-cql/src/frame/value.rs

muzarski · 2024-01-25T15:58:35Z

v8:

adjusted CqlValueDisplayer for CqlVarint
removed normalization during CqlVarint initialization. Now, normalization is performed during comparison. To compare two CqlVarint values, we compare their normalized sublices
restored docstring comments about database format of varint values - including information about non-normalized values
added docstrings for comparison logic

piodul · 2024-01-26T07:56:53Z

Now that #894 was merged, there are conflicts with main. Please rebase.

piodul

Please add steps to the rust.yml workflow which cargo check the crate when only num-bigint-03 or num-bigint-04 features are enabled.

piodul · 2024-01-26T07:58:40Z

scylla-cql/src/frame/value.rs

+/// As of now, users are guaranteed to fetch the exact same data they inserted
+/// previously (even if it was not normalized).


What about ScyllaDB's conversion of empty values to null? I think this behavior makes your statement incorrect.

In general, I'd avoid using statements that refer to "as of now" when they refer to some properties that can become false independently from the documentation (i.e. they don't apply to the property of the library but rather something external like ScyllaDB).

scylla-cql/src/frame/value.rs

scylla-cql/src/frame/value_tests.rs

piodul · 2024-01-26T08:48:25Z

scylla/src/transport/cql_types_test.rs

+    let table_name = "cql_varint_tests";
+    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(
+            format!(
+                "CREATE TABLE IF NOT EXISTS {} (id int PRIMARY KEY, val varint)",
+                table_name
+            ),
+            &[],
+        )
+        .await
+        .unwrap();
+
+    let prepared_insert = session
+        .prepare(format!(
+            "INSERT INTO {} (id, val) VALUES (0, ?)",
+            table_name
+        ))
+        .await
+        .unwrap();
+    let prepared_select = session
+        .prepare(format!("SELECT val FROM {} WHERE id = 0", table_name))
+        .await
+        .unwrap();
+
+    for test in tests {
+        let cql_varint = CqlVarint::from_signed_bytes_be_slice(&test);
+        session
+            .execute(&prepared_insert, (&cql_varint,))
+            .await
+            .unwrap();
+
+        let read_values: Vec<CqlVarint> = session
+            .execute(&prepared_select, &[])
+            .await
+            .unwrap()
+            .rows
+            .unwrap()
+            .into_typed::<(CqlVarint,)>()
+            .map(Result::unwrap)
+            .map(|row| row.0)
+            .collect::<Vec<_>>();
+
+        assert_eq!(read_values, vec![cql_varint])
+    }


Perhaps later we can adjust run_tests to take a list of (Cql?)Values instead of strings, use CqlValueDisplayer to print them when putting them into the query and finally compare the values fetched from the database.

muzarski · 2024-01-26T11:09:42Z

v9: rebased on top of master so the upcoming diff addressing review comments is clearer

Added a `CqlVarint` type which represents the native cql varint type. Varint value is represented as a signed binary in big-endian order.

Hidden public usages of the `num_bigint::BigInt` (version 0.3) behind `num-bigint-03` feature flag.

muzarski · 2024-01-26T12:24:30Z

v10: addressed review comments

Added test cases for varints that can't be contained in any primitive.

piodul

LGTM

piodul · 2024-01-26T12:44:16Z

scylla-cql/src/frame/value_tests.rs

 #[test]
-fn cql_varint_normalization_with_bigint() {
+fn cql_varint_normalization_with_bigint03() {


Nit: there is no corresponding test for 0.4. I just checked that the normalization behavior is the same in 0.4, fortunately. The test case can be added in a follow-up.

Yes, I've also checked that normalization works the same in this case. Wasn't sure if such test case is redundant or not.

muzarski changed the title ~~Num bigint as feature~~ Hide public usages of num_bigint::BigInt behind a feature flag Jan 3, 2024

piodul requested changes Jan 8, 2024

View reviewed changes

muzarski force-pushed the num-bigint-as-feature branch from c1fe677 to ce846b7 Compare January 11, 2024 16:46

muzarski force-pushed the num-bigint-as-feature branch from 85eb76a to eb0acf5 Compare January 12, 2024 10:46

muzarski requested a review from piodul January 12, 2024 10:52

muzarski force-pushed the num-bigint-as-feature branch from eb0acf5 to 336d094 Compare January 12, 2024 11:25

piodul requested changes Jan 12, 2024

View reviewed changes

scylla-cql/src/frame/value.rs Outdated Show resolved Hide resolved

scylla-cql/src/frame/value.rs Outdated Show resolved Hide resolved

scylla/src/transport/cql_types_test.rs Outdated Show resolved Hide resolved

scylla/Cargo.toml Outdated Show resolved Hide resolved

muzarski force-pushed the num-bigint-as-feature branch from 336d094 to fa1ff6f Compare January 12, 2024 13:49

muzarski requested a review from piodul January 12, 2024 13:51

muzarski force-pushed the num-bigint-as-feature branch from fa1ff6f to 4cc1b9f Compare January 12, 2024 14:06

muzarski force-pushed the num-bigint-as-feature branch from 4cc1b9f to 0e07e80 Compare January 12, 2024 15:15

avelanarius assigned muzarski Jan 15, 2024

avelanarius added this to the 0.12.0 milestone Jan 15, 2024

Lorak-mmk requested changes Jan 16, 2024

View reviewed changes

muzarski force-pushed the num-bigint-as-feature branch from 0e07e80 to d9e95b0 Compare January 19, 2024 04:02

muzarski requested a review from Lorak-mmk January 19, 2024 05:56

Lorak-mmk reviewed Jan 22, 2024

View reviewed changes

scylla-cql/src/frame/value.rs Outdated Show resolved Hide resolved

scylla/Cargo.toml Show resolved Hide resolved

Lorak-mmk reviewed Jan 22, 2024

View reviewed changes

piodul reviewed Jan 24, 2024

View reviewed changes

scylla/src/utils/pretty.rs Outdated Show resolved Hide resolved

piodul reviewed Jan 24, 2024

View reviewed changes

scylla-cql/src/frame/value.rs Outdated Show resolved Hide resolved

muzarski force-pushed the num-bigint-as-feature branch from d9e95b0 to b20e26e Compare January 25, 2024 15:55

muzarski requested review from Lorak-mmk and piodul January 25, 2024 15:58

piodul requested changes Jan 26, 2024

View reviewed changes

muzarski force-pushed the num-bigint-as-feature branch from b20e26e to 93c6941 Compare January 26, 2024 11:08

muzarski added 9 commits January 26, 2024 12:58

value: introduce CqlVarint type

e94b89e

Added a `CqlVarint` type which represents the native cql varint type. Varint value is represented as a signed binary in big-endian order.

varint: implement PartialEq for CqlVarint

65146eb

varint: implement std::hash::Hash for CqlVarint

9ddaedf

value_tests: add compute_hash util function

62acc15

value_tests: add tests for CqlVarint normalization

f701d8f

cql: wrap CqlVarint with CqlValue::Varint

53cf06c

tests: add tests for CqlVarint

ae623b0

cargo: hide num_bigint v0.3 behind feature flag

a851069

Hidden public usages of the `num_bigint::BigInt` (version 0.3) behind `num-bigint-03` feature flag.

cargo: add feature flag for num-bigint v0.4

5d4bb9d

muzarski force-pushed the num-bigint-as-feature branch from 93c6941 to a7f0787 Compare January 26, 2024 12:23

muzarski added 5 commits January 26, 2024 13:28

docstrings: add info about num_bigint features

9be7d9f

types_test: test cases for big varints

81c5867

Added test cases for varints that can't be contained in any primitive.

docs: update info about varint native data type

358814c

cargo: update Cargo.lock.msrv file

d30a9be

CI: add cargo checks with num_bigint features

21988eb

muzarski force-pushed the num-bigint-as-feature branch from a7f0787 to 21988eb Compare January 26, 2024 12:29

muzarski requested a review from piodul January 26, 2024 12:44

piodul approved these changes Jan 26, 2024

View reviewed changes

Lorak-mmk approved these changes Jan 26, 2024

View reviewed changes

Lorak-mmk merged commit 9a84387 into scylladb:main Jan 26, 2024
9 checks passed

Lorak-mmk mentioned this pull request Feb 1, 2024

Release 0.12 #924

Closed

muzarski deleted the num-bigint-as-feature branch October 29, 2024 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide public usages of `num_bigint::BigInt` behind a feature flag #902

Hide public usages of `num_bigint::BigInt` behind a feature flag #902

muzarski commented Jan 3, 2024 •

edited

Loading

muzarski commented Jan 11, 2024

muzarski commented Jan 12, 2024

muzarski commented Jan 12, 2024

muzarski commented Jan 12, 2024

muzarski commented Jan 12, 2024

Lorak-mmk Jan 16, 2024

muzarski Jan 18, 2024 •

edited

Loading

Lorak-mmk Jan 22, 2024

muzarski Jan 24, 2024

piodul Jan 24, 2024

piodul Jan 26, 2024

muzarski Jan 26, 2024

piodul Jan 26, 2024

muzarski commented Jan 19, 2024

Lorak-mmk left a comment

muzarski commented Jan 25, 2024

piodul commented Jan 26, 2024

piodul left a comment

piodul Jan 26, 2024

piodul Jan 26, 2024

muzarski commented Jan 26, 2024

muzarski commented Jan 26, 2024

piodul left a comment

piodul Jan 26, 2024

muzarski Jan 26, 2024

		/// As of now, users are guaranteed to fetch the exact same data they inserted
		/// previously (even if it was not normalized).

Hide public usages of num_bigint::BigInt behind a feature flag #902

Hide public usages of num_bigint::BigInt behind a feature flag #902

Conversation

muzarski commented Jan 3, 2024 • edited Loading

Changes:

Pre-review checklist

muzarski commented Jan 11, 2024

muzarski commented Jan 12, 2024

muzarski commented Jan 12, 2024

muzarski commented Jan 12, 2024

muzarski commented Jan 12, 2024

Choose a reason for hiding this comment

muzarski Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muzarski commented Jan 19, 2024

Lorak-mmk left a comment

Choose a reason for hiding this comment

muzarski commented Jan 25, 2024

piodul commented Jan 26, 2024

piodul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muzarski commented Jan 26, 2024

muzarski commented Jan 26, 2024

piodul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hide public usages of `num_bigint::BigInt` behind a feature flag #902

Hide public usages of `num_bigint::BigInt` behind a feature flag #902

muzarski commented Jan 3, 2024 •

edited

Loading

muzarski Jan 18, 2024 •

edited

Loading