There was a bug in `expr::search_and_replace`.
It doesn't preserve the `order` field of binary_operator.
`order` field is used to mark relations created
using the SCYLLA_CLUSTERING_BOUND.
It is a CQL feature used for internal queries inside Scylla.
It means that we should handle the restriction as a raw
clustering bound, not as an expression in the CQL language.
Losing the SCYLLA_CLUSTERING_BOUND marker could cause issues,
the database could end up selecting the wrong clustering ranges.
Fixes: #13055
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Closes#13056
(cherry picked from commit aa604bd935)
The CQL binary protocol introduced "unset" values in version 4
of the protocol. Unset values can be bound to variables, which
cause certain CQL fragments to be skipped. For example, the
fragment `SET a = :var` will not change the value of `a` if `:var`
is bound to an unset value.
Unsets, however, are very limited in where they can appear. They
can only appear at the top-level of an expression, and any computation
done with them is invalid. For example, `SET list_column = [3, :var]`
is invalid if `:var` is bound to unset.
This causes the code to be littered with checks for unset, and there
are plenty of tests dedicated to catching unsets. However, a simpler
way is possible - prevent the infiltration of unsets at the point of
entry (when evaluating a bind variable expression), and introduce
guards to check for the few cases where unsets are allowed.
This is what this long patch does. It performs the following:
(general)
1. unset is removed from the possible values of cql3::raw_value and
cql3::raw_value_view.
(external->cql3)
2. query_options is fortified with a vector of booleans,
unset_bind_variable_vector, where each boolean corresponds to a bind
variable index and is true when it is unset.
3. To avoid churn, two compatiblity structs are introduced:
cql3::raw_value{,_view}_vector_with_unset, which can be constructed
from a std::vector<raw_value{,_view/}>, which is what most callers
have. They can also be constructed with explicit unset vectors, for
the few cases they are needed.
(cql3->variables)
4. query_options::get_value_at() now throws if the requested bind variable
is unset. This replaces all the throwing checks in expression evaluation
and statement execution, which are removed.
5. A new query_options::is_unset() is added for the users that can tolerate
unset; though it is not used directly.
6. A new cql3::unset_operation_guard class guards against unsets. It accepts
an expression, and can be queried whether an unset is present. Two
conditions are checked: the expression must be a singleton bind
variable, and at runtime it must be bound to an unset value.
7. The modification_statement operations are split into two, via two
new subclasses of cql3::operation. cql3::operation_no_unset_support
ignores unsets completely. cql3::operation_skip_if_unset checks if
an operand is unset (luckily all operations have at most one operand that
tolerates unset) and applies unset_operation_guard to it.
8. The various sites that accept expressions or operations are modified
to check for should_skip_operation(). This are the loops around
operations in update_statement and delete_statement, and the checks
for unset in attributes (LIMIT and PER PARTITION LIMIT)
(tests)
9. Many unset tests are removed. It's now impossible to enter an
unset value into the expression evaluation machinery (there's
just no unset value), so it's impossible to test for it.
10. Other unset tests now have to be invoked via bind variables,
since there's no way to create an unset cql3::expr::constant.
11. Many tests have their exception message match strings relaxed.
Since unsets are now checked very early, we don't know the context
where they happen. It would be possible to reintroduce it (by adding
a format string parameter to cql3::unset_operation_guard), but it
seems not to be worth the effort. Usage of unsets is rare, and it is
explicit (at least with the Python driver, an unset cannot be
introduced by ommission).
I tried as an alternative to wrap cql3::raw_value{,_view} (that doesn't
recognize unsets) with cql3::maybe_unset_value (that does), but that
caused huge amounts of churn, so I abandoned that in favor of the
current approach.
Closes#12517
The function underlying_type() returns an data_type by value,
but the code assigned it to a reference.
At first I was sure this is an error
(assigning temporary value to a reference), but it turns out
that this is most likely correct due to C++ lifetime
extension rules.
I think it's better to avoid such unituitive tricks.
Assigning to value makes it clearer that the code
is correct and there are no dangling references.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Closes#12485
Now that we don't accept cql protocol version 1 or 2, we can
drop cql_serialization format everywhere, except when in the IDL
(since it's part of the inter-node protocol).
A few functions had duplicate versions, one with and one without
a cql_serialization_format parameter. They are deduplicated.
Care is taken that `partition_slice`, which communicates
the cql_serialization_format across nodes, still presents
a valid cql_serialization_format to other nodes when
transmitting itself and rejects protocol 1 and 2 serialization\
format when receiving. The IDL is unchanged.
One test checking the 16-bit serialization format is removed.
prepare_expression used to throw an error
when encountering a conjunction.
Now it's possible to use prepare_expression
to prepare an expression that contains
conjunctions.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Previously it was impossible to use expr::evaluate()
to get the value of a conjunction of elements
separated by ANDs.
Now it has been implemented.
NULL is treated as an "unkown value" - maybe true maybe false.
`TRUE AND NULL` evaluates to NULL because it might be true but also might be false.
`FALSE AND NULL` evaluates to FALSE because no matter what value NULL acts as, the result will still be FALSE.
Unset and empty values are not allowed.
Usually in CQL the rule is that when NULL occurs in an operation the whole expression
becomes NULL, but here we decided to deviate from this behavior.
Treating NULL as an "unkown value" is the standard SQL way of handing NULLs in conjunctions.
It works this way in MySQL and Postgres so we do it this way as well.
The evaluation short-circuits. Once FALSE is encountered the function returns FALSE
immediately without evaluating any further elements.
It works this way in Postgres as well, for example:
`SELECT true AND NULL AND 1/0 = 0` will throw a division by zero error
but `SELECT false AND 1/0 = 0` will successfully evaluate to FALSE.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
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
In ad3d2ee47d, we replaced `bool` as an expression element
(representing a boolean constant) with `constant`. But a comment
and a concept continue to mention it.
Remove the comment and the concept fragment.
Closes#12119
The messages used to contain UNSET_VALUE
in capital letters, but the tests
expect messages with 'unset value'.
Change the message so that it can
match the expected error text in tests.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Originally put braces around the cases because
there were local variables that I didn't want
to be shadowed.
Now there are no variables so the braces
can be removed without any problems.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
When evaluating a binary operation with
operations like EQUAL, LESS_THAN, IN
the logic of the operation is put
in a separate function to keep things clean.
IS_NOT NULL is the only exception,
it has its evaluate implementation
right in the evaluate(binary_operator)
function.
It would be cleaner to have it in
a separate dedicated function,
so it's moved to one.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
There is a more general version of limits()
which takes expressions as both the lhs and rhs
arguments.
There is no need for a specialized overload.
This specialized overload takes a tuple_constructor
as lhs, but we call evaluate() on both sides
of a binary operator before checking equality,
so this won't be useful at all.
Having multiple functions increases the risk
that one of them has a bug, while giving
dubious benfit.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Expressions like:
123 = NULL
NULL = 123
NULL = NULL
NULL != 123
should be tolerated, but evaluate to NULL.
The current code assumes that a binary operator
can only evaluate to a boolean - true or false.
Now a binary operator can also evaluate to NULL.
This should happen in cases when one of the
operator's sides is NULL.
A special class is introduced to represent a value
that can be one of three things: true, false or null.
It's better than using std::optional<bool>,
because optional has implicit conversions to bool
that could cause confusion and bugs.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
There is a more general version of equal()
which takes expressions as both the lhs and rhs
arguments.
There is no need for a specialized overload.
This specialized overload takes a tuple_constructor
as lhs, but we call evaluate() on both sides
of a binary operator before checking equality,
so this won't be useful at all.
Having multiple functions increases the risk
that one of them has a bug, while giving
dubious benfit.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
is_satisfied_by has to check if a binary_operator is satisfied
by some values. It used to be impossible to evaluate
a binary_operator, so is_satisfied had code to check
if its satisfied for a limited number of cases
occuring when filtering queries.
Now evaluate(binary_operator) has been implemented
and is_satisfied_by can use it to check if a binary_operator
evaluates to true.
This is cleaner and reduces code duplication.
Additionally cql tests will test the new evalute() implementation.
There is one special case with token().
When is_satisfied_by sees a restriction on token
it assumes that it's satisfied because it's
sure that these token restrictions were used
to generate partition ranges.
I had to leave this special case in because it's impossible
to evaluate(token). Once this is implemented I will remove
the special case because it's risky and prone to cause
bugs.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
The code to evaluate binary operators
was copied from is_satisfied_by.
is_satisfied_by wasn't able to evaluate
IS NOT NULL restrictions, so when such restriction
is encountered it throws an exception.
Implement proper handling for IS NOT NULL binary operators.
The switch ensures that all variants of oper_t are handled,
otherwise there would be a compilation error.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
evaluate() takes an expression and evaluates it
to a constant value. It wasn't possible to evalute
binary operators before, so it's added.
The code is based on is_satisfied_by,
which is currently used to check
whether a binary operator evaluates
to true or false.
It looks like is_satisfied_by and evalate()
do pretty much the same thing, one could be
implemented using the other.
In the future they might get merged
into a single function.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
like() used to only accept column_value as the lhs
to evaluate. Changed it to accept any generic expression.
This will allow to evaluate a more diverse set of
binary operators.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
contains_key() used to only accept column_value as the lhs
to evaluate. Changed it to accept any generic expression.
This will allow to evaluate a more diverse set of
binary operators.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
contains() used to only accept column_value as the lhs
to evaluate. Changed it to accept any generic expression.
This will allow to evaluate a more diverse set of
binary operators.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
In Scylla and Cassandra inserting an empty collection
that is not frozen, is interpreted as inserting a null value.
list_prepare_expression and set_prepare_expression
have an if which handles this behavior, but there
wasn't one in map_prepare_expression.
As a result preparing empty list or set would result in null,
but preparing an empty map wouldn't. This is inconsistent,
it's better to return null in all cases of empty nonfrozen
collections.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
map_prepare_expression takes a collection_constructor
of unprepared items and prepares it.
Elements of a map collection_constructor are tuples (key and value).
map_prepare_expression creates a prepared collection_constructor
by preparing each tuple and adding it to the result.
During this preparation it needs to set the type of the tuple.
There was a bug here - it took the type from unprepared
tuple_constructor and assigned it to the prepared one.
An unprepared tuple_constructor doesn't have a type
so it ended up assigning nullptr.
Instead of that it should create a tuple_type_impl instance
by looking at the types of map key and values,
and use this tuple_type_impl as the type of the prepared tuples.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
prepare_expression treats receiver as an optional argument,
it can be set to nullptr and the preparation should
still succeed when it's possible to infer the type of an expression.
preparing a bind_variable requires the receiver to be present,
because it doesn't contain any information about the type
of the bound value.
Added a check that the receiver is present.
Allowing to prepare a bind_variable without
the receiver present was a bug.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Requests like `col IN NULL` used to cause
an error - Invalid null value for colum col.
We would like to allow NULLs everywhere.
When a NULL occurs on either side
of a binary operator, the whole operation
should just evaluate to NULL.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Closes#11775
A binary operator like this:
{1: 2, 3: 4} CONTAINS KEY NULL
used to evaluate to `true`.
This is wrong, any operation involving null
on either side of the operator should evaluate
to NULL, which is interpreted as false.
This change is not backwards compatible.
Some existing code might break.
partially fixes: #10359
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
A binary operator like this:
[1, 2, 3] CONTAINS NULL
used to evaluate to `true`.
This is wrong, any operation involving null
on either side of the operator should evaluate
to NULL, which is interpreted as false.
This change is not backwards compatible.
Some existing code might break.
partially fixes: #10359
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
boolean_factors is a function that takes an expression
and extracts all children of the top level conjunction.
The problem is that it returns a vector<expression>,
which is inefficent.
Sometimes we would like to iterate over all boolean
factors without allocations. for_each_boolean_factor
is implemented for this purpose.
boolean_factors() can be implemented using
for_each_boolean_factor, so it's done to
reduce code duplication.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Previous commit added the ability to use GSI over non-frozen collections in queries,
but only the keys() and values() indexes. This commit adds support for the missing
index type - entries() index.
Signed-off-by: Karol Baryła <karol.baryla@scylladb.com>
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Previous commits added the possibility of creating GSI on non-frozen collections.
This (and next) commit allow those indexes to actually be used by queries.
This commit enables both keys() and values() indexes, as they are pretty similar.
When analyzing a WHERE clause, we want to separate individual
factors (usually relations), and later partition them into
partition key, clustering key, and regular column relations. The
first step is separation, for which this helper is added.
Currently, it is not required since the grammar supplies the
expression in separated form, but this will not work once it is
relaxed to allow any expression in the WHERE clause.
A unit test is added.
This PR removes all code that used classes `restriction`, `restrictions` and their children.
There were two fields in `statement_restrictions` that needed to be dealt with: `_clustering_columns_restrictions` and `_nonprimary_key_restrictions`.
Each function was reimplemented to operate on the new expression representaiion and eventually these fields weren't needed anymore.
After that the restriction classes weren't used anymore and could be deleted as well.
Now all of the code responsible for analyzing WHERE clause and planning a query works on expressions.
Closes#11069
* github.com:scylladb/scylla:
cql3: Remove all remaining restrictions code
cql3: Move a function from restrictions class to the test
cql3: Remove initial_key_restrictions
cql3: expr: Remove convert_to_restriction
cql3: Remove _new from _new_nonprimary_key_restrictions
cql3: Remove _nonprimary_key_restrictions field
cql3: Reimplement uses of _nonprimary_key_restrictions using expression
cql3: Keep a map of single column nonprimary key restrictions
cql3: Remove _new from _new_clustering_columns_restrictions
cql3: Remove _clustering_columns_restrictions from statement_restrictions
cql3: Use a variable instead of dynamic cast
cql3: Use the new map of single column clustering restrictions
cql3: Keep a map of single column clustering key restrictions
cql3: Return an expression in get_clustering_columns_restrctions()
cql3: Reimplement _clustering_columns_restrictions->has_supporting_index()
cql3: Don't create single element conjunction
cql3: Add expr::index_supports_some_column
cql3: Reimplement has_unrestricted_components()
cql3: Reimplement _clustering_columns_restrictions->need_filtering()
cql3: Reimplement num_prefix_columns_that_need_not_be_filtered
cql3: Use the new clustering restrictions field instead of ->expression
cql3: Reimplement _clustering_columns_restrictions->size() using expressions
cql3: Reimplement _clustering_columns_restrictions->get_column_defs() using expressions
cql3: Reimplement _clustering_columns_restrictions->is_all_eq() using expressions
cql3: expr: Add has_only_eq_binops function
cql3: Reimplement _clustering_columns_restrictions->empty() using expressions
The classes restriction, restrictions and its children
aren't used anywhere now and can be safely removed.
Some includes need to be modified for the code to compile.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Add a function that checks if there is an index
which supports one of the columns present in
the given expression.
This functionality will soon be needed for
clustering and nonprimary columns so it's
good to separate into a reusable function.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Add a function which checks that an expression
contains only binary operators with '='.
Right now this check is done only in a single place,
but soon the same check will have to be done
for clustering columns as well, so the code
is moved to a separate function to prevent duplication.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Moving `function`, `function_name` and `aggregate_function` into
db namespace to avoid including cql3 namespace into query-request.
For now, only minimal subset of cql3 function was moved to db.
Restrictions like
col IN (1)
get converted to
col = 1
as an optimization/simplification.
This used to be done in prepare_binary_operator,
but it fits way better inside of
validate_and_prepare_new_restriction.
When it was being done in prepare_binary_operator
the conversion happened before validation checks
and the error messages would describe an equality
restriction despite the user making an IN restriction.
Now the conversion happens after all validation
is finished, which ensures that all checks are
being done on the original expression.
Fixes: #10631
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Move checking for disallowed operators
earlier in the code flow.
This is needed to pass some tests that
expect one error message instead of the other.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
expr::to_restriction is currently used to
take a restriction from the WHERE clause,
prepare it, perform some validation checks
and finally convert it to an instance of
the restriction class.
Soon we will get rid of the restriction class.
In preparation for that expr::to_restriction
is split into two independent parts:
* The part that prepares and validates a binary_operator
* The part that converts a binary_operator to restriction
Thanks to this split getting rid of restriction class
will be painless, we will just stop using the
second part.
This commit splits expr::to_restriction into two functions;
* validate_and_prepare_new_restriction
* convert_to_restriction
that handle each of those parts.
All helper validation methods in the anonymous namespace
are copied from the to_restriction.cc file.
to_restriction.cc isn't the best filename for the new functionality,
so it has been renamed to restrictions.hh/cc.
In the future all the code regarding restrictions could be
put there to reduce clutter in expression.hh/cc
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
expr::to_restriction performs a check to see if
the restriction is of form: `col IS NOT NULL`
There is a mistake in this check.
It uses is<null>(prepared_binop.rhs)
to determine if the right hand side of binary operator
is a null, but the binary operator is already prepared.
During preparation expr::null is converted to expr::constant
and that wouldn't be detected by this check.
The check has been changed to check for null constant instead
of expr::null.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
There was a bug which caused incorrect results of limits()
for columns with reversed clustering order.
Such columns have reversed_type as their type and this
needs to be taken into account when comparing them.
It was introduced in 6d943e6cd0.
This commit replaced uses of get_value_comparator
with type_of. The difference between them is that
get_value_comparator applied ->without_reversed()
on the result type.
Because the type was reversed, comparisons like
1 < 2 evaluated to false.
This caused the test testIndexOnKeyWithReverseClustering
to fail, but sadly it wasn't caught by CI because
the CI itself has a bug that makes it skip some tests.
The test passes now, although it has to be run manually
to check that.
Fixes: #10918
Signed-off-by: cvybhu <jan.ciolek@scylladb.com>
Closes#10994
value_for is a method from the restriction class
which finds the value for a given column.
Under the hood it makes use of possible_lhs_values.
It will be needed to implement some functionality
that was implemented using restrictions before.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
