mirrors/scylladb
1
0
Fork 0
mirror of https://github.com/scylladb/scylladb.git synced 2026-05-12 19:02:12 +00:00
Code Issues Packages Projects Releases Wiki Activity

cql3: really ensure retrieval of columns for filtering

Browse Source 
Commit fd422c954e aimed to fix
issue #3803. In that issue, if a query SELECTed only certain columns but
did filtering (ALLOW FILTERING) over other unselected columns, the filtering
didn't work. The fix involved adding the columns being filtered to the set
of columns we read from disk, so they can be filtered.

But that commit included an optimization: If you have clustering keys
c1 and c2, and the query asks for a specific partition key and c1 < 3 and
c2 > 3, the "c1 < 3" part does NOT need to be filtered because it is already
done as a slice (a contiguous read from disk). The committed code erroneously
concluded that both c1 and c2 don't need to be filtered, which was wrong
(c2 *does* need to be read and filtered).

In this patch, we fix this optimization. Previously, we used the "prefix
length", which in the above example was 2 (both c1 and c2 were filtered)
but we need a new and more elaborate function,
num_prefix_columns_that_need_not_be_filtered(), to determine we can only
skip filtering of 1 (c1) and cannot skip the second.

Fixes #4121. This patch also adds a unit test to confirm this.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>

Message-Id: <20190123131212.6269-1-nyh@scylladb.com>
This commit is contained in:
Nadav Har'El
2019-01-23 15:12:12 +02:00
committed by Avi Kivity
parent 835ad406de
commit 76f1fcc346
4 changed files with 102 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
cql3/restrictions/primary_key_restrictions.hh
View File

@@ -100,6 +100,17 @@ public:
bool has_unrestricted_components(const schema& schema) const;
virtual bool needs_filtering(const schema& schema) const;
// How long a prefix of the restrictions could have resulted in
// need_filtering() == false. These restrictions do not need to be
// applied during filtering.
// For example, if we have the filter "c1 < 3 and c2 > 3", c1 does
// not need filtering (just a read stopping at c1=3) but c2 does,
// so num_prefix_columns_that_need_not_be_filtered() will be 1.
virtual unsigned int num_prefix_columns_that_need_not_be_filtered() const {
return 0;
}
virtual bool is_all_eq() const {
return false;
}

34
cql3/restrictions/single_column_primary_key_restrictions.hh
View File

@@ -404,6 +404,7 @@ public:
}
virtual bool needs_filtering(const schema& schema) const override;
virtual unsigned int num_prefix_columns_that_need_not_be_filtered() const override;
};
template<>
@@ -484,6 +485,39 @@ inline bool single_column_primary_key_restrictions<clustering_key>::needs_filter
return false;
}
// How many of the restrictions (in column order) do not need filtering
// because they are implemented as a slice (potentially, a contiguous disk
// read). For example, if we have the filter "c1 < 3 and c2 > 3", c1 does not
// need filtering but c2 does so num_prefix_columns_that_need_not_be_filtered
// will be 1.
// The implementation of num_prefix_columns_that_need_not_be_filtered() is
// closely tied to that of needs_filtering() above - basically, if only the
// first num_prefix_columns_that_need_not_be_filtered() restrictions existed,
// then needs_filtering() would have returned false.
template<>
inline unsigned single_column_primary_key_restrictions<clustering_key>::num_prefix_columns_that_need_not_be_filtered() const {
column_id position = 0;
unsigned int count = 0;
for (const auto& restriction : _restrictions->restrictions() | boost::adaptors::map_values) {
if (restriction->is_contains() || position != restriction->get_column_def().id) {
return count;
}
if (!restriction->is_slice()) {
position = restriction->get_column_def().id + 1;
}
count++;
}
return count;
}
template<>
inline unsigned single_column_primary_key_restrictions<partition_key>::num_prefix_columns_that_need_not_be_filtered() const {
// skip_filtering() is currently called only for clustering key
// restrictions, so it doesn't matter what we return here.
return 0;
}
}
}

6
cql3/restrictions/statement_restrictions.cc
View File

