cass_result: implement missing functions #136

muzarski · 2024-07-03T09:26:39Z

Fix: #35

Implemented two missing CassResult functions:

cass_result_column_type
cass_result_column_data_type

Also fixed cass_result_first_row implementation.

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
PR description sums up the changes and reasons why they should be introduced.
~~[ ] I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.~~
~~[ ] I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.~~

mykaul · 2024-07-03T09:31:34Z

Do we have tests to validate this?

muzarski · 2024-07-03T22:23:50Z

Do we have tests to validate this?

At first, I wasn't sure if there are integration tests checking this - unfortunately, the answer is no (previous implementation of cass_result_first_row would panic in some cases - this bug was never detected). Tomorrow I'll try to implement some cpp integration tests for the cass_result_* API.

muzarski · 2024-07-03T22:28:10Z

v2: Fixed cass_result_first_row

wprzytula · 2024-07-04T04:07:32Z

Do we have tests to validate this?

At first, I wasn't sure if there are integration tests checking this - unfortunately, the answer is no (previous implementation of cass_result_first_row would panic in some cases - this bug was never detected). Tomorrow I'll try to implement some cpp integration tests for the cass_result_* API.

WDYT about adding Rust unit tests first? They should be the first layer to detect errors, they have smaller overhead and, probably the most important for convenience of us maintainers, they are written in Rust, not in Datastax C++ test framework.

muzarski · 2024-07-05T16:01:17Z

v3: implemented unit tests for basic cass_result_* API.

scylla-rust-wrapper/src/query_result.rs

wprzytula · 2024-07-09T10:16:24Z

In this PR, you should instantly enable related integration tests and/or examples.

scylla-rust-wrapper/src/session.rs

scylla-rust-wrapper/src/query_result.rs

muzarski · 2024-09-05T11:11:50Z

This PR is waiting for: #143

dkropachev · 2024-10-07T13:52:37Z

This PR is waiting for: #143

#143 is in, can you continue working on it ?

muzarski · 2024-10-07T14:09:37Z

This PR is waiting for: #143

#143 is in, can you continue working on it ?

Yes

muzarski · 2024-10-07T14:40:30Z

Rebased on master

muzarski · 2024-10-07T15:33:27Z

v4:
Now, the result metadata (Vec<Arc<CassDataType>>) is stored in CassPrepared. It allows us to omit the allocations each time we execute the prepared statement. We don't have to construct each CassDataType from scratch (it can be a really costly operation) based on the column specs.

Now, CassResultData::from_result_payload accepts an additional argument, which is an optional vector of result column data types.

For simple queries, we provide None - then CassDataTypes are created based on column specs received from the server.
For prepared statements, we provide a Some(<metadata cached in CassPrepared>) to avoid additional allocations

scylla-rust-wrapper/src/prepared.rs

scylla-rust-wrapper/src/session.rs

muzarski · 2024-10-08T10:26:16Z

v4.1: wrapped the result data type metadata vector with an Arc.

The reason for this is that CassPrepared and CassResultData should share this vector, and this way we can omit an additional allocation each execution.

Note: Each individual CassDataType in the vector still needs to be wrapped with an Arc. This is because each CassValue (contained in CassResult in each CassRow) needs to hold information about the type.

scylla-rust-wrapper/src/prepared.rs

scylla-rust-wrapper/src/session.rs

scylla-rust-wrapper/src/query_result.rs

wprzytula · 2024-10-10T13:11:09Z

scylla-rust-wrapper/src/query_result.rs

