binding: fix by-name parameter binding and enable corresponding IT #187

muzarski · 2024-10-09T09:54:27Z

Motivation

Before this PR, binding values by name to unprepared queries was buggy:

Main issue was that we were not even reading name-to-index mapping (SimpleQuery::name_to_bound_index) - only writing to it. In result, we were not really binding by name, but rather by position, in the same order as cass_statement_bind_* calls appeared. The bound_values vector was directly passed to Session::query_* calls.
CassStatement::bind_cql_value would return CASS_OK when size of SimpleQuery::name_to_bound_index map exceeded the declared size of parameters (CassStatement::bound_values.len())
We wouldn't unquote the parameter names for unprepared queries in CassStatement::bind_cql_value

This PR solves this issue, and enables another integration test suite - NamedParametersTests.

Changes

Apart from the fixes to CassStatement::bind_cql_value, I also introduced a custom implementation of SerializeRow for unprepared queries execution. I created a SimpleQueryRowSerializer struct which holds necessary information to perform serialization. Now, with following context:

bound_values vector - directly taken from CassStatement::bound_values
an optional name-to-index mapping (from SimpleQuery::name_to_bound_index) - if it's non-empty, it means that we bind by names, otherwise we preserve the original order of bound_values.
column specs from RowSerializationContext

we are able to serialize values in correct order for unprepared queries.

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 implemented Rust unit tests for the features/changes 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.

Lorak-mmk · 2024-10-09T10:24:38Z

4 out of 6 tests from this suite are passing. We can enable them.

When it comes to SimpleStatementAnyOrder and SimpleStatementInvalidName, I don't think we will ever enable them. The reason is that rust-driver no longer supports 'WITH_NAMES_FOR_VALUES' flag (https://github.com/apache/cassandra/blob/trunk/doc/native_protocol_v4.spec#L379) since serialization refactor, thus does not support "by name" value binding.

I don't get it. Rust Driver prepares the statements underneath and should be able to handle named bind markers in queries.

The reason that analogous tests for prepared statements pass is because "name-to-indexes" mapping can be retrieved from the prepared statement's metadata received from the server upon preparation. Thanks to that, we can bind the values by name in correct order (in cpp-rust-driver layer).

muzarski · 2024-10-09T10:31:57Z

I don't get it. Rust Driver prepares the statements underneath and should be able to handle named bind markers in queries.

You are right. Probably, the custom SerializeRow implementation will do the job. I'll try it out

muzarski · 2024-10-09T16:40:59Z

I updated the description.

scylla-rust-wrapper/src/statement.rs

scylla-rust-wrapper/src/cass_error.rs

tests/src/integration/tests/test_named_parameters.cpp

muzarski · 2024-10-30T12:35:36Z

v2: rebased on #200. We don't match the specific SerializationError by stringification anymore - downcast_ref() method is used.

muzarski · 2024-10-30T12:57:45Z

v3: rewritten the bind_cql_value_by_name method according to @wprzytula suggestions - thank you! Now, the method logic is much clearer since we got rid of early returns.

muzarski · 2024-10-30T18:06:07Z

Rebased on master

wprzytula · 2024-12-01T21:27:49Z

The PR's name is a bit misleading. It sounds like it changes only tests and enables them; instead, it fixes bugs in the driver! This should definitely be indicated in the name, and also prioritised higher.

muzarski · 2024-12-04T00:55:59Z

v3.1: Applied some of the suggestions by @Lorak-mmk

muzarski · 2024-12-05T02:49:53Z

v4:

Applied a suggestion by @Lorak-mmk. Moved bound_values out of CassStatement, to new structs BoundSimpleQuery and BoundPreparedStatement. It reduces the complexity of binding functions, as we do not have to match against &self.statement in multiple places.
Defined two new integration test cases that perform by-name binding to multiple columns using the same bind marker

scylla-rust-wrapper/src/statement.rs

Current binding code is fairly complicated. This is because we need to match against the &[mut] self.statement in each function. To prevent this, we move the bound_values to lower layer - i.e. BoundPreparedStatement and BoundSimpleQuery. This commit does not change the logic at all. It is equivalent to the previous version.

`cass_statement_reset_paramters` should reset the name-to-index mapping used in simple queries.

I reduced mutability scope of some variables, renamed some variables so they have more descriptive names, and added some comments.