@@ -364,10 +364,10 @@ std::vector<const column_definition*> statement_restrictions::get_column_defs_fo
}
}
if (_clustering_columns_restrictions->needs_filtering(*_schema)) {
column_id first_non_prefix_id = _schema->clustering_key_columns().begin()->id +
_clustering_columns_restrictions->prefix_size(_schema);
column_id first_filtering_id = _schema->clustering_key_columns().begin()->id +
_clustering_columns_restrictions->num_prefix_columns_that_need_not_be_filtered();
for (auto&& cdef : _clustering_columns_restrictions->get_column_defs()) {
if ((cdef->id >= first_non_prefix_id) && (!column_uses_indexing(cdef))) {
if (cdef->id >= first_filtering_id && !column_uses_indexing(cdef)) {
column_defs_for_filtering.emplace_back(cdef);
}
}

54
tests/filtering_test.cc
View File

@@ -259,6 +259,60 @@ SEASTAR_TEST_CASE(test_allow_filtering_clustering_column) {
});
}
SEASTAR_TEST_CASE(test_allow_filtering_two_clustering_columns) {
return do_with_cql_env_thread([] (cql_test_env& e) {
e.execute_cql("CREATE TABLE t (p int, c1 int, c2 int, data int, PRIMARY KEY (p, c1, c2))").get();
e.execute_cql("INSERT INTO t (p, c1, c2, data) VALUES (1, 2, 3, 1)").get();
e.execute_cql("INSERT INTO t (p, c1, c2, data) VALUES (1, 3, 4, 2)").get();
e.execute_cql("INSERT INTO t (p, c1, c2, data) VALUES (1, 2, 5, 3)").get();
e.execute_cql("INSERT INTO t (p, c1, c2, data) VALUES (2, 3, 4, 4)").get();
auto res = e.execute_cql("SELECT * FROM t WHERE p = 1 and c1 < 3 and c2 > 3 ALLOW FILTERING").get0();
assert_that(res).is_rows().with_rows({
{
int32_type->decompose(1),
int32_type->decompose(2),
int32_type->decompose(5),
int32_type->decompose(3)
}
});
// In issue #4121, we noticed that although with "SELECT *" filtering
// was correct, when we select only a column *not* involved in the
// filtering, one of the constraints was ignored.
res = e.execute_cql("SELECT data FROM t WHERE p = 1 and c1 < 3 and c2 > 3 ALLOW FILTERING").get0();
assert_that(res).is_rows().with_rows({
{
int32_type->decompose(3),
// Because of issue #4126 our test code also sees as part of
// the results additional columns which were requested just
// for filtering, in this case c2 (=5) was necessary for the
// filtering but c1 was not. These columns may change in the
// future.
int32_type->decompose(5)
}
});
// Similar to the above test for issue #4121, but with more clustering
// key components, two of them form a slice, two more need filtering.
e.execute_cql("CREATE TABLE t2 (p int, c1 int, c2 int, c3 int, c4 int, data int, PRIMARY KEY (p, c1, c2, c3, c4))").get();
e.execute_cql("INSERT INTO t2 (p, c1, c2, c3, c4, data) VALUES (1, 1, 2, 3, 3, 1)").get();
e.execute_cql("INSERT INTO t2 (p, c1, c2, c3, c4, data) VALUES (1, 1, 2, 5, 8, 2)").get();
e.execute_cql("INSERT INTO t2 (p, c1, c2, c3, c4, data) VALUES (1, 1, 2, 5, 4, 3)").get();
e.execute_cql("INSERT INTO t2 (p, c1, c2, c3, c4, data) VALUES (1, 1, 4, 3, 4, 4)").get();
e.execute_cql("INSERT INTO t2 (p, c1, c2, c3, c4, data) VALUES (1, 2, 4, 4, 2, 5)").get();
res = e.execute_cql("SELECT data FROM t2 WHERE p = 1 and c1 = 1 and c2 < 3 and c3 > 4 and c4 < 7 ALLOW FILTERING").get0();
assert_that(res).is_rows().with_rows({
{
int32_type->decompose(3),
// Again, the following appear here just because of issue #4126
int32_type->decompose(5),
int32_type->decompose(4)
}
});
});
}
SEASTAR_TEST_CASE(test_allow_filtering_static_column) {
return do_with_cql_env_thread([] (cql_test_env& e) {
e.execute_cql("CREATE TABLE t (a int, b int, c int, s int static, PRIMARY KEY(a, b));").get();