Basic tests #65

Gor027 · 2022-07-20T08:55:47Z

This should be merged only after #66 is merged.
This PR is dependent upon #64

This PR adds implementation for some necessary functions to pass all BasicTests.

piodul · 2022-08-03T16:29:20Z

.github/workflows/build.yml

@@ -33,4 +33,4 @@ jobs:
      run: cmake -DCASS_BUILD_INTEGRATION_TESTS=ON . && make

    - name: Run integration tests on Scylla 5.0.0
-      run: valgrind --error-exitcode=123 ./cassandra-integration-tests --version=release:5.0.0 --category=CASSANDRA --verbose=ccm --gtest_filter="ClusterTests.*:BasicsTests.*RowsInRowsOut"
+      run: valgrind --error-exitcode=123 ./cassandra-integration-tests --version=release:5.0.0 --category=CASSANDRA --verbose=ccm --gtest_filter="ClusterTests.*:BasicsTests.*"


This change seems to remove tests from the filter. Is that intended?

Actually, it does not remove a test instead, it enables all BasicsTests including the RowsInRowsOut test in it.

Oh, I understand now. The filter used to include tests which match on BasicsTests.*RowsInRowsOut, now it is BasicsTests.* which matches on a larger number of tests.

piodul · 2022-08-03T16:34:53Z

scylla-rust-wrapper/src/query_result.rs