I refactored the function, so we get rid of early returns. Thanks to that, we can discover a bug: Previously, if we did not find a free index for a value with given name, we returned CASS_OK. cpp-driver returns an error in such case. Co-authored-by: Wojciech Przytuła <[email protected]>

muzarski · 2024-12-09T12:34:06Z

Rebased on master

Previously, we would search for the first value in `bound_values` vector that is `Unset` to prevent overwriting the values, that were previously possibly bound via by index binding. This however, is not consistent with cpp-driver. The cpp-driver overwrites the value in such case. To allocate new index, we simply get the current size of `name_to_bound` map size. Consider following example, where user declared 2 parameters (thus bound_values.len() == 2): - by_name("foo", 5) <- bound_values[0] = 5, name_to_bound_index["foo"] = 0 - by_index(1, 19) <- bound_values[1] = 19 - by_name("bar", 13) <- previously: returns an error, because we can't find an index where value is Unset. Now: bound_values[1] = 13, name_to_bound_index["bar"] = 1 I'm adding an integration test case for analogous scenario at the end of this PR.

Previously, we would store a quoted parameter name for simple queries. This is a bug. The quotes are only introduces to decide whether the name is case sensitive for prepared statements. To fix this, I extracted the common logic of unquoting to `CassStatement::bind_cql_value_by_name`. It will pass the unquoted parameter name to methods on corresponding statement kind.

In this commit, we also adjust one of the NamedParameterTests to this change. We are going to enable this test suite in the next commit.

In these tests, we use the same bind marker (:abc) to bind values to 3 different columns. We then validate the results. There are two test cases - one for prepared statements, and other one for unprepared statements.

muzarski · 2024-12-10T19:58:08Z

v4.1: Thanks to @Lorak-mmk I discovered a small inconsistency between cpp-rust and cpp-driver in regards to interleaving by name and by index bindings. I fixed it (commit: binding: adjust finding new index to cpp-driver semantics) and introduced integration test cases that verify this (last commit). I checked that the same test cases pass for cpp-driver.

wprzytula

I think I'd feel more secure about these changes with at least a number of unit tests of the BoundStatement. There are integration tests, that's true, but for some reason I tend to trust Rust unit tests targetting the specific area more.

muzarski · 2024-12-11T17:03:49Z

I think I'd feel more secure about these changes with at least a number of unit tests of the BoundStatement. There are integration tests, that's true, but for some reason I tend to trust Rust unit tests targetting the specific area more.

I think it would be possible to implement unit tests for simple queries, if I extracted another struct containing binding payload (everything from BoundSimpleQuery apart from Query). It would be possible because Query is not required during binding, and we could mock metadata for serialization.

However, I don't see how we could implement unit tests for prepared statements. Does rust driver allow to somehow mock PreparedStatement structure?

Anyhow, I'll merge it as is. There are multiple places where Rust unit tests would be useful, but currently (AFAIK) we don't have a way to mock structures from rust-driver.

muzarski self-assigned this Oct 9, 2024

muzarski marked this pull request as draft October 9, 2024 10:55

muzarski force-pushed the enable-named-parameters-tests branch 5 times, most recently from 93fdd2f to f7bb42a Compare October 9, 2024 16:00

muzarski changed the title ~~ci: enable passing NamedParameterTests~~ ci: enable NamedParameterTests Oct 9, 2024

muzarski marked this pull request as ready for review October 9, 2024 16:40

muzarski requested review from Lorak-mmk and wprzytula October 9, 2024 16:40

wprzytula requested changes Oct 10, 2024

View reviewed changes

muzarski mentioned this pull request Oct 10, 2024

errors: expose pub getters for type-erased errors scylladb/scylla-rust-driver#1087

Merged

5 tasks

muzarski force-pushed the enable-named-parameters-tests branch from f7bb42a to f9a965e Compare October 30, 2024 12:33

muzarski force-pushed the enable-named-parameters-tests branch from f9a965e to 9fe9d95 Compare October 30, 2024 12:55

muzarski mentioned this pull request Oct 30, 2024

Investigate and improve the performance of by_name parameter binding #201

Open

muzarski force-pushed the enable-named-parameters-tests branch from 9fe9d95 to 1c117a8 Compare October 30, 2024 13:07

muzarski requested a review from wprzytula October 30, 2024 13:10

muzarski force-pushed the enable-named-parameters-tests branch from 1c117a8 to 0b7cf56 Compare October 30, 2024 18:05

