From 76f1fcc346bcf3692f9bd42d27ee83cdf1e20163 Mon Sep 17 00:00:00 2001 From: Nadav Har'El Date: Wed, 23 Jan 2019 15:12:12 +0200 Subject: [PATCH] cql3: really ensure retrieval of columns for filtering Commit fd422c954ec6d301b5336bac56117fc899c4e93d 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 Message-Id: <20190123131212.6269-1-nyh@scylladb.com> --- cql3/restrictions/primary_key_restrictions.hh | 11 ++++ .../single_column_primary_key_restrictions.hh | 34 ++++++++++++ cql3/restrictions/statement_restrictions.cc | 6 +-- tests/filtering_test.cc | 54 +++++++++++++++++++ 4 files changed, 102 insertions(+), 3 deletions(-) diff --git a/cql3/restrictions/primary_key_restrictions.hh b/cql3/restrictions/primary_key_restrictions.hh index bc60484fe9..b9d1a9e888 100644 --- a/cql3/restrictions/primary_key_restrictions.hh +++ b/cql3/restrictions/primary_key_restrictions.hh @@ -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; } diff --git a/cql3/restrictions/single_column_primary_key_restrictions.hh b/cql3/restrictions/single_column_primary_key_restrictions.hh index b02f2eda20..d613c5265f 100644 --- a/cql3/restrictions/single_column_primary_key_restrictions.hh +++ b/cql3/restrictions/single_column_primary_key_restrictions.hh @@ -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::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::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::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; +} + + } } diff --git a/cql3/restrictions/statement_restrictions.cc b/cql3/restrictions/statement_restrictions.cc index 28904a79c2..f7668fe350 100644 --- a/cql3/restrictions/statement_restrictions.cc +++ b/cql3/restrictions/statement_restrictions.cc @@ -364,10 +364,10 @@ std::vector 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); } } diff --git a/tests/filtering_test.cc b/tests/filtering_test.cc index 7f5fd26b6b..b1315e0adc 100644 --- a/tests/filtering_test.cc +++ b/tests/filtering_test.cc @@ -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();