From c69075bbef9a372bb4369a881be840c6f7c38684 Mon Sep 17 00:00:00 2001 From: Juliusz Stasiewicz Date: Tue, 14 Jul 2020 12:59:01 +0200 Subject: [PATCH 1/2] cql3/restrictions: exclude NULLs from comparison in filtering NULLs used to give false positives in GT, LT, GEQ and LEQ ops performed upon `ALLOW FILTERING`. That was a consequence of not distinguishing NULL from an empty buffer. This patch excludes NULLs on high level, preventing them from entering any comparison, i.e. it assumes that any binary operator should return `false` whenever one of the operands is NULL (note: ATM filters such as `...WHERE x=NULL ALLOW FILTERING` return empty sets anyway). `restriction_test/regular_col_slice` had to be updated accordingly. Fixes #6295 --- cql3/restrictions/statement_restrictions.cc | 2 +- test/boost/restrictions_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cql3/restrictions/statement_restrictions.cc b/cql3/restrictions/statement_restrictions.cc index 91110c08e2..42962b1d60 100644 --- a/cql3/restrictions/statement_restrictions.cc +++ b/cql3/restrictions/statement_restrictions.cc @@ -807,7 +807,7 @@ bool limits(const binary_operator& opr, const column_value_eval_bag& bag) { } else if (columns.size() == 1) { auto lhs = get_value(columns[0], bag); if (!lhs) { - lhs = bytes(); // Compatible with old code, which feeds null to type comparators. + return false; // NULL never compares to anything (#6295) } const auto tup = get_tuple(*opr.rhs, bag.options); auto rhs = (tup && tup->size() == 1) ? tup->get_elements()[0] // Assume an external query WHERE (ck1)>(123). diff --git a/test/boost/restrictions_test.cc b/test/boost/restrictions_test.cc index 73cab66126..eca02ad863 100644 --- a/test/boost/restrictions_test.cc +++ b/test/boost/restrictions_test.cc @@ -232,7 +232,7 @@ SEASTAR_THREAD_TEST_CASE(regular_col_slice) { require_rows(e, "select * from t where q<11 and q>11 allow filtering", {}); require_rows(e, "select q from t where q<=12 and r>=21 allow filtering", {{I(11), I(21)}, {I(12), I(22)}}); cquery_nofail(e, "insert into t(p) values (4)"); - require_rows(e, "select q from t where q<12 allow filtering", {{std::nullopt}, {I(10)}, {I(11)}}); + require_rows(e, "select q from t where q<12 allow filtering", {{I(10)}, {I(11)}}); require_rows(e, "select q from t where q>10 allow filtering", {{I(11)}, {I(12)}, {I(13)}}); require_rows(e, "select q from t where q<12 and q>10 allow filtering", {{I(11)}}); }).get(); From c25398e8cfd3cad7e4c3948b8c53d73097f210ed Mon Sep 17 00:00:00 2001 From: Juliusz Stasiewicz Date: Wed, 1 Jul 2020 14:52:23 +0200 Subject: [PATCH 2/2] filtering_test: check that NULLs do not compare to normal values Tested operators are: `<` and `>`. Tests all types of NULLs except `duration` because durations are explicitly not comparable. Values to compare against were chosen arbitrarily. --- test/boost/restrictions_test.cc | 42 ++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/test/boost/restrictions_test.cc b/test/boost/restrictions_test.cc index eca02ad863..9e4f031ca8 100644 --- a/test/boost/restrictions_test.cc +++ b/test/boost/restrictions_test.cc @@ -260,7 +260,47 @@ SEASTAR_THREAD_TEST_CASE(regular_col_neq) { } #endif // 0 -SEASTAR_THREAD_TEST_CASE(null) { +namespace { +/// The `op` argument can be one of: "<", "<=", ">", ">=", "=". Remove float args to use "!=". +future<> test_lhs_null_with_operator(sstring op) { + return do_with_cql_env_thread([op = std::move(op)] (cql_test_env& e) { + cquery_nofail(e, "CREATE TABLE cf (pk int primary key, flt float," + " dbl double, dec decimal, b boolean, ti tinyint, si smallint," + " i int, bi bigint, vi varint, ip inet, u uuid, tu timeuuid," + " t time, d date, a ascii, txt text);"); + cquery_nofail(e, "INSERT INTO cf (pk) VALUES (0);"); + + // Now we have all types of NULLs + require_rows(e, format("SELECT * FROM cf WHERE flt {} 99.9 ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE dbl {} 99.9 ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE dec {} 99.9 ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE b {} true ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE ti {} 99 ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE si {} 999 ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE i {} 999 ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE bi {} 999 ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE vi {} 999 ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE ip {} '255.255.255.255' ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE u {} ffffffff-e89b-12d3-a456-426655440000 ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE tu {} ffffffff-e89b-12d3-a456-426655440000 ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE t {} '23:59:59' ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE d {} '2137-01-01' ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE a {} 'Z' ALLOW FILTERING;", op), {}); + require_rows(e, format("SELECT * FROM cf WHERE txt {} 'Ż' ALLOW FILTERING;", op), {}); + }); +} +} // anon. namespace + +// This test checks that NULLs on LHS of comparison operator never evaluate to `true` (#6295). +// The expected behavior aligns with C*, but the test also ensures that NULLs are not converted +// to empty buffers somewhere along the way (since empty buffers would lexicographically +// precede anything). +SEASTAR_THREAD_TEST_CASE(null_lhs) { + test_lhs_null_with_operator("<").get(); + test_lhs_null_with_operator(">").get(); +} + +SEASTAR_THREAD_TEST_CASE(null_rhs) { do_with_cql_env_thread([](cql_test_env& e) { cquery_nofail(e, "create table t (pk1 int, pk2 int, ck1 float, ck2 float, r text, primary key((pk1,pk2),ck1,ck2))"); const auto q = [&] (const char* stmt) { return e.execute_cql(stmt).get(); };