wprzytula approved these changes Dec 1, 2024

View reviewed changes

wprzytula added the bug Something isn't working label Dec 1, 2024

muzarski force-pushed the enable-named-parameters-tests branch from 0b7cf56 to e6d4f31 Compare December 2, 2024 15:44

muzarski changed the title ~~ci: enable NamedParameterTests~~ binding: fix by-name parameter binding and enable corresponding IT Dec 2, 2024

muzarski force-pushed the enable-named-parameters-tests branch 2 times, most recently from a7e6028 to f36a5f3 Compare December 4, 2024 00:55

muzarski requested a review from Lorak-mmk December 4, 2024 10:55

muzarski force-pushed the enable-named-parameters-tests branch from f36a5f3 to 95015a1 Compare December 5, 2024 02:42

Lorak-mmk reviewed Dec 6, 2024

View reviewed changes

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

muzarski and others added 4 commits December 9, 2024 13:33

statement: reset name-to-index mapping for simple queries

a658ba0

`cass_statement_reset_paramters` should reset the name-to-index mapping used in simple queries.

statement: cleanup bind_cql_value_by_name method

92b4af5

I reduced mutability scope of some variables, renamed some variables so they have more descriptive names, and added some comments.

muzarski force-pushed the enable-named-parameters-tests branch from 95015a1 to c6a1e6b Compare December 9, 2024 12:33

muzarski added this to the 0.4 milestone Dec 9, 2024

muzarski force-pushed the enable-named-parameters-tests branch from c6a1e6b to 658f1cf Compare December 10, 2024 19:53

muzarski added 7 commits December 10, 2024 20:54

query: implement SerializeRow for simple query's bound values

4c8b1b0

errors: map UnknownNamedParameterError to CassError

922e24a

In this commit, we also adjust one of the NamedParameterTests to this change. We are going to enable this test suite in the next commit.

ci: enable NamedParameterTests suite

c1d37c7

IT: define tests for by-name binding with reused bind marker

90b157d

In these tests, we use the same bind marker (:abc) to bind values to 3 different columns. We then validate the results. There are two test cases - one for prepared statements, and other one for unprepared statements.

IT: add test cases for interleaving by name and by index bindings

658ebb0

muzarski force-pushed the enable-named-parameters-tests branch from 658f1cf to 658ebb0 Compare December 10, 2024 19:54

muzarski requested review from Lorak-mmk and wprzytula December 10, 2024 19:58

Lorak-mmk approved these changes Dec 11, 2024

View reviewed changes

wprzytula approved these changes Dec 11, 2024

View reviewed changes

muzarski merged commit cb0077b into scylladb:master Dec 11, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

binding: fix by-name parameter binding and enable corresponding IT #187

binding: fix by-name parameter binding and enable corresponding IT #187

muzarski commented Oct 9, 2024 •

edited

Loading

Lorak-mmk commented Oct 9, 2024

muzarski commented Oct 9, 2024

muzarski commented Oct 9, 2024

muzarski commented Oct 30, 2024

muzarski commented Oct 30, 2024

muzarski commented Oct 30, 2024

wprzytula commented Dec 1, 2024

muzarski commented Dec 4, 2024

muzarski commented Dec 5, 2024

muzarski commented Dec 9, 2024

muzarski commented Dec 10, 2024

wprzytula left a comment •

edited

Loading

muzarski commented Dec 11, 2024 •

edited

Loading

binding: fix by-name parameter binding and enable corresponding IT #187

binding: fix by-name parameter binding and enable corresponding IT #187

Conversation

muzarski commented Oct 9, 2024 • edited Loading

Motivation

Changes

Pre-review checklist

Lorak-mmk commented Oct 9, 2024

muzarski commented Oct 9, 2024

muzarski commented Oct 9, 2024

muzarski commented Oct 30, 2024

muzarski commented Oct 30, 2024

muzarski commented Oct 30, 2024

wprzytula commented Dec 1, 2024

muzarski commented Dec 4, 2024

muzarski commented Dec 5, 2024

muzarski commented Dec 9, 2024

muzarski commented Dec 10, 2024

wprzytula left a comment • edited Loading

Choose a reason for hiding this comment

muzarski commented Dec 11, 2024 • edited Loading

muzarski commented Oct 9, 2024 •

edited

Loading

wprzytula left a comment •

edited

Loading

muzarski commented Dec 11, 2024 •

edited

Loading