From 4bb11076525df443d155e45331c82a3f34e2e649 Mon Sep 17 00:00:00 2001
From: Dejan Mircevski <dejan@scylladb.com>
Date: Wed, 16 Dec 2020 11:29:58 -0500
Subject: [PATCH] cql3: Fix range computation for p=1 AND p=1

Previously compute_bounds was assuming that primary-key columns are
restricted by exactly one equality, resulting in the following error:

query 'select p from t where p=1 and p=1' failed:
 std::bad_variant_access (std::get: wrong index for variant)

This patch removes that assumption and deals correctly with the
multiple-equalities case.  As a byproduct, it also stops raising
"invalid null value" exceptions for null RHS values.

Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
---
 .../single_column_primary_key_restrictions.hh | 22 ++++--------
 test/boost/restrictions_test.cc               | 35 +++++++++++++++----
 2 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/cql3/restrictions/single_column_primary_key_restrictions.hh b/cql3/restrictions/single_column_primary_key_restrictions.hh
index 8a006df3e3cf..6ba3ce17da16 100644
--- a/cql3/restrictions/single_column_primary_key_restrictions.hh
+++ b/cql3/restrictions/single_column_primary_key_restrictions.hh
@@ -212,30 +212,22 @@ private:
     std::vector<range_type> compute_bounds(const query_options& options) const {
         std::vector<range_type> ranges;
 
-        static constexpr auto invalid_null_msg = std::is_same<ValueType, partition_key>::value
-            ? "Invalid null value for partition key part %s" : "Invalid null value for clustering key part %s";
-
         // TODO: rewrite this to simply invoke possible_lhs_values on each clustering column, find the first
         // non-list, and take Cartesian product of that prefix.  No need for to_range() and std::get() here.
         if (_restrictions->is_all_eq()) {
-            if (_restrictions->size() == 1) {
-                auto&& e = *restrictions().begin();
-                const auto b = std::get<expr::binary_operator>(e.second->expression).rhs->bind_and_get(options);
-                if (!b) {
-                    throw exceptions::invalid_request_exception(sprint(invalid_null_msg, e.first->name_as_text()));
-                }
-                return {range_type::make_singular(ValueType::from_single_value(*_schema, to_bytes(b)))};
-            }
             std::vector<bytes> components;
             components.reserve(_restrictions->size());
             for (auto&& e : restrictions()) {
                 const column_definition* def = e.first;
                 assert(components.size() == _schema->position(*def));
-                const auto b = std::get<expr::binary_operator>(e.second->expression).rhs->bind_and_get(options);
-                if (!b) {
-                    throw exceptions::invalid_request_exception(sprint(invalid_null_msg, e.first->name_as_text()));
+                // Because _restrictions is all EQ, possible_lhs_values must return a list, not a range.
+                const auto b = std::get<expr::value_list>(possible_lhs_values(e.first, e.second->expression, options));
+                // Furthermore, this list is either a single element (when all RHSs are the same) or empty (when at
+                // least two are different, so the restrictions cannot hold simultaneously -- ie, c=1 AND c=2).
+                if (b.empty()) {
+                    return {};
                 }
-                components.emplace_back(to_bytes(b));
+                components.emplace_back(b.front());
             }
             return {range_type::make_singular(ValueType::from_exploded(*_schema, std::move(components)))};
         }
diff --git a/test/boost/restrictions_test.cc b/test/boost/restrictions_test.cc
index eb80ccc8bfec..adc373bc19d8 100644
--- a/test/boost/restrictions_test.cc
+++ b/test/boost/restrictions_test.cc
@@ -307,13 +307,12 @@ SEASTAR_THREAD_TEST_CASE(null_rhs) {
         using ire = exceptions::invalid_request_exception;
         using exception_predicate::message_contains;
         const char* expect = "Invalid null";
-        // TODO: investigate why null comparison isn't allowed here.
-        BOOST_REQUIRE_EXCEPTION(q("select * from t where pk1=0 and pk2=null"), ire, message_contains(expect));
+        cquery_nofail(e, "insert into t (pk1, pk2, ck1, ck2) values (11, 12, 21, 22)");
+        require_rows(e, "select * from t where pk1=0 and pk2=null", {});
         BOOST_REQUIRE_EXCEPTION(q("select * from t where pk1=0 and pk2=0 and (ck1,ck2)>=(0,null)"),
                                 ire, message_contains(expect));
-        BOOST_REQUIRE_EXCEPTION(q("select * from t where ck1=null"), ire, message_contains(expect));
-        BOOST_REQUIRE_EXCEPTION(q("select * from t where r=null and ck1=null allow filtering"),
-                                ire, message_contains(expect));
+        require_rows(e, "select * from t where ck1=null", {});
+        require_rows(e, "select * from t where r=null and ck1=null allow filtering", {});
         require_rows(e, "select * from t where pk1=0 and pk2=0 and ck1<null", {});
         require_rows(e, "select * from t where r>null and ck1<null allow filtering", {});
         cquery_nofail(e, "insert into t(pk1,pk2,ck1,ck2) values(11,21,101,201)");
@@ -324,7 +323,8 @@ SEASTAR_THREAD_TEST_CASE(null_rhs) {
         require_rows(e, "select * from t where pk1=null allow filtering", {});
 
         cquery_nofail(e, "create table tb (p int primary key)");
-        BOOST_REQUIRE_EXCEPTION(q("select * from tb where p=null"), ire, message_contains(expect));
+        cquery_nofail(e, "insert into tb (p) values (1)");
+        require_rows(e, "select * from tb where p=null", {});
     }).get();
 }
 
@@ -843,6 +843,29 @@ SEASTAR_THREAD_TEST_CASE(bounds_reversed) {
     }).get();
 }
 
