From 767ff594181dbe9a94bfe59564cecfc1da45e2db Mon Sep 17 00:00:00 2001 From: Piotr Sarna Date: Tue, 17 Mar 2020 19:28:32 +0100 Subject: [PATCH] cql: fix qualifying indexed columns for filtering When qualifying columns to be fetched for filtering, we also check if the target column is not used as an index - in which case there's no need of fetching it. However, the check was incorrectly assuming that any restriction is eligible for indexing, while it's currently only true for EQ. The fix makes a more specific check and contains many dynamic casts, but these will hopefully we gone once our long planned "restrictions rewrite" is done. This commit comes with a test. Fixes #5708 Tests: unit(dev) --- cql3/restrictions/statement_restrictions.cc | 27 ++++++++++--- test/boost/secondary_index_test.cc | 43 +++++++++++++++++++++ 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/cql3/restrictions/statement_restrictions.cc b/cql3/restrictions/statement_restrictions.cc index 7be4f095e0..5920fc00da 100644 --- a/cql3/restrictions/statement_restrictions.cc +++ b/cql3/restrictions/statement_restrictions.cc @@ -392,28 +392,45 @@ std::vector statement_restrictions::get_column_defs_fo if (need_filtering()) { auto& sim = db.find_column_family(_schema).get_index_manager(); auto [opt_idx, _] = find_idx(sim); - auto column_uses_indexing = [&opt_idx] (const column_definition* cdef) { - return opt_idx && opt_idx->depends_on(*cdef); + auto column_uses_indexing = [&opt_idx] (const column_definition* cdef, ::shared_ptr restr) { + return opt_idx && restr && restr->is_supported_by(*opt_idx); }; + auto single_pk_restrs = dynamic_pointer_cast(_partition_key_restrictions); if (_partition_key_restrictions->needs_filtering(*_schema)) { for (auto&& cdef : _partition_key_restrictions->get_column_defs()) { - if (!column_uses_indexing(cdef)) { + ::shared_ptr restr; + if (single_pk_restrs) { + auto it = single_pk_restrs->restrictions().find(cdef); + if (it != single_pk_restrs->restrictions().end()) { + restr = dynamic_pointer_cast(it->second); + } + } + if (!column_uses_indexing(cdef, restr)) { column_defs_for_filtering.emplace_back(cdef); } } } + auto single_ck_restrs = dynamic_pointer_cast(_clustering_columns_restrictions); const bool pk_has_unrestricted_components = _partition_key_restrictions->has_unrestricted_components(*_schema); if (pk_has_unrestricted_components || _clustering_columns_restrictions->needs_filtering(*_schema)) { column_id first_filtering_id = pk_has_unrestricted_components ? 0 : _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_filtering_id && !column_uses_indexing(cdef)) { + ::shared_ptr restr; + if (single_pk_restrs) { + auto it = single_ck_restrs->restrictions().find(cdef); + if (it != single_ck_restrs->restrictions().end()) { + restr = dynamic_pointer_cast(it->second); + } + } + if (cdef->id >= first_filtering_id && !column_uses_indexing(cdef, restr)) { column_defs_for_filtering.emplace_back(cdef); } } } for (auto&& cdef : _nonprimary_key_restrictions->get_column_defs()) { - if (!column_uses_indexing(cdef)) { + auto restr = dynamic_pointer_cast(_nonprimary_key_restrictions->get_restriction(*cdef)); + if (!column_uses_indexing(cdef, restr)) { column_defs_for_filtering.emplace_back(cdef); } } diff --git a/test/boost/secondary_index_test.cc b/test/boost/secondary_index_test.cc index d5a5e6583a..ac374c79f5 100644 --- a/test/boost/secondary_index_test.cc +++ b/test/boost/secondary_index_test.cc @@ -1223,3 +1223,46 @@ SEASTAR_TEST_CASE(test_computed_columns) { }); }); } + +// Ref: #5708 - filtering should be applied on an indexed column +// if the restriction is not eligible for indexing (it's not EQ) +SEASTAR_TEST_CASE(test_filtering_indexed_column) { + return do_with_cql_env_thread([] (auto& e) { + cquery_nofail(e, "CREATE TABLE test_index (a INT, b INT, c INT, d INT, e INT, PRIMARY KEY ((a, b),c));"); + cquery_nofail(e, "CREATE INDEX ON test_index(d);"); + cquery_nofail(e, "INSERT INTO test_index (a, b, c, d, e) VALUES (1, 2, 3, 4, 5);"); + cquery_nofail(e, "INSERT INTO test_index (a, b, c, d, e) VALUES (11, 22, 33, 44, 55);"); + cquery_nofail(e, "select c,e from ks.test_index where d = 44 ALLOW FILTERING;"); + cquery_nofail(e, "select a from ks.test_index where d > 43 ALLOW FILTERING;"); + + eventually([&] { + auto msg = cquery_nofail(e, "select c,e from ks.test_index where d = 44 ALLOW FILTERING;"); + assert_that(msg).is_rows().with_rows({ + {int32_type->decompose(33), int32_type->decompose(55)} + }); + }); + eventually([&] { + auto msg = cquery_nofail(e, "select a from ks.test_index where d > 43 ALLOW FILTERING;"); + // NOTE: Column d will also be fetched, because it needs to be filtered. + // It's not the case in the previous select, where d was only used as an index. + assert_that(msg).is_rows().with_rows({ + {int32_type->decompose(11), int32_type->decompose(44)} + }); + }); + + cquery_nofail(e, "CREATE INDEX ON test_index(b);"); + cquery_nofail(e, "CREATE INDEX ON test_index(c);"); + eventually([&] { + auto msg = cquery_nofail(e, "select e from ks.test_index where b > 12 ALLOW FILTERING;"); + assert_that(msg).is_rows().with_rows({ + {int32_type->decompose(55), int32_type->decompose(22)} + }); + }); + eventually([&] { + auto msg = cquery_nofail(e, "select d from ks.test_index where c > 25 ALLOW FILTERING;"); + assert_that(msg).is_rows().with_rows({ + {int32_type->decompose(44), int32_type->decompose(33)} + }); + }); + }); +}