Skip to content

Commit

Permalink
cql3: Fix range computation for p=1 AND p=1
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
Dejan Mircevski committed Dec 16, 2020
1 parent 7081e36 commit 4bb1107
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 21 deletions.
22 changes: 7 additions & 15 deletions cql3/restrictions/single_column_primary_key_restrictions.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)))};
}
Expand Down
35 changes: 29 additions & 6 deletions test/boost/restrictions_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)");
Expand All @@ -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();
}

Expand Down Expand Up @@ -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)))");
Expand Down

0 comments on commit 4bb1107

Please sign in to comment.