From 2babee2cdcac567128af3021a1ca8ebec80e0bb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Radwa=C5=84ski?= Date: Tue, 17 May 2022 15:48:27 +0200 Subject: [PATCH] column_computation.hh, schema.cc: compute_value interface refactor The compute_value function of column_computation has had previously the following signature: virtual bytes_opt compute_value(const schema& schema, const partition_key& key, const clustering_row& row) const override; This is superfluous, since never in the history of Scylla, the last parameter (row) was used in any implentation, and never did it happen that it returned bytes_opt. The absurdity of this interface can be seen especially when looking at call sites like following, where dummy empty row was created: ``` token_column.get_computation().compute_value( *_schema, pkv_linearized, clustering_row(clustering_key_prefix::make_empty())); ``` --- column_computation.hh | 8 ++++---- cql3/restrictions/statement_restrictions.cc | 11 +++-------- cql3/statements/select_statement.cc | 11 ++++------- db/view/view.cc | 7 ++----- schema.cc | 6 +++--- 5 files changed, 16 insertions(+), 27 deletions(-) diff --git a/column_computation.hh b/column_computation.hh index 48e88b2587..c779f66e84 100644 --- a/column_computation.hh +++ b/column_computation.hh @@ -22,7 +22,7 @@ using column_computation_ptr = std::unique_ptr; * Computed columns description is also available at docs/dev/system_schema_keyspace.md. They hold values * not provided directly by the user, but rather computed: from other column values and possibly other sources. * This class is able to serialize/deserialize column computations and perform the computation itself, - * based on given schema, partition key and clustering row. Responsibility for providing enough data + * based on given schema, and partition key. Responsibility for providing enough data * in the clustering row in order for computation to succeed belongs to the caller. In particular, * generating a value might involve performing a read-before-write if the computation is performed * on more values than are present in the update request. @@ -36,7 +36,7 @@ public: virtual column_computation_ptr clone() const = 0; virtual bytes serialize() const = 0; - virtual bytes_opt compute_value(const schema& schema, const partition_key& key, const clustering_row& row) const = 0; + virtual bytes compute_value(const schema& schema, const partition_key& key) const = 0; }; /* @@ -54,7 +54,7 @@ public: return std::make_unique(*this); } virtual bytes serialize() const override; - virtual bytes_opt compute_value(const schema& schema, const partition_key& key, const clustering_row& row) const override; + virtual bytes compute_value(const schema& schema, const partition_key& key) const override; }; @@ -75,5 +75,5 @@ public: return std::make_unique(*this); } virtual bytes serialize() const override; - virtual bytes_opt compute_value(const schema& schema, const partition_key& key, const clustering_row& row) const override; + virtual bytes compute_value(const schema& schema, const partition_key& key) const override; }; diff --git a/cql3/restrictions/statement_restrictions.cc b/cql3/restrictions/statement_restrictions.cc index 39368ca99d..e844e0c51f 100644 --- a/cql3/restrictions/statement_restrictions.cc +++ b/cql3/restrictions/statement_restrictions.cc @@ -1879,17 +1879,12 @@ std::vector statement_restrictions::get_global_index_cl std::transform(pk_value.cbegin(), pk_value.cend(), pkv_linearized.begin(), [] (const managed_bytes& mb) { return to_bytes(mb); }); auto& token_column = idx_tbl_schema.clustering_column_at(0); - bytes_opt token_bytes = token_column.get_computation().compute_value( - *_schema, pkv_linearized, clustering_row(clustering_key_prefix::make_empty())); - if (!token_bytes) { - on_internal_error(rlogger, - format("null value for token column in indexing table {}", - token_column.name_as_text())); - } + bytes token_bytes = token_column.get_computation().compute_value(*_schema, pkv_linearized); + // WARNING: We must not yield to another fiber from here until the function's end, lest this RHS be // overwritten. const_cast(expr::as((*_idx_tbl_ck_prefix)[0]).rhs) = - expr::constant(raw_value::make_value(*token_bytes), token_column.type); + expr::constant(raw_value::make_value(token_bytes), token_column.type); // Multi column restrictions are not added to _idx_tbl_ck_prefix, they are handled later by filtering. return get_single_column_clustering_bounds(options, idx_tbl_schema, *_idx_tbl_ck_prefix); diff --git a/cql3/statements/select_statement.cc b/cql3/statements/select_statement.cc index b97c047c55..95d2f1db75 100644 --- a/cql3/statements/select_statement.cc +++ b/cql3/statements/select_statement.cc @@ -965,18 +965,15 @@ static void append_base_key_to_index_ck(std::vector& explode bytes indexed_table_select_statement::compute_idx_token(const partition_key& key) const { const column_definition& cdef = *_view_schema->clustering_key_columns().begin(); clustering_row empty_row(clustering_key_prefix::make_empty()); - bytes_opt computed_value; + bytes computed_value; if (!cdef.is_computed()) { // FIXME(pgrabowski): this legacy code is here for backward compatibility and should be removed // once "computed_columns feature" is supported by every node - computed_value = legacy_token_column_computation().compute_value(*_schema, key, empty_row); + computed_value = legacy_token_column_computation().compute_value(*_schema, key); } else { - computed_value = cdef.get_computation().compute_value(*_schema, key, empty_row); + computed_value = cdef.get_computation().compute_value(*_schema, key); } - if (!computed_value) { - throw std::logic_error(format("No value computed for idx_token column {}", cdef.name())); - } - return *computed_value; + return computed_value; } lw_shared_ptr indexed_table_select_statement::generate_view_paging_state_from_base_query_results(lw_shared_ptr paging_state, diff --git a/db/view/view.cc b/db/view/view.cc index 06d433f588..1c8a1d6bc5 100644 --- a/db/view/view.cc +++ b/db/view/view.cc @@ -495,12 +495,9 @@ deletable_row& view_updates::get_view_row(const partition_key& base_key, const c if (!service::get_local_storage_proxy().local_db().find_column_family(_base->id()).get_index_manager().is_index(*_view)) { throw std::logic_error(format("Column {} doesn't exist in base and this view is not backing a secondary index", cdef.name_as_text())); } - computed_value = legacy_token_column_computation().compute_value(*_base, base_key, update); + computed_value = legacy_token_column_computation().compute_value(*_base, base_key); } else { - computed_value = cdef.get_computation().compute_value(*_base, base_key, update); - } - if (!computed_value) { - throw std::logic_error(format("No value computed for primary key column {}", cdef.name())); + computed_value = cdef.get_computation().compute_value(*_base, base_key); } return managed_bytes_view(linearized_values.emplace_back(*computed_value)); } diff --git a/schema.cc b/schema.cc index 78e0a5114c..96db89c797 100644 --- a/schema.cc +++ b/schema.cc @@ -1706,8 +1706,8 @@ bytes legacy_token_column_computation::serialize() const { return to_bytes(rjson::print(serialized)); } -bytes_opt legacy_token_column_computation::compute_value(const schema& schema, const partition_key& key, const clustering_row& row) const { - return dht::get_token(schema, key).data(); +bytes legacy_token_column_computation::compute_value(const schema& schema, const partition_key& key) const { + return {dht::get_token(schema, key).data()}; } bytes token_column_computation::serialize() const { @@ -1716,7 +1716,7 @@ bytes token_column_computation::serialize() const { return to_bytes(rjson::print(serialized)); } -bytes_opt token_column_computation::compute_value(const schema& schema, const partition_key& key, const clustering_row& row) const { +bytes token_column_computation::compute_value(const schema& schema, const partition_key& key) const { auto long_value = dht::token::to_int64(dht::get_token(schema, key)); return long_type->decompose(long_value); }