From ea901fdb9d56e7a36f8658dd37549dc06edefadb Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Tue, 29 Nov 2022 10:27:41 +0200 Subject: [PATCH] cql3: expr: fold `null` into untyped_constant/constant Our `null` expression, after the prepare stage, is redundant with a `constant` expression containing the value NULL. Remove it. Its role in the unprepared stage is taken over by untyped_constant, which gains a new type_class enumeration to represent it. Some subtleties: - Usually, handling of null and untyped_constant, or null and constant was the same, so they are just folded into each other - LWT "like" operator now has to discriminate between a literal string and a literal NULL - prepare and test_assignment were folded into the corresponing untyped_constant functions. Some care had to be taken to preserve error messages. Closes #12118 --- cql3/Cql.g | 4 +- cql3/column_condition.cc | 2 +- cql3/expr/expression.cc | 27 ++++-------- cql3/expr/expression.hh | 17 ++------ cql3/expr/prepare_expr.cc | 48 ++++++++------------- cql3/expr/restrictions.cc | 2 +- cql3/restrictions/statement_restrictions.cc | 12 ------ cql3/selection/selectable.cc | 6 --- test/boost/expr_test.cc | 15 ++++--- 9 files changed, 41 insertions(+), 92 deletions(-) diff --git a/cql3/Cql.g b/cql3/Cql.g index 853b9568c5..739bd42863 100644 --- a/cql3/Cql.g +++ b/cql3/Cql.g @@ -1513,7 +1513,7 @@ value returns [expression value] | l=collectionLiteral { $value = std::move(l); } | u=usertypeLiteral { $value = std::move(u); } | t=tupleLiteral { $value = std::move(t); } - | K_NULL { $value = null(); } + | K_NULL { $value = make_untyped_null(); } | e=marker { $value = std::move(e); } ; @@ -1678,7 +1678,7 @@ relation returns [expression e] | K_TOKEN l=tupleOfIdentifiers type=relationType t=term { $e = binary_operator(token{std::move(l.elements)}, type, std::move(t)); } | name=cident K_IS K_NOT K_NULL { - $e = binary_operator(unresolved_identifier{std::move(name)}, oper_t::IS_NOT, null()); } + $e = binary_operator(unresolved_identifier{std::move(name)}, oper_t::IS_NOT, make_untyped_null()); } | name=cident K_IN marker1=marker { $e = binary_operator(unresolved_identifier{std::move(name)}, oper_t::IN, std::move(marker1)); } | name=cident K_IN in_values=singleColumnInValues diff --git a/cql3/column_condition.cc b/cql3/column_condition.cc index fa43212832..17cdc204d5 100644 --- a/cql3/column_condition.cc +++ b/cql3/column_condition.cc @@ -309,7 +309,7 @@ column_condition::raw::prepare(data_dictionary::database db, const sstring& keys if (_op == expr::oper_t::LIKE) { auto literal_term = expr::as_if(&*_value); - if (literal_term) { + if (literal_term && literal_term->partial_type != expr::untyped_constant::type_class::null) { // Pass matcher object const sstring& pattern = literal_term->raw_text; return column_condition::condition(receiver, std::move(collection_element_expression), diff --git a/cql3/expr/expression.cc b/cql3/expr/expression.cc index 0bfc07cf88..959404c780 100644 --- a/cql3/expr/expression.cc +++ b/cql3/expr/expression.cc @@ -607,9 +607,6 @@ bool is_satisfied_by(const expression& restr, const evaluation_inputs& inputs) { [] (const field_selection&) -> bool { on_internal_error(expr_logger, "is_satisfied_by: a field selection cannot serve as a restriction by itself"); }, - [] (const null&) -> bool { - on_internal_error(expr_logger, "is_satisfied_by: NULL cannot serve as a restriction by itself"); - }, [] (const bind_variable&) -> bool { on_internal_error(expr_logger, "is_satisfied_by: a bind variable cannot serve as a restriction by itself"); }, @@ -698,6 +695,14 @@ expression make_conjunction(expression a, expression b) { return conjunction{std::move(children)}; } +untyped_constant +make_untyped_null() { + return { + .partial_type = untyped_constant::type_class::null, + .raw_text = "null", + }; +} + std::vector boolean_factors(expression e) { std::vector ret; @@ -888,9 +893,6 @@ value_set possible_lhs_values(const column_definition* cdef, const expression& e [] (const field_selection&) -> value_set { on_internal_error(expr_logger, "possible_lhs_values: field selections are not supported as the LHS of a binary expression"); }, - [] (const null&) -> value_set { - on_internal_error(expr_logger, "possible_lhs_values: nulls are not supported as the LHS of a binary expression"); - }, [] (const bind_variable&) -> value_set { on_internal_error(expr_logger, "possible_lhs_values: bind variables are not supported as the LHS of a binary expression"); }, @@ -929,9 +931,6 @@ value_set possible_lhs_values(const column_definition* cdef, const expression& e [] (const field_selection&) -> value_set { on_internal_error(expr_logger, "possible_lhs_values: a field selection cannot serve as a restriction by itself"); }, - [] (const null&) -> value_set { - on_internal_error(expr_logger, "possible_lhs_values: a NULL cannot serve as a restriction by itself"); - }, [] (const bind_variable&) -> value_set { on_internal_error(expr_logger, "possible_lhs_values: a bind variable cannot serve as a restriction by itself"); }, @@ -1026,9 +1025,6 @@ secondary_index::index::supports_expression_v is_supported_by_helper(const expre [&] (const field_selection&) -> ret_t { on_internal_error(expr_logger, "is_supported_by: field selections are not supported as the LHS of a binary expression"); }, - [&] (const null&) -> ret_t { - on_internal_error(expr_logger, "is_supported_by: nulls are not supported as the LHS of a binary expression"); - }, [&] (const bind_variable&) -> ret_t { on_internal_error(expr_logger, "is_supported_by: bind variables are not supported as the LHS of a binary expression"); }, @@ -1189,10 +1185,6 @@ std::ostream& operator<<(std::ostream& os, const expression::printer& pr) { [&] (const field_selection& fs) { fmt::print(os, "({}.{})", to_printer(fs.structure), fs.field); }, - [&] (const null&) { - // FIXME: adjust tests and change to NULL - fmt::print(os, "null"); - }, [&] (const bind_variable&) { // FIXME: store and present bind variable name fmt::print(os, "?"); @@ -1599,7 +1591,6 @@ std::vector extract_single_column_restrictions_for_column(const expr void operator()(const function_call&) {} void operator()(const cast&) {} void operator()(const field_selection&) {} - void operator()(const null&) {} void operator()(const bind_variable&) {} void operator()(const untyped_constant&) {} void operator()(const tuple_constructor&) {} @@ -1758,7 +1749,6 @@ cql3::raw_value evaluate(const expression& e, const evaluation_inputs& inputs) { on_internal_error(expr_logger, "Can't evaluate a untyped_constant "); }, - [](const null&) { return cql3::raw_value::make_null(); }, [](const constant& c) { return c.value; }, [&](const bind_variable& bind_var) { return evaluate(bind_var, inputs); }, [&](const tuple_constructor& tup) { return evaluate(tup, inputs); }, @@ -2316,7 +2306,6 @@ void fill_prepare_context(expression& e, prepare_context& ctx) { fill_prepare_context(s.sub, ctx); }, [](untyped_constant&) {}, - [](null&) {}, [](constant&) {}, }, e); } diff --git a/cql3/expr/expression.hh b/cql3/expr/expression.hh index 3e49e4dcab..6a7af3990a 100644 --- a/cql3/expr/expression.hh +++ b/cql3/expr/expression.hh @@ -76,7 +76,6 @@ struct column_mutation_attribute; struct function_call; struct cast; struct field_selection; -struct null; struct bind_variable; struct untyped_constant; struct constant; @@ -96,7 +95,6 @@ concept ExpressionElement || std::same_as || std::same_as || std::same_as - || std::same_as || std::same_as || std::same_as || std::same_as @@ -117,7 +115,6 @@ concept invocable_on_expression && std::invocable && std::invocable && std::invocable - && std::invocable && std::invocable && std::invocable && std::invocable @@ -138,7 +135,6 @@ concept invocable_on_expression_ref && std::invocable && std::invocable && std::invocable - && std::invocable && std::invocable && std::invocable && std::invocable @@ -199,7 +195,6 @@ bool operator==(const expression& e1, const expression& e2); template concept LeafExpression = std::same_as - || std::same_as || std::same_as || std::same_as || std::same_as @@ -345,12 +340,6 @@ struct field_selection { friend bool operator==(const field_selection&, const field_selection&) = default; }; -struct null { - data_type type; // may be null before prepare - - friend bool operator==(const null&, const null&) = default; -}; - struct bind_variable { int32_t bind_index; @@ -364,13 +353,15 @@ struct bind_variable { // A constant which does not yet have a date type. It is partially typed // (we know if it's floating or int) but not sized. struct untyped_constant { - enum type_class { integer, floating_point, string, boolean, duration, uuid, hex }; + enum type_class { integer, floating_point, string, boolean, duration, uuid, hex, null }; type_class partial_type; sstring raw_text; friend bool operator==(const untyped_constant&, const untyped_constant&) = default; }; +untyped_constant make_untyped_null(); + // Represents a constant value with known value and type // For null and unset the type can sometimes be set to empty_type struct constant { @@ -435,7 +426,7 @@ struct usertype_constructor { struct expression::impl final { using variant_type = std::variant< conjunction, binary_operator, column_value, token, unresolved_identifier, - column_mutation_attribute, function_call, cast, field_selection, null, + column_mutation_attribute, function_call, cast, field_selection, bind_variable, untyped_constant, constant, tuple_constructor, collection_constructor, usertype_constructor, subscript>; variant_type v; diff --git a/cql3/expr/prepare_expr.cc b/cql3/expr/prepare_expr.cc index e31c9ae41d..f87d03024d 100644 --- a/cql3/expr/prepare_expr.cc +++ b/cql3/expr/prepare_expr.cc @@ -123,7 +123,7 @@ usertype_constructor_prepare_expression(const usertype_constructor& u, data_dict auto iraw = u.elements.find(field); expression raw; if (iraw == u.elements.end()) { - raw = expr::null(); + raw = expr::make_untyped_null(); } else { raw = iraw->second; ++found_values; @@ -582,6 +582,7 @@ operator<<(std::ostream&out, untyped_constant::type_class t) case untyped_constant::type_class::boolean: return out << "BOOLEAN"; case untyped_constant::type_class::hex: return out << "HEX"; case untyped_constant::type_class::duration: return out << "DURATION"; + case untyped_constant::type_class::null: return out << "NULL"; } abort(); } @@ -609,8 +610,9 @@ static assignment_testable::test_result untyped_constant_test_assignment(const untyped_constant& uc, data_dictionary::database db, const sstring& keyspace, const column_specification& receiver) { + bool uc_is_null = uc.partial_type == untyped_constant::type_class::null; auto receiver_type = receiver.type->as_cql3_type(); - if (receiver_type.is_collection() || receiver_type.is_user_type()) { + if ((receiver_type.is_collection() || receiver_type.is_user_type()) && !uc_is_null) { return assignment_testable::test_result::NOT_ASSIGNABLE; } if (!receiver_type.is_native()) { @@ -675,6 +677,10 @@ untyped_constant_test_assignment(const untyped_constant& uc, data_dictionary::da return assignment_testable::test_result::EXACT_MATCH; } break; + case untyped_constant::type_class::null: + return receiver.type->is_counter() + ? assignment_testable::test_result::NOT_ASSIGNABLE + : assignment_testable::test_result::WEAKLY_ASSIGNABLE; } return assignment_testable::test_result::NOT_ASSIGNABLE; } @@ -688,9 +694,18 @@ untyped_constant_prepare_expression(const untyped_constant& uc, data_dictionary: return std::nullopt; } if (!is_assignable(untyped_constant_test_assignment(uc, db, keyspace, *receiver))) { + if (uc.partial_type != untyped_constant::type_class::null) { throw exceptions::invalid_request_exception(format("Invalid {} constant ({}) for \"{}\" of type {}", uc.partial_type, uc.raw_text, *receiver->name, receiver->type->as_cql3_type().to_string())); + } else { + throw exceptions::invalid_request_exception("Invalid null value for counter increment/decrement"); + } } + + if (uc.partial_type == untyped_constant::type_class::null) { + return constant::make_null(receiver->type); + } + raw_value raw_val = cql3::raw_value::make_value(untyped_constant_parsed_value(uc, receiver->type)); return constant(std::move(raw_val), receiver->type); } @@ -715,29 +730,6 @@ bind_variable_prepare_expression(const bind_variable& bv, data_dictionary::datab }; } -static -assignment_testable::test_result -null_test_assignment(data_dictionary::database db, - const sstring& keyspace, - const column_specification& receiver) { - return receiver.type->is_counter() - ? assignment_testable::test_result::NOT_ASSIGNABLE - : assignment_testable::test_result::WEAKLY_ASSIGNABLE; -} - -static -std::optional -null_prepare_expression(data_dictionary::database db, const sstring& keyspace, lw_shared_ptr receiver) { - if (!receiver) { - // TODO: It is not possible to infer the type of NULL, but perhaps we can have a matcing null_type that can be cast to anything - return std::nullopt; - } - if (!is_assignable(null_test_assignment(db, keyspace, *receiver))) { - throw exceptions::invalid_request_exception("Invalid null value for counter increment/decrement"); - } - return constant::make_null(receiver->type); -} - static sstring cast_display_name(const cast& c) { @@ -964,9 +956,6 @@ try_prepare_expression(const expression& expr, data_dictionary::database db, con [&] (const field_selection&) -> std::optional { on_internal_error(expr_logger, "field_selections are not yet reachable via prepare_expression()"); }, - [&] (const null&) -> std::optional { - return null_prepare_expression(db, keyspace, receiver); - }, [&] (const bind_variable& bv) -> std::optional { return bind_variable_prepare_expression(bv, db, keyspace, receiver); }, @@ -1028,9 +1017,6 @@ test_assignment(const expression& expr, data_dictionary::database db, const sstr [&] (const field_selection&) -> test_result { on_internal_error(expr_logger, "field_selections are not yet reachable via test_assignment()"); }, - [&] (const null&) -> test_result { - return null_test_assignment(db, keyspace, receiver); - }, [&] (const bind_variable& bv) -> test_result { return bind_variable_test_assignment(bv, db, keyspace, receiver); }, diff --git a/cql3/expr/restrictions.cc b/cql3/expr/restrictions.cc index 81ed6c9be6..a7fe48f253 100644 --- a/cql3/expr/restrictions.cc +++ b/cql3/expr/restrictions.cc @@ -144,7 +144,7 @@ void preliminary_binop_vaidation_checks(const binary_operator& binop) { } if (binop.op == oper_t::IS_NOT) { - bool rhs_is_null = is(binop.rhs) + bool rhs_is_null = (is(binop.rhs) && as(binop.rhs).partial_type == untyped_constant::type_class::null) || (is(binop.rhs) && as(binop.rhs).is_null()); if (!rhs_is_null) { throw exceptions::invalid_request_exception(format("Unsupported \"IS NOT\" relation: {}", pretty_binop_printer)); diff --git a/cql3/restrictions/statement_restrictions.cc b/cql3/restrictions/statement_restrictions.cc index d7bf7f2d54..3fcec7f67c 100644 --- a/cql3/restrictions/statement_restrictions.cc +++ b/cql3/restrictions/statement_restrictions.cc @@ -153,10 +153,6 @@ static std::vector extract_partition_range( on_internal_error(rlogger, "extract_partition_range(field_selection)"); } - void operator()(const null&) { - on_internal_error(rlogger, "extract_partition_range(null)"); - } - void operator()(const bind_variable&) { on_internal_error(rlogger, "extract_partition_range(bind_variable)"); } @@ -278,10 +274,6 @@ static std::vector extract_clustering_prefix_restrictions( on_internal_error(rlogger, "extract_clustering_prefix_restrictions(field_selection)"); } - void operator()(const null&) { - on_internal_error(rlogger, "extract_clustering_prefix_restrictions(null)"); - } - void operator()(const bind_variable&) { on_internal_error(rlogger, "extract_clustering_prefix_restrictions(bind_variable)"); } @@ -1239,10 +1231,6 @@ struct multi_column_range_accumulator { on_internal_error(rlogger, "field selection encountered outside binary operator"); } - void operator()(const null&) { - on_internal_error(rlogger, "null encountered outside binary operator"); - } - void operator()(const bind_variable&) { on_internal_error(rlogger, "bind variable encountered outside binary operator"); } diff --git a/cql3/selection/selectable.cc b/cql3/selection/selectable.cc index 2515caeec6..3a75138f22 100644 --- a/cql3/selection/selectable.cc +++ b/cql3/selection/selectable.cc @@ -222,9 +222,6 @@ prepare_selectable(const schema& s, const expr::expression& raw_selectable) { return make_shared(prepare_selectable(s, fs.structure), fs.field->prepare(s)); }, - [&] (const expr::null&) -> shared_ptr { - on_internal_error(slogger, "null found its way to selector context"); - }, [&] (const expr::bind_variable&) -> shared_ptr { on_internal_error(slogger, "bind_variable found its way to selector context"); }, @@ -283,9 +280,6 @@ selectable_processes_selection(const expr::expression& raw_selectable) { [&] (const expr::field_selection& fs) -> bool { return true; }, - [&] (const expr::null&) -> bool { - on_internal_error(slogger, "null found its way to selector context"); - }, [&] (const expr::bind_variable&) -> bool { on_internal_error(slogger, "bind_variable found its way to selector context"); }, diff --git a/test/boost/expr_test.cc b/test/boost/expr_test.cc index 0a8b9c4ea2..d1427c8d88 100644 --- a/test/boost/expr_test.cc +++ b/test/boost/expr_test.cc @@ -1529,7 +1529,7 @@ BOOST_AUTO_TEST_CASE(prepare_null) { schema_ptr table_schema = make_simple_test_schema(); auto [db, db_data] = make_data_dictionary_database(table_schema); - expression null_expr = null{}; + expression null_expr = make_untyped_null(); expression prepared = prepare_expression(null_expr, db, "test_ks", table_schema.get(), make_receiver(int32_type)); expression expected = constant::make_null(int32_type); @@ -1541,7 +1541,7 @@ BOOST_AUTO_TEST_CASE(prepare_null_no_type_fails) { schema_ptr table_schema = make_simple_test_schema(); auto [db, db_data] = make_data_dictionary_database(table_schema); - expression null_expr = null{}; + expression null_expr = make_untyped_null(); BOOST_REQUIRE_THROW(prepare_expression(null_expr, db, "test_ks", table_schema.get(), nullptr), exceptions::invalid_request_exception); } @@ -1863,7 +1863,7 @@ BOOST_AUTO_TEST_CASE(prepare_list_collection_constructor_with_null) { .style = collection_constructor::style_type::list, .elements = {untyped_constant{.partial_type = untyped_constant::type_class::integer, .raw_text = "123"}, untyped_constant{.partial_type = untyped_constant::type_class::integer, .raw_text = "456"}, - null{}}, + make_untyped_null()}, .type = nullptr}; data_type list_type = list_type_impl::get_instance(long_type, true); @@ -1992,7 +1992,7 @@ BOOST_AUTO_TEST_CASE(prepare_set_collection_constructor_with_null) { .elements = { untyped_constant{.partial_type = untyped_constant::type_class::integer, .raw_text = "789"}, - null{}, + make_untyped_null(), untyped_constant{.partial_type = untyped_constant::type_class::integer, .raw_text = "456"}, }, .type = nullptr}; @@ -2250,7 +2250,8 @@ BOOST_AUTO_TEST_CASE(prepare_map_collection_constructor_null_key) { untyped_constant{.partial_type = untyped_constant::type_class::integer, .raw_text = "30"}}, .type = nullptr}, tuple_constructor{ - .elements = {null{}, untyped_constant{.partial_type = untyped_constant::type_class::integer, + .elements = {make_untyped_null(), + untyped_constant{.partial_type = untyped_constant::type_class::integer, .raw_text = "-20"}}, .type = nullptr}, tuple_constructor{ @@ -2283,7 +2284,7 @@ BOOST_AUTO_TEST_CASE(prepare_map_collection_constructor_null_value) { .type = nullptr}, tuple_constructor{.elements = {untyped_constant{.partial_type = untyped_constant::type_class::integer, .raw_text = "2"}, - null{}}, + make_untyped_null()}, .type = nullptr}, tuple_constructor{.elements = {untyped_constant{.partial_type = untyped_constant::type_class::integer, .raw_text = "1"}, @@ -2358,7 +2359,7 @@ BOOST_AUTO_TEST_CASE(prepare_usertype_constructor_with_null) { constructor_elements.emplace( column_identifier("field1", true), untyped_constant{.partial_type = untyped_constant::type_class::integer, .raw_text = "152"}); - constructor_elements.emplace(column_identifier("field2", true), null{}); + constructor_elements.emplace(column_identifier("field2", true), make_untyped_null()); constructor_elements.emplace( column_identifier("field3", true), untyped_constant{.partial_type = untyped_constant::type_class::string, .raw_text = "ututu"});