+#[no_mangle]
+pub unsafe extern "C" fn cass_result_column_data_type(
+    result: *const CassResult,
+    index: size_t,
+) -> *const CassDataType {
+    let result_from_raw: &CassResult = ptr_to_ref(result);
+    let index_usize: usize = index.try_into().unwrap();


Justify unwrap.

Actually, I have a question. Here is the definition of size_t in types.rs:

pub type size_t = cass_uint64_t; pub type cass_uint64_t = u64;

Why? There is a libc::size_t which is an alias for usize. Couldn't we make use of it? Then, we wouldn't need to do a possibly lossy conversion each time (from u64 to usize).

cc: @Lorak-mmk

I believe that sizes of both size_t and usize depend on the architecture. I don't understand why we assume that size_t is always u64 (8 bytes long)

You are right of course. This alias was added probably by accident by @wprzytula in de6143a#diff-fffb385b3ac71791e0a2ddee4b04afa8c2d889e2713f4b1e4f29d216ec6045fd
Before that we already had definition for size_t generated by bindgen in build.rs:

cpp-rust-driver/scylla-rust-wrapper/build.rs

Lines 24 to 37 in 4d056ed

fn prepare_basic_types(out_path: &Path) {

let basic_bindings = bindgen::Builder::default()

.header("extern/cassandra.h")

.parse_callbacks(Box::new(bindgen::CargoCallbacks))

.layout_tests(true)

.generate_comments(false)

.allowlist_type("size_t")

.generate()

.expect("Unable to generate bindings");

basic_bindings

.write_to_file(out_path.join("basic_types.rs"))

.expect("Couldn't write bindings!");

}

This generated header is included in types.rs, so it should be safe to remove this wrong alias - even more, they should crash and not compile. However for some reason I can't remove it, it causes errors. This needs investigation and proper fix.

Generated basic_types.rs file is empty for some reason...

Adding .size_t_is_usize(false) call to the builder makes the definition generate again:

───────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── │ File: ./target/debug/build/scylla-cpp-driver-rust-e897748878d67d0e/out/basic_types.rs ───────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── 1 │ /* automatically generated by rust-bindgen 0.65.1 */ 2 │ 3 │ pub type size_t = ::std::os::raw::c_ulong; ───────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

Generated basic_types.rs file is empty for some reason...

Same happened for me when I was playing with build.rs and types.rs

Could you apply this fix and remove the redundant size_t alias?

I opened a separate PR that solves issues regarding bindings in basic_types.rs: #188

scylla-rust-wrapper/src/query_result.rs

scylla-rust-wrapper/src/session.rs

muzarski · 2024-10-10T14:00:28Z

Rebased on master

This way, we won't need to construct the data types from the `QueryResult` each time statement is executed. We will be able to simply make use of result metadata cached in prepared statement.

CassDataType can be a heavy object if the type is nested. In this commit we include info about column data types in result metadata (Arc<Vec<Arc<CassDataType>>>). This will be needed to implement cass_result_column_data_type function. We also adjust the construction of `CassValue::value_type` field and avoid allocations. We simply clone Arc from result metadata.

muzarski · 2024-10-10T16:07:32Z

v4.2: addressed @wprzytula comments

I'd like to introduce the size_t fix (maybe next to other binding fixes) in other PRs. I'm currently investigating it.

muzarski · 2024-10-10T16:09:19Z

v4.2.1: fixed clippy lint

Implemented two missing CassResult functions: - cass_result_column_type - cass_result_column_data_type

Previous implementation of cass_result_first_row is buggy. It panics when rows are empty. This commit fixes the implementation.

Implemented tests for two cases: - Rows result - i.e. result of SELECT query. - non-Rows result - e.g. result of INSERT query

This function unnecessarily accepted and returned an Option. It should be the caller's responsibility to call this function only if `result.rows` is Some.

muzarski · 2024-10-10T18:25:05Z

v4.2.2: replaced unwrap() with expect() for u64 to usize conversion.

muzarski self-assigned this Jul 3, 2024

muzarski mentioned this pull request Jul 3, 2024

CassResult missing functions #35

Closed

7 tasks

muzarski requested a review from Lorak-mmk July 3, 2024 09:29

muzarski force-pushed the cass_result_missing_functions branch from 0c0ef5b to b7813e3 Compare July 3, 2024 22:27

muzarski force-pushed the cass_result_missing_functions branch from b7813e3 to 12ac9e6 Compare July 5, 2024 15:57

muzarski requested a review from wprzytula July 5, 2024 15:58

muzarski force-pushed the cass_result_missing_functions branch from 12ac9e6 to ebf360c Compare July 5, 2024 16:00

wprzytula reviewed Jul 8, 2024

View reviewed changes

scylla-rust-wrapper/src/query_result.rs Outdated Show resolved Hide resolved

muzarski force-pushed the cass_result_missing_functions branch from ebf360c to d81ecec Compare July 9, 2024 09:43

wprzytula approved these changes Jul 9, 2024

View reviewed changes

Lorak-mmk self-assigned this Jul 9, 2024

Lorak-mmk requested changes Jul 10, 2024

View reviewed changes

scylla-rust-wrapper/src/session.rs Outdated Show resolved Hide resolved

scylla-rust-wrapper/src/query_result.rs Outdated Show resolved Hide resolved

Lorak-mmk removed their assignment Jul 31, 2024

muzarski force-pushed the cass_result_missing_functions branch from d81ecec to e99166e Compare October 7, 2024 14:40

muzarski force-pushed the cass_result_missing_functions branch from e99166e to de8d745 Compare October 7, 2024 15:26

muzarski force-pushed the cass_result_missing_functions branch 2 times, most recently from 69eb3bf to e488fe7 Compare October 7, 2024 15:37

muzarski requested review from Lorak-mmk and wprzytula October 7, 2024 15:43

Lorak-mmk reviewed Oct 8, 2024

View reviewed changes

scylla-rust-wrapper/src/prepared.rs Outdated Show resolved Hide resolved

scylla-rust-wrapper/src/session.rs Show resolved Hide resolved

muzarski force-pushed the cass_result_missing_functions branch from e488fe7 to b81fa26 Compare October 8, 2024 10:21

muzarski requested a review from Lorak-mmk October 8, 2024 10:26

Lorak-mmk approved these changes Oct 8, 2024

View reviewed changes

wprzytula requested changes Oct 10, 2024

View reviewed changes

muzarski force-pushed the cass_result_missing_functions branch from b81fa26 to 9bfa0c7 Compare October 10, 2024 14:00

muzarski added 2 commits October 10, 2024 16:04

prepared: hold information about result metadata types

1242397

This way, we won't need to construct the data types from the `QueryResult` each time statement is executed. We will be able to simply make use of result metadata cached in prepared statement.

muzarski force-pushed the cass_result_missing_functions branch from 9bfa0c7 to 056feb1 Compare October 10, 2024 16:05

muzarski force-pushed the cass_result_missing_functions branch from 056feb1 to 072ec01 Compare October 10, 2024 16:08

muzarski added 4 commits October 10, 2024 20:24

cass_result: implement missing functions

64e205f

Implemented two missing CassResult functions: - cass_result_column_type - cass_result_column_data_type

cass_result: fix cass_result_first_row

7063193

Previous implementation of cass_result_first_row is buggy. It panics when rows are empty. This commit fixes the implementation.

result: implement tests for cass_result basic api.

86ba58d

Implemented tests for two cases: - Rows result - i.e. result of SELECT query. - non-Rows result - e.g. result of INSERT query

result: simplify create_cass_rows_from_rows

538ffaf

This function unnecessarily accepted and returned an Option. It should be the caller's responsibility to call this function only if `result.rows` is Some.

muzarski force-pushed the cass_result_missing_functions branch from 072ec01 to 538ffaf Compare October 10, 2024 18:24

muzarski requested a review from wprzytula October 10, 2024 18:25

wprzytula approved these changes Oct 10, 2024

View reviewed changes

muzarski requested a review from Lorak-mmk October 11, 2024 11:24

Lorak-mmk approved these changes Oct 11, 2024

View reviewed changes

muzarski merged commit 4d006df into scylladb:master Oct 11, 2024
11 checks passed

muzarski mentioned this pull request Nov 27, 2024

Release 0.3.0 #209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cass_result: implement missing functions #136

cass_result: implement missing functions #136

muzarski commented Jul 3, 2024 •

edited

Loading

mykaul commented Jul 3, 2024

muzarski commented Jul 3, 2024

muzarski commented Jul 3, 2024

wprzytula commented Jul 4, 2024

muzarski commented Jul 5, 2024

wprzytula commented Jul 9, 2024

muzarski commented Sep 5, 2024

dkropachev commented Oct 7, 2024

muzarski commented Oct 7, 2024

muzarski commented Oct 7, 2024

muzarski commented Oct 7, 2024

muzarski commented Oct 8, 2024

wprzytula Oct 10, 2024

muzarski Oct 10, 2024

muzarski Oct 10, 2024

Lorak-mmk Oct 10, 2024

Lorak-mmk Oct 10, 2024

Lorak-mmk Oct 10, 2024 •

edited

Loading

muzarski Oct 10, 2024

Lorak-mmk Oct 10, 2024

muzarski Oct 10, 2024

muzarski commented Oct 10, 2024

muzarski commented Oct 10, 2024

muzarski commented Oct 10, 2024

muzarski commented Oct 10, 2024

	fn prepare_basic_types(out_path: &Path) {
	let basic_bindings = bindgen::Builder::default()
	.header("extern/cassandra.h")
	.parse_callbacks(Box::new(bindgen::CargoCallbacks))
	.layout_tests(true)
	.generate_comments(false)
	.allowlist_type("size_t")
	.generate()
	.expect("Unable to generate bindings");

	basic_bindings
	.write_to_file(out_path.join("basic_types.rs"))
	.expect("Couldn't write bindings!");
	}

cass_result: implement missing functions #136

cass_result: implement missing functions #136

Conversation

muzarski commented Jul 3, 2024 • edited Loading

Pre-review checklist

mykaul commented Jul 3, 2024

muzarski commented Jul 3, 2024

muzarski commented Jul 3, 2024

wprzytula commented Jul 4, 2024

muzarski commented Jul 5, 2024

wprzytula commented Jul 9, 2024

muzarski commented Sep 5, 2024

dkropachev commented Oct 7, 2024

muzarski commented Oct 7, 2024

muzarski commented Oct 7, 2024

muzarski commented Oct 7, 2024

muzarski commented Oct 8, 2024

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

Lorak-mmk Oct 10, 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

muzarski commented Oct 10, 2024

muzarski commented Oct 10, 2024

muzarski commented Oct 10, 2024

muzarski commented Oct 10, 2024

muzarski commented Jul 3, 2024 •

edited

Loading

Lorak-mmk Oct 10, 2024 •

edited

Loading