cql3: improve readability of prepare_context

This commits adds a few comments and changes a few variable names
so that it's easier to figure out what the code does.

When I first started looking at this part of the code it wasn't
obvious what's going on - what are _specs, how are they different
from _target_columns? What happens when a variable doesn't have a name?

I hope that this change will make it easier to understand for future readers.

Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>

cql3/Cql.g

@@ -147,8 +147,11 @@ using uexpression = uninitialized<expression>;
 
     listener_type* listener;
 
-    std::vector<::shared_ptr<cql3::column_identifier>> _bind_variables;
-    // index into _bind_variables
+    // Keeps the names of all bind variables. For bind variables without a name ('?'), the name is nullptr.
+    // Maps bind_index -> name.
+    std::vector<::shared_ptr<cql3::column_identifier>> _bind_variable_names;
+
+    // Maps name -> bind_index for all named bind variables.
     std::unordered_map<cql3::column_identifier, size_t> _named_bind_variables_indexes;
     std::vector<std::unique_ptr<TokenType>> _missing_tokens;
 
@@ -172,8 +175,8 @@ using uexpression = uninitialized<expression>;
         if (name && _named_bind_variables_indexes.contains(*name)) {
             return bind_variable{_named_bind_variables_indexes[*name]};
         }
-        auto marker = bind_variable{_bind_variables.size()};
-        _bind_variables.push_back(name);
+        auto marker = bind_variable{_bind_variable_names.size()};
+        _bind_variable_names.push_back(name);
         if (name) {
             _named_bind_variables_indexes[*name] = marker.bind_index;
         }
@@ -321,7 +324,7 @@ query returns [std::unique_ptr<raw::parsed_statement> stmnt]
     ;
 
 cqlStatement returns [std::unique_ptr<raw::parsed_statement> stmt]
-    @after{ if (stmt) { stmt->set_bound_variables(_bind_variables); } }
+    @after{ if (stmt) { stmt->set_bound_variables(_bind_variable_names); } }
     : st1= selectStatement             { $stmt = std::move(st1); }
     | st2= insertStatement             { $stmt = std::move(st2); }
     | st3= updateStatement             { $stmt = std::move(st3); }

cql3/prepare_context.cc

@@ -19,11 +19,11 @@ size_t prepare_context::bound_variables_size() const {
 }
 
 const std::vector<lw_shared_ptr<column_specification>>& prepare_context::get_variable_specifications() const & {
-    return _specs;
+    return _variable_specs;
 }
 
 std::vector<lw_shared_ptr<column_specification>> prepare_context::get_variable_specifications() && {
-    return std::move(_specs);
+    return std::move(_variable_specs);
 }
 
 std::vector<uint16_t> prepare_context::get_partition_key_bind_indexes(const schema& schema) const {
@@ -48,12 +48,12 @@ std::vector<uint16_t> prepare_context::get_partition_key_bind_indexes(const sche
 
 void prepare_context::add_variable_specification(int32_t bind_index, lw_shared_ptr<column_specification> spec) {
     auto name = _variable_names[bind_index];
-    if (_specs[bind_index]) {
+    if (_variable_specs[bind_index]) {
         // If the same variable is used in multiple places, check that the types are compatible
-        if (&spec->type->without_reversed() != &_specs[bind_index]->type->without_reversed()) {
+        if (&spec->type->without_reversed() != &_variable_specs[bind_index]->type->without_reversed()) {
             throw exceptions::invalid_request_exception(
                     fmt::format("variable :{} has type {} which doesn't match {}",
-                            *name, _specs[bind_index]->type->as_cql3_type(), spec->name));
+                            *name, _variable_specs[bind_index]->type->as_cql3_type(), spec->name));
         }
     }
     _target_columns[bind_index] = spec;
@@ -61,16 +61,16 @@ void prepare_context::add_variable_specification(int32_t bind_index, lw_shared_p
     if (name) {
         spec = make_lw_shared<column_specification>(spec->ks_name, spec->cf_name, name, spec->type);
     }
-    _specs[bind_index] = spec;
+    _variable_specs[bind_index] = spec;
 }
 
-void prepare_context::set_bound_variables(const std::vector<shared_ptr<column_identifier>>& prepare_meta) {
-    _variable_names = prepare_meta;
-    _specs.clear();
+void prepare_context::set_bound_variables(const std::vector<shared_ptr<column_identifier>>& bind_variable_names) {
+    _variable_names = bind_variable_names;
+    _variable_specs.clear();
     _target_columns.clear();
 
-    const size_t bn_size = prepare_meta.size();
-    _specs.resize(bn_size);
+    const size_t bn_size = bind_variable_names.size();
+    _variable_specs.resize(bn_size);
     _target_columns.resize(bn_size);
 }
 

cql3/prepare_context.hh

@@ -33,9 +33,17 @@ namespace functions { class function_call; }
  */
 class prepare_context final {
 private:
+    // Keeps names of all the bind variables. For bind variables without a name ('?'), the name is nullptr.
+    // Maps bind_index -> name.
     std::vector<shared_ptr<column_identifier>> _variable_names;
-    std::vector<lw_shared_ptr<column_specification>> _specs;
+
+    // Keeps column_specification for every bind_index. column_specification describes the name and type of this variable.
+    // It's different from _target_columns because it describes the name of this variable, not the target column.
+    std::vector<lw_shared_ptr<column_specification>> _variable_specs;
+
+    // The column to which bind variable with a given bind_index will be assigned to.
     std::vector<lw_shared_ptr<column_specification>> _target_columns;
+
     // A list of pointers to prepared `function_call` cache ids, that
     // participate in partition key ranges computation within an LWT statement.
     std::vector<::shared_ptr<std::optional<uint8_t>>> _pk_function_calls_cache_ids;
@@ -61,7 +69,7 @@ public:
 
     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);
+    void set_bound_variables(const std::vector<shared_ptr<column_identifier>>& bind_variable_names);
 
     void clear_pk_function_calls_cache();