+SEASTAR_THREAD_TEST_CASE(multi_eq_on_primary) {
+    do_with_cql_env_thread([](cql_test_env& e) {
+        cquery_nofail(e, "create table t (p int, c int, primary key (p, c))");
+        cquery_nofail(e, "insert into t (p, c) values (1, 11);");
+        cquery_nofail(e, "insert into t (p, c) values (2, 12);");
+        cquery_nofail(e, "insert into t (p, c) values (3, 13);");
+        require_rows(e, "select p from t where p=1 and p=1", {{I(1)}});
+        require_rows(e, "select p from t where p=1 and p=2", {});
+        require_rows(e, "select c from t where c=11 and c=11", {{I(11)}});
+        require_rows(e, "select c from t where c=11 and c=999", {});
+
+        cquery_nofail(e, "create table t2 (pk1 int, pk2 int, ck1 int, ck2 int, primary key ((pk1, pk2), ck1, ck2))");
+        cquery_nofail(e, "insert into t2 (pk1, pk2, ck1, ck2) values (1, 12, 21, 22);");
+        cquery_nofail(e, "insert into t2 (pk1, pk2, ck1, ck2) values (1, 12, 210, 220);");
+        require_rows(e, "select pk1 from t2 where pk1=1 and pk1=1 and pk2=12", {{I(1)}, {I(1)}});
+        require_rows(e, "select pk1 from t2 where pk1=0 and pk1=1 and pk2=12", {});
+        require_rows(e, "select pk1 from t2 where pk1=1 and pk2=12 and ck1=21 and ck1=21", {{I(1)}});
+        require_rows(e, "select pk1 from t2 where pk1=1 and pk2=12 and ck1=21 and ck1=21 and ck1=21", {{I(1)}});
+        require_rows(e, "select pk1 from t2 where pk1=1 and pk2=12 and ck1=21 and ck1=210", {});
+        require_rows(e, "select pk1 from t2 where pk1=1 and pk2=12 and ck1=21 and ck1=210 and ck1=210", {});
+    }).get();
+}
+
 SEASTAR_THREAD_TEST_CASE(token) {
     do_with_cql_env_thread([](cql_test_env& e) {
         cquery_nofail(e, "create table t (p int, q int, r int, primary key ((p, q)))");