[scylladb-dev] [PATCH v4 0/3] cql3: cache function calls evaluation for non-deterministic functions

2 views
Skip to first unread message

Pavel Solodovnikov

unread,
Jul 21, 2021, 5:42:40 AMJul 21
to scylla...@googlegroups.com, Pavel Solodovnikov
`function_call` AST nodes are created for each function
with side effects in a CQL query, i.e. non-deterministic
functions (`uuid()`, `now()` and some others timeuuid-related).

These nodes are evaluated either when a query itself is executed
or query restrictions are computed (e.g. partition/clustering
key ranges for LWT requests).

We need to cache the calls since otherwise when handling a
`bounce_to_shard` request for an LWT query, we can possibly
enter an infinite bouncing loop (in case a function is used
to calculate partition key ranges for a query), since the
results can be different each time.

Furthermore, we don't support bouncing more than one time.
Returning `bounce_to_shard` message more than one time
will result in a crash.

Caching works only for LWT statements, for that the CQL parser
is modified so the `function_call::bind_and_get` function
is aware whether a function call is a part of LWT statement
or not.

Function calls are indexed by the order in which they appear
within a statement while parsing. There is no need to
include any kind of statement identifier to the cache key
since `query_options` (which holds the cache) is limited
to a single statement, anyway.

Note that `function_call::raw` AST nodes are not created
for selection clauses of a SELECT statement hence they
can only accept only one of the following things as parameters:
* Other function calls.
* Literal values.
* Parameter markers.

In other words, only parameters that can be immediately reduced
to a byte buffer are allowed and we don't need to handle
database inputs to non-pure functions separately since they
are not possible in this context. Anyhow, we don't even have
a single non-pure function that accepts arguments, so precautions
are not needed at the moment.

Add a test written in `cql-pytest` framework to verify
that both prepared and unprepared lwt statements handle
`bounce_to_shard` messages correctly in such scenario.

Fixes: #8604

Tests: unit(dev, debug)

NOTE: the patchset uses `query_options` as a container for
cached values. This doesn't look clean and `service::query_state`
seems to be a better place to store them. But it's not
forwarded to most of the CQL code and would mean that a huge number
of places would have to be amended.
The series presents a trade-off to avoid forwarding `query_state`
everywhere (but maybe it's the thing that needs to be done, nonetheless).

Git URL: https://github.com/ManManson/scylla/commits/lwt_bounce_to_shard_cached_fn_v4

Changelog:

v4:
* Use typedef `computed_function_values` instead of `std::unordered_map`
to hold cached data in `query_options`.
* Throw exception if there is more than 256 function calls used inside
a statement.
* Assign id's and set LWT context for `function_call` nodes at
"prepare" step. Remove this code from CQL parser.
* Add "lwt" work to the test, fix comments per review comments.
* Rename `cql3::variable_specifications` to
`cql3::raw_prepare_metadata`. Now it is also used to store
pointers to `function_call` nodes. Visitor functions that
populate the structure (`collect_marker_specification`) are
also renamed to `collect_prepare_metadata`.
These references are used a bit later to set LWT context
for `function_call` nodes.
* Rename `cached_function_calls` to `take_cached_function_calls`
in `result_message::bounce_to_shard` since it is what it does.
* Remove "test: lib: copy `query_options` in
`single_node_cql_env::execute_cql()`" patch from the patch set
(already merged).

v3:
* Reduce the number of iterations in the test from 100 to 10.
* Slightly refactor the comments in the test. No need
to mention #8604 explicitly.

v2:
* Fn calls are now indexed by the position of a call within a
parsed CQL statement. The CQL parser is modified to set the IDs
properly.
* Detect LWT statements in CQL parser and forward this info
to the `function_call::bind_and_get()` to populate the cache only
for LWT statements.
* Fixed access functions in `query_options` to be able to move
the cache info out of the instance.
* Do not copy cache from individual sub-statements for batch statement,
instead move them, as well.
* Explicitly check for non-pure functions in
`function_call::bind_and_get()` since `function_call` nodes can
represent not only non-pure functions.
* Fix some missing std::move's when constucting a `bounce_to_shard`
message.
* Add some more comments.
* Rename `test_cql_non_deterministic_functions_lwt.py` to
just `test_non_deterministic_functions.py`.

Pavel Solodovnikov (3):
cql3: rename `variable_specifications` to `raw_prepare_metadata`
cql3: cache function calls evaluation for non-deterministic functions
cql-pytest: add a test for non-pure CQL functions

CMakeLists.txt | 2 +-
configure.py | 2 +-
cql3/abstract_marker.cc | 6 +-
cql3/abstract_marker.hh | 4 +-
cql3/attributes.cc | 8 +--
cql3/attributes.hh | 4 +-
cql3/column_condition.cc | 8 +--
cql3/column_condition.hh | 2 +-
cql3/functions/function_call.hh | 17 ++++-
cql3/functions/functions.cc | 37 ++++++++++-
cql3/lists.cc | 8 +--
cql3/lists.hh | 4 +-
cql3/maps.cc | 8 +--
cql3/maps.hh | 4 +-
cql3/multi_column_relation.hh | 22 +++----
cql3/operation.hh | 6 +-
cql3/query_options.cc | 16 +++++
cql3/query_options.hh | 29 +++++++++
...cifications.cc => raw_prepare_metadata.cc} | 28 ++++----
...cifications.hh => raw_prepare_metadata.hh} | 37 +++++++----
cql3/relation.hh | 42 ++++++------
cql3/restrictions/statement_restrictions.cc | 4 +-
cql3/restrictions/statement_restrictions.hh | 4 +-
cql3/sets.cc | 2 +-
cql3/sets.hh | 2 +-
cql3/single_column_relation.cc | 22 +++----
cql3/single_column_relation.hh | 16 ++---
cql3/statements/batch_statement.cc | 18 ++++--
cql3/statements/create_view_statement.cc | 2 +-
cql3/statements/delete_statement.cc | 10 +--
cql3/statements/modification_statement.cc | 46 +++++++++----
cql3/statements/modification_statement.hh | 2 +-
cql3/statements/prepared_statement.hh | 6 +-
cql3/statements/raw/delete_statement.hh | 2 +-
cql3/statements/raw/insert_statement.hh | 4 +-
cql3/statements/raw/modification_statement.hh | 6 +-
cql3/statements/raw/parsed_statement.cc | 18 +++---
cql3/statements/raw/parsed_statement.hh | 8 +--
cql3/statements/raw/select_statement.hh | 4 +-
cql3/statements/raw/update_statement.hh | 2 +-
cql3/statements/select_statement.cc | 35 +++++-----
cql3/statements/update_statement.cc | 28 ++++----
cql3/term.hh | 6 +-
cql3/token_relation.cc | 18 +++---
cql3/token_relation.hh | 12 ++--
cql3/tuples.hh | 4 +-
cql3/user_types.cc | 4 +-
cql3/user_types.hh | 2 +-
test/boost/statement_restrictions_test.cc | 4 +-
.../test_non_deterministic_functions.py | 51 +++++++++++++++
transport/messages/result_message.hh | 9 ++-
transport/server.cc | 64 ++++++++++++-------
transport/server.hh | 2 +-
53 files changed, 460 insertions(+), 251 deletions(-)
rename cql3/{variable_specifications.cc => raw_prepare_metadata.cc} (72%)
rename cql3/{variable_specifications.hh => raw_prepare_metadata.hh} (63%)
create mode 100644 test/cql-pytest/test_non_deterministic_functions.py

--
2.31.1

Pavel Solodovnikov

unread,
Jul 21, 2021, 5:42:42 AMJul 21
to scylla...@googlegroups.com, Pavel Solodovnikov
The class is repurposed to be more generic and also be able
to hold additional metadata related to function calls within
a CQL statement. Rename all methods appropriately.

Visitor functions in AST nodes (`collect_marker_specification`)
are also renamed to a more generic `collect_prepare_metadata`.

The name `raw_prepare_metadata` designates that this metadata
structure is a byproduct of `stmt::raw::prepare()` call and
is needed only for "prepare" step of query execution.

