From c80999fab4704d4ba2b3b7248bfb4299a63adcaf Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 12 Jun 2022 13:22:39 +0300 Subject: [PATCH] cql3: expr: push is_satisfied_by regular and static column extraction to callers is_satisfied_by() rearranges the static and regular columns from query::result_row_view form (which is a use-once iterator) to std::vector (which uses the standard value representation, and allows random access which expression evaluation needs). Doing it in is_saitisfied_by() means that it is done every time an expression is evaluated, which is wasteful. It's also done even if the expression doesn't need it at all. Push it out to callers, which already eliminates some calls. We still pass cql3::expr::selection, which is a layering violation, but that is left to another time. Note that in view.cc's check_if_matches(), we should have been able to move static_and_regular_columns calculation outside the loop. However, we get crashes if we do. This is likely due to a preexisting bug (which the zero iterations loop avoids). However, in selection.cc, we are able to avoid the computation when the code claims it is only handling partition keys or clustering keys. --- cql3/expr/expression.cc | 11 +++++++++-- cql3/expr/expression.hh | 7 +++++-- cql3/selection/selection.cc | 16 ++++++++-------- db/view/view.cc | 12 +++++------- 4 files changed, 27 insertions(+), 19 deletions(-) diff --git a/cql3/expr/expression.cc b/cql3/expr/expression.cc index 2b5169d5fe..e9f8699362 100644 --- a/cql3/expr/expression.cc +++ b/cql3/expr/expression.cc @@ -456,6 +456,8 @@ managed_bytes_opt next_value(query::result_row_view::iterator_type& iter, const return std::nullopt; } +} // anonymous namespace + /// Returns values of non-primary-key columns from selection. The kth element of the result /// corresponds to the kth column in selection. std::vector get_non_pk_values(const selection& selection, const query::result_row_view& static_row, @@ -481,6 +483,8 @@ std::vector get_non_pk_values(const selection& selection, con return vals; } +namespace { + /// True iff cv matches the CQL LIKE pattern. bool like(const column_value& cv, const raw_value_view& pattern, const column_value_eval_bag& bag) { if (!cv.col->type->is_string()) { @@ -796,9 +800,12 @@ expression make_conjunction(expression a, expression b) { } bool is_satisfied_by(const expression& restr, const evaluation_inputs& inputs) { - const auto regulars = get_non_pk_values(*inputs.selection, *inputs.static_row, inputs.row); + static const auto dummy_static_and_regular_columns = std::vector(); + auto& static_and_regular_columns = inputs.static_and_regular_columns + ? *inputs.static_and_regular_columns + : dummy_static_and_regular_columns; return is_satisfied_by( - restr, {*inputs.options, row_data_from_partition_slice{*inputs.partition_key, *inputs.clustering_key, regulars, *inputs.selection}}); + restr, {*inputs.options, row_data_from_partition_slice{*inputs.partition_key, *inputs.clustering_key, static_and_regular_columns, *inputs.selection}}); } template diff --git a/cql3/expr/expression.hh b/cql3/expr/expression.hh index a3c614a381..9c2b2d026c 100644 --- a/cql3/expr/expression.hh +++ b/cql3/expr/expression.hh @@ -451,12 +451,15 @@ extern std::ostream& operator<<(std::ostream&, oper_t); struct evaluation_inputs { const std::vector* partition_key = nullptr; const std::vector* clustering_key = nullptr; - const query::result_row_view* static_row = nullptr; - const query::result_row_view* row = nullptr; + const std::vector* static_and_regular_columns = nullptr; // indexes match `selection` member const cql3::selection::selection* selection = nullptr; const query_options* options = nullptr; }; +/// Helper for generating evaluation_inputs::static_and_regular_columns +std::vector get_non_pk_values(const cql3::selection::selection& selection, const query::result_row_view& static_row, + const query::result_row_view* row); + /// True iff restr evaluates to true, given these inputs extern bool is_satisfied_by( const expression& restr, const evaluation_inputs& inputs); diff --git a/cql3/selection/selection.cc b/cql3/selection/selection.cc index 86a45b39b3..b8056243ef 100644 --- a/cql3/selection/selection.cc +++ b/cql3/selection/selection.cc @@ -426,13 +426,14 @@ bool result_set_builder::restrictions_filter::do_filter(const selection& selecti auto clustering_columns_restrictions = _restrictions->get_clustering_columns_restrictions(); if (dynamic_pointer_cast(clustering_columns_restrictions)) { clustering_key_prefix ckey = clustering_key_prefix::from_exploded(clustering_key); + // FIXME: push to upper layer so it happens once per row + auto static_and_regular_columns = expr::get_non_pk_values(selection, static_row, row); return expr::is_satisfied_by( clustering_columns_restrictions->expression, expr::evaluation_inputs{ .partition_key = &partition_key, .clustering_key = &clustering_key, - .static_row = &static_row, - .row = row, + .static_and_regular_columns = &static_and_regular_columns, .selection = &selection, .options = &_options, }); @@ -454,13 +455,14 @@ bool result_set_builder::restrictions_filter::do_filter(const selection& selecti continue; } restrictions::single_column_restriction& restriction = *restr_it->second; + // FIXME: push to upper layer so it happens once per row + auto static_and_regular_columns = expr::get_non_pk_values(selection, static_row, row); bool regular_restriction_matches = expr::is_satisfied_by( restriction.expression, expr::evaluation_inputs{ .partition_key = &partition_key, .clustering_key = &clustering_key, - .static_row = &static_row, - .row = row, + .static_and_regular_columns = &static_and_regular_columns, .selection = &selection, .options = &_options, }); @@ -485,8 +487,7 @@ bool result_set_builder::restrictions_filter::do_filter(const selection& selecti expr::evaluation_inputs{ .partition_key = &partition_key, .clustering_key = &clustering_key, - .static_row = &static_row, - .row = row, + .static_and_regular_columns = nullptr, // partition key filtering only .selection = &selection, .options = &_options, })) { @@ -513,8 +514,7 @@ bool result_set_builder::restrictions_filter::do_filter(const selection& selecti expr::evaluation_inputs{ .partition_key = &partition_key, .clustering_key = &clustering_key, - .static_row = &static_row, - .row = row, + .static_and_regular_columns = nullptr, // clustering key checks only .selection = &selection, .options = &_options, })) { diff --git a/db/view/view.cc b/db/view/view.cc index 18073bdea5..243d8df8a5 100644 --- a/db/view/view.cc +++ b/db/view/view.cc @@ -249,8 +249,7 @@ bool partition_key_matches(const schema& base, const view_info& view, const dht: cql3::expr::evaluation_inputs{ .partition_key = &exploded_pk, .clustering_key = &exploded_ck, - .static_row = &dummy_row, - .row = &dummy_row, + .static_and_regular_columns = nullptr, // partition key filtering only .selection = selection.get(), .options = &dummy_options, }); @@ -267,7 +266,6 @@ bool clustering_prefix_matches(const schema& base, const view_info& view, const } auto selection = cql3::selection::selection::for_columns(base.shared_from_this(), ck_columns); uint64_t zero = 0; - auto dummy_row = query::result_row_view(ser::qr_row_view{simple_memory_input_stream(reinterpret_cast(&zero), 8)}); auto dummy_options = cql3::query_options({ }); // FIXME: pass nullptrs for some of theses dummies return cql3::expr::is_satisfied_by( @@ -275,8 +273,7 @@ bool clustering_prefix_matches(const schema& base, const view_info& view, const cql3::expr::evaluation_inputs{ .partition_key = &exploded_pk, .clustering_key = &exploded_ck, - .static_row = &dummy_row, - .row = &dummy_row, + .static_and_regular_columns = nullptr, // clustering key only filtering here .selection = selection.get(), .options = &dummy_options, }); @@ -340,6 +337,8 @@ public: return boost::algorithm::all_of( _view.select_statement().get_restrictions()->get_non_pk_restriction() | boost::adaptors::map_values, [&] (auto&& r) { + // FIXME: move outside all_of(). However, crashes. + auto static_and_regular_columns = cql3::expr::get_non_pk_values(*_selection, static_row, &row); // FIXME: pass dummy_options as nullptr auto dummy_options = cql3::query_options({}); return cql3::expr::is_satisfied_by( @@ -347,8 +346,7 @@ public: cql3::expr::evaluation_inputs{ .partition_key = &_pk, .clustering_key = &ck, - .static_row = &static_row, - .row = &row, + .static_and_regular_columns = &static_and_regular_columns, .selection = _selection.get(), .options = &dummy_options, });