Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IN(NULL) yields different results with prepared statements #6369

Closed
dekimir opened this issue May 4, 2020 · 1 comment
Closed

IN(NULL) yields different results with prepared statements #6369

dekimir opened this issue May 4, 2020 · 1 comment

Comments

@dekimir
Copy link
Contributor

dekimir commented May 4, 2020

When NULL is a member of the IN(...) tuple, it doesn't match any rows, even if the field value is NULL:

cqlsh:ks> CREATE TABLE t (p int PRIMARY KEY, r int);
cqlsh:ks> INSERT INTO t(p) VALUES (1);
cqlsh:ks> SELECT * FROM t WHERE r IN ( 99, null ) ALLOW FILTERING ;

But when this NULL is supplied via a prepared statement, it matches NULL fields. The following test fails:

SEASTAR_TEST_CASE(in_null) {
    return do_with_cql_env([](cql_test_env& e) {
        return seastar::async([&e] {
            cquery_nofail(e, "create table t (p int primary key, r int)");
            cquery_nofail(e, "insert into t(p) values (1)");
            require_rows(e, "select p from t where r in (99, null) allow filtering", {});
            auto stmt = e.prepare("select p from t where r in ? allow filtering").get0();
            auto list_type = list_type_impl::get_instance(int32_type, true);
            auto tuple_with_null = list_type->decompose(
                    make_list_value(list_type, {99, data_value::make_null(int32_type)}));
            require_rows(e, stmt, {cql3::raw_value::make_value(std::move(tuple_with_null))}, {});
        });
    });
}

This is inconsistent. The prepared-statement behaviour should match that of inlined CQL.

/cc @alecco

dekimir pushed a commit to dekimir/scylla that referenced this issue May 4, 2020
This testcase expects IN(NULL) to match NULL values by using prepared
statements.  This causes mismatch in restriction evaluation between
WIP code and old code.

Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
dekimir pushed a commit to dekimir/scylla that referenced this issue May 5, 2020
This testcase expects IN(NULL) to match NULL values by using prepared
statements.  This causes mismatch in restriction evaluation between
WIP code and old code.

Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
dekimir pushed a commit to dekimir/scylla that referenced this issue Jun 20, 2020
Instead of `restriction` class methods, use the new free functions.
Specific replacement actions are listed below.

Note that class `restrictions` (plural) remains intact -- both its
methods and its type hierarchy remain intact for now.