Signed-off-by: Pavel Solodovnikov <pa.solo...@scylladb.com>
---
CMakeLists.txt | 2 +-
configure.py | 2 +-
cql3/abstract_marker.cc | 6 +--
cql3/abstract_marker.hh | 4 +-
cql3/attributes.cc | 8 ++--
cql3/attributes.hh | 4 +-
cql3/column_condition.cc | 8 ++--
cql3/column_condition.hh | 2 +-
cql3/functions/function_call.hh | 2 +-
cql3/functions/functions.cc | 4 +-
cql3/lists.cc | 8 ++--
cql3/lists.hh | 4 +-
cql3/maps.cc | 8 ++--
cql3/maps.hh | 4 +-
cql3/multi_column_relation.hh | 22 +++++-----
cql3/operation.hh | 6 +--
...cifications.cc => raw_prepare_metadata.cc} | 24 +++++------
...cifications.hh => raw_prepare_metadata.hh} | 26 ++++++------
cql3/relation.hh | 42 +++++++++----------
cql3/restrictions/statement_restrictions.cc | 4 +-
cql3/restrictions/statement_restrictions.hh | 4 +-
cql3/sets.cc | 2 +-
cql3/sets.hh | 2 +-
cql3/single_column_relation.cc | 22 +++++-----
cql3/single_column_relation.hh | 16 +++----
cql3/statements/batch_statement.cc | 12 +++---
cql3/statements/create_view_statement.cc | 2 +-
cql3/statements/delete_statement.cc | 10 ++---
cql3/statements/modification_statement.cc | 20 ++++-----
cql3/statements/modification_statement.hh | 2 +-
cql3/statements/prepared_statement.hh | 6 +--
cql3/statements/raw/delete_statement.hh | 2 +-
cql3/statements/raw/insert_statement.hh | 4 +-
cql3/statements/raw/modification_statement.hh | 6 +--
cql3/statements/raw/parsed_statement.cc | 18 ++++----
cql3/statements/raw/parsed_statement.hh | 8 ++--
cql3/statements/raw/select_statement.hh | 4 +-
cql3/statements/raw/update_statement.hh | 2 +-
cql3/statements/select_statement.cc | 30 ++++++-------
cql3/statements/update_statement.cc | 28 ++++++-------
cql3/term.hh | 6 +--
cql3/token_relation.cc | 18 ++++----
cql3/token_relation.hh | 12 +++---
cql3/tuples.hh | 4 +-
cql3/user_types.cc | 4 +-
cql3/user_types.hh | 2 +-
test/boost/statement_restrictions_test.cc | 4 +-
47 files changed, 220 insertions(+), 220 deletions(-)
rename cql3/{variable_specifications.cc => raw_prepare_metadata.cc} (74%)
rename cql3/{variable_specifications.hh => raw_prepare_metadata.hh} (75%)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2459eac7f0..036875a364 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -410,7 +410,7 @@ set(scylla_sources
cql3/ut_name.cc
cql3/util.cc
cql3/values.cc
- cql3/variable_specifications.cc
+ cql3/raw_prepare_metadata.cc
data/cell.cc
database.cc
db/batchlog_manager.cc
diff --git a/configure.py b/configure.py
index aa973a3e60..8bbf4b54d7 100755
--- a/configure.py
+++ b/configure.py
@@ -843,7 +843,7 @@ scylla_core = (['database.cc',
'cql3/selection/selector.cc',
'cql3/restrictions/statement_restrictions.cc',
'cql3/result_set.cc',
- 'cql3/variable_specifications.cc',
+ 'cql3/raw_prepare_metadata.cc',
'db/consistency_level.cc',
'db/system_keyspace.cc',
'db/virtual_table.cc',
diff --git a/cql3/abstract_marker.cc b/cql3/abstract_marker.cc
index fd20149049..bf98a9b0bd 100644
--- a/cql3/abstract_marker.cc
+++ b/cql3/abstract_marker.cc
@@ -46,7 +46,7 @@
#include "cql3/maps.hh"
#include "cql3/sets.hh"
#include "cql3/user_types.hh"
-#include "cql3/variable_specifications.hh"
+#include "cql3/raw_prepare_metadata.hh"
#include "types/list.hh"

namespace cql3 {
@@ -56,8 +56,8 @@ abstract_marker::abstract_marker(int32_t bind_index, lw_shared_ptr<column_specif
, _receiver{std::move(receiver)}
{ }

-void abstract_marker::collect_marker_specification(variable_specifications& bound_names) const {
- bound_names.add(_bind_index, _receiver);
+void abstract_marker::collect_prepare_metadata(raw_prepare_metadata& meta) const {
+ meta.add_variable_specification(_bind_index, _receiver);
}

bool abstract_marker::contains_bind_marker() const {
diff --git a/cql3/abstract_marker.hh b/cql3/abstract_marker.hh
index 6dc883de8d..8aa16cfc47 100644
--- a/cql3/abstract_marker.hh
+++ b/cql3/abstract_marker.hh
@@ -46,7 +46,7 @@
namespace cql3 {

class column_specification;
-class variable_specifications;
+class raw_prepare_metadata;

/**
* A single bind marker.
@@ -58,7 +58,7 @@ class abstract_marker : public non_terminal {
public:
abstract_marker(int32_t bind_index, lw_shared_ptr<column_specification>&& receiver);

- virtual void collect_marker_specification(variable_specifications& bound_names) const override;
+ virtual void collect_prepare_metadata(raw_prepare_metadata& meta) const override;

virtual bool contains_bind_marker() const override;

diff --git a/cql3/attributes.cc b/cql3/attributes.cc
index 13a7eef626..5a02ce84df 100644
--- a/cql3/attributes.cc
+++ b/cql3/attributes.cc
@@ -136,15 +136,15 @@ db::timeout_clock::duration attributes::get_timeout(const query_options& options
return std::chrono::duration_cast<db::timeout_clock::duration>(std::chrono::nanoseconds(duration.nanoseconds));
}

-void attributes::collect_marker_specification(variable_specifications& bound_names) const {
+void attributes::collect_prepare_metadata(raw_prepare_metadata& meta) const {
if (_timestamp) {
- _timestamp->collect_marker_specification(bound_names);
+ _timestamp->collect_prepare_metadata(meta);
}
if (_time_to_live) {
- _time_to_live->collect_marker_specification(bound_names);
+ _time_to_live->collect_prepare_metadata(meta);
}
if (_timeout) {
- _timeout->collect_marker_specification(bound_names);
+ _timeout->collect_prepare_metadata(meta);
}
}

diff --git a/cql3/attributes.hh b/cql3/attributes.hh
index 386fe4427d..03dc5bfcca 100644
--- a/cql3/attributes.hh
+++ b/cql3/attributes.hh
@@ -46,7 +46,7 @@
namespace cql3 {

class query_options;
-class variable_specifications;
+class raw_prepare_metadata;

/**
* Utility class for the Parser to gather attributes for modification
@@ -74,7 +74,7 @@ class attributes final {

db::timeout_clock::duration get_timeout(const query_options& options) const;

- void collect_marker_specification(variable_specifications& bound_names) const;
+ void collect_prepare_metadata(raw_prepare_metadata& meta) const;

class raw final {
public:
diff --git a/cql3/column_condition.cc b/cql3/column_condition.cc
index c9875daca1..9207a8baff 100644
--- a/cql3/column_condition.cc
+++ b/cql3/column_condition.cc
@@ -118,17 +118,17 @@ uint32_t read_and_check_list_index(const cql3::raw_value_view& key) {

namespace cql3 {

-void column_condition::collect_marker_specificaton(variable_specifications& bound_names) const {
+void column_condition::collect_marker_specificaton(raw_prepare_metadata& meta) const {
if (_collection_element) {
- _collection_element->collect_marker_specification(bound_names);
+ _collection_element->collect_prepare_metadata(meta);
}
if (!_in_values.empty()) {
for (auto&& value : _in_values) {
- value->collect_marker_specification(bound_names);
+ value->collect_prepare_metadata(meta);
}
}
if (_value) {
- _value->collect_marker_specification(bound_names);
+ _value->collect_prepare_metadata(meta);
}
}

diff --git a/cql3/column_condition.hh b/cql3/column_condition.hh
index 68082571b8..54ff3dcfac 100644
--- a/cql3/column_condition.hh
+++ b/cql3/column_condition.hh
@@ -89,7 +89,7 @@ class column_condition final {
* @param boundNames the list of column specification where to collect the
* bind variables of this term in.
*/
- void collect_marker_specificaton(variable_specifications& bound_names) const;
+ void collect_marker_specificaton(raw_prepare_metadata& meta) const;

// Retrieve parameter marker values, if any, find the appropriate collection
// element if the cell is a collection and an element access is used in the expression,
diff --git a/cql3/functions/function_call.hh b/cql3/functions/function_call.hh
index d4c4fec9df..7ca9db9efd 100644
--- a/cql3/functions/function_call.hh
+++ b/cql3/functions/function_call.hh
@@ -57,7 +57,7 @@ class function_call : public non_terminal {
function_call(shared_ptr<scalar_function> fun, std::vector<shared_ptr<term>> terms)
: _fun(std::move(fun)), _terms(std::move(terms)) {
}
- virtual void collect_marker_specification(variable_specifications& bound_names) const override;
+ virtual void collect_prepare_metadata(raw_prepare_metadata& meta) const override;
virtual shared_ptr<terminal> bind(const query_options& options) override;
virtual cql3::raw_value_view bind_and_get(const query_options& options) override;
private:
diff --git a/cql3/functions/functions.cc b/cql3/functions/functions.cc
index d855706912..596ef665c1 100644
--- a/cql3/functions/functions.cc
+++ b/cql3/functions/functions.cc
@@ -430,9 +430,9 @@ functions::type_equals(const std::vector<data_type>& t1, const std::vector<data_
}

void
-function_call::collect_marker_specification(variable_specifications& bound_names) const {
+function_call::collect_prepare_metadata(raw_prepare_metadata& meta) const {
for (auto&& t : _terms) {
- t->collect_marker_specification(bound_names);
+ t->collect_prepare_metadata(meta);
}
}

diff --git a/cql3/lists.cc b/cql3/lists.cc
index 6a69da6cb1..ff1d5b0f1d 100644
--- a/cql3/lists.cc
+++ b/cql3/lists.cc
@@ -207,7 +207,7 @@ lists::delayed_value::contains_bind_marker() const {
}

void
-lists::delayed_value::collect_marker_specification(variable_specifications& bound_names) const {
+lists::delayed_value::collect_prepare_metadata(raw_prepare_metadata& meta) const {
}

shared_ptr<terminal>
@@ -275,9 +275,9 @@ lists::setter_by_index::requires_read() const {
}

void
-lists::setter_by_index::collect_marker_specification(variable_specifications& bound_names) const {
- operation::collect_marker_specification(bound_names);
- _idx->collect_marker_specification(bound_names);
+lists::setter_by_index::collect_prepare_metadata(raw_prepare_metadata& meta) const {
+ operation::collect_prepare_metadata(meta);
+ _idx->collect_prepare_metadata(meta);
}

void
diff --git a/cql3/lists.hh b/cql3/lists.hh
index c58a94ea7f..8efc45cc58 100644
--- a/cql3/lists.hh
+++ b/cql3/lists.hh
@@ -104,7 +104,7 @@ class lists {
: _elements(std::move(elements)) {
}
virtual bool contains_bind_marker() const override;
- virtual void collect_marker_specification(variable_specifications& bound_names) const override;
+ virtual void collect_prepare_metadata(raw_prepare_metadata& meta) const override;
virtual shared_ptr<terminal> bind(const query_options& options) override;
const std::vector<shared_ptr<term>>& get_elements() const {
return _elements;
@@ -140,7 +140,7 @@ class lists {
: operation(column, std::move(t)), _idx(std::move(idx)) {
}
virtual bool requires_read() const override;
- virtual void collect_marker_specification(variable_specifications& bound_names) const;
+ virtual void collect_prepare_metadata(raw_prepare_metadata& meta) const;
virtual void execute(mutation& m, const clustering_key_prefix& prefix, const update_parameters& params) override;
};

diff --git a/cql3/maps.cc b/cql3/maps.cc
index ef28986a85..791977a8e8 100644
--- a/cql3/maps.cc
+++ b/cql3/maps.cc
@@ -228,7 +228,7 @@ maps::delayed_value::contains_bind_marker() const {
}

void
-maps::delayed_value::collect_marker_specification(variable_specifications& bound_names) const {
+maps::delayed_value::collect_prepare_metadata(raw_prepare_metadata& meta) const {
}

shared_ptr<terminal>
@@ -306,9 +306,9 @@ maps::setter::execute(mutation& m, const clustering_key_prefix& row_key, const u
}

void
-maps::setter_by_key::collect_marker_specification(variable_specifications& bound_names) const {
- operation::collect_marker_specification(bound_names);
- _k->collect_marker_specification(bound_names);
+maps::setter_by_key::collect_prepare_metadata(raw_prepare_metadata& meta) const {
+ operation::collect_prepare_metadata(meta);
+ _k->collect_prepare_metadata(meta);
}

void
diff --git a/cql3/maps.hh b/cql3/maps.hh
index 57b9a4ad91..d8ac86501c 100644
--- a/cql3/maps.hh
+++ b/cql3/maps.hh
@@ -98,7 +98,7 @@ class maps {
: _comparator(std::move(comparator)), _elements(std::move(elements)) {
}
virtual bool contains_bind_marker() const override;
- virtual void collect_marker_specification(variable_specifications& bound_names) const override;
+ virtual void collect_prepare_metadata(raw_prepare_metadata& meta) const override;
shared_ptr<terminal> bind(const query_options& options);
};

@@ -126,7 +126,7 @@ class maps {
setter_by_key(const column_definition& column, shared_ptr<term> k, shared_ptr<term> t)
: operation(column, std::move(t)), _k(std::move(k)) {
}
- virtual void collect_marker_specification(variable_specifications& bound_names) const override;
+ virtual void collect_prepare_metadata(raw_prepare_metadata& meta) const override;
virtual void execute(mutation& m, const clustering_key_prefix& prefix, const update_parameters& params) override;
};

diff --git a/cql3/multi_column_relation.hh b/cql3/multi_column_relation.hh
index 1df5be96a4..8786b02c70 100644
--- a/cql3/multi_column_relation.hh
+++ b/cql3/multi_column_relation.hh
@@ -160,31 +160,31 @@ class multi_column_relation final : public relation {

protected:
virtual shared_ptr<restrictions::restriction> new_EQ_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names) override {
+ raw_prepare_metadata& meta) override {
auto rs = receivers(db, *schema);
std::vector<lw_shared_ptr<column_specification>> col_specs(rs.size());
std::transform(rs.begin(), rs.end(), col_specs.begin(), [] (auto cs) {
return cs->column_specification;
});
- auto t = to_term(col_specs, *get_value(), db, schema->ks_name(), bound_names);
+ auto t = to_term(col_specs, *get_value(), db, schema->ks_name(), meta);
return ::make_shared<restrictions::multi_column_restriction::EQ>(schema, rs, t);
}

virtual shared_ptr<restrictions::restriction> new_IN_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names) override {
+ raw_prepare_metadata& meta) override {
auto rs = receivers(db, *schema);
std::vector<lw_shared_ptr<column_specification>> col_specs(rs.size());
std::transform(rs.begin(), rs.end(), col_specs.begin(), [] (auto cs) {
return cs->column_specification;
});
if (_in_marker) {
- auto t = to_term(col_specs, *get_value(), db, schema->ks_name(), bound_names);
+ auto t = to_term(col_specs, *get_value(), db, schema->ks_name(), meta);
auto as_abstract_marker = static_pointer_cast<abstract_marker>(t);
return ::make_shared<restrictions::multi_column_restriction::IN_with_marker>(schema, rs, as_abstract_marker);
} else {
std::vector<::shared_ptr<term::raw>> raws(_in_values.size());
std::copy(_in_values.begin(), _in_values.end(), raws.begin());
- auto ts = to_terms(col_specs, raws, db, schema->ks_name(), bound_names);
+ auto ts = to_terms(col_specs, raws, db, schema->ks_name(), meta);
// Convert a single-item IN restriction to an EQ restriction
if (ts.size() == 1) {
return ::make_shared<restrictions::multi_column_restriction::EQ>(schema, rs, std::move(ts[0]));
@@ -194,24 +194,24 @@ class multi_column_relation final : public relation {
}

virtual shared_ptr<restrictions::restriction> new_slice_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names,
+ raw_prepare_metadata& meta,
statements::bound bound, bool inclusive) override {
auto rs = receivers(db, *schema);
std::vector<lw_shared_ptr<column_specification>> col_specs(rs.size());
std::transform(rs.begin(), rs.end(), col_specs.begin(), [] (auto cs) {
return cs->column_specification;
});
- auto t = to_term(col_specs, *get_value(), db, schema->ks_name(), bound_names);
+ auto t = to_term(col_specs, *get_value(), db, schema->ks_name(), meta);
return ::make_shared<restrictions::multi_column_restriction::slice>(schema, rs, bound, inclusive, t, _mode);
}

virtual shared_ptr<restrictions::restriction> new_contains_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names, bool is_key) override {
+ raw_prepare_metadata& meta, bool is_key) override {
throw exceptions::invalid_request_exception(format("{} cannot be used for Multi-column relations", get_operator()));
}

virtual ::shared_ptr<restrictions::restriction> new_LIKE_restriction(
- database& db, schema_ptr schema, variable_specifications& bound_names) override {
+ database& db, schema_ptr schema, raw_prepare_metadata& meta) override {
throw exceptions::invalid_request_exception("LIKE cannot be used for Multi-column relations");
}

@@ -224,10 +224,10 @@ class multi_column_relation final : public relation {

virtual shared_ptr<term> to_term(const std::vector<lw_shared_ptr<column_specification>>& receivers,
const term::raw& raw, database& db, const sstring& keyspace,
- variable_specifications& bound_names) const override {
+ raw_prepare_metadata& meta) const override {
const auto& as_multi_column_raw = dynamic_cast<const term::multi_column_raw&>(raw);
auto t = as_multi_column_raw.prepare(db, keyspace, receivers);
- t->collect_marker_specification(bound_names);
+ t->collect_prepare_metadata(meta);
return t;
}

diff --git a/cql3/operation.hh b/cql3/operation.hh
index 936b513dd7..8b1532b80a 100644
--- a/cql3/operation.hh
+++ b/cql3/operation.hh
@@ -103,12 +103,12 @@ class operation {
/**
* Collects the column specification for the bind variables of this operation.
*
- * @param bound_names the list of column specification where to collect the
+ * @param meta the list of column specification where to collect the
* bind variables of this term in.
*/
- virtual void collect_marker_specification(variable_specifications& bound_names) const {
+ virtual void collect_prepare_metadata(raw_prepare_metadata& meta) const {
if (_t) {
- _t->collect_marker_specification(bound_names);
+ _t->collect_prepare_metadata(meta);
}
}

diff --git a/cql3/variable_specifications.cc b/cql3/raw_prepare_metadata.cc
similarity index 74%
rename from cql3/variable_specifications.cc
rename to cql3/raw_prepare_metadata.cc
index b8e21d1225..d3fe69b34b 100644
--- a/cql3/variable_specifications.cc
+++ b/cql3/raw_prepare_metadata.cc
@@ -39,33 +39,33 @@
* along with Scylla. If not, see <http://www.gnu.org/licenses/>.
*/

-#include "cql3/variable_specifications.hh"
+#include "cql3/raw_prepare_metadata.hh"
#include "cql3/column_identifier.hh"
#include "cql3/column_specification.hh"

namespace cql3 {

-variable_specifications::variable_specifications(const std::vector<::shared_ptr<column_identifier>>& variable_names)
+raw_prepare_metadata::raw_prepare_metadata(const std::vector<::shared_ptr<column_identifier>>& variable_names)
: _variable_names{variable_names}
, _specs{variable_names.size()}
, _target_columns{variable_names.size()}
{ }
-lw_shared_ptr<variable_specifications> variable_specifications::empty() {
- return make_lw_shared<variable_specifications>(std::vector<::shared_ptr<column_identifier>>{});
+lw_shared_ptr<raw_prepare_metadata> raw_prepare_metadata::empty() {
+ return make_lw_shared<raw_prepare_metadata>(std::vector<::shared_ptr<column_identifier>>{});
}
-size_t variable_specifications::size() const {
+size_t raw_prepare_metadata::bound_variables_size() const {
return _variable_names.size();
}

-std::vector<lw_shared_ptr<column_specification>> variable_specifications::get_specifications() const & {
+std::vector<lw_shared_ptr<column_specification>> raw_prepare_metadata::get_variable_specifications() const & {
return std::vector<lw_shared_ptr<column_specification>>(_specs.begin(), _specs.end());
}

-std::vector<lw_shared_ptr<column_specification>> variable_specifications::get_specifications() && {
+std::vector<lw_shared_ptr<column_specification>> raw_prepare_metadata::get_variable_specifications() && {
return std::move(_specs);
}

-std::vector<uint16_t> variable_specifications::get_partition_key_bind_indexes(const schema& schema) const {
+std::vector<uint16_t> raw_prepare_metadata::get_partition_key_bind_indexes(const schema& schema) const {
auto count = schema.partition_key_columns().size();
std::vector<uint16_t> partition_key_positions(count, uint16_t(0));
std::vector<bool> set(count, false);
@@ -85,7 +85,7 @@ std::vector<uint16_t> variable_specifications::get_partition_key_bind_indexes(co
return partition_key_positions;
}

-void variable_specifications::add(int32_t bind_index, lw_shared_ptr<column_specification> spec) {
+void raw_prepare_metadata::add_variable_specification(int32_t bind_index, lw_shared_ptr<column_specification> spec) {
_target_columns[bind_index] = spec;
auto name = _variable_names[bind_index];
// Use the user name, if there is one
@@ -95,12 +95,12 @@ void variable_specifications::add(int32_t bind_index, lw_shared_ptr<column_speci
_specs[bind_index] = spec;
}

-void variable_specifications::set_bound_variables(const std::vector<shared_ptr<column_identifier>>& bound_names) {
- _variable_names = bound_names;
+void raw_prepare_metadata::set_bound_variables(const std::vector<shared_ptr<column_identifier>>& prepare_meta) {
+ _variable_names = prepare_meta;
_specs.clear();
_target_columns.clear();

- const size_t bn_size = bound_names.size();
+ const size_t bn_size = prepare_meta.size();
_specs.resize(bn_size);
_target_columns.resize(bn_size);
}
diff --git a/cql3/variable_specifications.hh b/cql3/raw_prepare_metadata.hh
similarity index 75%
rename from cql3/variable_specifications.hh
rename to cql3/raw_prepare_metadata.hh
index e261851e1d..0a9cd0b520 100644
--- a/cql3/variable_specifications.hh
+++ b/cql3/raw_prepare_metadata.hh
@@ -55,7 +55,11 @@ namespace cql3 {
class column_identifier;
class column_specification;

-class variable_specifications final {
+/**
+ * A metadata class currently holding bind variables specifications and
+ * populated at "prepare" step of query execution.
+ */
+class raw_prepare_metadata final {
private:
std::vector<shared_ptr<column_identifier>> _variable_names;
std::vector<lw_shared_ptr<column_specification>> _specs;
@@ -63,26 +67,22 @@ class variable_specifications final {

public:

- variable_specifications() = default;
- variable_specifications(const std::vector<::shared_ptr<column_identifier>>& variable_names);
+ raw_prepare_metadata() = default;
+ raw_prepare_metadata(const std::vector<::shared_ptr<column_identifier>>& variable_names);

- /**
- * Returns an empty instance of <code>VariableSpecifications</code>.
- * @return an empty instance of <code>VariableSpecifications</code>
- */
- static lw_shared_ptr<variable_specifications> empty();
+ static lw_shared_ptr<raw_prepare_metadata> empty();

- size_t size() const;
+ size_t bound_variables_size() const;

- std::vector<lw_shared_ptr<column_specification>> get_specifications() const &;
+ std::vector<lw_shared_ptr<column_specification>> get_variable_specifications() const &;

- std::vector<lw_shared_ptr<column_specification>> get_specifications() &&;
+ std::vector<lw_shared_ptr<column_specification>> get_variable_specifications() &&;

std::vector<uint16_t> get_partition_key_bind_indexes(const schema& schema) const;

- void add(int32_t bind_index, lw_shared_ptr<column_specification> spec);
+ void add_variable_specification(int32_t bind_index, lw_shared_ptr<column_specification> spec);

- void set_bound_variables(const std::vector<shared_ptr<column_identifier>>& bound_names);
+ void set_bound_variables(const std::vector<shared_ptr<column_identifier>>& prepare_meta);
};

}
diff --git a/cql3/relation.hh b/cql3/relation.hh
index 0734898aa9..7023697cc4 100644
--- a/cql3/relation.hh
+++ b/cql3/relation.hh
@@ -43,7 +43,7 @@

#include "schema_fwd.hh"
#include "column_identifier.hh"
-#include "variable_specifications.hh"
+#include "raw_prepare_metadata.hh"
#include "restrictions/restriction.hh"
#include "statements/bound.hh"
#include "term.hh"
@@ -139,29 +139,29 @@ class relation : public enable_shared_from_this<relation> {
* @return the <code>Restriction</code> corresponding to this <code>Relation</code>
* @throws InvalidRequestException if this <code>Relation</code> is not valid
*/
- virtual ::shared_ptr<restrictions::restriction> to_restriction(database& db, schema_ptr schema, variable_specifications& bound_names) final {
+ virtual ::shared_ptr<restrictions::restriction> to_restriction(database& db, schema_ptr schema, raw_prepare_metadata& meta) final {
if (_relation_type == expr::oper_t::EQ) {
- return new_EQ_restriction(db, schema, bound_names);
+ return new_EQ_restriction(db, schema, meta);
} else if (_relation_type == expr::oper_t::LT) {
- return new_slice_restriction(db, schema, bound_names, statements::bound::END, false);
+ return new_slice_restriction(db, schema, meta, statements::bound::END, false);
} else if (_relation_type == expr::oper_t::LTE) {
- return new_slice_restriction(db, schema, bound_names, statements::bound::END, true);
+ return new_slice_restriction(db, schema, meta, statements::bound::END, true);
} else if (_relation_type == expr::oper_t::GTE) {
- return new_slice_restriction(db, schema, bound_names, statements::bound::START, true);
+ return new_slice_restriction(db, schema, meta, statements::bound::START, true);
} else if (_relation_type == expr::oper_t::GT) {
- return new_slice_restriction(db, schema, bound_names, statements::bound::START, false);
+ return new_slice_restriction(db, schema, meta, statements::bound::START, false);
} else if (_relation_type == expr::oper_t::IN) {
- return new_IN_restriction(db, schema, bound_names);
+ return new_IN_restriction(db, schema, meta);
} else if (_relation_type == expr::oper_t::CONTAINS) {
- return new_contains_restriction(db, schema, bound_names, false);
+ return new_contains_restriction(db, schema, meta, false);
} else if (_relation_type == expr::oper_t::CONTAINS_KEY) {
- return new_contains_restriction(db, schema, bound_names, true);
+ return new_contains_restriction(db, schema, meta, true);
} else if (_relation_type == expr::oper_t::IS_NOT) {
// This case is not supposed to happen: statement_restrictions
// constructor does not call this function for views' IS_NOT.
throw exceptions::invalid_request_exception(format("Unsupported \"IS NOT\" relation: {}", to_string()));
} else if (_relation_type == expr::oper_t::LIKE) {
- return new_LIKE_restriction(db, schema, bound_names);
+ return new_LIKE_restriction(db, schema, meta);
} else {
throw exceptions::invalid_request_exception(format("Unsupported \"!=\" relation: {}", to_string()));
}
@@ -182,31 +182,31 @@ class relation : public enable_shared_from_this<relation> {
* @throws InvalidRequestException if the relation cannot be converted into an EQ restriction.
*/
virtual ::shared_ptr<restrictions::restriction> new_EQ_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names) = 0;
+ raw_prepare_metadata& meta) = 0;

/**
* Creates a new IN restriction instance.
*
* @param cfm the Column Family meta data
- * @param bound_names the variables specification where to collect the bind variables
+ * @param meta the variables specification where to collect the bind variables
* @return a new IN restriction instance
* @throws InvalidRequestException if the relation cannot be converted into an IN restriction.
*/
virtual ::shared_ptr<restrictions::restriction> new_IN_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names) = 0;
+ raw_prepare_metadata& meta) = 0;

/**
* Creates a new Slice restriction instance.
*
* @param cfm the Column Family meta data
- * @param bound_names the variables specification where to collect the bind variables
+ * @param meta the variables specification where to collect the bind variables
* @param bound the slice bound
* @param inclusive <code>true</code> if the bound is included.
* @return a new slice restriction instance
* @throws InvalidRequestException if the <code>Relation</code> is not valid
*/
virtual ::shared_ptr<restrictions::restriction> new_slice_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names,
+ raw_prepare_metadata& meta,
statements::bound bound,
bool inclusive) = 0;

@@ -214,19 +214,19 @@ class relation : public enable_shared_from_this<relation> {
* Creates a new Contains restriction instance.
*
* @param cfm the Column Family meta data
- * @param bound_names the variables specification where to collect the bind variables
+ * @param meta the variables specification where to collect the bind variables
* @param isKey <code>true</code> if the restriction to create is a CONTAINS KEY
* @return a new Contains <code>::shared_ptr<restrictions::restriction></code> instance
* @throws InvalidRequestException if the <code>Relation</code> is not valid
*/
virtual ::shared_ptr<restrictions::restriction> new_contains_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names, bool isKey) = 0;
+ raw_prepare_metadata& meta, bool isKey) = 0;

/**
* Creates a new LIKE restriction instance.
*/
virtual ::shared_ptr<restrictions::restriction> new_LIKE_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names) = 0;
+ raw_prepare_metadata& meta) = 0;

/**
* Renames an identifier in this Relation, if applicable.
@@ -253,7 +253,7 @@ class relation : public enable_shared_from_this<relation> {
const term::raw& raw,
database& db,
const sstring& keyspace,
- variable_specifications& boundNames) const = 0;
+ raw_prepare_metadata& boundNames) const = 0;

/**
* Converts the specified <code>Raw</code> terms into a <code>Term</code>s.
@@ -269,7 +269,7 @@ class relation : public enable_shared_from_this<relation> {
const std::vector<::shared_ptr<term::raw>>& raws,
database& db,
const sstring& keyspace,
- variable_specifications& boundNames) const {
+ raw_prepare_metadata& boundNames) const {
std::vector<::shared_ptr<term>> terms;
for (const auto& r : raws) {
terms.emplace_back(to_term(receivers, *r, db, keyspace, boundNames));
diff --git a/cql3/restrictions/statement_restrictions.cc b/cql3/restrictions/statement_restrictions.cc
index b7651e5b43..1d85f2a51c 100644
--- a/cql3/restrictions/statement_restrictions.cc
+++ b/cql3/restrictions/statement_restrictions.cc
@@ -305,7 +305,7 @@ statement_restrictions::statement_restrictions(database& db,
schema_ptr schema,
statements::statement_type type,
const std::vector<::shared_ptr<relation>>& where_clause,
- variable_specifications& bound_names,
+ raw_prepare_metadata& meta,
bool selects_only_static_columns,
bool for_view,
bool allow_filtering)
@@ -336,7 +336,7 @@ statement_restrictions::statement_restrictions(database& db,
throw exceptions::invalid_request_exception(format("restriction '{}' is only supported in materialized view creation", relation->to_string()));
}
} else {
- const auto restriction = relation->to_restriction(db, schema, bound_names);
+ const auto restriction = relation->to_restriction(db, schema, meta);
if (dynamic_pointer_cast<multi_column_restriction>(restriction)) {
_clustering_columns_restrictions = _clustering_columns_restrictions->merge_to(_schema, restriction);
} else if (has_token(restriction->expression)) {
diff --git a/cql3/restrictions/statement_restrictions.hh b/cql3/restrictions/statement_restrictions.hh
index b17fcfe8dd..ce429314ff 100644
--- a/cql3/restrictions/statement_restrictions.hh
+++ b/cql3/restrictions/statement_restrictions.hh
@@ -49,7 +49,7 @@
#include "cql3/restrictions/primary_key_restrictions.hh"
#include "cql3/restrictions/single_column_restrictions.hh"
#include "cql3/relation.hh"
-#include "cql3/variable_specifications.hh"
+#include "cql3/raw_prepare_metadata.hh"
#include "cql3/statements/statement_type.hh"

namespace cql3 {
@@ -151,7 +151,7 @@ class statement_restrictions {
schema_ptr schema,
statements::statement_type type,
const std::vector<::shared_ptr<relation>>& where_clause,
- variable_specifications& bound_names,
+ raw_prepare_metadata& meta,
bool selects_only_static_columns,
bool for_view = false,
bool allow_filtering = false);
diff --git a/cql3/sets.cc b/cql3/sets.cc
index 883290a46b..75c44dff18 100644
--- a/cql3/sets.cc
+++ b/cql3/sets.cc
@@ -201,7 +201,7 @@ sets::delayed_value::contains_bind_marker() const {
}

void
-sets::delayed_value::collect_marker_specification(variable_specifications& bound_names) const {
+sets::delayed_value::collect_prepare_metadata(raw_prepare_metadata& meta) const {
}

shared_ptr<terminal>
diff --git a/cql3/sets.hh b/cql3/sets.hh
index 21ed2883e3..664eaf73b3 100644
--- a/cql3/sets.hh
+++ b/cql3/sets.hh
@@ -94,7 +94,7 @@ class sets {
: _comparator(std::move(comparator)), _elements(std::move(elements)) {
}
virtual bool contains_bind_marker() const override;
- virtual void collect_marker_specification(variable_specifications& bound_names) const override;
+ virtual void collect_prepare_metadata(raw_prepare_metadata& meta) const override;
virtual shared_ptr<terminal> bind(const query_options& options);
};

diff --git a/cql3/single_column_relation.cc b/cql3/single_column_relation.cc
index fa004a2984..2d5219e0a0 100644
--- a/cql3/single_column_relation.cc
+++ b/cql3/single_column_relation.cc
@@ -57,26 +57,26 @@ single_column_relation::to_term(const std::vector<lw_shared_ptr<column_specifica
const term::raw& raw,
database& db,
const sstring& keyspace,
- variable_specifications& bound_names) const {
+ raw_prepare_metadata& meta) const {
// TODO: optimize vector away, accept single column_specification
assert(receivers.size() == 1);
auto term = raw.prepare(db, keyspace, receivers[0]);
- term->collect_marker_specification(bound_names);
+ term->collect_prepare_metadata(meta);
return term;
}

::shared_ptr<restrictions::restriction>
-single_column_relation::new_EQ_restriction(database& db, schema_ptr schema, variable_specifications& bound_names) {
+single_column_relation::new_EQ_restriction(database& db, schema_ptr schema, raw_prepare_metadata& meta) {
const column_definition& column_def = to_column_definition(*schema, *_entity);
if (!_map_key) {
auto r = ::make_shared<restrictions::single_column_restriction>(column_def);
- auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), meta);
r->expression = binary_operator{&column_def, expr::oper_t::EQ, std::move(term)};
return r;
}
auto&& receivers = to_receivers(*schema, column_def);
- auto&& entry_key = to_term({receivers[0]}, *_map_key, db, schema->ks_name(), bound_names);
- auto&& entry_value = to_term({receivers[1]}, *_value, db, schema->ks_name(), bound_names);
+ auto&& entry_key = to_term({receivers[0]}, *_map_key, db, schema->ks_name(), meta);
+ auto&& entry_value = to_term({receivers[1]}, *_value, db, schema->ks_name(), meta);
auto r = make_shared<restrictions::single_column_restriction>(column_def);
r->expression = binary_operator{
column_value(&column_def, std::move(entry_key)), oper_t::EQ, std::move(entry_value)};
@@ -84,18 +84,18 @@ single_column_relation::new_EQ_restriction(database& db, schema_ptr schema, vari
}

::shared_ptr<restrictions::restriction>
-single_column_relation::new_IN_restriction(database& db, schema_ptr schema, variable_specifications& bound_names) {
+single_column_relation::new_IN_restriction(database& db, schema_ptr schema, raw_prepare_metadata& meta) {
using namespace restrictions;
const column_definition& column_def = to_column_definition(*schema, *_entity);
auto receivers = to_receivers(*schema, column_def);
assert(_in_values.empty() || !_value);
if (_value) {
- auto term = to_term(receivers, *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(receivers, *_value, db, schema->ks_name(), meta);
auto r = ::make_shared<single_column_restriction>(column_def);
r->expression = binary_operator{&column_def, expr::oper_t::IN, std::move(term)};
return r;
}
- auto terms = to_terms(receivers, _in_values, db, schema->ks_name(), bound_names);
+ auto terms = to_terms(receivers, _in_values, db, schema->ks_name(), meta);
// Convert a single-item IN restriction to an EQ restriction
if (terms.size() == 1) {
auto r = ::make_shared<single_column_restriction>(column_def);
@@ -110,13 +110,13 @@ single_column_relation::new_IN_restriction(database& db, schema_ptr schema, vari

::shared_ptr<restrictions::restriction>
single_column_relation::new_LIKE_restriction(
- database& db, schema_ptr schema, variable_specifications& bound_names) {
+ database& db, schema_ptr schema, raw_prepare_metadata& meta) {
const column_definition& column_def = to_column_definition(*schema, *_entity);
if (!column_def.type->is_string()) {
throw exceptions::invalid_request_exception(
format("LIKE is allowed only on string types, which {} is not", column_def.name_as_text()));
}
- auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), meta);
auto r = ::make_shared<restrictions::single_column_restriction>(column_def);
r->expression = binary_operator{&column_def, expr::oper_t::LIKE, std::move(term)};
return r;
diff --git a/cql3/single_column_relation.hh b/cql3/single_column_relation.hh
index 3acf11d8c1..3779e679d0 100644
--- a/cql3/single_column_relation.hh
+++ b/cql3/single_column_relation.hh
@@ -117,7 +117,7 @@ class single_column_relation final : public relation {
protected:
virtual ::shared_ptr<term> to_term(const std::vector<lw_shared_ptr<column_specification>>& receivers,
const term::raw& raw, database& db, const sstring& keyspace,
- variable_specifications& bound_names) const override;
+ raw_prepare_metadata& meta) const override;

#if 0
public SingleColumnRelation withNonStrictOperator()
@@ -146,13 +146,13 @@ class single_column_relation final : public relation {

protected:
virtual ::shared_ptr<restrictions::restriction> new_EQ_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names);
+ raw_prepare_metadata& meta);

virtual ::shared_ptr<restrictions::restriction> new_IN_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names) override;
+ raw_prepare_metadata& meta) override;

virtual ::shared_ptr<restrictions::restriction> new_slice_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names,
+ raw_prepare_metadata& meta,
statements::bound bound,
bool inclusive) override {
auto&& column_def = to_column_definition(*schema, *_entity);
@@ -169,17 +169,17 @@ class single_column_relation final : public relation {
throw exceptions::invalid_request_exception("Slice restrictions are not supported on duration columns");
}

- auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), meta);
auto r = ::make_shared<restrictions::single_column_restriction>(column_def);
r->expression = expr::binary_operator{&column_def, _relation_type, std::move(term)};
return r;
}

virtual shared_ptr<restrictions::restriction> new_contains_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names,
+ raw_prepare_metadata& meta,
bool is_key) override {
auto&& column_def = to_column_definition(*schema, *_entity);
- auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), meta);
auto r = ::make_shared<restrictions::single_column_restriction>(column_def);
r->expression = expr::binary_operator{
&column_def, is_key ? expr::oper_t::CONTAINS_KEY : expr::oper_t::CONTAINS, std::move(term)};
@@ -187,7 +187,7 @@ class single_column_relation final : public relation {
}

virtual ::shared_ptr<restrictions::restriction> new_LIKE_restriction(
- database& db, schema_ptr schema, variable_specifications& bound_names) override;
+ database& db, schema_ptr schema, raw_prepare_metadata& meta) override;

virtual ::shared_ptr<relation> maybe_rename_identifier(const column_identifier::raw& from, column_identifier::raw to) override {
return *_entity == from
diff --git a/cql3/statements/batch_statement.cc b/cql3/statements/batch_statement.cc
index a19473b3f1..30706675e0 100644
--- a/cql3/statements/batch_statement.cc
+++ b/cql3/statements/batch_statement.cc
@@ -433,7 +433,7 @@ namespace raw {

std::unique_ptr<prepared_statement>
batch_statement::prepare(database& db, cql_stats& stats) {
- auto&& bound_names = get_bound_variables();
+ auto&& meta = get_prepare_metadata();

std::optional<sstring> first_ks;
std::optional<sstring> first_cf;
@@ -449,20 +449,20 @@ batch_statement::prepare(database& db, cql_stats& stats) {
} else {
have_multiple_cfs = first_ks.value() != parsed->keyspace() || first_cf.value() != parsed->column_family();
}
- statements.emplace_back(parsed->prepare(db, bound_names, stats));
+ statements.emplace_back(parsed->prepare(db, meta, stats));
}

auto&& prep_attrs = _attrs->prepare(db, "[batch]", "[batch]");
- prep_attrs->collect_marker_specification(bound_names);
+ prep_attrs->collect_prepare_metadata(meta);

- cql3::statements::batch_statement batch_statement_(bound_names.size(), _type, std::move(statements), std::move(prep_attrs), stats);
+ cql3::statements::batch_statement batch_statement_(meta.bound_variables_size(), _type, std::move(statements), std::move(prep_attrs), stats);

std::vector<uint16_t> partition_key_bind_indices;
if (!have_multiple_cfs && batch_statement_.get_statements().size() > 0) {
- partition_key_bind_indices = bound_names.get_partition_key_bind_indexes(*batch_statement_.get_statements()[0].statement->s);
+ partition_key_bind_indices = meta.get_partition_key_bind_indexes(*batch_statement_.get_statements()[0].statement->s);
}
return std::make_unique<prepared_statement>(make_shared<cql3::statements::batch_statement>(std::move(batch_statement_)),
- bound_names.get_specifications(),
+ meta.get_variable_specifications(),
std::move(partition_key_bind_indices));
}

diff --git a/cql3/statements/create_view_statement.cc b/cql3/statements/create_view_statement.cc
index 19d9b12c25..c7cda35611 100644
--- a/cql3/statements/create_view_statement.cc
+++ b/cql3/statements/create_view_statement.cc
@@ -222,7 +222,7 @@ future<shared_ptr<cql_transport::event::schema_change>> create_view_statement::a
return def;
}));

- if (!_variables.empty()) {
+ if (!_prep_meta.empty()) {
throw exceptions::invalid_request_exception(format("Cannot use query parameters in CREATE MATERIALIZED VIEW statements"));
}

diff --git a/cql3/statements/delete_statement.cc b/cql3/statements/delete_statement.cc
index 2cb0e2bbf3..4d33bf4ab5 100644
--- a/cql3/statements/delete_statement.cc
+++ b/cql3/statements/delete_statement.cc
@@ -85,9 +85,9 @@ void delete_statement::add_update_for_key(mutation& m, const query::clustering_r
namespace raw {

::shared_ptr<cql3::statements::modification_statement>
-delete_statement::prepare_internal(database& db, schema_ptr schema, variable_specifications& bound_names,
+delete_statement::prepare_internal(database& db, schema_ptr schema, raw_prepare_metadata& meta,
std::unique_ptr<attributes> attrs, cql_stats& stats) const {
- auto stmt = ::make_shared<cql3::statements::delete_statement>(statement_type::DELETE, bound_names.size(), schema, std::move(attrs), stats);
+ auto stmt = ::make_shared<cql3::statements::delete_statement>(statement_type::DELETE, meta.bound_variables_size(), schema, std::move(attrs), stats);

for (auto&& deletion : _deletions) {
auto&& id = deletion->affected_column().prepare_column_identifier(*schema);
@@ -103,11 +103,11 @@ delete_statement::prepare_internal(database& db, schema_ptr schema, variable_spe
}

auto&& op = deletion->prepare(db, schema->ks_name(), *def);
- op->collect_marker_specification(bound_names);
+ op->collect_prepare_metadata(meta);
stmt->add_operation(op);
}
- prepare_conditions(db, *schema, bound_names, *stmt);
- stmt->process_where_clause(db, _where_clause, bound_names);
+ prepare_conditions(db, *schema, meta, *stmt);
+ stmt->process_where_clause(db, _where_clause, meta);
if (has_slice(stmt->restrictions().get_clustering_columns_restrictions()->expression)) {
if (!schema->is_compound()) {
throw exceptions::invalid_request_exception("Range deletions on \"compact storage\" schemas are not supported");
diff --git a/cql3/statements/modification_statement.cc b/cql3/statements/modification_statement.cc
index d1af58f12b..f5487fefb1 100644
--- a/cql3/statements/modification_statement.cc
+++ b/cql3/statements/modification_statement.cc
@@ -389,7 +389,7 @@ void modification_statement::build_cas_result_set_metadata() {
}

void
-modification_statement::process_where_clause(database& db, std::vector<relation_ptr> where_clause, variable_specifications& names) {
+modification_statement::process_where_clause(database& db, std::vector<relation_ptr> where_clause, raw_prepare_metadata& names) {
_restrictions = restrictions::statement_restrictions(db, s, type, where_clause, names,
applies_only_to_static_columns(), _selects_a_collection, false);
/*
@@ -461,24 +461,24 @@ namespace raw {
std::unique_ptr<prepared_statement>
modification_statement::prepare(database& db, cql_stats& stats) {
schema_ptr schema = validation::validate_column_family(db, keyspace(), column_family());
- auto bound_names = get_bound_variables();
- auto statement = prepare(db, bound_names, stats);
- auto partition_key_bind_indices = bound_names.get_partition_key_bind_indexes(*schema);
- return std::make_unique<prepared_statement>(std::move(statement), bound_names, std::move(partition_key_bind_indices));
+ auto meta = get_prepare_metadata();
+ auto statement = prepare(db, meta, stats);
+ auto partition_key_bind_indices = meta.get_partition_key_bind_indexes(*schema);
+ return std::make_unique<prepared_statement>(std::move(statement), meta, std::move(partition_key_bind_indices));
}

::shared_ptr<cql3::statements::modification_statement>
-modification_statement::prepare(database& db, variable_specifications& bound_names, cql_stats& stats) const {
+modification_statement::prepare(database& db, raw_prepare_metadata& meta, cql_stats& stats) const {
schema_ptr schema = validation::validate_column_family(db, keyspace(), column_family());

auto prepared_attributes = _attrs->prepare(db, keyspace(), column_family());
- prepared_attributes->collect_marker_specification(bound_names);
+ prepared_attributes->collect_prepare_metadata(meta);

- return prepare_internal(db, schema, bound_names, std::move(prepared_attributes), stats);
+ return prepare_internal(db, schema, meta, std::move(prepared_attributes), stats);
}

void
-modification_statement::prepare_conditions(database& db, const schema& schema, variable_specifications& bound_names,
+modification_statement::prepare_conditions(database& db, const schema& schema, raw_prepare_metadata& meta,
cql3::statements::modification_statement& stmt) const
{
if (_if_not_exists || _if_exists || !_conditions.empty()) {
@@ -508,7 +508,7 @@ modification_statement::prepare_conditions(database& db, const schema& schema, v
}

auto condition = entry.second->prepare(db, keyspace(), *def);
- condition->collect_marker_specificaton(bound_names);
+ condition->collect_marker_specificaton(meta);

if (def->is_primary_key()) {
throw exceptions::invalid_request_exception(format("PRIMARY KEY column '{}' cannot have IF conditions", *id));
diff --git a/cql3/statements/modification_statement.hh b/cql3/statements/modification_statement.hh
index 87a69c0662..39c8ed3f89 100644
--- a/cql3/statements/modification_statement.hh
+++ b/cql3/statements/modification_statement.hh
@@ -194,7 +194,7 @@ class modification_statement : public cql_statement_opt_metadata {
return _is_raw_counter_shard_write.value_or(false);
}

- void process_where_clause(database& db, std::vector<relation_ptr> where_clause, variable_specifications& names);
+ void process_where_clause(database& db, std::vector<relation_ptr> where_clause, raw_prepare_metadata& names);

// CAS statement returns a result set. Prepare result set metadata
// so that get_result_metadata() returns a meaningful value.
diff --git a/cql3/statements/prepared_statement.hh b/cql3/statements/prepared_statement.hh
index fe28aea9b1..4bdd341a8d 100644
--- a/cql3/statements/prepared_statement.hh
+++ b/cql3/statements/prepared_statement.hh
@@ -51,7 +51,7 @@

namespace cql3 {

-class variable_specifications;
+class raw_prepare_metadata;
class column_specification;
class cql_statement;

@@ -74,9 +74,9 @@ class prepared_statement : public seastar::weakly_referencable<prepared_statemen

prepared_statement(seastar::shared_ptr<cql_statement> statement_, std::vector<seastar::lw_shared_ptr<column_specification>> bound_names_, std::vector<uint16_t> partition_key_bind_indices);

- prepared_statement(seastar::shared_ptr<cql_statement> statement_, const variable_specifications& names, const std::vector<uint16_t>& partition_key_bind_indices);
+ prepared_statement(seastar::shared_ptr<cql_statement> statement_, const raw_prepare_metadata& meta, const std::vector<uint16_t>& partition_key_bind_indices);

- prepared_statement(seastar::shared_ptr<cql_statement> statement_, variable_specifications&& names, std::vector<uint16_t>&& partition_key_bind_indices);
+ prepared_statement(seastar::shared_ptr<cql_statement> statement_, raw_prepare_metadata&& meta, std::vector<uint16_t>&& partition_key_bind_indices);

prepared_statement(seastar::shared_ptr<cql_statement>&& statement_);

diff --git a/cql3/statements/raw/delete_statement.hh b/cql3/statements/raw/delete_statement.hh
index 0d1e273985..442d0d6fc6 100644
--- a/cql3/statements/raw/delete_statement.hh
+++ b/cql3/statements/raw/delete_statement.hh
@@ -69,7 +69,7 @@ class delete_statement : public modification_statement {
bool if_exists);
protected:
virtual ::shared_ptr<cql3::statements::modification_statement> prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;
+ raw_prepare_metadata& meta, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;
};

}
diff --git a/cql3/statements/raw/insert_statement.hh b/cql3/statements/raw/insert_statement.hh
index 5ec5d58b76..84faf065a9 100644
--- a/cql3/statements/raw/insert_statement.hh
+++ b/cql3/statements/raw/insert_statement.hh
@@ -77,7 +77,7 @@ class insert_statement : public raw::modification_statement {
bool if_not_exists);

virtual ::shared_ptr<cql3::statements::modification_statement> prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;
+ raw_prepare_metadata& meta, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;

};

@@ -98,7 +98,7 @@ class insert_json_statement : public raw::modification_statement {
insert_json_statement(cf_name name, std::unique_ptr<attributes::raw> attrs, ::shared_ptr<term::raw> json_value, bool if_not_exists, bool default_unset);

virtual ::shared_ptr<cql3::statements::modification_statement> prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;
+ raw_prepare_metadata& meta, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;

};

diff --git a/cql3/statements/raw/modification_statement.hh b/cql3/statements/raw/modification_statement.hh
index a4da08671a..1c5bfe5282 100644
--- a/cql3/statements/raw/modification_statement.hh
+++ b/cql3/statements/raw/modification_statement.hh
@@ -73,15 +73,15 @@ class modification_statement : public cf_statement {

public:
virtual std::unique_ptr<prepared_statement> prepare(database& db, cql_stats& stats) override;
- ::shared_ptr<cql3::statements::modification_statement> prepare(database& db, variable_specifications& bound_names, cql_stats& stats) const;
+ ::shared_ptr<cql3::statements::modification_statement> prepare(database& db, raw_prepare_metadata& meta, cql_stats& stats) const;
protected:
virtual ::shared_ptr<cql3::statements::modification_statement> prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const = 0;
+ raw_prepare_metadata& meta, std::unique_ptr<attributes> attrs, cql_stats& stats) const = 0;

// Helper function used by child classes to prepare conditions for a prepared statement.
// Must be called before processing WHERE clause, because to perform sanity checks there
// we need to know what kinds of conditions (static, regular) the statement has.
- void prepare_conditions(database& db, const schema& schema, variable_specifications& bound_names,
+ void prepare_conditions(database& db, const schema& schema, raw_prepare_metadata& meta,
cql3::statements::modification_statement& stmt) const;
};

diff --git a/cql3/statements/raw/parsed_statement.cc b/cql3/statements/raw/parsed_statement.cc
index dc58ef7811..b44132b8a1 100644
--- a/cql3/statements/raw/parsed_statement.cc
+++ b/cql3/statements/raw/parsed_statement.cc
@@ -53,17 +53,17 @@ namespace raw {
parsed_statement::~parsed_statement()
{ }

-variable_specifications& parsed_statement::get_bound_variables() {
- return _variables;
+raw_prepare_metadata& parsed_statement::get_prepare_metadata() {
+ return _prep_meta;
}

-const variable_specifications& parsed_statement::get_bound_variables() const {
- return _variables;
+const raw_prepare_metadata& parsed_statement::get_prepare_metadata() const {
+ return _prep_meta;
}

// Used by the parser and preparable statement
void parsed_statement::set_bound_variables(const std::vector<::shared_ptr<column_identifier>>& bound_names) {
- _variables.set_bound_variables(bound_names);
+ _prep_meta.set_bound_variables(bound_names);
}

}
@@ -74,12 +74,12 @@ prepared_statement::prepared_statement(::shared_ptr<cql_statement> statement_, s
, partition_key_bind_indices(std::move(partition_key_bind_indices))
{ }

-prepared_statement::prepared_statement(::shared_ptr<cql_statement> statement_, const variable_specifications& names, const std::vector<uint16_t>& partition_key_bind_indices)
- : prepared_statement(statement_, names.get_specifications(), partition_key_bind_indices)
+prepared_statement::prepared_statement(::shared_ptr<cql_statement> statement_, const raw_prepare_metadata& meta, const std::vector<uint16_t>& partition_key_bind_indices)
+ : prepared_statement(statement_, meta.get_variable_specifications(), partition_key_bind_indices)
{ }

-prepared_statement::prepared_statement(::shared_ptr<cql_statement> statement_, variable_specifications&& names, std::vector<uint16_t>&& partition_key_bind_indices)
- : prepared_statement(statement_, std::move(names).get_specifications(), std::move(partition_key_bind_indices))
+prepared_statement::prepared_statement(::shared_ptr<cql_statement> statement_, raw_prepare_metadata&& meta, std::vector<uint16_t>&& partition_key_bind_indices)
+ : prepared_statement(statement_, std::move(meta).get_variable_specifications(), std::move(partition_key_bind_indices))
{ }

prepared_statement::prepared_statement(::shared_ptr<cql_statement>&& statement_)
diff --git a/cql3/statements/raw/parsed_statement.hh b/cql3/statements/raw/parsed_statement.hh
index 3643a7e9ab..a508b7f4a2 100644
--- a/cql3/statements/raw/parsed_statement.hh
+++ b/cql3/statements/raw/parsed_statement.hh
@@ -41,7 +41,7 @@

#pragma once

-#include "cql3/variable_specifications.hh"
+#include "cql3/raw_prepare_metadata.hh"
#include "cql3/column_specification.hh"

#include <seastar/core/shared_ptr.hh>
@@ -64,13 +64,13 @@ namespace raw {

class parsed_statement {
protected:
- variable_specifications _variables;
+ raw_prepare_metadata _prep_meta;

public:
virtual ~parsed_statement();

- variable_specifications& get_bound_variables();
- const variable_specifications& get_bound_variables() const;
+ raw_prepare_metadata& get_prepare_metadata();
+ const raw_prepare_metadata& get_prepare_metadata() const;

void set_bound_variables(const std::vector<::shared_ptr<column_identifier>>& bound_names);

diff --git a/cql3/statements/raw/select_statement.hh b/cql3/statements/raw/select_statement.hh
index 9c6bcb0df1..c396cb2261 100644
--- a/cql3/statements/raw/select_statement.hh
+++ b/cql3/statements/raw/select_statement.hh
@@ -127,13 +127,13 @@ class select_statement : public cf_statement
::shared_ptr<restrictions::statement_restrictions> prepare_restrictions(
database& db,
schema_ptr schema,
- variable_specifications& bound_names,
+ raw_prepare_metadata& meta,
::shared_ptr<selection::selection> selection,
bool for_view = false,
bool allow_filtering = false);

/** Returns a ::shared_ptr<term> for the limit or null if no limit is set */
- ::shared_ptr<term> prepare_limit(database& db, variable_specifications& bound_names, ::shared_ptr<term::raw> limit);
+ ::shared_ptr<term> prepare_limit(database& db, raw_prepare_metadata& meta, ::shared_ptr<term::raw> limit);

static void verify_ordering_is_allowed(const restrictions::statement_restrictions& restrictions);

diff --git a/cql3/statements/raw/update_statement.hh b/cql3/statements/raw/update_statement.hh
index d03aa055ef..a1b25c516c 100644
--- a/cql3/statements/raw/update_statement.hh
+++ b/cql3/statements/raw/update_statement.hh
@@ -81,7 +81,7 @@ class update_statement : public raw::modification_statement {
conditions_vector conditions, bool if_exists);
protected:
virtual ::shared_ptr<cql3::statements::modification_statement> prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;
+ raw_prepare_metadata& meta, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;
};

}
diff --git a/cql3/statements/select_statement.cc b/cql3/statements/select_statement.cc
index c25d1c7418..ce0f176ceb 100644
--- a/cql3/statements/select_statement.cc
+++ b/cql3/statements/select_statement.cc
@@ -1402,7 +1402,7 @@ void select_statement::maybe_jsonize_select_clause(database& db, schema_ptr sche

std::unique_ptr<prepared_statement> select_statement::prepare(database& db, cql_stats& stats, bool for_view) {
schema_ptr schema = validation::validate_column_family(db, keyspace(), column_family());
- variable_specifications& bound_names = get_bound_variables();
+ raw_prepare_metadata& meta = get_prepare_metadata();

maybe_jsonize_select_clause(db, schema);

@@ -1410,7 +1410,7 @@ std::unique_ptr<prepared_statement> select_statement::prepare(database& db, cql_
? selection::selection::wildcard(schema)
: selection::selection::from_selectors(db, schema, _select_clause);

- auto restrictions = prepare_restrictions(db, schema, bound_names, selection, for_view, _parameters->allow_filtering());
+ auto restrictions = prepare_restrictions(db, schema, meta, selection, for_view, _parameters->allow_filtering());

if (_parameters->is_distinct()) {
validate_distinct_selection(*schema, *selection, *restrictions);
@@ -1432,53 +1432,53 @@ std::unique_ptr<prepared_statement> select_statement::prepare(database& db, cql_

::shared_ptr<cql3::statements::select_statement> stmt;
auto prepared_attrs = _attrs->prepare(db, keyspace(), column_family());
- prepared_attrs->collect_marker_specification(bound_names);
+ prepared_attrs->collect_prepare_metadata(meta);
if (restrictions->uses_secondary_indexing()) {
stmt = indexed_table_select_statement::prepare(
db,
schema,
- bound_names.size(),
+ meta.bound_variables_size(),
_parameters,
std::move(selection),
std::move(restrictions),
std::move(group_by_cell_indices),
is_reversed_,
std::move(ordering_comparator),
- prepare_limit(db, bound_names, _limit),
- prepare_limit(db, bound_names, _per_partition_limit),
+ prepare_limit(db, meta, _limit),
+ prepare_limit(db, meta, _per_partition_limit),
stats,
std::move(prepared_attrs));
} else {
stmt = ::make_shared<cql3::statements::primary_key_select_statement>(
schema,
- bound_names.size(),
+ meta.bound_variables_size(),
_parameters,
std::move(selection),
std::move(restrictions),
std::move(group_by_cell_indices),
is_reversed_,
std::move(ordering_comparator),
- prepare_limit(db, bound_names, _limit),
- prepare_limit(db, bound_names, _per_partition_limit),
+ prepare_limit(db, meta, _limit),
+ prepare_limit(db, meta, _per_partition_limit),
stats,
std::move(prepared_attrs));
}

- auto partition_key_bind_indices = bound_names.get_partition_key_bind_indexes(*schema);
+ auto partition_key_bind_indices = meta.get_partition_key_bind_indexes(*schema);

- return std::make_unique<prepared_statement>(std::move(stmt), bound_names, std::move(partition_key_bind_indices));
+ return std::make_unique<prepared_statement>(std::move(stmt), meta, std::move(partition_key_bind_indices));
}

::shared_ptr<restrictions::statement_restrictions>
select_statement::prepare_restrictions(database& db,
schema_ptr schema,
- variable_specifications& bound_names,
+ raw_prepare_metadata& meta,
::shared_ptr<selection::selection> selection,
bool for_view,
bool allow_filtering)
{
try {
- return ::make_shared<restrictions::statement_restrictions>(db, schema, statement_type::SELECT, std::move(_where_clause), bound_names,
+ return ::make_shared<restrictions::statement_restrictions>(db, schema, statement_type::SELECT, std::move(_where_clause), meta,
selection->contains_only_static_columns(), for_view, allow_filtering);
} catch (const exceptions::unrecognized_entity_exception& e) {
if (contains_alias(e.entity)) {
@@ -1490,14 +1490,14 @@ select_statement::prepare_restrictions(database& db,

/** Returns a ::shared_ptr<term> for the limit or null if no limit is set */
::shared_ptr<term>
-select_statement::prepare_limit(database& db, variable_specifications& bound_names, ::shared_ptr<term::raw> limit)
+select_statement::prepare_limit(database& db, raw_prepare_metadata& meta, ::shared_ptr<term::raw> limit)
{
if (!limit) {
return {};
}

auto prep_limit = limit->prepare(db, keyspace(), limit_receiver());
- prep_limit->collect_marker_specification(bound_names);
+ prep_limit->collect_prepare_metadata(meta);
return prep_limit;
}

diff --git a/cql3/statements/update_statement.cc b/cql3/statements/update_statement.cc
index ea2de9b4e9..d39af45a8a 100644
--- a/cql3/statements/update_statement.cc
+++ b/cql3/statements/update_statement.cc
@@ -300,9 +300,9 @@ insert_statement::insert_statement(cf_name name,

::shared_ptr<cql3::statements::modification_statement>
insert_statement::prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const
+ raw_prepare_metadata& meta, std::unique_ptr<attributes> attrs, cql_stats& stats) const
{
- auto stmt = ::make_shared<cql3::statements::update_statement>(statement_type::INSERT, bound_names.size(), schema, std::move(attrs), stats);
+ auto stmt = ::make_shared<cql3::statements::update_statement>(statement_type::INSERT, meta.bound_variables_size(), schema, std::move(attrs), stats);

// Created from an INSERT
if (stmt->is_counter()) {
@@ -337,12 +337,12 @@ insert_statement::prepare_internal(database& db, schema_ptr schema,
relations.push_back(::make_shared<single_column_relation>(col, expr::oper_t::EQ, value));
} else {
auto operation = operation::set_value(value).prepare(db, keyspace(), *def);
- operation->collect_marker_specification(bound_names);
+ operation->collect_prepare_metadata(meta);
stmt->add_operation(std::move(operation));
};
}
- prepare_conditions(db, *schema, bound_names, *stmt);
- stmt->process_where_clause(db, relations, bound_names);
+ prepare_conditions(db, *schema, meta, *stmt);
+ stmt->process_where_clause(db, relations, meta);
return stmt;
}

@@ -359,16 +359,16 @@ insert_json_statement::insert_json_statement(cf_name name,

::shared_ptr<cql3::statements::modification_statement>
insert_json_statement::prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const
+ raw_prepare_metadata& meta, std::unique_ptr<attributes> attrs, cql_stats& stats) const
{
// FIXME: handle _if_not_exists. For now, mark it used to quiet the compiler. #8682
(void)_if_not_exists;
assert(dynamic_pointer_cast<constants::literal>(_json_value) || dynamic_pointer_cast<abstract_marker::raw>(_json_value));
auto json_column_placeholder = ::make_shared<column_identifier>("", true);
auto prepared_json_value = _json_value->prepare(db, "", make_lw_shared<column_specification>("", "", json_column_placeholder, utf8_type));
- prepared_json_value->collect_marker_specification(bound_names);
- auto stmt = ::make_shared<cql3::statements::insert_prepared_json_statement>(bound_names.size(), schema, std::move(attrs), stats, std::move(prepared_json_value), _default_unset);
- prepare_conditions(db, *schema, bound_names, *stmt);
+ prepared_json_value->collect_prepare_metadata(meta);
+ auto stmt = ::make_shared<cql3::statements::insert_prepared_json_statement>(meta.bound_variables_size(), schema, std::move(attrs), stats, std::move(prepared_json_value), _default_unset);
+ prepare_conditions(db, *schema, meta, *stmt);
return stmt;
}

@@ -384,9 +384,9 @@ update_statement::update_statement(cf_name name,

::shared_ptr<cql3::statements::modification_statement>
update_statement::prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const
+ raw_prepare_metadata& meta, std::unique_ptr<attributes> attrs, cql_stats& stats) const
{
- auto stmt = ::make_shared<cql3::statements::update_statement>(statement_type::UPDATE, bound_names.size(), schema, std::move(attrs), stats);
+ auto stmt = ::make_shared<cql3::statements::update_statement>(statement_type::UPDATE, meta.bound_variables_size(), schema, std::move(attrs), stats);

for (auto&& entry : _updates) {
auto id = entry.first->prepare_column_identifier(*schema);
@@ -396,15 +396,15 @@ update_statement::prepare_internal(database& db, schema_ptr schema,
}

auto operation = entry.second->prepare(db, keyspace(), *def);
- operation->collect_marker_specification(bound_names);
+ operation->collect_prepare_metadata(meta);

if (def->is_primary_key()) {
throw exceptions::invalid_request_exception(format("PRIMARY KEY part {} found in SET part", *entry.first));
}
stmt->add_operation(std::move(operation));
}
- prepare_conditions(db, *schema, bound_names, *stmt);
- stmt->process_where_clause(db, _where_clause, bound_names);
+ prepare_conditions(db, *schema, meta, *stmt);
+ stmt->process_where_clause(db, _where_clause, meta);
return stmt;
}

diff --git a/cql3/term.hh b/cql3/term.hh
index 145da86f15..3837c5ba4c 100644
--- a/cql3/term.hh
+++ b/cql3/term.hh
@@ -48,7 +48,7 @@
namespace cql3 {

class terminal;
-class variable_specifications;
+class raw_prepare_metadata;

/**
* A CQL3 term, i.e. a column value with or without bind variables.
@@ -68,7 +68,7 @@ class term : public ::enable_shared_from_this<term> {
* @param boundNames the variables specification where to collect the
* bind variables of this term in.
*/
- virtual void collect_marker_specification(variable_specifications& bound_names) const = 0;
+ virtual void collect_prepare_metadata(raw_prepare_metadata& meta) const = 0;

/**
* Bind the values in this term to the values contained in {@code values}.
@@ -161,7 +161,7 @@ class term : public ::enable_shared_from_this<term> {
*/
class terminal : public term {
public:
- virtual void collect_marker_specification(variable_specifications& bound_names) const {
+ virtual void collect_prepare_metadata(raw_prepare_metadata& meta) const {
}

virtual ::shared_ptr<terminal> bind(const query_options& options) override {
diff --git a/cql3/token_relation.cc b/cql3/token_relation.cc
index 5a6e612f83..20bab49388 100644
--- a/cql3/token_relation.cc
+++ b/cql3/token_relation.cc
@@ -81,10 +81,10 @@ std::vector<lw_shared_ptr<cql3::column_specification>> cql3::token_relation::to_

::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_EQ_restriction(
database& db, schema_ptr schema,
- variable_specifications& bound_names) {
+ raw_prepare_metadata& meta) {
auto column_defs = get_column_definitions(*schema);
auto term = to_term(to_receivers(*schema, column_defs), *_value, db,
- schema->ks_name(), bound_names);
+ schema->ks_name(), meta);
auto r = ::make_shared<restrictions::token_restriction>(column_defs);
using namespace expr;
r->expression = binary_operator{token{}, oper_t::EQ, std::move(term)};
@@ -93,7 +93,7 @@ ::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_EQ_restr

::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_IN_restriction(
database& db, schema_ptr schema,
- variable_specifications& bound_names) {
+ raw_prepare_metadata& meta) {
throw exceptions::invalid_request_exception(
format("{} cannot be used with the token function",
get_operator()));
@@ -101,12 +101,12 @@ ::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_IN_restr

::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_slice_restriction(
database& db, schema_ptr schema,
- variable_specifications& bound_names,
+ raw_prepare_metadata& meta,
statements::bound bound,
bool inclusive) {
auto column_defs = get_column_definitions(*schema);
auto term = to_term(to_receivers(*schema, column_defs), *_value, db,
- schema->ks_name(), bound_names);
+ schema->ks_name(), meta);
auto r = ::make_shared<restrictions::token_restriction>(column_defs);
using namespace expr;
r->expression = binary_operator{token{}, pick_operator(bound, inclusive), std::move(term)};
@@ -115,14 +115,14 @@ ::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_slice_re

::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_contains_restriction(
database& db, schema_ptr schema,
- variable_specifications& bound_names, bool isKey) {
+ raw_prepare_metadata& meta, bool isKey) {
throw exceptions::invalid_request_exception(
format("{} cannot be used with the token function",
get_operator()));
}

::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_LIKE_restriction(
- database&, schema_ptr, variable_specifications&) {
+ database&, schema_ptr, raw_prepare_metadata&) {
throw exceptions::invalid_request_exception("LIKE cannot be used with the token function");
}

@@ -133,9 +133,9 @@ sstring cql3::token_relation::to_string() const {
::shared_ptr<cql3::term> cql3::token_relation::to_term(
const std::vector<lw_shared_ptr<column_specification>>& receivers,
const term::raw& raw, database& db, const sstring& keyspace,
- variable_specifications& bound_names) const {
+ raw_prepare_metadata& meta) const {
auto term = raw.prepare(db, keyspace, receivers.front());
- term->collect_marker_specification(bound_names);
+ term->collect_prepare_metadata(meta);
return term;
}

diff --git a/cql3/token_relation.hh b/cql3/token_relation.hh
index 7a986de41d..5e50fc1021 100644
--- a/cql3/token_relation.hh
+++ b/cql3/token_relation.hh
@@ -98,25 +98,25 @@ class token_relation : public relation {

::shared_ptr<restrictions::restriction> new_EQ_restriction(database& db,
schema_ptr schema,
- variable_specifications& bound_names) override;
+ raw_prepare_metadata& meta) override;

::shared_ptr<restrictions::restriction> new_IN_restriction(database& db,
schema_ptr schema,
- variable_specifications& bound_names) override;
+ raw_prepare_metadata& meta) override;

::shared_ptr<restrictions::restriction> new_slice_restriction(database& db,
schema_ptr schema,
- variable_specifications& bound_names,
+ raw_prepare_metadata& meta,
statements::bound bound,
bool inclusive) override;

::shared_ptr<restrictions::restriction> new_contains_restriction(
database& db, schema_ptr schema,
- variable_specifications& bound_names, bool isKey) override;
+ raw_prepare_metadata& meta, bool isKey) override;

::shared_ptr<restrictions::restriction> new_LIKE_restriction(database& db,
schema_ptr schema,
- variable_specifications& bound_names) override;
+ raw_prepare_metadata& meta) override;

::shared_ptr<relation> maybe_rename_identifier(const column_identifier::raw& from, column_identifier::raw to) override;

@@ -127,7 +127,7 @@ class token_relation : public relation {
const term::raw& raw,
database& db,
const sstring& keyspace,
- variable_specifications& bound_names) const override;
+ raw_prepare_metadata& meta) const override;
};

}
diff --git a/cql3/tuples.hh b/cql3/tuples.hh
index 4ef78d6029..5bbc6fcab3 100644
--- a/cql3/tuples.hh
+++ b/cql3/tuples.hh
@@ -153,9 +153,9 @@ class tuples {
return std::any_of(_elements.begin(), _elements.end(), std::mem_fn(&term::contains_bind_marker));
}

- virtual void collect_marker_specification(variable_specifications& bound_names) const override {
+ virtual void collect_prepare_metadata(raw_prepare_metadata& meta) const override {
for (auto&& term : _elements) {
- term->collect_marker_specification(bound_names);
+ term->collect_prepare_metadata(meta);
}
}
private:
diff --git a/cql3/user_types.cc b/cql3/user_types.cc
index 309ab0e57a..e76463f06a 100644
--- a/cql3/user_types.cc
+++ b/cql3/user_types.cc
@@ -185,9 +185,9 @@ bool user_types::delayed_value::contains_bind_marker() const {
return boost::algorithm::any_of(_values, std::mem_fn(&term::contains_bind_marker));
}

-void user_types::delayed_value::collect_marker_specification(variable_specifications& bound_names) const {
+void user_types::delayed_value::collect_prepare_metadata(raw_prepare_metadata& meta) const {
for (auto&& v : _values) {
- v->collect_marker_specification(bound_names);
+ v->collect_prepare_metadata(meta);
}
}

diff --git a/cql3/user_types.hh b/cql3/user_types.hh
index 6186befe4a..f66a531cec 100644
--- a/cql3/user_types.hh
+++ b/cql3/user_types.hh
@@ -93,7 +93,7 @@ class user_types {
public:
delayed_value(user_type type, std::vector<shared_ptr<term>> values);
virtual bool contains_bind_marker() const override;
- virtual void collect_marker_specification(variable_specifications& bound_names) const;
+ virtual void collect_prepare_metadata(raw_prepare_metadata& meta) const;
private:
std::vector<managed_bytes_opt> bind_internal(const query_options& options);
public:
diff --git a/test/boost/statement_restrictions_test.cc b/test/boost/statement_restrictions_test.cc
index c5fa60f9d6..3546cbba6d 100644
--- a/test/boost/statement_restrictions_test.cc
+++ b/test/boost/statement_restrictions_test.cc
@@ -39,13 +39,13 @@ namespace {
query::clustering_row_ranges slice(
const std::vector<relation_ptr>& where_clause, cql_test_env& env,
const sstring& table_name = "t", const sstring& keyspace_name = "ks") {
- variable_specifications bound_names;
+ raw_prepare_metadata meta;
return restrictions::statement_restrictions(
env.local_db(),
env.local_db().find_schema(keyspace_name, table_name),
statements::statement_type::SELECT,
where_clause,
- bound_names,
+ meta,
/*contains_only_static_columns=*/false,
/*for_view=*/false,
/*allow_filtering=*/true)
--
2.31.1

Pavel Solodovnikov

unread,
Jul 21, 2021, 5:42:43 AMJul 21
to scylla...@googlegroups.com, Pavel Solodovnikov
And reuse these values when handling `bounce_to_shard` messages.

Otherwise such a function (e.g. `uuid()`) can yield a different
value when a statement re-executed on the other shard.

It can lead to an infinite number of `bounce_to_shard` messages
sent in case the function value is used to calculate partition
key ranges for the query. Which, in turn, will cause crashes
since we don't support bouncing more than one time and the second
hop will result in a crash.

Caching works only for LWT statements, for that the CQL parser
is modified so the `function_call::bind_and_get` function
is aware whether a function call is a part of LWT statement
or not.

There is no need to include any kind of statement identifier
to the cache key since `query_options` (which holds the cache)
is limited to a single statement, anyway.

Note that `function_call::raw` AST nodes are not created
for selection clauses of a SELECT statement hence they
can only accept only one of the following things as parameters:
* Other function calls.
* Literal values.
* Parameter markers.

In other words, only parameters that can be immediately reduced
to a byte buffer are allowed and we don't need to handle
database inputs to non-pure functions separately since they
are not possible in this context. Anyhow, we don't even have
a single non-pure function that accepts arguments, so precautions
are not needed at the moment.

Tests: unit(dev, debug)

Signed-off-by: Pavel Solodovnikov <pa.solo...@scylladb.com>
---
cql3/functions/function_call.hh | 15 +++++-
cql3/functions/functions.cc | 33 ++++++++++++
cql3/query_options.cc | 16 ++++++
cql3/query_options.hh | 29 ++++++++++
cql3/raw_prepare_metadata.cc | 4 ++
cql3/raw_prepare_metadata.hh | 15 +++++-
cql3/statements/batch_statement.cc | 6 ++-
cql3/statements/modification_statement.cc | 26 ++++++++-
cql3/statements/select_statement.cc | 5 +-
transport/messages/result_message.hh | 9 +++-
transport/server.cc | 64 ++++++++++++++---------
transport/server.hh | 2 +-
12 files changed, 191 insertions(+), 33 deletions(-)

diff --git a/cql3/functions/function_call.hh b/cql3/functions/function_call.hh
index 7ca9db9efd..3ddf824179 100644
--- a/cql3/functions/function_call.hh
+++ b/cql3/functions/function_call.hh
@@ -53,11 +53,24 @@ namespace functions {
class function_call : public non_terminal {
const shared_ptr<scalar_function> _fun;
const std::vector<shared_ptr<term>> _terms;
+ // 0-based index of the function call within a CQL statement.
+ // Used to populate the cache of execution results while passing to
+ // another shard (handling `bounce_to_shard` messages) in LWT statements.
+ uint8_t _id;
+ // This flag is set to `true` if the `function_call` AST element
+ // is a part of LWT statement.
+ bool _in_lwt_context;
public:
function_call(shared_ptr<scalar_function> fun, std::vector<shared_ptr<term>> terms)
- : _fun(std::move(fun)), _terms(std::move(terms)) {
+ : _fun(std::move(fun)), _terms(std::move(terms)), _in_lwt_context(false) {
}
virtual void collect_prepare_metadata(raw_prepare_metadata& meta) const override;
+ void set_id(uint8_t id) {
+ _id = id;
+ }
+ void set_lwt_context() {
+ _in_lwt_context = true;
+ }
virtual shared_ptr<terminal> bind(const query_options& options) override;
virtual cql3::raw_value_view bind_and_get(const query_options& options) override;
private:
diff --git a/cql3/functions/functions.cc b/cql3/functions/functions.cc
index 596ef665c1..d8809d9c82 100644
--- a/cql3/functions/functions.cc
+++ b/cql3/functions/functions.cc
@@ -36,6 +36,7 @@
#include "types/user.hh"
#include "concrete_types.hh"
#include "as_json_function.hh"
+#include "cql3/raw_prepare_metadata.hh"

#include "error_injection_fcts.hh"

@@ -431,6 +432,19 @@ functions::type_equals(const std::vector<data_type>& t1, const std::vector<data_

void
function_call::collect_prepare_metadata(raw_prepare_metadata& meta) const {
+ constexpr auto fn_limit = std::numeric_limits<uint8_t>::max();
+ auto& fn_calls = meta.function_calls();
+ if (fn_calls.size() == fn_limit) {
+ throw exceptions::invalid_request_exception(
+ format("Too many function calls within one statement. Max supported number is {}", fn_limit));
+ }
+ // FIXME: Hacking around `const` specifier in the `collect_prepare_metadata`
+ // declaration since we also need to modify the current instance along
+ // with prepare metadata.
+ auto non_const_self = static_pointer_cast<function_call>(
+ const_cast<function_call*>(this)->shared_from_this());
+ non_const_self->set_id(fn_calls.size());
+ fn_calls.emplace_back(std::move(non_const_self));
for (auto&& t : _terms) {
t->collect_prepare_metadata(meta);
}
@@ -454,7 +468,26 @@ function_call::bind_and_get(const query_options& options) {
}
buffers.push_back(to_bytes_opt(val));
}
+ if (_in_lwt_context && !_fun->is_pure()) {
+ // Populate the cache only for LWT statements. Note that this code
+ // works only in places where `function_call::raw` AST nodes are
+ // created.
+ // These cases do not include selection clause in SELECT statement,
+ // hence no database inputs are possibly allowed to the functions
+ // evaluated here.
+ // We can cache every non-deterministic call here as this code branch
+ // acts the same way as if all arguments are equivalent to literal
+ // values at this point (already calculated).
+ auto query_cached_fn_calls = options.cached_function_calls();
+ auto cached_value_it = query_cached_fn_calls.find(_id);
+ if (query_cached_fn_calls.end() != cached_value_it) {
+ return raw_value_view::make_temporary(raw_value::make_value(cached_value_it->second));
+ }
+ }
auto result = execute_internal(options.get_cql_serialization_format(), *_fun, std::move(buffers));
+ if (_in_lwt_context && !_fun->is_pure()) {
+ options.cache_function_call(_id, result);
+ }
return cql3::raw_value_view::make_temporary(cql3::raw_value::make_value(result));
}

diff --git a/cql3/query_options.cc b/cql3/query_options.cc
index a0b1b44f14..7394b0cc55 100644
--- a/cql3/query_options.cc
+++ b/cql3/query_options.cc
@@ -191,4 +191,20 @@ db::consistency_level query_options::check_serial_consistency() const {
throw exceptions::protocol_exception("Consistency level for LWT is missing for a request with conditions");
}

+void query_options::cache_function_call(computed_function_values::key_type id, computed_function_values::mapped_type value) const {
+ _cached_fn_calls.emplace(id, std::move(value));
}
+
+const computed_function_values& query_options::cached_function_calls() const {
+ return _cached_fn_calls;
+}
+
+computed_function_values&& query_options::take_cached_function_calls() {
+ return std::move(_cached_fn_calls);
+}
+
+void query_options::set_cached_function_calls(computed_function_values vals) {
+ _cached_fn_calls = std::move(vals);
+}
+
+}
\ No newline at end of file
diff --git a/cql3/query_options.hh b/cql3/query_options.hh
index 0c3e665dd3..a3a342493c 100644
--- a/cql3/query_options.hh
+++ b/cql3/query_options.hh
@@ -57,6 +57,8 @@ extern const cql_config default_cql_config;

class column_specification;

+using computed_function_values = std::unordered_map<uint8_t, bytes_opt>;
+
/**
* Options for a query.
*/
@@ -91,6 +93,26 @@ class query_options {
// 2) monotonic
// 3) unique.
mutable int _list_append_seq = 0;
+
+ // Cached `function_call` evaluation results. `function_call` AST nodes
+ // are created for each function with side effects in a CQL query, i.e.
+ // non-deterministic functions (`uuid()`, `now()` and some others
+ // timeuuid-related).
+ //
+ // These nodes are evaluated either when a query itself is executed
+ // or query restrictions are computed (e.g. partition/clustering
+ // key ranges for LWT requests).
+ //
+ // We need to cache the calls since otherwise when handling a
+ // `bounce_to_shard` request for an LWT query, we can possibly enter an
+ // infinite bouncing loop (in case a function is used to calculate
+ // partition key ranges for a query), since the results can be different
+ // each time. Furthermore, we don't support bouncing more than one time.
+ // Refs: #8604 (https://github.com/scylladb/scylla/issues/8604)
+ //
+ // FIXME: mutable is a hack! `query_state` is not available at
+ // evaluation sites and we only have a const reference to `query_options`.
+ mutable computed_function_values _cached_fn_calls;
private:
/**
* @brief Batch query_options constructor.
@@ -276,6 +298,13 @@ class query_options {
}

void prepare(const std::vector<lw_shared_ptr<column_specification>>& specs);
+
+ // FIXME: should be non-const since it's mutating internals!
+ void cache_function_call(computed_function_values::key_type id, computed_function_values::mapped_type value) const;
+ const computed_function_values& cached_function_calls() const;
+ computed_function_values&& take_cached_function_calls();
+ void set_cached_function_calls(computed_function_values vals);
+
private:
void fill_value_views();
};
diff --git a/cql3/raw_prepare_metadata.cc b/cql3/raw_prepare_metadata.cc
index d3fe69b34b..d4ccfe5c0d 100644
--- a/cql3/raw_prepare_metadata.cc
+++ b/cql3/raw_prepare_metadata.cc
@@ -105,4 +105,8 @@ void raw_prepare_metadata::set_bound_variables(const std::vector<shared_ptr<colu
_target_columns.resize(bn_size);
}

+raw_prepare_metadata::function_calls_t& raw_prepare_metadata::function_calls() {
+ return _fn_calls;
+}
+
}
diff --git a/cql3/raw_prepare_metadata.hh b/cql3/raw_prepare_metadata.hh
index 0a9cd0b520..256e3e4b81 100644
--- a/cql3/raw_prepare_metadata.hh
+++ b/cql3/raw_prepare_metadata.hh
@@ -54,16 +54,25 @@ namespace cql3 {

class column_identifier;
class column_specification;
+namespace functions { class function_call; }

/**
- * A metadata class currently holding bind variables specifications and
- * populated at "prepare" step of query execution.
+ * A metadata class currently holding bind variables specifications and
+ * references to `function_call` nodes (needed at the end of "prepare" to
+ * set LWT execution context for the instances if needed).
+ *
+ * Populated at "prepare" step of query execution.
*/
class raw_prepare_metadata final {
private:
std::vector<shared_ptr<column_identifier>> _variable_names;
std::vector<lw_shared_ptr<column_specification>> _specs;
std::vector<lw_shared_ptr<column_specification>> _target_columns;
+ // A list of pointers to prepared `function_call` AST nodes.
+ // Used to set additional state for these calls at "prepare" step of a
+ // statement life cycle.
+ using function_calls_t = std::vector<::shared_ptr<cql3::functions::function_call>>;
+ function_calls_t _fn_calls;

public:

@@ -83,6 +92,8 @@ class raw_prepare_metadata final {
void add_variable_specification(int32_t bind_index, lw_shared_ptr<column_specification> spec);

void set_bound_variables(const std::vector<shared_ptr<column_identifier>>& prepare_meta);
+
+ function_calls_t& function_calls();
};

}
diff --git a/cql3/statements/batch_statement.cc b/cql3/statements/batch_statement.cc
index 30706675e0..ff0f6decda 100644
--- a/cql3/statements/batch_statement.cc
+++ b/cql3/statements/batch_statement.cc
@@ -359,6 +359,8 @@ future<shared_ptr<cql_transport::messages::result_message>> batch_statement::exe
auto cas_timeout = now + cfg.cas_timeout; // Ballot contention timeout.
auto read_timeout = now + cfg.read_timeout; // Query timeout.

+ computed_function_values cached_fn_calls;
+
for (size_t i = 0; i < _statements.size(); ++i) {

modification_statement& statement = *_statements[i].statement;
@@ -377,6 +379,8 @@ future<shared_ptr<cql_transport::messages::result_message>> batch_statement::exe
} else if (keys.size() != 1 || keys.front().equal(request->key().front(), dht::ring_position_comparator(*schema)) == false) {
throw exceptions::invalid_request_exception("BATCH with conditions cannot span multiple partitions");
}
+ cached_fn_calls.merge(std::move(const_cast<cql3::query_options&>(statement_options).take_cached_function_calls()));
+
std::vector<query::clustering_range> ranges = statement.create_clustering_ranges(statement_options, json_cache);

request->add_row_update(statement, std::move(ranges), std::move(json_cache), statement_options);
@@ -389,7 +393,7 @@ future<shared_ptr<cql_transport::messages::result_message>> batch_statement::exe
if (shard != this_shard_id()) {
proxy.get_stats().replica_cross_shard_ops++;
return make_ready_future<shared_ptr<cql_transport::messages::result_message>>(
- make_shared<cql_transport::messages::result_message::bounce_to_shard>(shard));
+ ::make_shared<cql_transport::messages::result_message::bounce_to_shard>(shard, std::move(cached_fn_calls)));
}

return proxy.cas(schema, request, request->read_command(proxy), request->key(),
diff --git a/cql3/statements/modification_statement.cc b/cql3/statements/modification_statement.cc
index f5487fefb1..8b1611808b 100644
--- a/cql3/statements/modification_statement.cc
+++ b/cql3/statements/modification_statement.cc
@@ -334,7 +334,10 @@ modification_statement::execute_with_condition(service::storage_proxy& proxy, se
if (shard != this_shard_id()) {
proxy.get_stats().replica_cross_shard_ops++;
return make_ready_future<shared_ptr<cql_transport::messages::result_message>>(
- make_shared<cql_transport::messages::result_message::bounce_to_shard>(shard));
+ ::make_shared<cql_transport::messages::result_message::bounce_to_shard>(shard,
+ // const_cast is a hack! this should really be done via `query_state` but
+ // would involve a massive refactoring.
+ std::move(const_cast<cql3::query_options&>(options).take_cached_function_calls())));
}

return proxy.cas(s, request, request->read_command(proxy), request->key(),
@@ -474,7 +477,26 @@ modification_statement::prepare(database& db, raw_prepare_metadata& meta, cql_st
auto prepared_attributes = _attrs->prepare(db, keyspace(), column_family());
prepared_attributes->collect_prepare_metadata(meta);

- return prepare_internal(db, schema, meta, std::move(prepared_attributes), stats);
+ auto prepared_stmt = prepare_internal(db, schema, meta, std::move(prepared_attributes), stats);
+ // For LWT statements, set an additional flag for each non-deterministic
+ // function call within a statement, making it aware that it's being
+ // executed in LWT context.
+ //
+ // This is important since these calls can possibly affect partition key
+ // ranges computation. For such cases we need to forward the computed
+ // execution result of the function when redirecting the query execution to
+ // another shard. Otherwise, it's possible that we end up bouncing
+ // indefinitely between various shards when evaluating a non-deterministic
+ // function each time on each shard.
+ //
+ // Set the flags after the main prepare procedure has already finished,
+ // by that time we've already collected the necessary metadata.
+ if (prepared_stmt->has_conditions()) {
+ for (auto& fn : meta.function_calls()) {
+ fn->set_lwt_context();
+ }
+ }
+ return prepared_stmt;
}

void
diff --git a/cql3/statements/select_statement.cc b/cql3/statements/select_statement.cc
index ce0f176ceb..f367b3cb38 100644
--- a/cql3/statements/select_statement.cc
+++ b/cql3/statements/select_statement.cc
@@ -364,7 +364,10 @@ select_statement::do_execute(service::storage_proxy& proxy,
if (this_shard_id() != shard) {
proxy.get_stats().replica_cross_shard_ops++;
return make_ready_future<shared_ptr<cql_transport::messages::result_message>>(
- make_shared<cql_transport::messages::result_message::bounce_to_shard>(shard));
+ ::make_shared<cql_transport::messages::result_message::bounce_to_shard>(shard,
+ // const_cast is a hack! this should really be done via `query_state` but
+ // would involve a massive refactoring.
+ std::move(const_cast<cql3::query_options&>(options).take_cached_function_calls())));
}
}

diff --git a/transport/messages/result_message.hh b/transport/messages/result_message.hh
index 0ba03d68ce..32d1d095e4 100644
--- a/transport/messages/result_message.hh
+++ b/transport/messages/result_message.hh
@@ -25,6 +25,7 @@
#include "cql3/result_set.hh"
#include "cql3/statements/prepared_statement.hh"
#include "cql3/cql_statement.hh"
+#include "cql3/query_options.hh"

#include "transport/messages/result_message_base.hh"
#include "transport/event.hh"
@@ -107,8 +108,11 @@ std::ostream& operator<<(std::ostream& os, const result_message::void_message& m
// it is a sure sign of a error.
class result_message::bounce_to_shard : public result_message {
unsigned _shard;
+ cql3::computed_function_values _cached_fn_calls;
public:
- bounce_to_shard(unsigned shard) : _shard(shard) {}
+ bounce_to_shard(unsigned shard, cql3::computed_function_values cached_fn_calls)
+ : _shard(shard), _cached_fn_calls(std::move(cached_fn_calls))
+ {}
virtual void accept(result_message::visitor& v) const override {
v.visit(*this);
}
@@ -116,6 +120,9 @@ class result_message::bounce_to_shard : public result_message {
return _shard;
}

+ cql3::computed_function_values&& take_cached_function_calls() {
+ return std::move(_cached_fn_calls);
+ }
};

std::ostream& operator<<(std::ostream& os, const result_message::bounce_to_shard& msg);
diff --git a/transport/server.cc b/transport/server.cc
index 8ef9ff50d8..613dc8bf49 100644
--- a/transport/server.cc
+++ b/transport/server.cc
@@ -853,18 +853,21 @@ make_result(int16_t stream, messages::result_message& msg, const tracing::trace_

template<typename Process>
future<foreign_ptr<std::unique_ptr<cql_server::response>>>
-cql_server::connection::process_on_shard(unsigned shard, uint16_t stream, fragmented_temporary_buffer::istream is,
+cql_server::connection::process_on_shard(::shared_ptr<messages::result_message::bounce_to_shard> bounce_msg, uint16_t stream, fragmented_temporary_buffer::istream is,
service::client_state& cs, service_permit permit, tracing::trace_state_ptr trace_state, Process process_fn) {
- return _server.container().invoke_on(shard, _server._config.bounce_request_smp_service_group,
+ return _server.container().invoke_on(*bounce_msg->move_to_shard(), _server._config.bounce_request_smp_service_group,
[this, is = std::move(is), cs = cs.move_to_other_shard(), stream, permit = std::move(permit), process_fn,
- gt = tracing::global_trace_state_ptr(std::move(trace_state))] (cql_server& server) {
+ gt = tracing::global_trace_state_ptr(std::move(trace_state)),
+ cached_vals = std::move(bounce_msg->take_cached_function_calls())] (cql_server& server) {
service::client_state client_state = cs.get();
- return do_with(bytes_ostream(), std::move(client_state), [this, &server, is = std::move(is), stream, process_fn,
- trace_state = tracing::trace_state_ptr(gt)]
- (bytes_ostream& linearization_buffer, service::client_state& client_state) mutable {
+ return do_with(bytes_ostream(), std::move(client_state), std::move(cached_vals),
+ [this, &server, is = std::move(is), stream, process_fn,
+ trace_state = tracing::trace_state_ptr(gt)] (bytes_ostream& linearization_buffer,
+ service::client_state& client_state,
+ cql3::computed_function_values& cached_vals) mutable {
request_reader in(is, linearization_buffer);
return process_fn(client_state, server._query_processor, in, stream, _version, _cql_serialization_format,
- /* FIXME */empty_service_permit(), std::move(trace_state), false).then([] (auto msg) {
+ /* FIXME */empty_service_permit(), std::move(trace_state), false, std::move(cached_vals)).then([] (auto msg) {
// result here has to be foreign ptr
return std::get<foreign_ptr<std::unique_ptr<cql_server::response>>>(std::move(msg));
});
@@ -872,6 +875,10 @@ cql_server::connection::process_on_shard(unsigned shard, uint16_t stream, fragme
});
}

+using process_fn_return_type = std::variant<
+ foreign_ptr<std::unique_ptr<cql_server::response>>,
+ ::shared_ptr<messages::result_message::bounce_to_shard>>;
+
template<typename Process>
future<foreign_ptr<std::unique_ptr<cql_server::response>>>
cql_server::connection::process(uint16_t stream, request_reader in, service::client_state& client_state, service_permit permit,
@@ -879,26 +886,29 @@ cql_server::connection::process(uint16_t stream, request_reader in, service::cli
fragmented_temporary_buffer::istream is = in.get_stream();

return process_fn(client_state, _server._query_processor, in, stream,
- _version, _cql_serialization_format, permit, trace_state, true)
+ _version, _cql_serialization_format, permit, trace_state, true, {})
.then([stream, &client_state, this, is, permit, process_fn, trace_state]
- (std::variant<foreign_ptr<std::unique_ptr<cql_server::response>>, unsigned> msg) mutable {
- unsigned* shard = std::get_if<unsigned>(&msg);
- if (shard) {
- return process_on_shard(*shard, stream, is, client_state, std::move(permit), trace_state, process_fn);
+ (process_fn_return_type msg) mutable {
+ auto* bounce_msg = std::get_if<shared_ptr<messages::result_message::bounce_to_shard>>(&msg);
+ if (bounce_msg) {
+ return process_on_shard(*bounce_msg, stream, is, client_state, std::move(permit), trace_state, process_fn);
}
return make_ready_future<foreign_ptr<std::unique_ptr<cql_server::response>>>(std::get<foreign_ptr<std::unique_ptr<cql_server::response>>>(std::move(msg)));
});
}

-static future<std::variant<foreign_ptr<std::unique_ptr<cql_server::response>>, unsigned>>
+static future<process_fn_return_type>
process_query_internal(service::client_state& client_state, distributed<cql3::query_processor>& qp, request_reader in,
uint16_t stream, cql_protocol_version_type version, cql_serialization_format serialization_format,
- service_permit permit, tracing::trace_state_ptr trace_state, bool init_trace) {
+ service_permit permit, tracing::trace_state_ptr trace_state, bool init_trace, cql3::computed_function_values cached_fn_calls) {
auto query = in.read_long_string_view();
auto q_state = std::make_unique<cql_query_state>(client_state, trace_state, std::move(permit));
auto& query_state = q_state->query_state;
q_state->options = in.read_options(version, serialization_format, qp.local().get_cql_config());
auto& options = *q_state->options;
+ if (!cached_fn_calls.empty()) {
+ options.set_cached_function_calls(std::move(cached_fn_calls));
+ }
auto skip_metadata = options.skip_metadata();

if (init_trace) {
@@ -913,10 +923,10 @@ process_query_internal(service::client_state& client_state, distributed<cql3::qu

return qp.local().execute_direct(query, query_state, options).then([q_state = std::move(q_state), stream, skip_metadata, version] (auto msg) {
if (msg->move_to_shard()) {
- return std::variant<foreign_ptr<std::unique_ptr<cql_server::response>>, unsigned>(*msg->move_to_shard());
+ return process_fn_return_type(dynamic_pointer_cast<messages::result_message::bounce_to_shard>(msg));
} else {
tracing::trace(q_state->query_state.get_trace_state(), "Done processing - preparing a result");
- return std::variant<foreign_ptr<std::unique_ptr<cql_server::response>>, unsigned>(make_foreign(make_result(stream, *msg, q_state->query_state.get_trace_state(), version, skip_metadata)));
+ return process_fn_return_type(make_foreign(make_result(stream, *msg, q_state->query_state.get_trace_state(), version, skip_metadata)));
}
});
}
@@ -957,10 +967,10 @@ future<std::unique_ptr<cql_server::response>> cql_server::connection::process_pr
});
}

-static future<std::variant<foreign_ptr<std::unique_ptr<cql_server::response>>, unsigned>>
+static future<process_fn_return_type>
process_execute_internal(service::client_state& client_state, distributed<cql3::query_processor>& qp, request_reader in,
uint16_t stream, cql_protocol_version_type version, cql_serialization_format serialization_format,
- service_permit permit, tracing::trace_state_ptr trace_state, bool init_trace) {
+ service_permit permit, tracing::trace_state_ptr trace_state, bool init_trace, cql3::computed_function_values cached_fn_calls) {
cql3::prepared_cache_key_type cache_key(in.read_short_bytes());
auto& id = cql3::prepared_cache_key_type::cql_id(cache_key);
bool needs_authorization = false;
@@ -989,6 +999,9 @@ process_execute_internal(service::client_state& client_state, distributed<cql3::
q_state->options = in.read_options(version, serialization_format, qp.local().get_cql_config());
}
auto& options = *q_state->options;
+ if (!cached_fn_calls.empty()) {
+ options.set_cached_function_calls(std::move(cached_fn_calls));
+ }
auto skip_metadata = options.skip_metadata();

if (init_trace) {
@@ -1022,10 +1035,10 @@ process_execute_internal(service::client_state& client_state, distributed<cql3::
return qp.local().execute_prepared(std::move(prepared), std::move(cache_key), query_state, options, needs_authorization)
.then([trace_state = query_state.get_trace_state(), skip_metadata, q_state = std::move(q_state), stream, version] (auto msg) {
if (msg->move_to_shard()) {
- return std::variant<foreign_ptr<std::unique_ptr<cql_server::response>>, unsigned>(*msg->move_to_shard());
+ return process_fn_return_type(dynamic_pointer_cast<messages::result_message::bounce_to_shard>(msg));
} else {
tracing::trace(q_state->query_state.get_trace_state(), "Done processing - preparing a result");
- return std::variant<foreign_ptr<std::unique_ptr<cql_server::response>>, unsigned>(make_foreign(make_result(stream, *msg, q_state->query_state.get_trace_state(), version, skip_metadata)));
+ return process_fn_return_type(make_foreign(make_result(stream, *msg, q_state->query_state.get_trace_state(), version, skip_metadata)));
}
});
}
@@ -1036,10 +1049,10 @@ future<foreign_ptr<std::unique_ptr<cql_server::response>>> cql_server::connectio
return process(stream, in, client_state, std::move(permit), std::move(trace_state), process_execute_internal);
}

-static future<std::variant<foreign_ptr<std::unique_ptr<cql_server::response>>, unsigned>>
+static future<process_fn_return_type>
process_batch_internal(service::client_state& client_state, distributed<cql3::query_processor>& qp, request_reader in,
uint16_t stream, cql_protocol_version_type version, cql_serialization_format serialization_format,
- service_permit permit, tracing::trace_state_ptr trace_state, bool init_trace) {
+ service_permit permit, tracing::trace_state_ptr trace_state, bool init_trace, cql3::computed_function_values cached_fn_calls) {
if (version == 1) {
throw exceptions::protocol_exception("BATCH messages are not support in version 1 of the protocol");
}
@@ -1130,6 +1143,9 @@ process_batch_internal(service::client_state& client_state, distributed<cql3::qu
q_state->options = std::make_unique<cql3::query_options>(cql3::query_options::make_batch_options(std::move(*in.read_options(version < 3 ? 1 : version, serialization_format,
qp.local().get_cql_config())), std::move(values)));
auto& options = *q_state->options;
+ if (!cached_fn_calls.empty()) {
+ options.set_cached_function_calls(std::move(cached_fn_calls));
+ }

if (init_trace) {
tracing::set_consistency_level(trace_state, options.get_consistency());
@@ -1142,10 +1158,10 @@ process_batch_internal(service::client_state& client_state, distributed<cql3::qu
return qp.local().execute_batch(batch, query_state, options, std::move(pending_authorization_entries))
.then([stream, batch, q_state = std::move(q_state), trace_state = query_state.get_trace_state(), version] (auto msg) {
if (msg->move_to_shard()) {
- return std::variant<foreign_ptr<std::unique_ptr<cql_server::response>>, unsigned>(*msg->move_to_shard());
+ return process_fn_return_type(dynamic_pointer_cast<messages::result_message::bounce_to_shard>(msg));
} else {
tracing::trace(q_state->query_state.get_trace_state(), "Done processing - preparing a result");
- return std::variant<foreign_ptr<std::unique_ptr<cql_server::response>>, unsigned>(make_foreign(make_result(stream, *msg, trace_state, version)));
+ return process_fn_return_type(make_foreign(make_result(stream, *msg, trace_state, version)));
}
});
}
diff --git a/transport/server.hh b/transport/server.hh
index dc22b23f9f..da7d71e1b3 100644
--- a/transport/server.hh
+++ b/transport/server.hh
@@ -258,7 +258,7 @@ class cql_server : public seastar::peering_sharded_service<cql_server>, public g
Process process_fn);
template<typename Process>
future<foreign_ptr<std::unique_ptr<cql_server::response>>>
- process_on_shard(unsigned shard, uint16_t stream, fragmented_temporary_buffer::istream is, service::client_state& cs,
+ process_on_shard(::shared_ptr<messages::result_message::bounce_to_shard> bounce_msg, uint16_t stream, fragmented_temporary_buffer::istream is, service::client_state& cs,
service_permit permit, tracing::trace_state_ptr trace_state, Process process_fn);

void write_response(foreign_ptr<std::unique_ptr<cql_server::response>>&& response, service_permit permit = empty_service_permit(), cql_compression compression = cql_compression::none);
--
2.31.1

Pavel Solodovnikov

unread,
Jul 21, 2021, 5:42:44 AMJul 21
to scylla...@googlegroups.com, Pavel Solodovnikov
Introduce a test using `cql-pytest` framework to assert that
both prepared an unprepared LWT statements (insert with
`IF NOT EXISTS`) with a non-deterministic function call
work correctly in case its evaluation affects partition
key range computation (hence the choice of `cas_shard()`
for lwt query).

Tests: cql-pytest/test_non_deterministic_functions.py

Signed-off-by: Pavel Solodovnikov <pa.solo...@scylladb.com>
---
.../test_non_deterministic_functions.py | 51 +++++++++++++++++++
1 file changed, 51 insertions(+)
create mode 100644 test/cql-pytest/test_non_deterministic_functions.py

diff --git a/test/cql-pytest/test_non_deterministic_functions.py b/test/cql-pytest/test_non_deterministic_functions.py
new file mode 100644
index 0000000000..37fa2c5288
--- /dev/null
+++ b/test/cql-pytest/test_non_deterministic_functions.py
@@ -0,0 +1,51 @@
+# Copyright 2021-present ScyllaDB
+#
+# This file is part of Scylla.
+#
+# Scylla is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Scylla is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with Scylla. If not, see <http://www.gnu.org/licenses/>.
+
+from util import new_test_table
+
+def test_lwt_uuid_fn_pk_insert(cql, test_keyspace):
+ '''Non-deterministic CQL functions should be evaluated just before query execution.
+ Check that these functions (on the example of `uuid()`) work consistently both
+ for un-prepared and prepared statements for LWT queries (e.g. execution order
+ is correct and `bounce_to_shard` messages forward the same value for the execution
+ on another shard).
+ '''
+ with new_test_table(cql, test_keyspace, "pk uuid PRIMARY KEY") as table:
+ # Test that both unprepared and prepared statements work exactly the
+ # same way as described above.
+ # The number of inserted rows should be exactly the same as the number
+ # of query executions to show that each time `uuid()` call yields
+ # a different value.
+ # Insert statements should not crash and handle `bounce_to_shard`
+ # cross-shard request correctly by forwarding cached results
+ # from the initial shard to the target (until #8604 is fixed, the
+ # test would crash).
+
+ insert_str = f"INSERT INTO {table} (pk) VALUES (uuid()) IF NOT EXISTS"
+ select_str = f"SELECT * FROM {table}"
+ num_iterations = 10
+
+ for i in range(num_iterations):
+ cql.execute(insert_str)
+ rows = list(cql.execute(select_str))
+ assert len(rows) == num_iterations
+
+ insert_stmt = cql.prepare(insert_str)
+ for i in range(num_iterations):
+ cql.execute(insert_stmt, [])
+ rows = list(cql.execute(select_str))
+ assert len(rows) == num_iterations * 2
\ No newline at end of file
--
2.31.1

Pavel Solodovnikov

unread,
Jul 21, 2021, 6:01:19 AMJul 21
to Avi Kivity, scylladb-dev
Avi, I've decided to proceed further with the initial way to fix it
(storing and passing around computed results from one shard to
another).

Given another thought on your proposed approach (make function
deterministic instead and store random state in query_options), it has
some more drawbacks (at least as I see it):
* `uuid()` uses `make_random_uuid()` which has a function-static
random engine. In order to save or restore the state, we would need to
expose it. I don't quite like the idea.
* std interface allows to save the random engine and distribution
state, but for interface uniformity reasons it's serialized as string.
It would take more space than a uuid value, for sure.
* We would still need to carry around a vector of random engine states
in the `query_options`, because we still use the same random generator
from `make_random_uuid()`. And
there are a ton of other users which constantly keep using it,
modifying the engine state in a non-deterministic manner.
Even if we use a random generator for `uuid()` exclusively, it still
cannot be assumed that no other query modifies the state in-between.
And constructing a random gen for each query
should be quite expensive, so we don't want to do that either.
Hence, every execution would need to carry the random state which
was used to compute the function value.

So neither it's simpler, nor it's more efficient to do it this way.

Konstantin Osipov

unread,
Jul 22, 2021, 9:12:15 AMJul 22
to Pavel Solodovnikov, sa...@scylladb.com, de...@scylladb.com, scylla...@googlegroups.com
* Pavel Solodovnikov <pa.solo...@scylladb.com> [21/07/21 12:44]:
> The class is repurposed to be more generic and also be able
> to hold additional metadata related to function calls within
> a CQL statement. Rename all methods appropriately.
>
> Visitor functions in AST nodes (`collect_marker_specification`)
> are also renamed to a more generic `collect_prepare_metadata`.
>
> The name `raw_prepare_metadata` designates that this metadata
> structure is a byproduct of `stmt::raw::prepare()` call and
> is needed only for "prepare" step of query execution.

I got curious because I haven't seen such a class name before.
I understand how the name got created, but I found it confusing,
and the closest thing I found was class Name_resolution_context in
MySQL/MariaDB code base.

Dejan, Piotr, do you find the new name good? Do you have good
suggestions for the name?

Other options that came to my mind are bind_context, bind_specification,
bind_description, bind_metadata.

The reason I think bind is a good part of this name is that this
class in continues to store information about the values we're
going to bind to fields when the statement is executed.

raw_prepare_metadata is ambiguous because it may mean both
metadata collected during raw prepare and metadata needed for raw
prepare.
Konstantin Osipov, Moscow, Russia
https://scylladb.com

Konstantin Osipov

unread,
Jul 22, 2021, 9:20:33 AMJul 22
to Pavel Solodovnikov, scylla...@googlegroups.com
* Pavel Solodovnikov <pa.solo...@scylladb.com> [21/07/21 12:44]:
> + // This flag is set to `true` if the `function_call` AST element
> + // is a part of LWT statement.

Aren't we requiring the caching of non-pure function execution
results only if the function output is used as a partition key of
the LWT data modification statement? Would it be possible to make
all - the name, the comment and the usage - as specific as that?
Sorry if I missed some review discussion, I suggest this is only
set for partition key inputs.

> + //
> + // This is important since these calls can possibly affect partition key
> + // ranges computation. For such cases we need to forward the computed
> + // execution result of the function when redirecting the query execution to
> + // another shard. Otherwise, it's possible that we end up bouncing
> + // indefinitely between various shards when evaluating a non-deterministic
> + // function each time on each shard.
> + //
> + // Set the flags after the main prepare procedure has already finished,
> + // by that time we've already collected the necessary metadata.
> + if (prepared_stmt->has_conditions()) {
> + for (auto& fn : meta.function_calls()) {
> + fn->set_lwt_context();
> + }
> + }
> + return prepared_stmt;
> }
>

Piotr Sarna

unread,
Jul 22, 2021, 9:31:01 AMJul 22
to Konstantin Osipov, Pavel Solodovnikov, de...@scylladb.com, scylla...@googlegroups.com
On 7/22/21 3:12 PM, Konstantin Osipov wrote:
> * Pavel Solodovnikov <pa.solo...@scylladb.com> [21/07/21 12:44]:
>> The class is repurposed to be more generic and also be able
>> to hold additional metadata related to function calls within
>> a CQL statement. Rename all methods appropriately.
>>
>> Visitor functions in AST nodes (`collect_marker_specification`)
>> are also renamed to a more generic `collect_prepare_metadata`.
>>
>> The name `raw_prepare_metadata` designates that this metadata
>> structure is a byproduct of `stmt::raw::prepare()` call and
>> is needed only for "prepare" step of query execution.
> I got curious because I haven't seen such a class name before.
> I understand how the name got created, but I found it confusing,
> and the closest thing I found was class Name_resolution_context in
> MySQL/MariaDB code base.
>
> Dejan, Piotr, do you find the new name good? Do you have good
> suggestions for the name?
>
> Other options that came to my mind are bind_context, bind_specification,
> bind_description, bind_metadata.
bind_specification sounds good. The visitor function also needs a new
name in this case, but collect_bind_specification is also quite a
descriptive name.

Pavel Solodovnikov

unread,
Jul 22, 2021, 9:52:33 AMJul 22
to Piotr Sarna, Konstantin Osipov, Dejan Mircevski, scylladb-dev
On Thu, Jul 22, 2021 at 4:31 PM Piotr Sarna <sa...@scylladb.com> wrote:
>
> On 7/22/21 3:12 PM, Konstantin Osipov wrote:
> > * Pavel Solodovnikov <pa.solo...@scylladb.com> [21/07/21 12:44]:
> >> The class is repurposed to be more generic and also be able
> >> to hold additional metadata related to function calls within
> >> a CQL statement. Rename all methods appropriately.
> >>
> >> Visitor functions in AST nodes (`collect_marker_specification`)
> >> are also renamed to a more generic `collect_prepare_metadata`.
> >>
> >> The name `raw_prepare_metadata` designates that this metadata
> >> structure is a byproduct of `stmt::raw::prepare()` call and
> >> is needed only for "prepare" step of query execution.
> > I got curious because I haven't seen such a class name before.
> > I understand how the name got created, but I found it confusing,
> > and the closest thing I found was class Name_resolution_context in
> > MySQL/MariaDB code base.
> >
> > Dejan, Piotr, do you find the new name good? Do you have good
> > suggestions for the name?
> >
> > Other options that came to my mind are bind_context, bind_specification,
> > bind_description, bind_metadata.
> bind_specification sounds good. The visitor function also needs a new
> name in this case, but collect_bind_specification is also quite a
> descriptive name.
Sounds good enough for me, though I would personally prefer something
like `prepare_context` to remind the reader that this exists only for
the duration of the `prepare()` call. And I also don't like very much
using the `bind` word, because who knows what could be added to this
structure in the future, and this doesn't need to "bind" smth1 to
smth2 at `execute()` step.

Pavel Solodovnikov

unread,
Jul 22, 2021, 9:55:00 AMJul 22
to Konstantin Osipov, Pavel Solodovnikov, scylladb-dev
On Thu, Jul 22, 2021 at 4:20 PM Konstantin Osipov <kos...@scylladb.com> wrote:
>
> * Pavel Solodovnikov <pa.solo...@scylladb.com> [21/07/21 12:44]:
> > + // This flag is set to `true` if the `function_call` AST element
> > + // is a part of LWT statement.
>
> Aren't we requiring the caching of non-pure function execution
> results only if the function output is used as a partition key of
> the LWT data modification statement? Would it be possible to make
> all - the name, the comment and the usage - as specific as that?
No, we aren't. But the execution order of functions is such that
partition key constraints will be evaluated first. And if we
understand that a shard hop is needed, then other functions will not
be executed, hence not cached.
Nonetheless, it would be good to state more explicitly how the flag works.
This got me to think about another problem: I don't check if it's a
repeated query execution or not. The second execution will
unnecessarily cache the values one more time. This has to be fixed.

Piotr Sarna

unread,
Jul 22, 2021, 9:57:43 AMJul 22
to Pavel Solodovnikov, Konstantin Osipov, Dejan Mircevski, scylladb-dev
prepare_context is just as fine for me.

Konstantin Osipov

unread,
Jul 22, 2021, 10:04:16 AMJul 22
to Pavel Solodovnikov, Piotr Sarna, Dejan Mircevski, scylladb-dev
* Pavel Solodovnikov <pa.solo...@scylladb.com> [21/07/22 16:55]:
> > >
> > > Dejan, Piotr, do you find the new name good? Do you have good
> > > suggestions for the name?
> > >
> > > Other options that came to my mind are bind_context, bind_specification,
> > > bind_description, bind_metadata.
> > bind_specification sounds good. The visitor function also needs a new
> > name in this case, but collect_bind_specification is also quite a
> > descriptive name.
> Sounds good enough for me, though I would personally prefer something
> like `prepare_context` to remind the reader that this exists only for
> the duration of the `prepare()` call. And I also don't like very much
> using the `bind` word, because who knows what could be added to this
> structure in the future, and this doesn't need to "bind" smth1 to
> smth2 at `execute()` step.

OK, prepare_context is also good for me.

Pavel Solodovnikov

unread,
Jul 26, 2021, 8:31:07 AMJul 26
to scylla...@googlegroups.com, Pavel Solodovnikov
`function_call` AST nodes are created for each function
with side effects in a CQL query, i.e. non-deterministic
functions (`uuid()`, `now()` and some others timeuuid-related).

These nodes are evaluated either when a query itself is executed
or query restrictions are computed (e.g. partition/clustering
key ranges for LWT requests).

We need to cache the calls since otherwise when handling a
`bounce_to_shard` request for an LWT query, we can possibly
enter an infinite bouncing loop (in case a function is used
to calculate partition key ranges for a query), since the
results can be different each time.

Furthermore, we don't support bouncing more than one time.
Returning `bounce_to_shard` message more than one time
will result in a crash.

Caching works only for LWT statements and only for the function
calls that affect partition key range computation for the query.

`variable_specifications` class is renamed to `prepare_context`
and generalized to record information about each `function_call`
AST node and modify them, as needed:
* Set a special flag denoting that a function call is a part
of LWT statement.
* Check whether a given function call is a part of partition key
statement restriction.
* Assign ids for caching if above is true.

There is no need to include any kind of statement identifier
in the cache key since `query_options` (which holds the cache)
Git URL: https://github.com/ManManson/scylla/commits/lwt_bounce_to_shard_cached_fn_v5

Changelog:

v5:
* Rename `raw_prepare_metadata` to `prepare_context` per review
comments. Also, rename `term::collect_prepare_metadata` to
`term::fill_prepare_context`.
* Cache only function calls that affect partition keys for an LWT
statement.
* Clarify in the comments that caching is only enabled for the calls
which are a part of partition key statement restrictions.

Note: I decided to not handle the case of bounced execution (i.e.
distinguish between the first and the bounced query execution and don't
cache in the second case), because IMO it's not worth the effort, given
that caching is restricted to include only partition keys computation.
to the `function_call::bind_and_get()` to populate the cache only
for LWT statements.
* Fixed access functions in `query_options` to be able to move
the cache info out of the instance.
* Do not copy cache from individual sub-statements for batch statement,
instead move them, as well.
* Explicitly check for non-pure functions in
`function_call::bind_and_get()` since `function_call` nodes can
represent not only non-pure functions.
* Fix some missing std::move's when constucting a `bounce_to_shard`
message.
* Add some more comments.
* Rename `test_cql_non_deterministic_functions_lwt.py` to
just `test_non_deterministic_functions.py`.

Pavel Solodovnikov (3):
cql3: rename `variable_specifications` to `prepare_context`
cql3: cache function calls evaluation for non-deterministic functions
cql-pytest: add a test for non-pure CQL functions

CMakeLists.txt | 2 +-
configure.py | 2 +-
cql3/abstract_marker.cc | 6 +-
cql3/abstract_marker.hh | 4 +-
cql3/attributes.cc | 8 +--
cql3/attributes.hh | 4 +-
cql3/column_condition.cc | 8 +--
cql3/column_condition.hh | 2 +-
cql3/functions/function_call.hh | 21 +++++-
cql3/functions/functions.cc | 40 +++++++++++-
cql3/lists.cc | 8 +--
cql3/lists.hh | 4 +-
cql3/maps.cc | 8 +--
cql3/maps.hh | 4 +-
cql3/multi_column_relation.hh | 22 +++----
cql3/operation.hh | 6 +-
...e_specifications.cc => prepare_context.cc} | 28 ++++----
...e_specifications.hh => prepare_context.hh} | 52 +++++++++++----
cql3/query_options.cc | 16 +++++
cql3/query_options.hh | 29 +++++++++
cql3/relation.hh | 44 ++++++-------
cql3/restrictions/statement_restrictions.cc | 4 +-
cql3/restrictions/statement_restrictions.hh | 4 +-
cql3/sets.cc | 2 +-
cql3/sets.hh | 2 +-
cql3/single_column_relation.cc | 32 ++++++----
cql3/single_column_relation.hh | 16 ++---
cql3/statements/batch_statement.cc | 18 ++++--
cql3/statements/create_view_statement.cc | 2 +-
cql3/statements/delete_statement.cc | 10 +--
cql3/statements/modification_statement.cc | 48 ++++++++++----
cql3/statements/modification_statement.hh | 2 +-
cql3/statements/prepared_statement.hh | 6 +-
cql3/statements/raw/delete_statement.hh | 2 +-
cql3/statements/raw/insert_statement.hh | 4 +-
cql3/statements/raw/modification_statement.hh | 6 +-
cql3/statements/raw/parsed_statement.cc | 18 +++---
cql3/statements/raw/parsed_statement.hh | 8 +--
cql3/statements/raw/select_statement.hh | 4 +-
cql3/statements/raw/update_statement.hh | 2 +-
cql3/statements/select_statement.cc | 35 +++++-----
cql3/statements/update_statement.cc | 28 ++++----
cql3/term.hh | 6 +-
cql3/token_relation.cc | 18 +++---
cql3/token_relation.hh | 12 ++--
cql3/tuples.hh | 4 +-
cql3/user_types.cc | 4 +-
cql3/user_types.hh | 2 +-
test/boost/statement_restrictions_test.cc | 4 +-
.../test_non_deterministic_functions.py | 51 +++++++++++++++
transport/messages/result_message.hh | 9 ++-
transport/server.cc | 64 ++++++++++++-------
transport/server.hh | 2 +-
53 files changed, 494 insertions(+), 253 deletions(-)
rename cql3/{variable_specifications.cc => prepare_context.cc} (74%)
rename cql3/{variable_specifications.hh => prepare_context.hh} (52%)

Pavel Solodovnikov

unread,
Jul 26, 2021, 8:31:09 AMJul 26
to scylla...@googlegroups.com, Pavel Solodovnikov
The class is repurposed to be more generic and also be able
to hold additional metadata related to function calls within
a CQL statement. Rename all methods appropriately.

Visitor functions in AST nodes (`collect_marker_specification`)
are also renamed to a more generic `fill_prepare_context`.

The name `prepare_context` designates that this metadata
structure is a byproduct of `stmt::raw::prepare()` call and
is needed only for "prepare" step of query execution.

Signed-off-by: Pavel Solodovnikov <pa.solo...@scylladb.com>
---
CMakeLists.txt | 2 +-
configure.py | 2 +-
cql3/abstract_marker.cc | 6 +--
cql3/abstract_marker.hh | 4 +-
cql3/attributes.cc | 8 ++--
cql3/attributes.hh | 4 +-
cql3/column_condition.cc | 8 ++--
cql3/column_condition.hh | 2 +-
cql3/functions/function_call.hh | 2 +-
cql3/functions/functions.cc | 4 +-
cql3/lists.cc | 8 ++--
cql3/lists.hh | 4 +-
cql3/maps.cc | 8 ++--
cql3/maps.hh | 4 +-
cql3/multi_column_relation.hh | 22 +++++-----
cql3/operation.hh | 6 +--
...e_specifications.cc => prepare_context.cc} | 24 +++++-----
...e_specifications.hh => prepare_context.hh} | 26 +++++------
cql3/relation.hh | 44 +++++++++----------
cql3/restrictions/statement_restrictions.cc | 4 +-
cql3/restrictions/statement_restrictions.hh | 4 +-
cql3/sets.cc | 2 +-
cql3/sets.hh | 2 +-
cql3/single_column_relation.cc | 22 +++++-----
cql3/single_column_relation.hh | 16 +++----
cql3/statements/batch_statement.cc | 12 ++---
cql3/statements/create_view_statement.cc | 2 +-
cql3/statements/delete_statement.cc | 10 ++---
cql3/statements/modification_statement.cc | 22 +++++-----
cql3/statements/modification_statement.hh | 2 +-
cql3/statements/prepared_statement.hh | 6 +--
cql3/statements/raw/delete_statement.hh | 2 +-
cql3/statements/raw/insert_statement.hh | 4 +-
cql3/statements/raw/modification_statement.hh | 6 +--
cql3/statements/raw/parsed_statement.cc | 18 ++++----
cql3/statements/raw/parsed_statement.hh | 8 ++--
cql3/statements/raw/select_statement.hh | 4 +-
cql3/statements/raw/update_statement.hh | 2 +-
cql3/statements/select_statement.cc | 30 ++++++-------
cql3/statements/update_statement.cc | 28 ++++++------
cql3/term.hh | 6 +--
cql3/token_relation.cc | 18 ++++----
cql3/token_relation.hh | 12 ++---
cql3/tuples.hh | 4 +-
cql3/user_types.cc | 4 +-
cql3/user_types.hh | 2 +-
test/boost/statement_restrictions_test.cc | 4 +-
47 files changed, 222 insertions(+), 222 deletions(-)
rename cql3/{variable_specifications.cc => prepare_context.cc} (75%)
rename cql3/{variable_specifications.hh => prepare_context.hh} (75%)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2459eac7f0..55573f174d 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -410,7 +410,7 @@ set(scylla_sources
cql3/ut_name.cc
cql3/util.cc
cql3/values.cc
- cql3/variable_specifications.cc
+ cql3/prepare_context.cc
data/cell.cc
database.cc
db/batchlog_manager.cc
diff --git a/configure.py b/configure.py
index aa973a3e60..3ee1d2a4e5 100755
--- a/configure.py
+++ b/configure.py
@@ -843,7 +843,7 @@ scylla_core = (['database.cc',
'cql3/selection/selector.cc',
'cql3/restrictions/statement_restrictions.cc',
'cql3/result_set.cc',
- 'cql3/variable_specifications.cc',
+ 'cql3/prepare_context.cc',
'db/consistency_level.cc',
'db/system_keyspace.cc',
'db/virtual_table.cc',
diff --git a/cql3/abstract_marker.cc b/cql3/abstract_marker.cc
index fd20149049..a98145eafe 100644
--- a/cql3/abstract_marker.cc
+++ b/cql3/abstract_marker.cc
@@ -46,7 +46,7 @@
#include "cql3/maps.hh"
#include "cql3/sets.hh"
#include "cql3/user_types.hh"
-#include "cql3/variable_specifications.hh"
+#include "cql3/prepare_context.hh"
#include "types/list.hh"

namespace cql3 {
@@ -56,8 +56,8 @@ abstract_marker::abstract_marker(int32_t bind_index, lw_shared_ptr<column_specif
, _receiver{std::move(receiver)}
{ }

-void abstract_marker::collect_marker_specification(variable_specifications& bound_names) const {
- bound_names.add(_bind_index, _receiver);
+void abstract_marker::fill_prepare_context(prepare_context& ctx) const {
+ ctx.add_variable_specification(_bind_index, _receiver);
}

bool abstract_marker::contains_bind_marker() const {
diff --git a/cql3/abstract_marker.hh b/cql3/abstract_marker.hh
index 6dc883de8d..41e1d06651 100644
--- a/cql3/abstract_marker.hh
+++ b/cql3/abstract_marker.hh
@@ -46,7 +46,7 @@
namespace cql3 {

class column_specification;
-class variable_specifications;
+class prepare_context;

/**
* A single bind marker.
@@ -58,7 +58,7 @@ class abstract_marker : public non_terminal {
public:
abstract_marker(int32_t bind_index, lw_shared_ptr<column_specification>&& receiver);

- virtual void collect_marker_specification(variable_specifications& bound_names) const override;
+ virtual void fill_prepare_context(prepare_context& ctx) const override;

virtual bool contains_bind_marker() const override;

diff --git a/cql3/attributes.cc b/cql3/attributes.cc
index 13a7eef626..9c60cf5dfc 100644
--- a/cql3/attributes.cc
+++ b/cql3/attributes.cc
@@ -136,15 +136,15 @@ db::timeout_clock::duration attributes::get_timeout(const query_options& options
return std::chrono::duration_cast<db::timeout_clock::duration>(std::chrono::nanoseconds(duration.nanoseconds));
}

-void attributes::collect_marker_specification(variable_specifications& bound_names) const {
+void attributes::fill_prepare_context(prepare_context& ctx) const {
if (_timestamp) {
- _timestamp->collect_marker_specification(bound_names);
+ _timestamp->fill_prepare_context(ctx);
}
if (_time_to_live) {
- _time_to_live->collect_marker_specification(bound_names);
+ _time_to_live->fill_prepare_context(ctx);
}
if (_timeout) {
- _timeout->collect_marker_specification(bound_names);
+ _timeout->fill_prepare_context(ctx);
}
}

diff --git a/cql3/attributes.hh b/cql3/attributes.hh
index 386fe4427d..6fe28d9bc9 100644
--- a/cql3/attributes.hh
+++ b/cql3/attributes.hh
@@ -46,7 +46,7 @@
namespace cql3 {

class query_options;
-class variable_specifications;
+class prepare_context;

/**
* Utility class for the Parser to gather attributes for modification
@@ -74,7 +74,7 @@ class attributes final {

db::timeout_clock::duration get_timeout(const query_options& options) const;

- void collect_marker_specification(variable_specifications& bound_names) const;
+ void fill_prepare_context(prepare_context& ctx) const;

class raw final {
public:
diff --git a/cql3/column_condition.cc b/cql3/column_condition.cc
index c9875daca1..fdf03a985a 100644
--- a/cql3/column_condition.cc
+++ b/cql3/column_condition.cc
@@ -118,17 +118,17 @@ uint32_t read_and_check_list_index(const cql3::raw_value_view& key) {

namespace cql3 {

-void column_condition::collect_marker_specificaton(variable_specifications& bound_names) const {
+void column_condition::collect_marker_specificaton(prepare_context& ctx) const {
if (_collection_element) {
- _collection_element->collect_marker_specification(bound_names);
+ _collection_element->fill_prepare_context(ctx);
}
if (!_in_values.empty()) {
for (auto&& value : _in_values) {
- value->collect_marker_specification(bound_names);
+ value->fill_prepare_context(ctx);
}
}
if (_value) {
- _value->collect_marker_specification(bound_names);
+ _value->fill_prepare_context(ctx);
}
}

diff --git a/cql3/column_condition.hh b/cql3/column_condition.hh
index 68082571b8..d4f6a595bb 100644
--- a/cql3/column_condition.hh
+++ b/cql3/column_condition.hh
@@ -89,7 +89,7 @@ class column_condition final {
* @param boundNames the list of column specification where to collect the
* bind variables of this term in.
*/
- void collect_marker_specificaton(variable_specifications& bound_names) const;
+ void collect_marker_specificaton(prepare_context& ctx) const;

// Retrieve parameter marker values, if any, find the appropriate collection
// element if the cell is a collection and an element access is used in the expression,
diff --git a/cql3/functions/function_call.hh b/cql3/functions/function_call.hh
index d4c4fec9df..ba4485a72f 100644
--- a/cql3/functions/function_call.hh
+++ b/cql3/functions/function_call.hh
@@ -57,7 +57,7 @@ class function_call : public non_terminal {
function_call(shared_ptr<scalar_function> fun, std::vector<shared_ptr<term>> terms)
: _fun(std::move(fun)), _terms(std::move(terms)) {
}
- virtual void collect_marker_specification(variable_specifications& bound_names) const override;
+ virtual void fill_prepare_context(prepare_context& ctx) const override;
virtual shared_ptr<terminal> bind(const query_options& options) override;
virtual cql3::raw_value_view bind_and_get(const query_options& options) override;
private:
diff --git a/cql3/functions/functions.cc b/cql3/functions/functions.cc
index d855706912..ff0d7a2fa9 100644
--- a/cql3/functions/functions.cc
+++ b/cql3/functions/functions.cc
@@ -430,9 +430,9 @@ functions::type_equals(const std::vector<data_type>& t1, const std::vector<data_
}

void
-function_call::collect_marker_specification(variable_specifications& bound_names) const {
+function_call::fill_prepare_context(prepare_context& ctx) const {
for (auto&& t : _terms) {
- t->collect_marker_specification(bound_names);
+ t->fill_prepare_context(ctx);
}
}

diff --git a/cql3/lists.cc b/cql3/lists.cc
index 6a69da6cb1..13c30474ed 100644
--- a/cql3/lists.cc
+++ b/cql3/lists.cc
@@ -207,7 +207,7 @@ lists::delayed_value::contains_bind_marker() const {
}

void
-lists::delayed_value::collect_marker_specification(variable_specifications& bound_names) const {
+lists::delayed_value::fill_prepare_context(prepare_context& ctx) const {
}

shared_ptr<terminal>
@@ -275,9 +275,9 @@ lists::setter_by_index::requires_read() const {
}

void
-lists::setter_by_index::collect_marker_specification(variable_specifications& bound_names) const {
- operation::collect_marker_specification(bound_names);
- _idx->collect_marker_specification(bound_names);
+lists::setter_by_index::fill_prepare_context(prepare_context& ctx) const {
+ operation::fill_prepare_context(ctx);
+ _idx->fill_prepare_context(ctx);
}

void
diff --git a/cql3/lists.hh b/cql3/lists.hh
index c58a94ea7f..f7653145ea 100644
--- a/cql3/lists.hh
+++ b/cql3/lists.hh
@@ -104,7 +104,7 @@ class lists {
: _elements(std::move(elements)) {
}
virtual bool contains_bind_marker() const override;
- virtual void collect_marker_specification(variable_specifications& bound_names) const override;
+ virtual void fill_prepare_context(prepare_context& ctx) const override;
virtual shared_ptr<terminal> bind(const query_options& options) override;
const std::vector<shared_ptr<term>>& get_elements() const {
return _elements;
@@ -140,7 +140,7 @@ class lists {
: operation(column, std::move(t)), _idx(std::move(idx)) {
}
virtual bool requires_read() const override;
- virtual void collect_marker_specification(variable_specifications& bound_names) const;
+ virtual void fill_prepare_context(prepare_context& ctx) const;
virtual void execute(mutation& m, const clustering_key_prefix& prefix, const update_parameters& params) override;
};

diff --git a/cql3/maps.cc b/cql3/maps.cc
index ef28986a85..6c599b7ae4 100644
--- a/cql3/maps.cc
+++ b/cql3/maps.cc
@@ -228,7 +228,7 @@ maps::delayed_value::contains_bind_marker() const {
}

void
-maps::delayed_value::collect_marker_specification(variable_specifications& bound_names) const {
+maps::delayed_value::fill_prepare_context(prepare_context& ctx) const {
}

shared_ptr<terminal>
@@ -306,9 +306,9 @@ maps::setter::execute(mutation& m, const clustering_key_prefix& row_key, const u
}

void
-maps::setter_by_key::collect_marker_specification(variable_specifications& bound_names) const {
- operation::collect_marker_specification(bound_names);
- _k->collect_marker_specification(bound_names);
+maps::setter_by_key::fill_prepare_context(prepare_context& ctx) const {
+ operation::fill_prepare_context(ctx);
+ _k->fill_prepare_context(ctx);
}

void
diff --git a/cql3/maps.hh b/cql3/maps.hh
index 57b9a4ad91..07713c8004 100644
--- a/cql3/maps.hh
+++ b/cql3/maps.hh
@@ -98,7 +98,7 @@ class maps {
: _comparator(std::move(comparator)), _elements(std::move(elements)) {
}
virtual bool contains_bind_marker() const override;
- virtual void collect_marker_specification(variable_specifications& bound_names) const override;
+ virtual void fill_prepare_context(prepare_context& ctx) const override;
shared_ptr<terminal> bind(const query_options& options);
};

@@ -126,7 +126,7 @@ class maps {
setter_by_key(const column_definition& column, shared_ptr<term> k, shared_ptr<term> t)
: operation(column, std::move(t)), _k(std::move(k)) {
}
- virtual void collect_marker_specification(variable_specifications& bound_names) const override;
+ virtual void fill_prepare_context(prepare_context& ctx) const override;
virtual void execute(mutation& m, const clustering_key_prefix& prefix, const update_parameters& params) override;
};

diff --git a/cql3/multi_column_relation.hh b/cql3/multi_column_relation.hh
index 1df5be96a4..4228cdc889 100644
--- a/cql3/multi_column_relation.hh
+++ b/cql3/multi_column_relation.hh
@@ -160,31 +160,31 @@ class multi_column_relation final : public relation {

protected:
virtual shared_ptr<restrictions::restriction> new_EQ_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names) override {
+ prepare_context& ctx) override {
auto rs = receivers(db, *schema);
std::vector<lw_shared_ptr<column_specification>> col_specs(rs.size());
std::transform(rs.begin(), rs.end(), col_specs.begin(), [] (auto cs) {
return cs->column_specification;
});
- auto t = to_term(col_specs, *get_value(), db, schema->ks_name(), bound_names);
+ auto t = to_term(col_specs, *get_value(), db, schema->ks_name(), ctx);
return ::make_shared<restrictions::multi_column_restriction::EQ>(schema, rs, t);
}

virtual shared_ptr<restrictions::restriction> new_IN_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names) override {
+ prepare_context& ctx) override {
auto rs = receivers(db, *schema);
std::vector<lw_shared_ptr<column_specification>> col_specs(rs.size());
std::transform(rs.begin(), rs.end(), col_specs.begin(), [] (auto cs) {
return cs->column_specification;
});
if (_in_marker) {
- auto t = to_term(col_specs, *get_value(), db, schema->ks_name(), bound_names);
+ auto t = to_term(col_specs, *get_value(), db, schema->ks_name(), ctx);
auto as_abstract_marker = static_pointer_cast<abstract_marker>(t);
return ::make_shared<restrictions::multi_column_restriction::IN_with_marker>(schema, rs, as_abstract_marker);
} else {
std::vector<::shared_ptr<term::raw>> raws(_in_values.size());
std::copy(_in_values.begin(), _in_values.end(), raws.begin());
- auto ts = to_terms(col_specs, raws, db, schema->ks_name(), bound_names);
+ auto ts = to_terms(col_specs, raws, db, schema->ks_name(), ctx);
// Convert a single-item IN restriction to an EQ restriction
if (ts.size() == 1) {
return ::make_shared<restrictions::multi_column_restriction::EQ>(schema, rs, std::move(ts[0]));
@@ -194,24 +194,24 @@ class multi_column_relation final : public relation {
}

virtual shared_ptr<restrictions::restriction> new_slice_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names,
+ prepare_context& ctx,
statements::bound bound, bool inclusive) override {
auto rs = receivers(db, *schema);
std::vector<lw_shared_ptr<column_specification>> col_specs(rs.size());
std::transform(rs.begin(), rs.end(), col_specs.begin(), [] (auto cs) {
return cs->column_specification;
});
- auto t = to_term(col_specs, *get_value(), db, schema->ks_name(), bound_names);
+ auto t = to_term(col_specs, *get_value(), db, schema->ks_name(), ctx);
return ::make_shared<restrictions::multi_column_restriction::slice>(schema, rs, bound, inclusive, t, _mode);
}

virtual shared_ptr<restrictions::restriction> new_contains_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names, bool is_key) override {
+ prepare_context& ctx, bool is_key) override {
throw exceptions::invalid_request_exception(format("{} cannot be used for Multi-column relations", get_operator()));
}

virtual ::shared_ptr<restrictions::restriction> new_LIKE_restriction(
- database& db, schema_ptr schema, variable_specifications& bound_names) override {
+ database& db, schema_ptr schema, prepare_context& ctx) override {
throw exceptions::invalid_request_exception("LIKE cannot be used for Multi-column relations");
}

@@ -224,10 +224,10 @@ class multi_column_relation final : public relation {

virtual shared_ptr<term> to_term(const std::vector<lw_shared_ptr<column_specification>>& receivers,
const term::raw& raw, database& db, const sstring& keyspace,
- variable_specifications& bound_names) const override {
+ prepare_context& ctx) const override {
const auto& as_multi_column_raw = dynamic_cast<const term::multi_column_raw&>(raw);
auto t = as_multi_column_raw.prepare(db, keyspace, receivers);
- t->collect_marker_specification(bound_names);
+ t->fill_prepare_context(ctx);
return t;
}

diff --git a/cql3/operation.hh b/cql3/operation.hh
index 936b513dd7..d24eb5d2ca 100644
--- a/cql3/operation.hh
+++ b/cql3/operation.hh
@@ -103,12 +103,12 @@ class operation {
/**
* Collects the column specification for the bind variables of this operation.
*
- * @param bound_names the list of column specification where to collect the
+ * @param meta the list of column specification where to collect the
* bind variables of this term in.
*/
- virtual void collect_marker_specification(variable_specifications& bound_names) const {
+ virtual void fill_prepare_context(prepare_context& ctx) const {
if (_t) {
- _t->collect_marker_specification(bound_names);
+ _t->fill_prepare_context(ctx);
}
}

diff --git a/cql3/variable_specifications.cc b/cql3/prepare_context.cc
similarity index 75%
rename from cql3/variable_specifications.cc
rename to cql3/prepare_context.cc
index b8e21d1225..94c8b8778f 100644
--- a/cql3/variable_specifications.cc
+++ b/cql3/prepare_context.cc
@@ -39,33 +39,33 @@
* along with Scylla. If not, see <http://www.gnu.org/licenses/>.
*/

-#include "cql3/variable_specifications.hh"
+#include "cql3/prepare_context.hh"
#include "cql3/column_identifier.hh"
#include "cql3/column_specification.hh"

namespace cql3 {

-variable_specifications::variable_specifications(const std::vector<::shared_ptr<column_identifier>>& variable_names)
+prepare_context::prepare_context(const std::vector<::shared_ptr<column_identifier>>& variable_names)
: _variable_names{variable_names}
, _specs{variable_names.size()}
, _target_columns{variable_names.size()}
{ }
-lw_shared_ptr<variable_specifications> variable_specifications::empty() {
- return make_lw_shared<variable_specifications>(std::vector<::shared_ptr<column_identifier>>{});
+lw_shared_ptr<prepare_context> prepare_context::empty() {
+ return make_lw_shared<prepare_context>(std::vector<::shared_ptr<column_identifier>>{});
}
-size_t variable_specifications::size() const {
+size_t prepare_context::bound_variables_size() const {
return _variable_names.size();
}

-std::vector<lw_shared_ptr<column_specification>> variable_specifications::get_specifications() const & {
+std::vector<lw_shared_ptr<column_specification>> prepare_context::get_variable_specifications() const & {
return std::vector<lw_shared_ptr<column_specification>>(_specs.begin(), _specs.end());
}

-std::vector<lw_shared_ptr<column_specification>> variable_specifications::get_specifications() && {
+std::vector<lw_shared_ptr<column_specification>> prepare_context::get_variable_specifications() && {
return std::move(_specs);
}

-std::vector<uint16_t> variable_specifications::get_partition_key_bind_indexes(const schema& schema) const {
+std::vector<uint16_t> prepare_context::get_partition_key_bind_indexes(const schema& schema) const {
auto count = schema.partition_key_columns().size();
std::vector<uint16_t> partition_key_positions(count, uint16_t(0));
std::vector<bool> set(count, false);
@@ -85,7 +85,7 @@ std::vector<uint16_t> variable_specifications::get_partition_key_bind_indexes(co
return partition_key_positions;
}

-void variable_specifications::add(int32_t bind_index, lw_shared_ptr<column_specification> spec) {
+void prepare_context::add_variable_specification(int32_t bind_index, lw_shared_ptr<column_specification> spec) {
_target_columns[bind_index] = spec;
auto name = _variable_names[bind_index];
// Use the user name, if there is one
@@ -95,12 +95,12 @@ void variable_specifications::add(int32_t bind_index, lw_shared_ptr<column_speci
_specs[bind_index] = spec;
}

-void variable_specifications::set_bound_variables(const std::vector<shared_ptr<column_identifier>>& bound_names) {
- _variable_names = bound_names;
+void prepare_context::set_bound_variables(const std::vector<shared_ptr<column_identifier>>& prepare_meta) {
+ _variable_names = prepare_meta;
_specs.clear();
_target_columns.clear();

- const size_t bn_size = bound_names.size();
+ const size_t bn_size = prepare_meta.size();
_specs.resize(bn_size);
_target_columns.resize(bn_size);
}
diff --git a/cql3/variable_specifications.hh b/cql3/prepare_context.hh
similarity index 75%
rename from cql3/variable_specifications.hh
rename to cql3/prepare_context.hh
index e261851e1d..310bf99011 100644
--- a/cql3/variable_specifications.hh
+++ b/cql3/prepare_context.hh
@@ -55,7 +55,11 @@ namespace cql3 {
class column_identifier;
class column_specification;

-class variable_specifications final {
+/**
+ * A metadata class currently holding bind variables specifications and
+ * populated at "prepare" step of query execution.
+ */
+class prepare_context final {
private:
std::vector<shared_ptr<column_identifier>> _variable_names;
std::vector<lw_shared_ptr<column_specification>> _specs;
@@ -63,26 +67,22 @@ class variable_specifications final {

public:

- variable_specifications() = default;
- variable_specifications(const std::vector<::shared_ptr<column_identifier>>& variable_names);
+ prepare_context() = default;
+ prepare_context(const std::vector<::shared_ptr<column_identifier>>& variable_names);

- /**
- * Returns an empty instance of <code>VariableSpecifications</code>.
- * @return an empty instance of <code>VariableSpecifications</code>
- */
- static lw_shared_ptr<variable_specifications> empty();
+ static lw_shared_ptr<prepare_context> empty();

- size_t size() const;
+ size_t bound_variables_size() const;

- std::vector<lw_shared_ptr<column_specification>> get_specifications() const &;
+ std::vector<lw_shared_ptr<column_specification>> get_variable_specifications() const &;

- std::vector<lw_shared_ptr<column_specification>> get_specifications() &&;
+ std::vector<lw_shared_ptr<column_specification>> get_variable_specifications() &&;

std::vector<uint16_t> get_partition_key_bind_indexes(const schema& schema) const;

- void add(int32_t bind_index, lw_shared_ptr<column_specification> spec);
+ void add_variable_specification(int32_t bind_index, lw_shared_ptr<column_specification> spec);

- void set_bound_variables(const std::vector<shared_ptr<column_identifier>>& bound_names);
+ void set_bound_variables(const std::vector<shared_ptr<column_identifier>>& prepare_meta);
};

}
diff --git a/cql3/relation.hh b/cql3/relation.hh
index 0734898aa9..861db2d10e 100644
--- a/cql3/relation.hh
+++ b/cql3/relation.hh
@@ -43,7 +43,7 @@

#include "schema_fwd.hh"
#include "column_identifier.hh"
-#include "variable_specifications.hh"
+#include "prepare_context.hh"
#include "restrictions/restriction.hh"
#include "statements/bound.hh"
#include "term.hh"
@@ -139,29 +139,29 @@ class relation : public enable_shared_from_this<relation> {
* @return the <code>Restriction</code> corresponding to this <code>Relation</code>
* @throws InvalidRequestException if this <code>Relation</code> is not valid
*/
- virtual ::shared_ptr<restrictions::restriction> to_restriction(database& db, schema_ptr schema, variable_specifications& bound_names) final {
+ virtual ::shared_ptr<restrictions::restriction> to_restriction(database& db, schema_ptr schema, prepare_context& ctx) final {
if (_relation_type == expr::oper_t::EQ) {
- return new_EQ_restriction(db, schema, bound_names);
+ return new_EQ_restriction(db, schema, ctx);
} else if (_relation_type == expr::oper_t::LT) {
- return new_slice_restriction(db, schema, bound_names, statements::bound::END, false);
+ return new_slice_restriction(db, schema, ctx, statements::bound::END, false);
} else if (_relation_type == expr::oper_t::LTE) {
- return new_slice_restriction(db, schema, bound_names, statements::bound::END, true);
+ return new_slice_restriction(db, schema, ctx, statements::bound::END, true);
} else if (_relation_type == expr::oper_t::GTE) {
- return new_slice_restriction(db, schema, bound_names, statements::bound::START, true);
+ return new_slice_restriction(db, schema, ctx, statements::bound::START, true);
} else if (_relation_type == expr::oper_t::GT) {
- return new_slice_restriction(db, schema, bound_names, statements::bound::START, false);
+ return new_slice_restriction(db, schema, ctx, statements::bound::START, false);
} else if (_relation_type == expr::oper_t::IN) {
- return new_IN_restriction(db, schema, bound_names);
+ return new_IN_restriction(db, schema, ctx);
} else if (_relation_type == expr::oper_t::CONTAINS) {
- return new_contains_restriction(db, schema, bound_names, false);
+ return new_contains_restriction(db, schema, ctx, false);
} else if (_relation_type == expr::oper_t::CONTAINS_KEY) {
- return new_contains_restriction(db, schema, bound_names, true);
+ return new_contains_restriction(db, schema, ctx, true);
} else if (_relation_type == expr::oper_t::IS_NOT) {
// This case is not supposed to happen: statement_restrictions
// constructor does not call this function for views' IS_NOT.
throw exceptions::invalid_request_exception(format("Unsupported \"IS NOT\" relation: {}", to_string()));
} else if (_relation_type == expr::oper_t::LIKE) {
- return new_LIKE_restriction(db, schema, bound_names);
+ return new_LIKE_restriction(db, schema, ctx);
} else {
throw exceptions::invalid_request_exception(format("Unsupported \"!=\" relation: {}", to_string()));
}
@@ -182,31 +182,31 @@ class relation : public enable_shared_from_this<relation> {
* @throws InvalidRequestException if the relation cannot be converted into an EQ restriction.
*/
virtual ::shared_ptr<restrictions::restriction> new_EQ_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names) = 0;
+ prepare_context& ctx) = 0;

/**
* Creates a new IN restriction instance.
*
* @param cfm the Column Family meta data
- * @param bound_names the variables specification where to collect the bind variables
+ * @param meta the variables specification where to collect the bind variables
* @return a new IN restriction instance
* @throws InvalidRequestException if the relation cannot be converted into an IN restriction.
*/
virtual ::shared_ptr<restrictions::restriction> new_IN_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names) = 0;
+ prepare_context& ctx) = 0;

/**
* Creates a new Slice restriction instance.
*
* @param cfm the Column Family meta data
- * @param bound_names the variables specification where to collect the bind variables
+ * @param meta the variables specification where to collect the bind variables
* @param bound the slice bound
* @param inclusive <code>true</code> if the bound is included.
* @return a new slice restriction instance
* @throws InvalidRequestException if the <code>Relation</code> is not valid
*/
virtual ::shared_ptr<restrictions::restriction> new_slice_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names,
+ prepare_context& ctx,
statements::bound bound,
bool inclusive) = 0;

@@ -214,19 +214,19 @@ class relation : public enable_shared_from_this<relation> {
* Creates a new Contains restriction instance.
*
* @param cfm the Column Family meta data
- * @param bound_names the variables specification where to collect the bind variables
+ * @param meta the variables specification where to collect the bind variables
* @param isKey <code>true</code> if the restriction to create is a CONTAINS KEY
* @return a new Contains <code>::shared_ptr<restrictions::restriction></code> instance
* @throws InvalidRequestException if the <code>Relation</code> is not valid
*/
virtual ::shared_ptr<restrictions::restriction> new_contains_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names, bool isKey) = 0;
+ prepare_context& ctx, bool isKey) = 0;

/**
* Creates a new LIKE restriction instance.
*/
virtual ::shared_ptr<restrictions::restriction> new_LIKE_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names) = 0;
+ prepare_context& ctx) = 0;

/**
* Renames an identifier in this Relation, if applicable.
@@ -253,7 +253,7 @@ class relation : public enable_shared_from_this<relation> {
const term::raw& raw,
database& db,
const sstring& keyspace,
- variable_specifications& boundNames) const = 0;
+ prepare_context& ctx) const = 0;

/**
* Converts the specified <code>Raw</code> terms into a <code>Term</code>s.
@@ -269,10 +269,10 @@ class relation : public enable_shared_from_this<relation> {
const std::vector<::shared_ptr<term::raw>>& raws,
database& db,
const sstring& keyspace,
- variable_specifications& boundNames) const {
+ prepare_context& ctx) const {
std::vector<::shared_ptr<term>> terms;
for (const auto& r : raws) {
- terms.emplace_back(to_term(receivers, *r, db, keyspace, boundNames));
+ terms.emplace_back(to_term(receivers, *r, db, keyspace, ctx));
}
return terms;
}
diff --git a/cql3/restrictions/statement_restrictions.cc b/cql3/restrictions/statement_restrictions.cc
index b7651e5b43..962becd8e4 100644
--- a/cql3/restrictions/statement_restrictions.cc
+++ b/cql3/restrictions/statement_restrictions.cc
@@ -305,7 +305,7 @@ statement_restrictions::statement_restrictions(database& db,
schema_ptr schema,
statements::statement_type type,
const std::vector<::shared_ptr<relation>>& where_clause,
- variable_specifications& bound_names,
+ prepare_context& ctx,
bool selects_only_static_columns,
bool for_view,
bool allow_filtering)
@@ -336,7 +336,7 @@ statement_restrictions::statement_restrictions(database& db,
throw exceptions::invalid_request_exception(format("restriction '{}' is only supported in materialized view creation", relation->to_string()));
}
} else {
- const auto restriction = relation->to_restriction(db, schema, bound_names);
+ const auto restriction = relation->to_restriction(db, schema, ctx);
if (dynamic_pointer_cast<multi_column_restriction>(restriction)) {
_clustering_columns_restrictions = _clustering_columns_restrictions->merge_to(_schema, restriction);
} else if (has_token(restriction->expression)) {
diff --git a/cql3/restrictions/statement_restrictions.hh b/cql3/restrictions/statement_restrictions.hh
index b17fcfe8dd..3521051d7c 100644
--- a/cql3/restrictions/statement_restrictions.hh
+++ b/cql3/restrictions/statement_restrictions.hh
@@ -49,7 +49,7 @@
#include "cql3/restrictions/primary_key_restrictions.hh"
#include "cql3/restrictions/single_column_restrictions.hh"
#include "cql3/relation.hh"
-#include "cql3/variable_specifications.hh"
+#include "cql3/prepare_context.hh"
#include "cql3/statements/statement_type.hh"

namespace cql3 {
@@ -151,7 +151,7 @@ class statement_restrictions {
schema_ptr schema,
statements::statement_type type,
const std::vector<::shared_ptr<relation>>& where_clause,
- variable_specifications& bound_names,
+ prepare_context& ctx,
bool selects_only_static_columns,
bool for_view = false,
bool allow_filtering = false);
diff --git a/cql3/sets.cc b/cql3/sets.cc
index 883290a46b..62095f1bf6 100644
--- a/cql3/sets.cc
+++ b/cql3/sets.cc
@@ -201,7 +201,7 @@ sets::delayed_value::contains_bind_marker() const {
}

void
-sets::delayed_value::collect_marker_specification(variable_specifications& bound_names) const {
+sets::delayed_value::fill_prepare_context(prepare_context& ctx) const {
}

shared_ptr<terminal>
diff --git a/cql3/sets.hh b/cql3/sets.hh
index 21ed2883e3..5683501e46 100644
--- a/cql3/sets.hh
+++ b/cql3/sets.hh
@@ -94,7 +94,7 @@ class sets {
: _comparator(std::move(comparator)), _elements(std::move(elements)) {
}
virtual bool contains_bind_marker() const override;
- virtual void collect_marker_specification(variable_specifications& bound_names) const override;
+ virtual void fill_prepare_context(prepare_context& ctx) const override;
virtual shared_ptr<terminal> bind(const query_options& options);
};

diff --git a/cql3/single_column_relation.cc b/cql3/single_column_relation.cc
index fa004a2984..9aee583d5f 100644
--- a/cql3/single_column_relation.cc
+++ b/cql3/single_column_relation.cc
@@ -57,26 +57,26 @@ single_column_relation::to_term(const std::vector<lw_shared_ptr<column_specifica
const term::raw& raw,
database& db,
const sstring& keyspace,
- variable_specifications& bound_names) const {
+ prepare_context& ctx) const {
// TODO: optimize vector away, accept single column_specification
assert(receivers.size() == 1);
auto term = raw.prepare(db, keyspace, receivers[0]);
- term->collect_marker_specification(bound_names);
+ term->fill_prepare_context(ctx);
return term;
}

::shared_ptr<restrictions::restriction>
-single_column_relation::new_EQ_restriction(database& db, schema_ptr schema, variable_specifications& bound_names) {
+single_column_relation::new_EQ_restriction(database& db, schema_ptr schema, prepare_context& ctx) {
const column_definition& column_def = to_column_definition(*schema, *_entity);
if (!_map_key) {
auto r = ::make_shared<restrictions::single_column_restriction>(column_def);
- auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), ctx);
r->expression = binary_operator{&column_def, expr::oper_t::EQ, std::move(term)};
return r;
}
auto&& receivers = to_receivers(*schema, column_def);
- auto&& entry_key = to_term({receivers[0]}, *_map_key, db, schema->ks_name(), bound_names);
- auto&& entry_value = to_term({receivers[1]}, *_value, db, schema->ks_name(), bound_names);
+ auto&& entry_key = to_term({receivers[0]}, *_map_key, db, schema->ks_name(), ctx);
+ auto&& entry_value = to_term({receivers[1]}, *_value, db, schema->ks_name(), ctx);
auto r = make_shared<restrictions::single_column_restriction>(column_def);
r->expression = binary_operator{
column_value(&column_def, std::move(entry_key)), oper_t::EQ, std::move(entry_value)};
@@ -84,18 +84,18 @@ single_column_relation::new_EQ_restriction(database& db, schema_ptr schema, vari
}

::shared_ptr<restrictions::restriction>
-single_column_relation::new_IN_restriction(database& db, schema_ptr schema, variable_specifications& bound_names) {
+single_column_relation::new_IN_restriction(database& db, schema_ptr schema, prepare_context& ctx) {
using namespace restrictions;
const column_definition& column_def = to_column_definition(*schema, *_entity);
auto receivers = to_receivers(*schema, column_def);
assert(_in_values.empty() || !_value);
if (_value) {
- auto term = to_term(receivers, *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(receivers, *_value, db, schema->ks_name(), ctx);
auto r = ::make_shared<single_column_restriction>(column_def);
r->expression = binary_operator{&column_def, expr::oper_t::IN, std::move(term)};
return r;
}
- auto terms = to_terms(receivers, _in_values, db, schema->ks_name(), bound_names);
+ auto terms = to_terms(receivers, _in_values, db, schema->ks_name(), ctx);
// Convert a single-item IN restriction to an EQ restriction
if (terms.size() == 1) {
auto r = ::make_shared<single_column_restriction>(column_def);
@@ -110,13 +110,13 @@ single_column_relation::new_IN_restriction(database& db, schema_ptr schema, vari

::shared_ptr<restrictions::restriction>
single_column_relation::new_LIKE_restriction(
- database& db, schema_ptr schema, variable_specifications& bound_names) {
+ database& db, schema_ptr schema, prepare_context& ctx) {
const column_definition& column_def = to_column_definition(*schema, *_entity);
if (!column_def.type->is_string()) {
throw exceptions::invalid_request_exception(
format("LIKE is allowed only on string types, which {} is not", column_def.name_as_text()));
}
- auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), ctx);
auto r = ::make_shared<restrictions::single_column_restriction>(column_def);
r->expression = binary_operator{&column_def, expr::oper_t::LIKE, std::move(term)};
return r;
diff --git a/cql3/single_column_relation.hh b/cql3/single_column_relation.hh
index 3acf11d8c1..d30316dbd5 100644
--- a/cql3/single_column_relation.hh
+++ b/cql3/single_column_relation.hh
@@ -117,7 +117,7 @@ class single_column_relation final : public relation {
protected:
virtual ::shared_ptr<term> to_term(const std::vector<lw_shared_ptr<column_specification>>& receivers,
const term::raw& raw, database& db, const sstring& keyspace,
- variable_specifications& bound_names) const override;
+ prepare_context& ctx) const override;

#if 0
public SingleColumnRelation withNonStrictOperator()
@@ -146,13 +146,13 @@ class single_column_relation final : public relation {

protected:
virtual ::shared_ptr<restrictions::restriction> new_EQ_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names);
+ prepare_context& ctx);

virtual ::shared_ptr<restrictions::restriction> new_IN_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names) override;
+ prepare_context& ctx) override;

virtual ::shared_ptr<restrictions::restriction> new_slice_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names,
+ prepare_context& ctx,
statements::bound bound,
bool inclusive) override {
auto&& column_def = to_column_definition(*schema, *_entity);
@@ -169,17 +169,17 @@ class single_column_relation final : public relation {
throw exceptions::invalid_request_exception("Slice restrictions are not supported on duration columns");
}

- auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), ctx);
auto r = ::make_shared<restrictions::single_column_restriction>(column_def);
r->expression = expr::binary_operator{&column_def, _relation_type, std::move(term)};
return r;
}

virtual shared_ptr<restrictions::restriction> new_contains_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names,
+ prepare_context& ctx,
bool is_key) override {
auto&& column_def = to_column_definition(*schema, *_entity);
- auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), ctx);
auto r = ::make_shared<restrictions::single_column_restriction>(column_def);
r->expression = expr::binary_operator{
&column_def, is_key ? expr::oper_t::CONTAINS_KEY : expr::oper_t::CONTAINS, std::move(term)};
@@ -187,7 +187,7 @@ class single_column_relation final : public relation {
}

virtual ::shared_ptr<restrictions::restriction> new_LIKE_restriction(
- database& db, schema_ptr schema, variable_specifications& bound_names) override;
+ database& db, schema_ptr schema, prepare_context& ctx) override;

virtual ::shared_ptr<relation> maybe_rename_identifier(const column_identifier::raw& from, column_identifier::raw to) override {
return *_entity == from
diff --git a/cql3/statements/batch_statement.cc b/cql3/statements/batch_statement.cc
index a19473b3f1..11097f4e52 100644
--- a/cql3/statements/batch_statement.cc
+++ b/cql3/statements/batch_statement.cc
@@ -433,7 +433,7 @@ namespace raw {

std::unique_ptr<prepared_statement>
batch_statement::prepare(database& db, cql_stats& stats) {
- auto&& bound_names = get_bound_variables();
+ auto&& meta = get_prepare_context();

std::optional<sstring> first_ks;
std::optional<sstring> first_cf;
@@ -449,20 +449,20 @@ batch_statement::prepare(database& db, cql_stats& stats) {
} else {
have_multiple_cfs = first_ks.value() != parsed->keyspace() || first_cf.value() != parsed->column_family();
}
- statements.emplace_back(parsed->prepare(db, bound_names, stats));
+ statements.emplace_back(parsed->prepare(db, meta, stats));
}

auto&& prep_attrs = _attrs->prepare(db, "[batch]", "[batch]");
- prep_attrs->collect_marker_specification(bound_names);
+ prep_attrs->fill_prepare_context(meta);

- cql3::statements::batch_statement batch_statement_(bound_names.size(), _type, std::move(statements), std::move(prep_attrs), stats);
+ cql3::statements::batch_statement batch_statement_(meta.bound_variables_size(), _type, std::move(statements), std::move(prep_attrs), stats);

std::vector<uint16_t> partition_key_bind_indices;
if (!have_multiple_cfs && batch_statement_.get_statements().size() > 0) {
- partition_key_bind_indices = bound_names.get_partition_key_bind_indexes(*batch_statement_.get_statements()[0].statement->s);
+ partition_key_bind_indices = meta.get_partition_key_bind_indexes(*batch_statement_.get_statements()[0].statement->s);
}
return std::make_unique<prepared_statement>(make_shared<cql3::statements::batch_statement>(std::move(batch_statement_)),
- bound_names.get_specifications(),
+ meta.get_variable_specifications(),
std::move(partition_key_bind_indices));
}

diff --git a/cql3/statements/create_view_statement.cc b/cql3/statements/create_view_statement.cc
index 19d9b12c25..17800f4632 100644
--- a/cql3/statements/create_view_statement.cc
+++ b/cql3/statements/create_view_statement.cc
@@ -222,7 +222,7 @@ future<shared_ptr<cql_transport::event::schema_change>> create_view_statement::a
return def;
}));

- if (!_variables.empty()) {
+ if (!_prepare_ctx.empty()) {
throw exceptions::invalid_request_exception(format("Cannot use query parameters in CREATE MATERIALIZED VIEW statements"));
}

diff --git a/cql3/statements/delete_statement.cc b/cql3/statements/delete_statement.cc
index 2cb0e2bbf3..af26fc80c0 100644
--- a/cql3/statements/delete_statement.cc
+++ b/cql3/statements/delete_statement.cc
@@ -85,9 +85,9 @@ void delete_statement::add_update_for_key(mutation& m, const query::clustering_r
namespace raw {

::shared_ptr<cql3::statements::modification_statement>
-delete_statement::prepare_internal(database& db, schema_ptr schema, variable_specifications& bound_names,
+delete_statement::prepare_internal(database& db, schema_ptr schema, prepare_context& ctx,
std::unique_ptr<attributes> attrs, cql_stats& stats) const {
- auto stmt = ::make_shared<cql3::statements::delete_statement>(statement_type::DELETE, bound_names.size(), schema, std::move(attrs), stats);
+ auto stmt = ::make_shared<cql3::statements::delete_statement>(statement_type::DELETE, ctx.bound_variables_size(), schema, std::move(attrs), stats);

for (auto&& deletion : _deletions) {
auto&& id = deletion->affected_column().prepare_column_identifier(*schema);
@@ -103,11 +103,11 @@ delete_statement::prepare_internal(database& db, schema_ptr schema, variable_spe
}

auto&& op = deletion->prepare(db, schema->ks_name(), *def);
- op->collect_marker_specification(bound_names);
+ op->fill_prepare_context(ctx);
stmt->add_operation(op);
}
- prepare_conditions(db, *schema, bound_names, *stmt);
- stmt->process_where_clause(db, _where_clause, bound_names);
+ prepare_conditions(db, *schema, ctx, *stmt);
+ stmt->process_where_clause(db, _where_clause, ctx);
if (has_slice(stmt->restrictions().get_clustering_columns_restrictions()->expression)) {
if (!schema->is_compound()) {
throw exceptions::invalid_request_exception("Range deletions on \"compact storage\" schemas are not supported");
diff --git a/cql3/statements/modification_statement.cc b/cql3/statements/modification_statement.cc
index d1af58f12b..042c003f30 100644
--- a/cql3/statements/modification_statement.cc
+++ b/cql3/statements/modification_statement.cc
@@ -389,8 +389,8 @@ void modification_statement::build_cas_result_set_metadata() {
}

void
-modification_statement::process_where_clause(database& db, std::vector<relation_ptr> where_clause, variable_specifications& names) {
- _restrictions = restrictions::statement_restrictions(db, s, type, where_clause, names,
+modification_statement::process_where_clause(database& db, std::vector<relation_ptr> where_clause, prepare_context& ctx) {
+ _restrictions = restrictions::statement_restrictions(db, s, type, where_clause, ctx,
applies_only_to_static_columns(), _selects_a_collection, false);
/*
* If there's no clustering columns restriction, we may assume that EXISTS
@@ -461,24 +461,24 @@ namespace raw {
std::unique_ptr<prepared_statement>
modification_statement::prepare(database& db, cql_stats& stats) {
schema_ptr schema = validation::validate_column_family(db, keyspace(), column_family());
- auto bound_names = get_bound_variables();
- auto statement = prepare(db, bound_names, stats);
- auto partition_key_bind_indices = bound_names.get_partition_key_bind_indexes(*schema);
- return std::make_unique<prepared_statement>(std::move(statement), bound_names, std::move(partition_key_bind_indices));
+ auto meta = get_prepare_context();
+ auto statement = prepare(db, meta, stats);
+ auto partition_key_bind_indices = meta.get_partition_key_bind_indexes(*schema);
+ return std::make_unique<prepared_statement>(std::move(statement), meta, std::move(partition_key_bind_indices));
}

::shared_ptr<cql3::statements::modification_statement>
-modification_statement::prepare(database& db, variable_specifications& bound_names, cql_stats& stats) const {
+modification_statement::prepare(database& db, prepare_context& ctx, cql_stats& stats) const {
schema_ptr schema = validation::validate_column_family(db, keyspace(), column_family());

auto prepared_attributes = _attrs->prepare(db, keyspace(), column_family());
- prepared_attributes->collect_marker_specification(bound_names);
+ prepared_attributes->fill_prepare_context(ctx);

- return prepare_internal(db, schema, bound_names, std::move(prepared_attributes), stats);
+ return prepare_internal(db, schema, ctx, std::move(prepared_attributes), stats);
}

void
-modification_statement::prepare_conditions(database& db, const schema& schema, variable_specifications& bound_names,
+modification_statement::prepare_conditions(database& db, const schema& schema, prepare_context& ctx,
cql3::statements::modification_statement& stmt) const
{
if (_if_not_exists || _if_exists || !_conditions.empty()) {
@@ -508,7 +508,7 @@ modification_statement::prepare_conditions(database& db, const schema& schema, v
}

auto condition = entry.second->prepare(db, keyspace(), *def);
- condition->collect_marker_specificaton(bound_names);
+ condition->collect_marker_specificaton(ctx);

if (def->is_primary_key()) {
throw exceptions::invalid_request_exception(format("PRIMARY KEY column '{}' cannot have IF conditions", *id));
diff --git a/cql3/statements/modification_statement.hh b/cql3/statements/modification_statement.hh
index 87a69c0662..205efa85c8 100644
--- a/cql3/statements/modification_statement.hh
+++ b/cql3/statements/modification_statement.hh
@@ -194,7 +194,7 @@ class modification_statement : public cql_statement_opt_metadata {
return _is_raw_counter_shard_write.value_or(false);
}

- void process_where_clause(database& db, std::vector<relation_ptr> where_clause, variable_specifications& names);
+ void process_where_clause(database& db, std::vector<relation_ptr> where_clause, prepare_context& ctx);

// CAS statement returns a result set. Prepare result set metadata
// so that get_result_metadata() returns a meaningful value.
diff --git a/cql3/statements/prepared_statement.hh b/cql3/statements/prepared_statement.hh
index fe28aea9b1..152595472e 100644
--- a/cql3/statements/prepared_statement.hh
+++ b/cql3/statements/prepared_statement.hh
@@ -51,7 +51,7 @@

namespace cql3 {

-class variable_specifications;
+class prepare_context;
class column_specification;
class cql_statement;

@@ -74,9 +74,9 @@ class prepared_statement : public seastar::weakly_referencable<prepared_statemen

prepared_statement(seastar::shared_ptr<cql_statement> statement_, std::vector<seastar::lw_shared_ptr<column_specification>> bound_names_, std::vector<uint16_t> partition_key_bind_indices);

- prepared_statement(seastar::shared_ptr<cql_statement> statement_, const variable_specifications& names, const std::vector<uint16_t>& partition_key_bind_indices);
+ prepared_statement(seastar::shared_ptr<cql_statement> statement_, const prepare_context& ctx, const std::vector<uint16_t>& partition_key_bind_indices);

- prepared_statement(seastar::shared_ptr<cql_statement> statement_, variable_specifications&& names, std::vector<uint16_t>&& partition_key_bind_indices);
+ prepared_statement(seastar::shared_ptr<cql_statement> statement_, prepare_context&& ctx, std::vector<uint16_t>&& partition_key_bind_indices);

prepared_statement(seastar::shared_ptr<cql_statement>&& statement_);

diff --git a/cql3/statements/raw/delete_statement.hh b/cql3/statements/raw/delete_statement.hh
index 0d1e273985..e0147a826a 100644
--- a/cql3/statements/raw/delete_statement.hh
+++ b/cql3/statements/raw/delete_statement.hh
@@ -69,7 +69,7 @@ class delete_statement : public modification_statement {
bool if_exists);
protected:
virtual ::shared_ptr<cql3::statements::modification_statement> prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;
+ prepare_context& ctx, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;
};

}
diff --git a/cql3/statements/raw/insert_statement.hh b/cql3/statements/raw/insert_statement.hh
index 5ec5d58b76..e00720b754 100644
--- a/cql3/statements/raw/insert_statement.hh
+++ b/cql3/statements/raw/insert_statement.hh
@@ -77,7 +77,7 @@ class insert_statement : public raw::modification_statement {
bool if_not_exists);

virtual ::shared_ptr<cql3::statements::modification_statement> prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;
+ prepare_context& ctx, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;

};

@@ -98,7 +98,7 @@ class insert_json_statement : public raw::modification_statement {
insert_json_statement(cf_name name, std::unique_ptr<attributes::raw> attrs, ::shared_ptr<term::raw> json_value, bool if_not_exists, bool default_unset);

virtual ::shared_ptr<cql3::statements::modification_statement> prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;
+ prepare_context& ctx, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;

};

diff --git a/cql3/statements/raw/modification_statement.hh b/cql3/statements/raw/modification_statement.hh
index a4da08671a..b9ac799f33 100644
--- a/cql3/statements/raw/modification_statement.hh
+++ b/cql3/statements/raw/modification_statement.hh
@@ -73,15 +73,15 @@ class modification_statement : public cf_statement {

public:
virtual std::unique_ptr<prepared_statement> prepare(database& db, cql_stats& stats) override;
- ::shared_ptr<cql3::statements::modification_statement> prepare(database& db, variable_specifications& bound_names, cql_stats& stats) const;
+ ::shared_ptr<cql3::statements::modification_statement> prepare(database& db, prepare_context& ctx, cql_stats& stats) const;
protected:
virtual ::shared_ptr<cql3::statements::modification_statement> prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const = 0;
+ prepare_context& ctx, std::unique_ptr<attributes> attrs, cql_stats& stats) const = 0;

// Helper function used by child classes to prepare conditions for a prepared statement.
// Must be called before processing WHERE clause, because to perform sanity checks there
// we need to know what kinds of conditions (static, regular) the statement has.
- void prepare_conditions(database& db, const schema& schema, variable_specifications& bound_names,
+ void prepare_conditions(database& db, const schema& schema, prepare_context& ctx,
cql3::statements::modification_statement& stmt) const;
};

diff --git a/cql3/statements/raw/parsed_statement.cc b/cql3/statements/raw/parsed_statement.cc
index dc58ef7811..e075f83f84 100644
--- a/cql3/statements/raw/parsed_statement.cc
+++ b/cql3/statements/raw/parsed_statement.cc
@@ -53,17 +53,17 @@ namespace raw {
parsed_statement::~parsed_statement()
{ }

-variable_specifications& parsed_statement::get_bound_variables() {
- return _variables;
+prepare_context& parsed_statement::get_prepare_context() {
+ return _prepare_ctx;
}

-const variable_specifications& parsed_statement::get_bound_variables() const {
- return _variables;
+const prepare_context& parsed_statement::get_prepare_context() const {
+ return _prepare_ctx;
}

// Used by the parser and preparable statement
void parsed_statement::set_bound_variables(const std::vector<::shared_ptr<column_identifier>>& bound_names) {
- _variables.set_bound_variables(bound_names);
+ _prepare_ctx.set_bound_variables(bound_names);
}

}
@@ -74,12 +74,12 @@ prepared_statement::prepared_statement(::shared_ptr<cql_statement> statement_, s
, partition_key_bind_indices(std::move(partition_key_bind_indices))
{ }

-prepared_statement::prepared_statement(::shared_ptr<cql_statement> statement_, const variable_specifications& names, const std::vector<uint16_t>& partition_key_bind_indices)
- : prepared_statement(statement_, names.get_specifications(), partition_key_bind_indices)
+prepared_statement::prepared_statement(::shared_ptr<cql_statement> statement_, const prepare_context& ctx, const std::vector<uint16_t>& partition_key_bind_indices)
+ : prepared_statement(statement_, ctx.get_variable_specifications(), partition_key_bind_indices)
{ }

-prepared_statement::prepared_statement(::shared_ptr<cql_statement> statement_, variable_specifications&& names, std::vector<uint16_t>&& partition_key_bind_indices)
- : prepared_statement(statement_, std::move(names).get_specifications(), std::move(partition_key_bind_indices))
+prepared_statement::prepared_statement(::shared_ptr<cql_statement> statement_, prepare_context&& ctx, std::vector<uint16_t>&& partition_key_bind_indices)
+ : prepared_statement(statement_, std::move(ctx).get_variable_specifications(), std::move(partition_key_bind_indices))
{ }

prepared_statement::prepared_statement(::shared_ptr<cql_statement>&& statement_)
diff --git a/cql3/statements/raw/parsed_statement.hh b/cql3/statements/raw/parsed_statement.hh
index 3643a7e9ab..3cba0795f5 100644
--- a/cql3/statements/raw/parsed_statement.hh
+++ b/cql3/statements/raw/parsed_statement.hh
@@ -41,7 +41,7 @@

#pragma once

-#include "cql3/variable_specifications.hh"
+#include "cql3/prepare_context.hh"
#include "cql3/column_specification.hh"

#include <seastar/core/shared_ptr.hh>
@@ -64,13 +64,13 @@ namespace raw {

class parsed_statement {
protected:
- variable_specifications _variables;
+ prepare_context _prepare_ctx;

public:
virtual ~parsed_statement();

- variable_specifications& get_bound_variables();
- const variable_specifications& get_bound_variables() const;
+ prepare_context& get_prepare_context();
+ const prepare_context& get_prepare_context() const;

void set_bound_variables(const std::vector<::shared_ptr<column_identifier>>& bound_names);

diff --git a/cql3/statements/raw/select_statement.hh b/cql3/statements/raw/select_statement.hh
index 9c6bcb0df1..bd35291f7f 100644
--- a/cql3/statements/raw/select_statement.hh
+++ b/cql3/statements/raw/select_statement.hh
@@ -127,13 +127,13 @@ class select_statement : public cf_statement
::shared_ptr<restrictions::statement_restrictions> prepare_restrictions(
database& db,
schema_ptr schema,
- variable_specifications& bound_names,
+ prepare_context& ctx,
::shared_ptr<selection::selection> selection,
bool for_view = false,
bool allow_filtering = false);

/** Returns a ::shared_ptr<term> for the limit or null if no limit is set */
- ::shared_ptr<term> prepare_limit(database& db, variable_specifications& bound_names, ::shared_ptr<term::raw> limit);
+ ::shared_ptr<term> prepare_limit(database& db, prepare_context& ctx, ::shared_ptr<term::raw> limit);

static void verify_ordering_is_allowed(const restrictions::statement_restrictions& restrictions);

diff --git a/cql3/statements/raw/update_statement.hh b/cql3/statements/raw/update_statement.hh
index d03aa055ef..2757a2a417 100644
--- a/cql3/statements/raw/update_statement.hh
+++ b/cql3/statements/raw/update_statement.hh
@@ -81,7 +81,7 @@ class update_statement : public raw::modification_statement {
conditions_vector conditions, bool if_exists);
protected:
virtual ::shared_ptr<cql3::statements::modification_statement> prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;
+ prepare_context& ctx, std::unique_ptr<attributes> attrs, cql_stats& stats) const override;
};

}
diff --git a/cql3/statements/select_statement.cc b/cql3/statements/select_statement.cc
index c25d1c7418..3c43f197a2 100644
--- a/cql3/statements/select_statement.cc
+++ b/cql3/statements/select_statement.cc
@@ -1402,7 +1402,7 @@ void select_statement::maybe_jsonize_select_clause(database& db, schema_ptr sche

std::unique_ptr<prepared_statement> select_statement::prepare(database& db, cql_stats& stats, bool for_view) {
schema_ptr schema = validation::validate_column_family(db, keyspace(), column_family());
- variable_specifications& bound_names = get_bound_variables();
+ prepare_context& ctx = get_prepare_context();

maybe_jsonize_select_clause(db, schema);

@@ -1410,7 +1410,7 @@ std::unique_ptr<prepared_statement> select_statement::prepare(database& db, cql_
? selection::selection::wildcard(schema)
: selection::selection::from_selectors(db, schema, _select_clause);

- auto restrictions = prepare_restrictions(db, schema, bound_names, selection, for_view, _parameters->allow_filtering());
+ auto restrictions = prepare_restrictions(db, schema, ctx, selection, for_view, _parameters->allow_filtering());

if (_parameters->is_distinct()) {
validate_distinct_selection(*schema, *selection, *restrictions);
@@ -1432,53 +1432,53 @@ std::unique_ptr<prepared_statement> select_statement::prepare(database& db, cql_

::shared_ptr<cql3::statements::select_statement> stmt;
auto prepared_attrs = _attrs->prepare(db, keyspace(), column_family());
- prepared_attrs->collect_marker_specification(bound_names);
+ prepared_attrs->fill_prepare_context(ctx);
if (restrictions->uses_secondary_indexing()) {
stmt = indexed_table_select_statement::prepare(
db,
schema,
- bound_names.size(),
+ ctx.bound_variables_size(),
_parameters,
std::move(selection),
std::move(restrictions),
std::move(group_by_cell_indices),
is_reversed_,
std::move(ordering_comparator),
- prepare_limit(db, bound_names, _limit),
- prepare_limit(db, bound_names, _per_partition_limit),
+ prepare_limit(db, ctx, _limit),
+ prepare_limit(db, ctx, _per_partition_limit),
stats,
std::move(prepared_attrs));
} else {
stmt = ::make_shared<cql3::statements::primary_key_select_statement>(
schema,
- bound_names.size(),
+ ctx.bound_variables_size(),
_parameters,
std::move(selection),
std::move(restrictions),
std::move(group_by_cell_indices),
is_reversed_,
std::move(ordering_comparator),
- prepare_limit(db, bound_names, _limit),
- prepare_limit(db, bound_names, _per_partition_limit),
+ prepare_limit(db, ctx, _limit),
+ prepare_limit(db, ctx, _per_partition_limit),
stats,
std::move(prepared_attrs));
}

- auto partition_key_bind_indices = bound_names.get_partition_key_bind_indexes(*schema);
+ auto partition_key_bind_indices = ctx.get_partition_key_bind_indexes(*schema);

- return std::make_unique<prepared_statement>(std::move(stmt), bound_names, std::move(partition_key_bind_indices));
+ return std::make_unique<prepared_statement>(std::move(stmt), ctx, std::move(partition_key_bind_indices));
}

::shared_ptr<restrictions::statement_restrictions>
select_statement::prepare_restrictions(database& db,
schema_ptr schema,
- variable_specifications& bound_names,
+ prepare_context& ctx,
::shared_ptr<selection::selection> selection,
bool for_view,
bool allow_filtering)
{
try {
- return ::make_shared<restrictions::statement_restrictions>(db, schema, statement_type::SELECT, std::move(_where_clause), bound_names,
+ return ::make_shared<restrictions::statement_restrictions>(db, schema, statement_type::SELECT, std::move(_where_clause), ctx,
selection->contains_only_static_columns(), for_view, allow_filtering);
} catch (const exceptions::unrecognized_entity_exception& e) {
if (contains_alias(e.entity)) {
@@ -1490,14 +1490,14 @@ select_statement::prepare_restrictions(database& db,

/** Returns a ::shared_ptr<term> for the limit or null if no limit is set */
::shared_ptr<term>
-select_statement::prepare_limit(database& db, variable_specifications& bound_names, ::shared_ptr<term::raw> limit)
+select_statement::prepare_limit(database& db, prepare_context& ctx, ::shared_ptr<term::raw> limit)
{
if (!limit) {
return {};
}

auto prep_limit = limit->prepare(db, keyspace(), limit_receiver());
- prep_limit->collect_marker_specification(bound_names);
+ prep_limit->fill_prepare_context(ctx);
return prep_limit;
}

diff --git a/cql3/statements/update_statement.cc b/cql3/statements/update_statement.cc
index ea2de9b4e9..16b7e97c7a 100644
--- a/cql3/statements/update_statement.cc
+++ b/cql3/statements/update_statement.cc
@@ -300,9 +300,9 @@ insert_statement::insert_statement(cf_name name,

::shared_ptr<cql3::statements::modification_statement>
insert_statement::prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const
+ prepare_context& ctx, std::unique_ptr<attributes> attrs, cql_stats& stats) const
{
- auto stmt = ::make_shared<cql3::statements::update_statement>(statement_type::INSERT, bound_names.size(), schema, std::move(attrs), stats);
+ auto stmt = ::make_shared<cql3::statements::update_statement>(statement_type::INSERT, ctx.bound_variables_size(), schema, std::move(attrs), stats);

// Created from an INSERT
if (stmt->is_counter()) {
@@ -337,12 +337,12 @@ insert_statement::prepare_internal(database& db, schema_ptr schema,
relations.push_back(::make_shared<single_column_relation>(col, expr::oper_t::EQ, value));
} else {
auto operation = operation::set_value(value).prepare(db, keyspace(), *def);
- operation->collect_marker_specification(bound_names);
+ operation->fill_prepare_context(ctx);
stmt->add_operation(std::move(operation));
};
}
- prepare_conditions(db, *schema, bound_names, *stmt);
- stmt->process_where_clause(db, relations, bound_names);
+ prepare_conditions(db, *schema, ctx, *stmt);
+ stmt->process_where_clause(db, relations, ctx);
return stmt;
}

@@ -359,16 +359,16 @@ insert_json_statement::insert_json_statement(cf_name name,

::shared_ptr<cql3::statements::modification_statement>
insert_json_statement::prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const
+ prepare_context& ctx, std::unique_ptr<attributes> attrs, cql_stats& stats) const
{
// FIXME: handle _if_not_exists. For now, mark it used to quiet the compiler. #8682
(void)_if_not_exists;
assert(dynamic_pointer_cast<constants::literal>(_json_value) || dynamic_pointer_cast<abstract_marker::raw>(_json_value));
auto json_column_placeholder = ::make_shared<column_identifier>("", true);
auto prepared_json_value = _json_value->prepare(db, "", make_lw_shared<column_specification>("", "", json_column_placeholder, utf8_type));
- prepared_json_value->collect_marker_specification(bound_names);
- auto stmt = ::make_shared<cql3::statements::insert_prepared_json_statement>(bound_names.size(), schema, std::move(attrs), stats, std::move(prepared_json_value), _default_unset);
- prepare_conditions(db, *schema, bound_names, *stmt);
+ prepared_json_value->fill_prepare_context(ctx);
+ auto stmt = ::make_shared<cql3::statements::insert_prepared_json_statement>(ctx.bound_variables_size(), schema, std::move(attrs), stats, std::move(prepared_json_value), _default_unset);
+ prepare_conditions(db, *schema, ctx, *stmt);
return stmt;
}

@@ -384,9 +384,9 @@ update_statement::update_statement(cf_name name,

::shared_ptr<cql3::statements::modification_statement>
update_statement::prepare_internal(database& db, schema_ptr schema,
- variable_specifications& bound_names, std::unique_ptr<attributes> attrs, cql_stats& stats) const
+ prepare_context& ctx, std::unique_ptr<attributes> attrs, cql_stats& stats) const
{
- auto stmt = ::make_shared<cql3::statements::update_statement>(statement_type::UPDATE, bound_names.size(), schema, std::move(attrs), stats);
+ auto stmt = ::make_shared<cql3::statements::update_statement>(statement_type::UPDATE, ctx.bound_variables_size(), schema, std::move(attrs), stats);

for (auto&& entry : _updates) {
auto id = entry.first->prepare_column_identifier(*schema);
@@ -396,15 +396,15 @@ update_statement::prepare_internal(database& db, schema_ptr schema,
}

auto operation = entry.second->prepare(db, keyspace(), *def);
- operation->collect_marker_specification(bound_names);
+ operation->fill_prepare_context(ctx);

if (def->is_primary_key()) {
throw exceptions::invalid_request_exception(format("PRIMARY KEY part {} found in SET part", *entry.first));
}
stmt->add_operation(std::move(operation));
}
- prepare_conditions(db, *schema, bound_names, *stmt);
- stmt->process_where_clause(db, _where_clause, bound_names);
+ prepare_conditions(db, *schema, ctx, *stmt);
+ stmt->process_where_clause(db, _where_clause, ctx);
return stmt;
}

diff --git a/cql3/term.hh b/cql3/term.hh
index 145da86f15..ee5fdafc99 100644
--- a/cql3/term.hh
+++ b/cql3/term.hh
@@ -48,7 +48,7 @@
namespace cql3 {

class terminal;
-class variable_specifications;
+class prepare_context;

/**
* A CQL3 term, i.e. a column value with or without bind variables.
@@ -68,7 +68,7 @@ class term : public ::enable_shared_from_this<term> {
* @param boundNames the variables specification where to collect the
* bind variables of this term in.
*/
- virtual void collect_marker_specification(variable_specifications& bound_names) const = 0;
+ virtual void fill_prepare_context(prepare_context& ctx) const = 0;

/**
* Bind the values in this term to the values contained in {@code values}.
@@ -161,7 +161,7 @@ class term : public ::enable_shared_from_this<term> {
*/
class terminal : public term {
public:
- virtual void collect_marker_specification(variable_specifications& bound_names) const {
+ virtual void fill_prepare_context(prepare_context& ctx) const {
}

virtual ::shared_ptr<terminal> bind(const query_options& options) override {
diff --git a/cql3/token_relation.cc b/cql3/token_relation.cc
index 5a6e612f83..02d54b4080 100644
--- a/cql3/token_relation.cc
+++ b/cql3/token_relation.cc
@@ -81,10 +81,10 @@ std::vector<lw_shared_ptr<cql3::column_specification>> cql3::token_relation::to_

::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_EQ_restriction(
database& db, schema_ptr schema,
- variable_specifications& bound_names) {
+ prepare_context& ctx) {
auto column_defs = get_column_definitions(*schema);
auto term = to_term(to_receivers(*schema, column_defs), *_value, db,
- schema->ks_name(), bound_names);
+ schema->ks_name(), ctx);
auto r = ::make_shared<restrictions::token_restriction>(column_defs);
using namespace expr;
r->expression = binary_operator{token{}, oper_t::EQ, std::move(term)};
@@ -93,7 +93,7 @@ ::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_EQ_restr

::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_IN_restriction(
database& db, schema_ptr schema,
- variable_specifications& bound_names) {
+ prepare_context& ctx) {
throw exceptions::invalid_request_exception(
format("{} cannot be used with the token function",
get_operator()));
@@ -101,12 +101,12 @@ ::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_IN_restr

::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_slice_restriction(
database& db, schema_ptr schema,
- variable_specifications& bound_names,
+ prepare_context& ctx,
statements::bound bound,
bool inclusive) {
auto column_defs = get_column_definitions(*schema);
auto term = to_term(to_receivers(*schema, column_defs), *_value, db,
- schema->ks_name(), bound_names);
+ schema->ks_name(), ctx);
auto r = ::make_shared<restrictions::token_restriction>(column_defs);
using namespace expr;
r->expression = binary_operator{token{}, pick_operator(bound, inclusive), std::move(term)};
@@ -115,14 +115,14 @@ ::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_slice_re

::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_contains_restriction(
database& db, schema_ptr schema,
- variable_specifications& bound_names, bool isKey) {
+ prepare_context& ctx, bool isKey) {
throw exceptions::invalid_request_exception(
format("{} cannot be used with the token function",
get_operator()));
}

::shared_ptr<cql3::restrictions::restriction> cql3::token_relation::new_LIKE_restriction(
- database&, schema_ptr, variable_specifications&) {
+ database&, schema_ptr, prepare_context&) {
throw exceptions::invalid_request_exception("LIKE cannot be used with the token function");
}

@@ -133,9 +133,9 @@ sstring cql3::token_relation::to_string() const {
::shared_ptr<cql3::term> cql3::token_relation::to_term(
const std::vector<lw_shared_ptr<column_specification>>& receivers,
const term::raw& raw, database& db, const sstring& keyspace,
- variable_specifications& bound_names) const {
+ prepare_context& ctx) const {
auto term = raw.prepare(db, keyspace, receivers.front());
- term->collect_marker_specification(bound_names);
+ term->fill_prepare_context(ctx);
return term;
}

diff --git a/cql3/token_relation.hh b/cql3/token_relation.hh
index 7a986de41d..98387986aa 100644
--- a/cql3/token_relation.hh
+++ b/cql3/token_relation.hh
@@ -98,25 +98,25 @@ class token_relation : public relation {

::shared_ptr<restrictions::restriction> new_EQ_restriction(database& db,
schema_ptr schema,
- variable_specifications& bound_names) override;
+ prepare_context& ctx) override;

::shared_ptr<restrictions::restriction> new_IN_restriction(database& db,
schema_ptr schema,
- variable_specifications& bound_names) override;
+ prepare_context& ctx) override;

::shared_ptr<restrictions::restriction> new_slice_restriction(database& db,
schema_ptr schema,
- variable_specifications& bound_names,
+ prepare_context& ctx,
statements::bound bound,
bool inclusive) override;

::shared_ptr<restrictions::restriction> new_contains_restriction(
database& db, schema_ptr schema,
- variable_specifications& bound_names, bool isKey) override;
+ prepare_context& ctx, bool isKey) override;

::shared_ptr<restrictions::restriction> new_LIKE_restriction(database& db,
schema_ptr schema,
- variable_specifications& bound_names) override;
+ prepare_context& ctx) override;

::shared_ptr<relation> maybe_rename_identifier(const column_identifier::raw& from, column_identifier::raw to) override;

@@ -127,7 +127,7 @@ class token_relation : public relation {
const term::raw& raw,
database& db,
const sstring& keyspace,
- variable_specifications& bound_names) const override;
+ prepare_context& ctx) const override;
};

}
diff --git a/cql3/tuples.hh b/cql3/tuples.hh
index 4ef78d6029..4b4b2df258 100644
--- a/cql3/tuples.hh
+++ b/cql3/tuples.hh
@@ -153,9 +153,9 @@ class tuples {
return std::any_of(_elements.begin(), _elements.end(), std::mem_fn(&term::contains_bind_marker));
}

- virtual void collect_marker_specification(variable_specifications& bound_names) const override {
+ virtual void fill_prepare_context(prepare_context& ctx) const override {
for (auto&& term : _elements) {
- term->collect_marker_specification(bound_names);
+ term->fill_prepare_context(ctx);
}
}
private:
diff --git a/cql3/user_types.cc b/cql3/user_types.cc
index 309ab0e57a..33fb1ad3c8 100644
--- a/cql3/user_types.cc
+++ b/cql3/user_types.cc
@@ -185,9 +185,9 @@ bool user_types::delayed_value::contains_bind_marker() const {
return boost::algorithm::any_of(_values, std::mem_fn(&term::contains_bind_marker));
}

-void user_types::delayed_value::collect_marker_specification(variable_specifications& bound_names) const {
+void user_types::delayed_value::fill_prepare_context(prepare_context& ctx) const {
for (auto&& v : _values) {
- v->collect_marker_specification(bound_names);
+ v->fill_prepare_context(ctx);
}
}

diff --git a/cql3/user_types.hh b/cql3/user_types.hh
index 6186befe4a..d94bcfbfa4 100644
--- a/cql3/user_types.hh
+++ b/cql3/user_types.hh
@@ -93,7 +93,7 @@ class user_types {
public:
delayed_value(user_type type, std::vector<shared_ptr<term>> values);
virtual bool contains_bind_marker() const override;
- virtual void collect_marker_specification(variable_specifications& bound_names) const;
+ virtual void fill_prepare_context(prepare_context& ctx) const;
private:
std::vector<managed_bytes_opt> bind_internal(const query_options& options);
public:
diff --git a/test/boost/statement_restrictions_test.cc b/test/boost/statement_restrictions_test.cc
index c5fa60f9d6..7491f5f0d8 100644
--- a/test/boost/statement_restrictions_test.cc
+++ b/test/boost/statement_restrictions_test.cc
@@ -39,13 +39,13 @@ namespace {
query::clustering_row_ranges slice(
const std::vector<relation_ptr>& where_clause, cql_test_env& env,
const sstring& table_name = "t", const sstring& keyspace_name = "ks") {
- variable_specifications bound_names;
+ prepare_context ctx;
return restrictions::statement_restrictions(
env.local_db(),
env.local_db().find_schema(keyspace_name, table_name),
statements::statement_type::SELECT,
where_clause,
- bound_names,
+ ctx,

Pavel Solodovnikov

unread,
Jul 26, 2021, 8:31:10 AMJul 26
to scylla...@googlegroups.com, Pavel Solodovnikov
Introduce a test using `cql-pytest` framework to assert that
both prepared an unprepared LWT statements (insert with
`IF NOT EXISTS`) with a non-deterministic function call
work correctly in case its evaluation affects partition
key range computation (hence the choice of `cas_shard()`
for lwt query).

Tests: cql-pytest/test_non_deterministic_functions.py

Signed-off-by: Pavel Solodovnikov <pa.solo...@scylladb.com>
---
\ No newline at end of file
--
2.31.1

Pavel Solodovnikov

unread,
Jul 26, 2021, 8:31:10 AMJul 26
to scylla...@googlegroups.com, Pavel Solodovnikov
And reuse these values when handling `bounce_to_shard` messages.

Otherwise such a function (e.g. `uuid()`) can yield a different
value when a statement re-executed on the other shard.

It can lead to an infinite number of `bounce_to_shard` messages
sent in case the function value is used to calculate partition
key ranges for the query. Which, in turn, will cause crashes
since we don't support bouncing more than one time and the second
hop will result in a crash.

Caching works only for LWT statements and only for the function
calls that affect partition key range computation for the query.

`variable_specifications` class is renamed to `prepare_context`
and generalized to record information about each `function_call`
AST node and modify them, as needed:
* Set a special flag denoting that a function call is a part
of LWT statement.
* Check whether a given function call is a part of partition key
statement restriction.
* Assign ids for caching if above is true.

There is no need to include any kind of statement identifier
in the cache key since `query_options` (which holds the cache)
is limited to a single statement, anyway.

Note that `function_call::raw` AST nodes are not created
for selection clauses of a SELECT statement hence they
can only accept only one of the following things as parameters:
* Other function calls.
* Literal values.
* Parameter markers.

In other words, only parameters that can be immediately reduced
to a byte buffer are allowed and we don't need to handle
database inputs to non-pure functions separately since they
are not possible in this context. Anyhow, we don't even have
a single non-pure function that accepts arguments, so precautions
are not needed at the moment.

Tests: unit(dev, debug)

Signed-off-by: Pavel Solodovnikov <pa.solo...@scylladb.com>
---
cql3/functions/function_call.hh | 19 ++++++-
cql3/functions/functions.cc | 36 +++++++++++++
cql3/prepare_context.cc | 4 ++
cql3/prepare_context.hh | 30 ++++++++++-
cql3/query_options.cc | 16 ++++++
cql3/query_options.hh | 29 ++++++++++
cql3/single_column_relation.cc | 10 ++++
cql3/statements/batch_statement.cc | 6 ++-
cql3/statements/modification_statement.cc | 26 ++++++++-
cql3/statements/select_statement.cc | 5 +-
transport/messages/result_message.hh | 9 +++-
transport/server.cc | 64 ++++++++++++++---------
transport/server.hh | 2 +-
13 files changed, 223 insertions(+), 33 deletions(-)

diff --git a/cql3/functions/function_call.hh b/cql3/functions/function_call.hh
index ba4485a72f..1d08530baa 100644
--- a/cql3/functions/function_call.hh
+++ b/cql3/functions/function_call.hh
@@ -53,11 +53,28 @@ namespace functions {
class function_call : public non_terminal {
const shared_ptr<scalar_function> _fun;
const std::vector<shared_ptr<term>> _terms;
+ // 0-based index of the function call within a CQL statement.
+ // Used to populate the cache of execution results while passing to
+ // another shard (handling `bounce_to_shard` messages) in LWT statements.
+ //
+ // The id is set only for the function calls that are a part of statement
+ // restrictions for the partition key. Otherwise, the id is not set and
+ // the call is not considered when using or populating the cache.
+ std::optional<uint8_t> _id;
+ // This flag is set to `true` if the `function_call` AST element
+ // is a part of LWT statement.
+ bool _in_lwt_context;
public:
function_call(shared_ptr<scalar_function> fun, std::vector<shared_ptr<term>> terms)
- : _fun(std::move(fun)), _terms(std::move(terms)) {
+ : _fun(std::move(fun)), _terms(std::move(terms)), _in_lwt_context(false) {
}
virtual void fill_prepare_context(prepare_context& ctx) const override;
+ void set_id(uint8_t id) {
+ _id = id;
+ }
+ void set_lwt_context() {
+ _in_lwt_context = true;
+ }
virtual shared_ptr<terminal> bind(const query_options& options) override;
virtual cql3::raw_value_view bind_and_get(const query_options& options) override;
private:
diff --git a/cql3/functions/functions.cc b/cql3/functions/functions.cc
index ff0d7a2fa9..cc3c1acd59 100644
--- a/cql3/functions/functions.cc
+++ b/cql3/functions/functions.cc
@@ -36,6 +36,7 @@
#include "types/user.hh"
#include "concrete_types.hh"
#include "as_json_function.hh"
+#include "cql3/prepare_context.hh"

#include "error_injection_fcts.hh"

@@ -431,6 +432,21 @@ functions::type_equals(const std::vector<data_type>& t1, const std::vector<data_

void
function_call::fill_prepare_context(prepare_context& ctx) const {
+ if (ctx.is_processing_pk_restrictions()) {
+ constexpr auto fn_limit = std::numeric_limits<uint8_t>::max();
+ auto& fn_calls = ctx.function_calls();
+ if (fn_calls.size() == fn_limit) {
+ throw exceptions::invalid_request_exception(
+ format("Too many function calls within one statement. Max supported number is {}", fn_limit));
+ }
+ // FIXME: Hacking around `const` specifier in the `collect_prepare_metadata`
+ // declaration since we also need to modify the current instance along
+ // with prepare metadata.
+ auto non_const_self = static_pointer_cast<function_call>(
+ const_cast<function_call*>(this)->shared_from_this());
+ non_const_self->set_id(fn_calls.size());
+ fn_calls.emplace_back(std::move(non_const_self));
+ }
for (auto&& t : _terms) {
t->fill_prepare_context(ctx);
}
@@ -454,7 +470,27 @@ function_call::bind_and_get(const query_options& options) {
}
buffers.push_back(to_bytes_opt(val));
}
+ const bool use_value_cache = _in_lwt_context && !_fun->is_pure() && _id;
+ if (use_value_cache) {
+ // Populate the cache only for LWT statements. Note that this code
+ // works only in places where `function_call::raw` AST nodes are
+ // created.
+ // These cases do not include selection clause in SELECT statement,
+ // hence no database inputs are possibly allowed to the functions
+ // evaluated here.
+ // We can cache every non-deterministic call here as this code branch
+ // acts the same way as if all arguments are equivalent to literal
+ // values at this point (already calculated).
+ auto query_cached_fn_calls = options.cached_function_calls();
+ auto cached_value_it = query_cached_fn_calls.find(*_id);
+ if (query_cached_fn_calls.end() != cached_value_it) {
+ return raw_value_view::make_temporary(raw_value::make_value(cached_value_it->second));
+ }
+ }
auto result = execute_internal(options.get_cql_serialization_format(), *_fun, std::move(buffers));
+ if (use_value_cache) {
+ options.cache_function_call(*_id, result);
+ }
return cql3::raw_value_view::make_temporary(cql3::raw_value::make_value(result));
}

diff --git a/cql3/prepare_context.cc b/cql3/prepare_context.cc
index 94c8b8778f..bc919fe887 100644
--- a/cql3/prepare_context.cc
+++ b/cql3/prepare_context.cc
@@ -105,4 +105,8 @@ void prepare_context::set_bound_variables(const std::vector<shared_ptr<column_id
_target_columns.resize(bn_size);
}

+prepare_context::function_calls_t& prepare_context::function_calls() {
+ return _fn_calls;
+}
+
}
diff --git a/cql3/prepare_context.hh b/cql3/prepare_context.hh
index 310bf99011..a4bd3ccf08 100644
--- a/cql3/prepare_context.hh
+++ b/cql3/prepare_context.hh
@@ -54,16 +54,30 @@ namespace cql3 {

class column_identifier;
class column_specification;
+namespace functions { class function_call; }

/**
- * A metadata class currently holding bind variables specifications and
- * populated at "prepare" step of query execution.
+ * Metadata class currently holding bind variables specifications and
+ * `function_call` AST nodes inside a query partition key restrictions.
+ * Populated and maintained at "prepare" step of query execution.
*/
class prepare_context final {
private:
std::vector<shared_ptr<column_identifier>> _variable_names;
std::vector<lw_shared_ptr<column_specification>> _specs;
std::vector<lw_shared_ptr<column_specification>> _target_columns;
+ // A list of pointers to prepared `function_call` AST nodes, that
+ // participate in partition key ranges computation.
+ // Used to set additional state for these calls at "prepare" step of a
+ // statement life cycle.
+ using function_calls_t = std::vector<::shared_ptr<cql3::functions::function_call>>;
+ function_calls_t _fn_calls;
+ // The flag denoting whether the context is currently in partition key
+ // processing mode (inside query restrictions AST nodes). If set to true,
+ // then every `function_call` instance will be recorded in the context and
+ // will be assigned an identifier, which will then be used for caching
+ // the function call results.
+ bool _processing_pk_restrictions = false;

public:

@@ -83,6 +97,18 @@ class prepare_context final {
void add_variable_specification(int32_t bind_index, lw_shared_ptr<column_specification> spec);

void set_bound_variables(const std::vector<shared_ptr<column_identifier>>& prepare_meta);
+
+ function_calls_t& function_calls();
+
+ // Inform the context object that it has started or ended processing the
+ // partition key part of statement restrictions.
+ void set_processing_pk_restrictions(bool flag) {
+ _processing_pk_restrictions = flag;
+ }
+
+ bool is_processing_pk_restrictions() const {
+ return _processing_pk_restrictions;
+ }
};

}
diff --git a/cql3/query_options.cc b/cql3/query_options.cc
index a0b1b44f14..7394b0cc55 100644
--- a/cql3/query_options.cc
+++ b/cql3/query_options.cc
@@ -191,4 +191,20 @@ db::consistency_level query_options::check_serial_consistency() const {
throw exceptions::protocol_exception("Consistency level for LWT is missing for a request with conditions");
}

+void query_options::cache_function_call(computed_function_values::key_type id, computed_function_values::mapped_type value) const {
+ _cached_fn_calls.emplace(id, std::move(value));
}
+
+const computed_function_values& query_options::cached_function_calls() const {
+ return _cached_fn_calls;
+}
+
+computed_function_values&& query_options::take_cached_function_calls() {
+ return std::move(_cached_fn_calls);
+}
+
+void query_options::set_cached_function_calls(computed_function_values vals) {
+ _cached_fn_calls = std::move(vals);
+}
+
+}
\ No newline at end of file
diff --git a/cql3/single_column_relation.cc b/cql3/single_column_relation.cc
index 9aee583d5f..03d124f303 100644
--- a/cql3/single_column_relation.cc
+++ b/cql3/single_column_relation.cc
@@ -48,6 +48,8 @@
#include "types/map.hh"
#include "types/list.hh"

+#include <seastar/util/defer.hh>
+
using namespace cql3::expr;

namespace cql3 {
@@ -68,6 +70,10 @@ single_column_relation::to_term(const std::vector<lw_shared_ptr<column_specifica
::shared_ptr<restrictions::restriction>
single_column_relation::new_EQ_restriction(database& db, schema_ptr schema, prepare_context& ctx) {
const column_definition& column_def = to_column_definition(*schema, *_entity);
+ auto reset_processing_pk_column = defer([&ctx] { ctx.set_processing_pk_restrictions(false); });
+ if (column_def.is_partition_key()) {
+ ctx.set_processing_pk_restrictions(true);
+ }
if (!_map_key) {
auto r = ::make_shared<restrictions::single_column_restriction>(column_def);
auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), ctx);
@@ -87,6 +93,10 @@ ::shared_ptr<restrictions::restriction>
single_column_relation::new_IN_restriction(database& db, schema_ptr schema, prepare_context& ctx) {
using namespace restrictions;
const column_definition& column_def = to_column_definition(*schema, *_entity);
+ auto reset_processing_pk_column = defer([&ctx] { ctx.set_processing_pk_restrictions(false); });
+ if (column_def.is_partition_key()) {
+ ctx.set_processing_pk_restrictions(true);
+ }
auto receivers = to_receivers(*schema, column_def);
assert(_in_values.empty() || !_value);
if (_value) {
diff --git a/cql3/statements/batch_statement.cc b/cql3/statements/batch_statement.cc
index 11097f4e52..14af52fa54 100644
--- a/cql3/statements/batch_statement.cc
+++ b/cql3/statements/batch_statement.cc
@@ -359,6 +359,8 @@ future<shared_ptr<cql_transport::messages::result_message>> batch_statement::exe
auto cas_timeout = now + cfg.cas_timeout; // Ballot contention timeout.
auto read_timeout = now + cfg.read_timeout; // Query timeout.

+ computed_function_values cached_fn_calls;
+
for (size_t i = 0; i < _statements.size(); ++i) {

modification_statement& statement = *_statements[i].statement;
@@ -377,6 +379,8 @@ future<shared_ptr<cql_transport::messages::result_message>> batch_statement::exe
} else if (keys.size() != 1 || keys.front().equal(request->key().front(), dht::ring_position_comparator(*schema)) == false) {
throw exceptions::invalid_request_exception("BATCH with conditions cannot span multiple partitions");
}
+ cached_fn_calls.merge(std::move(const_cast<cql3::query_options&>(statement_options).take_cached_function_calls()));
+
std::vector<query::clustering_range> ranges = statement.create_clustering_ranges(statement_options, json_cache);

request->add_row_update(statement, std::move(ranges), std::move(json_cache), statement_options);
@@ -389,7 +393,7 @@ future<shared_ptr<cql_transport::messages::result_message>> batch_statement::exe
if (shard != this_shard_id()) {
proxy.get_stats().replica_cross_shard_ops++;
return make_ready_future<shared_ptr<cql_transport::messages::result_message>>(
- make_shared<cql_transport::messages::result_message::bounce_to_shard>(shard));
+ ::make_shared<cql_transport::messages::result_message::bounce_to_shard>(shard, std::move(cached_fn_calls)));
}

return proxy.cas(schema, request, request->read_command(proxy), request->key(),
diff --git a/cql3/statements/modification_statement.cc b/cql3/statements/modification_statement.cc
index 042c003f30..b0dd97098d 100644
--- a/cql3/statements/modification_statement.cc
+++ b/cql3/statements/modification_statement.cc
@@ -334,7 +334,10 @@ modification_statement::execute_with_condition(service::storage_proxy& proxy, se
if (shard != this_shard_id()) {
proxy.get_stats().replica_cross_shard_ops++;
return make_ready_future<shared_ptr<cql_transport::messages::result_message>>(
- make_shared<cql_transport::messages::result_message::bounce_to_shard>(shard));
+ ::make_shared<cql_transport::messages::result_message::bounce_to_shard>(shard,
+ // const_cast is a hack! this should really be done via `query_state` but
+ // would involve a massive refactoring.
+ std::move(const_cast<cql3::query_options&>(options).take_cached_function_calls())));
}

return proxy.cas(s, request, request->read_command(proxy), request->key(),
@@ -474,7 +477,26 @@ modification_statement::prepare(database& db, prepare_context& ctx, cql_stats& s
auto prepared_attributes = _attrs->prepare(db, keyspace(), column_family());
prepared_attributes->fill_prepare_context(ctx);

- return prepare_internal(db, schema, ctx, std::move(prepared_attributes), stats);
+ auto prepared_stmt = prepare_internal(db, schema, ctx, std::move(prepared_attributes), stats);
+ // For LWT statements, set an additional flag for each non-deterministic
+ // function call within a statement, making it aware that it's being
+ // executed in LWT context.
+ //
+ // This is important since these calls can possibly affect partition key
+ // ranges computation. For such cases we need to forward the computed
+ // execution result of the function when redirecting the query execution to
+ // another shard. Otherwise, it's possible that we end up bouncing
+ // indefinitely between various shards when evaluating a non-deterministic
+ // function each time on each shard.
+ //
+ // Set the flags after the main prepare procedure has already finished,
+ // by that time we've already collected the necessary metadata.
+ if (prepared_stmt->has_conditions()) {
+ for (auto& fn : ctx.function_calls()) {
+ fn->set_lwt_context();
+ }
+ }
+ return prepared_stmt;
}

void
diff --git a/cql3/statements/select_statement.cc b/cql3/statements/select_statement.cc
index 3c43f197a2..a17083c733 100644
--- a/cql3/statements/select_statement.cc
+++ b/cql3/statements/select_statement.cc
@@ -364,7 +364,10 @@ select_statement::do_execute(service::storage_proxy& proxy,
if (this_shard_id() != shard) {
proxy.get_stats().replica_cross_shard_ops++;
return make_ready_future<shared_ptr<cql_transport::messages::result_message>>(
- make_shared<cql_transport::messages::result_message::bounce_to_shard>(shard));
+ ::make_shared<cql_transport::messages::result_message::bounce_to_shard>(shard,
+ // const_cast is a hack! this should really be done via `query_state` but
+ // would involve a massive refactoring.
+ std::move(const_cast<cql3::query_options&>(options).take_cached_function_calls())));
}
}

+ cql3::computed_function_values&& take_cached_function_calls() {
+ return std::move(_cached_fn_calls);
+ }

Pavel Solodovnikov

unread,
Jul 27, 2021, 5:42:43 AMJul 27
to Konstantin Osipov, scylladb-dev
Kostja, please review.

пн, 26 июл. 2021 г., 15:31 Pavel Solodovnikov <pa.solo...@scylladb.com>:

Pavel Solodovnikov

unread,
Jul 28, 2021, 10:53:23 AMJul 28
to Konstantin Osipov, scylladb-dev
Review ping.

вт, 27 июл. 2021 г., 12:42 Pavel Solodovnikov <pa.solo...@scylladb.com>:

Konstantin Osipov

unread,
Jul 28, 2021, 2:44:50 PMJul 28
to Pavel Solodovnikov, scylla...@googlegroups.com
* Pavel Solodovnikov <pa.solo...@scylladb.com> [21/07/26 15:32]:

The design of the patch is looking good to me. I'd check the
design with Avi as well.

I only have a few follow up nits on the style.

> @@ -53,11 +53,28 @@ namespace functions {
> class function_call : public non_terminal {
> const shared_ptr<scalar_function> _fun;
> const std::vector<shared_ptr<term>> _terms;
> + // 0-based index of the function call within a CQL statement.
> + // Used to populate the cache of execution results while passing to
> + // another shard (handling `bounce_to_shard` messages) in LWT statements.
> + //
> + // The id is set only for the function calls that are a part of statement
> + // restrictions for the partition key. Otherwise, the id is not set and
> + // the call is not considered when using or populating the cache.
> + std::optional<uint8_t> _id;
> + // This flag is set to `true` if the `function_call` AST element
> + // is a part of LWT statement.
> + bool _in_lwt_context;

Actually it's set to true only for function calls in lwt primary
key context now.


> + void set_lwt_context() {
> + _in_lwt_context = true;
> + }

Suggest to rename accordingly.

> void
> function_call::fill_prepare_context(prepare_context& ctx) const {
> + if (ctx.is_processing_pk_restrictions()) {
> + constexpr auto fn_limit = std::numeric_limits<uint8_t>::max();
> + auto& fn_calls = ctx.function_calls();

ctx.function_calls() is not all function calls any more, so I
suggest to rename. I'd also hide this implementation inside
prepare_context method, since we're resizing a member of it here, but that's a
matter of taste.

> + if (fn_calls.size() == fn_limit) {
> + throw exceptions::invalid_request_exception(
> + format("Too many function calls within one statement. Max supported number is {}", fn_limit));
> + }
> + // FIXME: Hacking around `const` specifier in the `collect_prepare_metadata`
> + // declaration since we also need to modify the current instance along
> + // with prepare metadata.

I don't see how you can fix it, so I'd remove the excuses, and
only explain what's going on in the comment.
I'd have a query_options method to find_cached_value() by id, but then again
it's a matter of taste.
> + }
> auto result = execute_internal(options.get_cql_serialization_format(), *_fun, std::move(buffers));
> + if (use_value_cache) {
> + options.cache_function_call(*_id, result);

Just like here you have a call to cache a value.

> + }
> return cql3::raw_value_view::make_temporary(cql3::raw_value::make_value(result));
> +prepare_context::function_calls_t& prepare_context::function_calls() {
> + return _fn_calls;
> +}

And kill this getter. Again it's a nit, please feel free to
reject.

> + // A list of pointers to prepared `function_call` AST nodes, that
> + // participate in partition key ranges computation.
> + // Used to set additional state for these calls at "prepare" step of a
> + // statement life cycle.
> + using function_calls_t = std::vector<::shared_ptr<cql3::functions::function_call>>;
> + function_calls_t _fn_calls;

_pk_fn_calls perhaps.

> +
> +computed_function_values&& query_options::take_cached_function_calls() {
> + return std::move(_cached_fn_calls);
> +}
> +
> +void query_options::set_cached_function_calls(computed_function_values vals) {
> + _cached_fn_calls = std::move(vals);
> +}

Why do you need take & set rather than juest adjust query_options
move constructor?

> +
> + // Cached `function_call` evaluation results. `function_call` AST nodes
> + // are created for each function with side effects in a CQL query, i.e.
> + // non-deterministic functions (`uuid()`, `now()` and some others
> + // timeuuid-related).
> + //
> + // These nodes are evaluated either when a query itself is executed
> + // or query restrictions are computed (e.g. partition/clustering
> + // key ranges for LWT requests).
> + //
> + // We need to cache the calls since otherwise when handling a
> + // `bounce_to_shard` request for an LWT query, we can possibly enter an
> + // infinite bouncing loop (in case a function is used to calculate
> + // partition key ranges for a query), since the results can be different
> + // each time. Furthermore, we don't support bouncing more than one time.
> + // Refs: #8604 (https://github.com/scylladb/scylla/issues/8604)
> + //
> + // FIXME: mutable is a hack! `query_state` is not available at
> + // evaluation sites and we only have a const reference to `query_options`.

I'd cut the comment a bit, highlighting the second part and
killing the fixme, nobody's going to fix it anyway, better have a
couple of mutable members than remove const specifier from all
usages.

> + mutable computed_function_values _cached_fn_calls;
> private:
> /**
> * @brief Batch query_options constructor.
> @@ -276,6 +298,13 @@ class query_options {
> }
>
> void prepare(const std::vector<lw_shared_ptr<column_specification>>& specs);
> +
> + // FIXME: should be non-const since it's mutating internals!
> + void cache_function_call(computed_function_values::key_type id, computed_function_values::mapped_type value) const;
> + const computed_function_values& cached_function_calls() const;
> + computed_function_values&& take_cached_function_calls();
> + void set_cached_function_calls(computed_function_values vals);

> + // const_cast is a hack! this should really be done via `query_state` but

I think having 5 fixme's is a bit too much.

> + // would involve a massive refactoring.
> + std::move(const_cast<cql3::query_options&>(options).take_cached_function_calls())));
> }

Konstantin Osipov

unread,
Jul 28, 2021, 2:46:21 PMJul 28
to Pavel Solodovnikov, scylladb-dev, a...@scylladb.com
* Pavel Solodovnikov <pa.solo...@scylladb.com> [21/07/28 17:53]:
> Review ping.

Avi, are you happy with the fix now that caching only happens for
partition keys used in LWT statements? This resolves all the
concerns about the efficiency of the cache or possible misuse.

> > Kostja, please review.

Pavel Solodovnikov

unread,
Jul 29, 2021, 4:43:52 AMJul 29
to Konstantin Osipov, Avi Kivity, scylladb-dev
On Wed, Jul 28, 2021 at 9:44 PM Konstantin Osipov <kos...@scylladb.com> wrote:
>
> * Pavel Solodovnikov <pa.solo...@scylladb.com> [21/07/26 15:32]:
>
> The design of the patch is looking good to me. I'd check the
> design with Avi as well.
>
> I only have a few follow up nits on the style.
>
> > @@ -53,11 +53,28 @@ namespace functions {
> > class function_call : public non_terminal {
> > const shared_ptr<scalar_function> _fun;
> > const std::vector<shared_ptr<term>> _terms;
> > + // 0-based index of the function call within a CQL statement.
> > + // Used to populate the cache of execution results while passing to
> > + // another shard (handling `bounce_to_shard` messages) in LWT statements.
> > + //
> > + // The id is set only for the function calls that are a part of statement
> > + // restrictions for the partition key. Otherwise, the id is not set and
> > + // the call is not considered when using or populating the cache.
> > + std::optional<uint8_t> _id;
> > + // This flag is set to `true` if the `function_call` AST element
> > + // is a part of LWT statement.
> > + bool _in_lwt_context;
>
> Actually it's set to true only for function calls in lwt primary
> key context now.
No, it's not. The combination `_in_lwt_context &&
static_cast<bool>(_id)` would mean that a call is eligible for cache
(i.e. partition key evaluation within an lwt statement).
I don't like the idea to merge these two things, because they are set
separately at different moments.

>
>
> > + void set_lwt_context() {
> > + _in_lwt_context = true;
> > + }
>
> Suggest to rename accordingly.
>
> > void
> > function_call::fill_prepare_context(prepare_context& ctx) const {
> > + if (ctx.is_processing_pk_restrictions()) {
> > + constexpr auto fn_limit = std::numeric_limits<uint8_t>::max();
> > + auto& fn_calls = ctx.function_calls();
>
> ctx.function_calls() is not all function calls any more, so I
> suggest to rename.
Makes sense. I'll rename the function to `pk_function_calls()`, then.

> I'd also hide this implementation inside
> prepare_context method, since we're resizing a member of it here, but that's a
> matter of taste.
Maybe you are right. I will add an `add_pk_function_call()` to the
`prepare_context`.

>
> > + if (fn_calls.size() == fn_limit) {
> > + throw exceptions::invalid_request_exception(
> > + format("Too many function calls within one statement. Max supported number is {}", fn_limit));
> > + }
> > + // FIXME: Hacking around `const` specifier in the `collect_prepare_metadata`
> > + // declaration since we also need to modify the current instance along
> > + // with prepare metadata.
>
> I don't see how you can fix it, so I'd remove the excuses, and
> only explain what's going on in the comment.
I would rather keep these FIXME's. Below I'll explain why.
Of course, no one is going to fix the issues in the foreseeable
future, but nonetheless, it's good to inform a reader that something
architecturally wrong is going on here, motivating them to rewrite the
whole thing from scratch. :)
Also, I think we should stop pretending that `query_options` is passed
via const ref and nothing is modified. Just drop the `const` and
accept non-const reference throughout the cql hierarchy.

Mutable variables usage can be justified, but only in case it modifies
internal state implicitly, for example, increment a counter under the
hoods of some `get_smth_idx()` method or such. But having a const
`set_function_call()` is just outright wrong.
Makes sense.

> > + }
> > auto result = execute_internal(options.get_cql_serialization_format(), *_fun, std::move(buffers));
> > + if (use_value_cache) {
> > + options.cache_function_call(*_id, result);
>
> Just like here you have a call to cache a value.
>
> > + }
> > return cql3::raw_value_view::make_temporary(cql3::raw_value::make_value(result));
> > +prepare_context::function_calls_t& prepare_context::function_calls() {
> > + return _fn_calls;
> > +}
>
> And kill this getter. Again it's a nit, please feel free to
> reject.
I need it to `fn->set_lwt_context()` at the end of
`modification_statement::prepare()`.

>
> > + // A list of pointers to prepared `function_call` AST nodes, that
> > + // participate in partition key ranges computation.
> > + // Used to set additional state for these calls at "prepare" step of a
> > + // statement life cycle.
> > + using function_calls_t = std::vector<::shared_ptr<cql3::functions::function_call>>;
> > + function_calls_t _fn_calls;
>
> _pk_fn_calls perhaps.
Yes.

>
> > +
> > +computed_function_values&& query_options::take_cached_function_calls() {
> > + return std::move(_cached_fn_calls);
> > +}
> > +
> > +void query_options::set_cached_function_calls(computed_function_values vals) {
> > + _cached_fn_calls = std::move(vals);
> > +}
>
> Why do you need take & set rather than juest adjust query_options
> move constructor?
query_options is constructed via
`cql_transport::request_reader::read_options`, I don't like the idea
of passing `computed_function_values` there, IMO it doesn't belong to
this function.
`take_cached_function_calls()` is needed to move the data out of
`query_options` from one shard and forward it to the other shard.


>
> > +
> > + // Cached `function_call` evaluation results. `function_call` AST nodes
> > + // are created for each function with side effects in a CQL query, i.e.
> > + // non-deterministic functions (`uuid()`, `now()` and some others
> > + // timeuuid-related).
> > + //
> > + // These nodes are evaluated either when a query itself is executed
> > + // or query restrictions are computed (e.g. partition/clustering
> > + // key ranges for LWT requests).
> > + //
> > + // We need to cache the calls since otherwise when handling a
> > + // `bounce_to_shard` request for an LWT query, we can possibly enter an
> > + // infinite bouncing loop (in case a function is used to calculate
> > + // partition key ranges for a query), since the results can be different
> > + // each time. Furthermore, we don't support bouncing more than one time.
> > + // Refs: #8604 (https://github.com/scylladb/scylla/issues/8604)
> > + //
> > + // FIXME: mutable is a hack! `query_state` is not available at
> > + // evaluation sites and we only have a const reference to `query_options`.
>
> I'd cut the comment a bit, highlighting the second part and
> killing the fixme, nobody's going to fix it anyway, better have a
> couple of mutable members than remove const specifier from all
> usages.
It's more-or-less easily fixable, actually. Maybe I would do this if I
have some spare time :)
FIXMEs immediately make other readers aware of the problems in the code.
I would like to see a FIXME pointing out what prevents me from writing
clean code. Otherwise, I would need to follow the call chain (which is
quite long) and in the end find myself in a situation where I need to
hack around anyway.

>
> > + mutable computed_function_values _cached_fn_calls;
> > private:
> > /**
> > * @brief Batch query_options constructor.
> > @@ -276,6 +298,13 @@ class query_options {
> > }
> >
> > void prepare(const std::vector<lw_shared_ptr<column_specification>>& specs);
> > +
> > + // FIXME: should be non-const since it's mutating internals!
> > + void cache_function_call(computed_function_values::key_type id, computed_function_values::mapped_type value) const;
> > + const computed_function_values& cached_function_calls() const;
> > + computed_function_values&& take_cached_function_calls();
> > + void set_cached_function_calls(computed_function_values vals);
>
> > + // const_cast is a hack! this should really be done via `query_state` but
>
> I think having 5 fixme's is a bit too much.
Avi, do we have some FIXME policy? If you also think it's too much,
I'll drop them.

Avi Kivity

unread,
Jul 29, 2021, 4:49:24 AMJul 29
to Konstantin Osipov, Pavel Solodovnikov, scylladb-dev

On 28/07/2021 21.46, Konstantin Osipov wrote:
> * Pavel Solodovnikov <pa.solo...@scylladb.com> [21/07/28 17:53]:
>> Review ping.
> Avi, are you happy with the fix now that caching only happens for
> partition keys used in LWT statements? This resolves all the
> concerns about the efficiency of the cache or possible misuse.
>

I am, will review.

Konstantin Osipov

unread,
Jul 29, 2021, 6:42:27 AMJul 29
to Pavel Solodovnikov, Avi Kivity, scylladb-dev
* Pavel Solodovnikov <pa.solo...@scylladb.com> [21/07/29 11:47]:
> > > + // the call is not considered when using or populating the cache.
> > > + std::optional<uint8_t> _id;
> > > + // This flag is set to `true` if the `function_call` AST element
> > > + // is a part of LWT statement.
> > > + bool _in_lwt_context;
> >
> > Actually it's set to true only for function calls in lwt primary
> > key context now.
> No, it's not. The combination `_in_lwt_context &&
> static_cast<bool>(_id)` would mean that a call is eligible for cache
> (i.e. partition key evaluation within an lwt statement).
> I don't like the idea to merge these two things, because they are set
> separately at different moments.

I did not understand this comment. Aren't you setting
_in_lwt_context now only inside is_processing_pk_restrictions?
>
> >
> >
> > > + void set_lwt_context() {
> > > + _in_lwt_context = true;
> > > + }
> >
> > Suggest to rename accordingly.
> >
> > > void
> > > function_call::fill_prepare_context(prepare_context& ctx) const {
> > > + if (ctx.is_processing_pk_restrictions()) {
> > > + constexpr auto fn_limit = std::numeric_limits<uint8_t>::max();
> > > + auto& fn_calls = ctx.function_calls();

Pavel Solodovnikov

unread,
Jul 29, 2021, 7:10:03 AMJul 29
to Konstantin Osipov, scylladb-dev
On Thu, Jul 29, 2021 at 1:42 PM Konstantin Osipov <kos...@scylladb.com> wrote:
>
> * Pavel Solodovnikov <pa.solo...@scylladb.com> [21/07/29 11:47]:
> > > > + // the call is not considered when using or populating the cache.
> > > > + std::optional<uint8_t> _id;
> > > > + // This flag is set to `true` if the `function_call` AST element
> > > > + // is a part of LWT statement.
> > > > + bool _in_lwt_context;
> > >
> > > Actually it's set to true only for function calls in lwt primary
> > > key context now.
> > No, it's not. The combination `_in_lwt_context &&
> > static_cast<bool>(_id)` would mean that a call is eligible for cache
> > (i.e. partition key evaluation within an lwt statement).
> > I don't like the idea to merge these two things, because they are set
> > separately at different moments.
>
> I did not understand this comment. Aren't you setting
> _in_lwt_context now only inside is_processing_pk_restrictions?
Yes, you are right. I misunderstood the question at first.
What I meant is that collecting `pk_fn_calls` and later iterating them
to `set_lwt_context()` are not done at the same time, they are
independent. But now I understand your suggestion to rename the flag
makes sense.

Avi Kivity

unread,
Jul 29, 2021, 8:29:58 AMJul 29
to Pavel Solodovnikov, scylla...@googlegroups.com
There is selectable::with_function::raw, which can accept database input.


Note: I have a series that begins to merge the with_function and
function_call, see https://github.com/scylladb/scylla/pull/9087.
This would be much, much cleaner in a world where the AST is a tree of
cql3::expr::expression nodes. I'm not suggesting to hold back, but we
can expect some simplification when we convert.


Perhaps we can replace the nodes that calculate the primary key
restrictions with nodes that look up from the stored cache. That is,
split the restriction expression into three parts, one to calculate the
primary key, another to calculate the shard, and a third that replaces
function calls with lookups. This makes it unnecessary to depend on
determinstic execution. e.g if we have


   UPDATE foo SET x = 1 WHERE pk1 = now() and pk2 = uuid() IF NOT EXISTS


we have the first expression "copy now() into cache[0]", the second is
"find shard for cache[0]", and the third is "the primary key is
cache[0]". So we won't be bouncing anything, just computing in stages,
with different stages happening on different shards.



> for (auto&& t : _terms) {
> t->fill_prepare_context(ctx);
> }
> @@ -454,7 +470,27 @@ function_call::bind_and_get(const query_options& options) {
> }
> buffers.push_back(to_bytes_opt(val));
> }
> + const bool use_value_cache = _in_lwt_context && !_fun->is_pure() && _id;


Isn't it enough to check _id? The you already know is_pure() at the
prepare stage. Can you know _in_lwt_context there too?

Avi Kivity

unread,
Jul 29, 2021, 8:30:59 AMJul 29
to Pavel Solodovnikov, scylla...@googlegroups.com

On 26/07/2021 15.31, Pavel Solodovnikov wrote:
Looks good. I think we can make it better but that will have to wait
until more expr-driven refactoring is done.

Pavel Solodovnikov

unread,
Jul 29, 2021, 9:34:41 AMJul 29
to Avi Kivity, scylladb-dev
That would be great.

>
>
>
> > for (auto&& t : _terms) {
> > t->fill_prepare_context(ctx);
> > }
> > @@ -454,7 +470,27 @@ function_call::bind_and_get(const query_options& options) {
> > }
> > buffers.push_back(to_bytes_opt(val));
> > }
> > + const bool use_value_cache = _in_lwt_context && !_fun->is_pure() && _id;
>
>
> Isn't it enough to check _id? The you already know is_pure() at the
> prepare stage. Can you know _in_lwt_context there too?
Indeed, it is. `!_fun->is_pure()` can be checked at
`function_call::fill_prepare_context`. Then AST nodes are only added
to prepare cache in case of non-pure functions. Also,
`_in_lwt_context` isn't needed, because we can just clear the fn calls
vector at the end of `raw::modification_statement::prepare()` in case
of non-lwt statements instead of setting a boolean flag and later
checking it.

This ensures that the `prepare_context::pk_function_calls()` is not
empty only for LWT statements and every entry is a non-pure function.

I'll fix it in v6.

Pavel Solodovnikov

unread,
Jul 29, 2021, 6:29:45 PMJul 29
to scylla...@googlegroups.com, Pavel Solodovnikov
`function_call` AST nodes are created for each function
with side effects in a CQL query, i.e. non-deterministic
functions (`uuid()`, `now()` and some others timeuuid-related).

These nodes are evaluated either when a query itself is executed
or query restrictions are computed (e.g. partition/clustering
key ranges for LWT requests).

We need to cache the calls since otherwise when handling a
`bounce_to_shard` request for an LWT query, we can possibly
enter an infinite bouncing loop (in case a function is used
to calculate partition key ranges for a query), since the
results can be different each time.

Furthermore, we don't support bouncing more than one time.
Returning `bounce_to_shard` message more than one time
will result in a crash.

Caching works only for LWT statements and only for the function
calls that affect partition key range computation for the query.

`variable_specifications` class is renamed to `prepare_context`
and generalized to record information about each `function_call`
AST node and modify them, as needed:
* Check whether a given function call is a part of partition key
statement restriction.
* Assign ids for caching if above is true and the call is a part
of an LWT statement.

There is no need to include any kind of statement identifier
in the cache key since `query_options` (which holds the cache)
is limited to a single statement, anyway.

Function calls are indexed by the order in which they appear
within a statement while parsing. There is no need to
include any kind of statement identifier to the cache key
since `query_options` (which holds the cache) is limited
to a single statement, anyway.

Note that `function_call::raw` AST nodes are not created
for selection clauses of a SELECT statement hence they
can only accept only one of the following things as parameters:
* Other function calls.
* Literal values.
* Parameter markers.

In other words, only parameters that can be immediately reduced
to a byte buffer are allowed and we don't need to handle
database inputs to non-pure functions separately since they
are not possible in this context. Anyhow, we don't even have
a single non-pure function that accepts arguments, so precautions
are not needed at the moment.

Add a test written in `cql-pytest` framework to verify
that both prepared and unprepared lwt statements handle
`bounce_to_shard` messages correctly in such scenario.

Fixes: #8604

Tests: unit(dev, debug)

NOTE: the patchset uses `query_options` as a container for
cached values. This doesn't look clean and `service::query_state`
seems to be a better place to store them. But it's not
forwarded to most of the CQL code and would mean that a huge number
of places would have to be amended.
The series presents a trade-off to avoid forwarding `query_state`
everywhere (but maybe it's the thing that needs to be done, nonetheless).

Git URL: https://github.com/ManManson/scylla/commits/lwt_bounce_to_shard_cached_fn_v6

Changelog:

v6:
* Remove `function_call::_in_lwt_context` use `function_call::_id`
for the same purpose (just reset the ids at
`modification_statement::prepare()` if it's not an LWT statement).
* Move check `!_fun->is_pure()` from execution
step (`function_call::bind_and_get`) to an earlier prepare
step (`function_call::fill_prepare_context`).
* Adjust the comments appropriately.
* Remove some extra FIXMEs.
* Rename all `function_calls()` functions to `pk_function_calls()`
to emphasize that these are function calls from partition key
restrictions.
* Move fn insertion logic from `function_call::fill_prepare_context`
to a separate `prepare_context::add_pk_function_call`.
* Wrap look up logic for pk fn values into
`query_options::find_cached_pk_function_call`.
to the `function_call::bind_and_get()` to populate the cache only
for LWT statements.
* Fixed access functions in `query_options` to be able to move
the cache info out of the instance.
* Do not copy cache from individual sub-statements for batch statement,
instead move them, as well.
* Explicitly check for non-pure functions in
`function_call::bind_and_get()` since `function_call` nodes can
represent not only non-pure functions.
* Fix some missing std::move's when constucting a `bounce_to_shard`
message.
* Add some more comments.
* Rename `test_cql_non_deterministic_functions_lwt.py` to
just `test_non_deterministic_functions.py`.

Pavel Solodovnikov (3):
cql3: rename `variable_specifications` to `prepare_context`
cql3: cache function calls evaluation for non-deterministic functions
cql-pytest: add a test for non-pure CQL functions

CMakeLists.txt | 2 +-
configure.py | 2 +-
cql3/abstract_marker.cc | 6 +-
cql3/abstract_marker.hh | 4 +-
cql3/attributes.cc | 8 +--
cql3/attributes.hh | 4 +-
cql3/column_condition.cc | 8 +--
cql3/column_condition.hh | 2 +-
cql3/functions/function_call.hh | 13 +++-
cql3/functions/functions.cc | 30 ++++++++-
cql3/lists.cc | 8 +--
cql3/lists.hh | 4 +-
cql3/maps.cc | 8 +--
cql3/maps.hh | 4 +-
cql3/multi_column_relation.hh | 22 +++----
cql3/operation.hh | 6 +-
...e_specifications.cc => prepare_context.cc} | 40 ++++++++----
...e_specifications.hh => prepare_context.hh} | 54 ++++++++++++----
cql3/query_options.cc | 24 +++++++
cql3/query_options.hh | 29 +++++++++
cql3/relation.hh | 44 ++++++-------
cql3/restrictions/statement_restrictions.cc | 4 +-
cql3/restrictions/statement_restrictions.hh | 4 +-
cql3/sets.cc | 2 +-
cql3/sets.hh | 2 +-
cql3/single_column_relation.cc | 32 ++++++----
cql3/single_column_relation.hh | 16 ++---
cql3/statements/batch_statement.cc | 18 ++++--
cql3/statements/create_view_statement.cc | 2 +-
cql3/statements/delete_statement.cc | 10 +--
cql3/statements/modification_statement.cc | 53 +++++++++++----
cql3/statements/modification_statement.hh | 2 +-
cql3/statements/prepared_statement.hh | 6 +-
cql3/statements/raw/delete_statement.hh | 2 +-
cql3/statements/raw/insert_statement.hh | 4 +-
cql3/statements/raw/modification_statement.hh | 6 +-
cql3/statements/raw/parsed_statement.cc | 18 +++---
cql3/statements/raw/parsed_statement.hh | 8 +--
cql3/statements/raw/select_statement.hh | 4 +-
cql3/statements/raw/update_statement.hh | 2 +-
cql3/statements/select_statement.cc | 33 +++++-----
cql3/statements/update_statement.cc | 28 ++++----
cql3/term.hh | 6 +-
cql3/token_relation.cc | 18 +++---
cql3/token_relation.hh | 12 ++--
cql3/tuples.hh | 4 +-
cql3/user_types.cc | 4 +-
cql3/user_types.hh | 2 +-
test/boost/statement_restrictions_test.cc | 4 +-
.../test_non_deterministic_functions.py | 51 +++++++++++++++
transport/messages/result_message.hh | 9 ++-
transport/server.cc | 64 ++++++++++++-------
transport/server.hh | 2 +-
53 files changed, 502 insertions(+), 252 deletions(-)
rename cql3/{variable_specifications.cc => prepare_context.cc} (66%)
rename cql3/{variable_specifications.hh => prepare_context.hh} (50%)

Pavel Solodovnikov

unread,
Jul 29, 2021, 6:29:47 PMJul 29
to scylla...@googlegroups.com, Pavel Solodovnikov
The class is repurposed to be more generic and also be able
to hold additional metadata related to function calls within
a CQL statement. Rename all methods appropriately.

Visitor functions in AST nodes (`collect_marker_specification`)
are also renamed to a more generic `fill_prepare_context`.

The name `prepare_context` designates that this metadata
structure is a byproduct of `stmt::raw::prepare()` call and
is needed only for "prepare" step of query execution.

Signed-off-by: Pavel Solodovnikov <pa.solo...@scylladb.com>
---
CMakeLists.txt | 2 +-
configure.py | 2 +-
cql3/abstract_marker.cc | 6 +--
cql3/abstract_marker.hh | 4 +-
cql3/attributes.cc | 8 ++--
cql3/attributes.hh | 4 +-
cql3/column_condition.cc | 8 ++--
cql3/column_condition.hh | 2 +-
cql3/functions/function_call.hh | 2 +-
cql3/functions/functions.cc | 4 +-
cql3/lists.cc | 8 ++--
cql3/lists.hh | 4 +-
cql3/maps.cc | 8 ++--
cql3/maps.hh | 4 +-
cql3/multi_column_relation.hh | 22 +++++-----
cql3/operation.hh | 6 +--
...e_specifications.cc => prepare_context.cc} | 24 +++++-----
...e_specifications.hh => prepare_context.hh} | 26 +++++------
cql3/relation.hh | 44 +++++++++----------
cql3/restrictions/statement_restrictions.cc | 4 +-
cql3/restrictions/statement_restrictions.hh | 4 +-
cql3/sets.cc | 2 +-
cql3/sets.hh | 2 +-
cql3/single_column_relation.cc | 22 +++++-----
cql3/single_column_relation.hh | 16 +++----
cql3/statements/batch_statement.cc | 12 ++---
cql3/statements/create_view_statement.cc | 2 +-
cql3/statements/delete_statement.cc | 10 ++---
cql3/statements/modification_statement.cc | 22 +++++-----
cql3/statements/modification_statement.hh | 2 +-
cql3/statements/prepared_statement.hh | 6 +--
cql3/statements/raw/delete_statement.hh | 2 +-
cql3/statements/raw/insert_statement.hh | 4 +-
cql3/statements/raw/modification_statement.hh | 6 +--
cql3/statements/raw/parsed_statement.cc | 18 ++++----
cql3/statements/raw/parsed_statement.hh | 8 ++--
cql3/statements/raw/select_statement.hh | 4 +-
cql3/statements/raw/update_statement.hh | 2 +-
cql3/statements/select_statement.cc | 30 ++++++-------
cql3/statements/update_statement.cc | 28 ++++++------
cql3/term.hh | 6 +--
cql3/token_relation.cc | 18 ++++----
cql3/token_relation.hh | 12 ++---
cql3/tuples.hh | 4 +-
cql3/user_types.cc | 4 +-
cql3/user_types.hh | 2 +-
test/boost/statement_restrictions_test.cc | 4 +-
diff --git a/cql3/functions/function_call.hh b/cql3/functions/function_call.hh
index d4c4fec9df..ba4485a72f 100644
--- a/cql3/functions/function_call.hh
+++ b/cql3/functions/function_call.hh
@@ -57,7 +57,7 @@ class function_call : public non_terminal {
function_call(shared_ptr<scalar_function> fun, std::vector<shared_ptr<term>> terms)
: _fun(std::move(fun)), _terms(std::move(terms)) {
}
- virtual void collect_marker_specification(variable_specifications& bound_names) const override;
+ virtual void fill_prepare_context(prepare_context& ctx) const override;
virtual shared_ptr<terminal> bind(const query_options& options) override;
virtual cql3::raw_value_view bind_and_get(const query_options& options) override;
private:
diff --git a/cql3/functions/functions.cc b/cql3/functions/functions.cc
index d855706912..ff0d7a2fa9 100644
--- a/cql3/functions/functions.cc
+++ b/cql3/functions/functions.cc
@@ -430,9 +430,9 @@ functions::type_equals(const std::vector<data_type>& t1, const std::vector<data_
}

void
+ virtual void fill_prepare_context(prepare_context& ctx) const override;
virtual shared_ptr<terminal> bind(const query_options& options) override;
@@ -55,7 +55,11 @@ namespace cql3 {
class column_identifier;
class column_specification;

-class variable_specifications final {
+/**
+ * A metadata class currently holding bind variables specifications and
+ * populated at "prepare" step of query execution.
+ */
+class prepare_context final {
private:
std::vector<shared_ptr<column_identifier>> _variable_names;
std::vector<lw_shared_ptr<column_specification>> _specs;
+ virtual void fill_prepare_context(prepare_context& ctx) const override;
virtual shared_ptr<terminal> bind(const query_options& options);
};

diff --git a/cql3/single_column_relation.cc b/cql3/single_column_relation.cc
index fa004a2984..9aee583d5f 100644
--- a/cql3/single_column_relation.cc
+++ b/cql3/single_column_relation.cc
@@ -57,26 +57,26 @@ single_column_relation::to_term(const std::vector<lw_shared_ptr<column_specifica
const term::raw& raw,
database& db,
const sstring& keyspace,
- variable_specifications& bound_names) const {
+ prepare_context& ctx) const {
// TODO: optimize vector away, accept single column_specification
assert(receivers.size() == 1);
auto term = raw.prepare(db, keyspace, receivers[0]);
- term->collect_marker_specification(bound_names);
+ term->fill_prepare_context(ctx);
return term;
}

::shared_ptr<restrictions::restriction>
-single_column_relation::new_EQ_restriction(database& db, schema_ptr schema, variable_specifications& bound_names) {
+single_column_relation::new_EQ_restriction(database& db, schema_ptr schema, prepare_context& ctx) {
const column_definition& column_def = to_column_definition(*schema, *_entity);
if (!_map_key) {
auto r = ::make_shared<restrictions::single_column_restriction>(column_def);
- auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), ctx);
r->expression = binary_operator{&column_def, expr::oper_t::EQ, std::move(term)};
return r;
}
auto&& receivers = to_receivers(*schema, column_def);
- auto&& entry_key = to_term({receivers[0]}, *_map_key, db, schema->ks_name(), bound_names);
- auto&& entry_value = to_term({receivers[1]}, *_value, db, schema->ks_name(), bound_names);
+ auto&& entry_key = to_term({receivers[0]}, *_map_key, db, schema->ks_name(), ctx);
+ auto&& entry_value = to_term({receivers[1]}, *_value, db, schema->ks_name(), ctx);
auto r = make_shared<restrictions::single_column_restriction>(column_def);
r->expression = binary_operator{
column_value(&column_def, std::move(entry_key)), oper_t::EQ, std::move(entry_value)};
@@ -84,18 +84,18 @@ single_column_relation::new_EQ_restriction(database& db, schema_ptr schema, vari
}

::shared_ptr<restrictions::restriction>
-single_column_relation::new_IN_restriction(database& db, schema_ptr schema, variable_specifications& bound_names) {
+single_column_relation::new_IN_restriction(database& db, schema_ptr schema, prepare_context& ctx) {
using namespace restrictions;
const column_definition& column_def = to_column_definition(*schema, *_entity);
auto receivers = to_receivers(*schema, column_def);
assert(_in_values.empty() || !_value);
if (_value) {
- auto term = to_term(receivers, *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(receivers, *_value, db, schema->ks_name(), ctx);
auto r = ::make_shared<single_column_restriction>(column_def);
r->expression = binary_operator{&column_def, expr::oper_t::IN, std::move(term)};
return r;
}
- auto terms = to_terms(receivers, _in_values, db, schema->ks_name(), bound_names);
+ auto terms = to_terms(receivers, _in_values, db, schema->ks_name(), ctx);
// Convert a single-item IN restriction to an EQ restriction
if (terms.size() == 1) {
auto r = ::make_shared<single_column_restriction>(column_def);
@@ -110,13 +110,13 @@ single_column_relation::new_IN_restriction(database& db, schema_ptr schema, vari

::shared_ptr<restrictions::restriction>
single_column_relation::new_LIKE_restriction(
- database& db, schema_ptr schema, variable_specifications& bound_names) {
+ database& db, schema_ptr schema, prepare_context& ctx) {
const column_definition& column_def = to_column_definition(*schema, *_entity);
if (!column_def.type->is_string()) {
throw exceptions::invalid_request_exception(
format("LIKE is allowed only on string types, which {} is not", column_def.name_as_text()));
}
- auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), ctx);
auto r = ::make_shared<restrictions::single_column_restriction>(column_def);
- auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), ctx);
auto r = ::make_shared<restrictions::single_column_restriction>(column_def);
r->expression = expr::binary_operator{&column_def, _relation_type, std::move(term)};
return r;
}

virtual shared_ptr<restrictions::restriction> new_contains_restriction(database& db, schema_ptr schema,
- variable_specifications& bound_names,
+ prepare_context& ctx,
bool is_key) override {
auto&& column_def = to_column_definition(*schema, *_entity);
- auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), bound_names);
+ auto term = to_term(to_receivers(*schema, column_def), *_value, db, schema->ks_name(), ctx);
auto r = ::make_shared<restrictions::single_column_restriction>(column_def);
r->expression = expr::binary_operator{
&column_def, is_key ? expr::oper_t::CONTAINS_KEY : expr::oper_t::CONTAINS, std::move(term)};
@@ -187,7 +187,7 @@ class single_column_relation final : public relation {
}

virtual ::shared_ptr<restrictions::restriction> new_LIKE_restriction(
- database& db, schema_ptr schema, variable_specifications& bound_names) override;
+ database& db, schema_ptr schema, prepare_context& ctx) override;

virtual ::shared_ptr<relation> maybe_rename_identifier(const column_identifier::raw& from, col