+    }
+
+    let paging_state_usize: usize = paging_state_size.try_into().unwrap();
+    let mut b = BytesMut::from(slice::from_raw_parts(


Bytes::copy_from_slice will be more explicit here than Bytes::from. Its name more clearly conveys what exactly it is doing.

I think BytesMut does not have such a method as copy_from_slice or similar. However, I have slightly changed the implementation to first ensure the necessary capacity allocated and then to create Bytes from a slice, so that we can avoid unnecessary reallocation.

Now it's even better than what I requested, thanks.

piodul · 2022-08-03T16:53:35Z

scylla-rust-wrapper/src/query_result.rs

+        *paging_state = result_from_raw
+            .metadata
+            .paging_state
+            .clone()
+            .unwrap()
+            .as_ptr() as *const c_char;


You are (most probably) returning a pointer to freed memory here.

.clone() creates a temporary Option<Bytes> object, cloned from paging_state. I'm not entirely sure how it will work under the hood, but one of two things will happen:

the new object will increase a reference count of the memory the old Bytes object pointed to (semantics similar to Arc),

or the new object will allocate its own memory and will copy the contents from the old Bytes (semantics similar to Box).

If it's the first one, then you will just increase and decrease the reference count unnecessarily, but it will work.

If it's the second, then it's worse. You then call as_ptr() to get the pointer to the contents of the temporary Bytes, return it, but then the temporary object is deallocated, and the pointer you just returned points to freed memory.

I guess that the lifetime of the memory pointed by the result of cass_result_paging_state_token is bound to the CassResult, right? If so, then you should call as_ref().unwrap().as_ptr(), similarly to the previous statement.

piodul · 2022-08-03T16:55:28Z

scylla-rust-wrapper/src/query_result.rs

+
+    let result_from_raw = ptr_to_ref(result);
+
+    if result_from_raw.metadata.paging_state.is_none() {


Instead of checking for is_none() and then unwrap()-ping, I'd recommend converting the if {} else {} block into match block.

piodul · 2022-08-03T17:11:48Z

tests/src/integration/tests/test_basics.cpp

@@ -254,7 +254,7 @@ CASSANDRA_INTEGRATION_TEST_F(BasicsTests, UnsetParameters) {

  // Execute the insert statement and validate the error code
  Result result = session_.execute(insert_statement, false);
-  if (server_version_ >= "2.2.0") {
+  if (server_version_ >= "release:2.2.0") {


I'm not sure it is the best way to do it. All other tests which perform such a comparison will probably have to be updated in such fashion. Perhaps we should strip the release: prefix elsewhere, before server_version_ is initialized.

@jul-stas is this problem handled in the original driver?

I have changed the implementation to set server_version_ to 3.0.8 in case provided version has the prefix release:. The cpp-driver tests are also run with the same version on Jenkins, and some of the tests which are not disabled may be skipped: e.g. NoCompactEnabledConnection test in BasicsTests.

Gor027 · 2022-08-08T11:21:06Z

V2:

Changed cass_statement_set_paging_state_token implementation to first ensure necessary capacity and then create Bytes from a slice.
server_version_ is statically set to 3.0.8 if the provided version has the prefix release:.

piodul · 2022-08-08T13:34:28Z

scylla-rust-wrapper/src/query_result.rs

+
+    let value_from_raw: &CassValue = ptr_to_ref(value);
+
+    match &value_from_raw.value {


AFAIK the original cass_value_get_bytes works on all CQL types (see here). I understand that it might be difficult to implement it properly right now because the rust driver doesn't allow to return raw, serialized values but rather returns CqlValue - so please add a TODO or a FIXME comment about it.

piodul · 2022-08-08T15:31:35Z

scylla-rust-wrapper/src/query_result.rs

+    }
+
+    let paging_state_usize: usize = paging_state_size.try_into().unwrap();
+    let mut b = BytesMut::from(slice::from_raw_parts(


Now it's even better than what I requested, thanks.

piodul

LGTM after adding the TODO comment.

@jul-stas please take a look, especially at the part which sets the version to "3.0.8".

Added implementation of cass_value_get_bytes to retrieve bytes of a Blob value. Added paging state token setter for CassStatement. Added paging state token getter from query result. The database version is set to 3.0.8 if the provided version in tests has prefix `release:`. This will skip some of the tests which are also skipped in the cpp-driver tests and are not disabled.

Gor027 force-pushed the basic_tests branch 4 times, most recently from 99414af to d6726c1 Compare July 28, 2022 13:26

Gor027 mentioned this pull request Jul 28, 2022

Prepared tests #68

Merged

4 tasks

Gor027 force-pushed the basic_tests branch from d6726c1 to 23c2f49 Compare August 2, 2022 08:21

piodul requested changes Aug 3, 2022

View reviewed changes

Gor027 force-pushed the basic_tests branch 2 times, most recently from 2887397 to e3cbc30 Compare August 8, 2022 11:07

Gor027 force-pushed the basic_tests branch from e3cbc30 to 3ecaf9b Compare August 8, 2022 11:25

piodul reviewed Aug 8, 2022

View reviewed changes

Gor027 force-pushed the basic_tests branch from 3ecaf9b to 8e8196d Compare August 8, 2022 15:56

piodul approved these changes Aug 8, 2022

View reviewed changes

Gor027 requested a review from jul-stas August 9, 2022 09:20

avelanarius merged commit bdd3e3a into scylladb:master Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic tests #65

Basic tests #65

Gor027 commented Jul 20, 2022 •

edited

Loading

piodul Aug 3, 2022

Gor027 Aug 8, 2022

piodul Aug 8, 2022 •

edited

Loading

piodul Aug 3, 2022

Gor027 Aug 8, 2022

piodul Aug 8, 2022

piodul Aug 3, 2022

piodul Aug 3, 2022

piodul Aug 3, 2022

Gor027 Aug 8, 2022

Gor027 commented Aug 8, 2022

piodul Aug 8, 2022

piodul Aug 8, 2022

piodul left a comment


		let result_from_raw = ptr_to_ref(result);

		if result_from_raw.metadata.paging_state.is_none() {


		let value_from_raw: &CassValue = ptr_to_ref(value);

		match &value_from_raw.value {

Basic tests #65

Basic tests #65

Conversation

Gor027 commented Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piodul Aug 8, 2022 • 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

Choose a reason for hiding this comment

Gor027 commented Aug 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piodul left a comment

Choose a reason for hiding this comment

Gor027 commented Jul 20, 2022 •

edited

Loading

piodul Aug 8, 2022 •

edited

Loading