Ensure full test coverage of the replacement code with new file
test/boost/restrictions_test.cc and some extra testcases in
test/cql/*.

Drop some existing tests because they codify buggy behaviour
(reference scylladb#6369, scylladb#6382).  Drop others because they forbid relation
combinations that are now allowed (eg, mixing equality and
inequality, comparing to NULL, etc.).

Here are some specific categories of what was replaced:

- restriction::is_foo predicates are replaced by using the free
  function find_if; sometimes it is used transitively (see, eg,
  has_slice)

- restriction::is_multi_column is replaced by dynamic casts (recall
  that the `restrictions` class hierarchy still exists)

- utility methods is_satisfied_by, is_supported_by, to_string, and
  uses_function are replaced by eponymous free functions; note that
  restrictions::uses_function still exists

- restriction::apply_to is replaced by free function
  replace_column_def

- when checking infinite_bound_range_deletions, the has_bound is
  replaced by local free function bounded_ck

- restriction::bounds and restriction::value are replaced by the more
  general free function possible_lhs_values

- using free functions allows us to simplify the
  multi_column_restriction and token_restriction hierarchies; their
  methods merge_with and uses_function became identical in all
  subclasses, so they were moved to the base class

- single_column_primary_key_restrictions<clustering_key>::needs_filtering
  was changed to reuse num_prefix_columns_that_need_not_be_filtered,
  which uses free functions

Fixes scylladb#5799.
Fixes scylladb#6369.
Fixes scylladb#6371.
Fixes scylladb#6372.
Fixes scylladb#6382.

Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
dekimir pushed a commit to dekimir/scylla that referenced this issue Jun 26, 2020
Instead of `restriction` class methods, use the new free functions.
Specific replacement actions are listed below.

Note that class `restrictions` (plural) remains intact -- both its
methods and its type hierarchy remain intact for now.

Ensure full test coverage of the replacement code with new file
test/boost/restrictions_test.cc and some extra testcases in
test/cql/*.

Drop some existing tests because they codify buggy behaviour
(reference scylladb#6369, scylladb#6382).  Drop others because they forbid relation
combinations that are now allowed (eg, mixing equality and
inequality, comparing to NULL, etc.).

Here are some specific categories of what was replaced:

- restriction::is_foo predicates are replaced by using the free
  function find_if; sometimes it is used transitively (see, eg,
  has_slice)

- restriction::is_multi_column is replaced by dynamic casts (recall
  that the `restrictions` class hierarchy still exists)

- utility methods is_satisfied_by, is_supported_by, to_string, and
  uses_function are replaced by eponymous free functions; note that
  restrictions::uses_function still exists

- restriction::apply_to is replaced by free function
  replace_column_def

- when checking infinite_bound_range_deletions, the has_bound is
  replaced by local free function bounded_ck

- restriction::bounds and restriction::value are replaced by the more
  general free function possible_lhs_values

- using free functions allows us to simplify the
  multi_column_restriction and token_restriction hierarchies; their
  methods merge_with and uses_function became identical in all
  subclasses, so they were moved to the base class

- single_column_primary_key_restrictions<clustering_key>::needs_filtering
  was changed to reuse num_prefix_columns_that_need_not_be_filtered,
  which uses free functions

Fixes scylladb#5799.
Fixes scylladb#6369.
Fixes scylladb#6371.
Fixes scylladb#6372.
Fixes scylladb#6382.

Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
dekimir added a commit to dekimir/scylla that referenced this issue Jun 26, 2020
test/boost/user_types_test was relying on NULL being treated
differently in prepared statements than in regular statements (scylladb#6369).
This is no longer the case, so modify the expected test result
accordingly.

Signed-off-by: Dejan Mircevski <github@mircevski.com>
dekimir pushed a commit to dekimir/scylla that referenced this issue Jun 26, 2020
Instead of `restriction` class methods, use the new free functions.
Specific replacement actions are listed below.

Note that class `restrictions` (plural) remains intact -- both its
methods and its type hierarchy remain intact for now.

Ensure full test coverage of the replacement code with new file
test/boost/restrictions_test.cc and some extra testcases in
test/cql/*.

Drop some existing tests because they codify buggy behaviour
(reference scylladb#6369, scylladb#6382).  Drop others because they forbid relation
combinations that are now allowed (eg, mixing equality and
inequality, comparing to NULL, etc.).

Here are some specific categories of what was replaced:

- restriction::is_foo predicates are replaced by using the free
  function find_if; sometimes it is used transitively (see, eg,
  has_slice)

- restriction::is_multi_column is replaced by dynamic casts (recall
  that the `restrictions` class hierarchy still exists)

- utility methods is_satisfied_by, is_supported_by, to_string, and
  uses_function are replaced by eponymous free functions; note that
  restrictions::uses_function still exists

- restriction::apply_to is replaced by free function
  replace_column_def

- when checking infinite_bound_range_deletions, the has_bound is
  replaced by local free function bounded_ck

- restriction::bounds and restriction::value are replaced by the more
  general free function possible_lhs_values

- using free functions allows us to simplify the
  multi_column_restriction and token_restriction hierarchies; their
  methods merge_with and uses_function became identical in all
  subclasses, so they were moved to the base class

- single_column_primary_key_restrictions<clustering_key>::needs_filtering
  was changed to reuse num_prefix_columns_that_need_not_be_filtered,
  which uses free functions

Fixes scylladb#5799.
Fixes scylladb#6369.
Fixes scylladb#6371.
Fixes scylladb#6372.
Fixes scylladb#6382.

Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
dekimir pushed a commit to dekimir/scylla that referenced this issue Aug 14, 2020
This testcase was temporarily commented out in 37ebe52, because it
relied on buggy (scylladb#6369) behaviour fixed by that commit.  Specifically,
it expected a NULL comparison to match a NULL cell value.  We now
bring it back, with corrected result expectation.

Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
dekimir pushed a commit to dekimir/scylla that referenced this issue Aug 14, 2020
This testcase was temporarily commented out in 37ebe52, because it
relied on buggy (scylladb#6369) behaviour fixed by that commit.  Specifically,
it expected a NULL comparison to match a NULL cell value.  We now
bring it back, with corrected result expectation.

Tests: unit (dev)

Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
avikivity pushed a commit that referenced this issue Aug 16, 2020
This testcase was temporarily commented out in 37ebe52, because it
relied on buggy (#6369) behaviour fixed by that commit.  Specifically,
it expected a NULL comparison to match a NULL cell value.  We now
bring it back, with corrected result expectation.

Tests: unit (dev)

Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
@avikivity
Copy link
Member

Minor, not backporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants