From 58a2d2ad98f03aade03a6d50a8cf02e826dba85b Mon Sep 17 00:00:00 2001 From: smoczy123 Date: Thu, 21 Nov 2024 22:15:48 +0100 Subject: [PATCH 1/3] index, replica, schema: adjust DESCRIBE to provide information about custom class when applicable Added function returning custom index class name. Added printing custom index class name when using DESCRIBE. --- index/secondary_index_manager.cc | 26 ++++++++++++++++++++++++++ index/secondary_index_manager.hh | 5 +++++ replica/schema_describe_helper.hh | 6 ++++++ schema/schema.cc | 10 ++++++++++ schema/schema.hh | 1 + 5 files changed, 48 insertions(+) diff --git a/index/secondary_index_manager.cc b/index/secondary_index_manager.cc index 170f26e7db93..329091d21bc3 100644 --- a/index/secondary_index_manager.cc +++ b/index/secondary_index_manager.cc @@ -8,6 +8,8 @@ * SPDX-License-Identifier: (AGPL-3.0-or-later and Apache-2.0) */ +#include +#include #include #include "index/secondary_index_manager.hh" @@ -15,6 +17,7 @@ #include "cql3/statements/index_target.hh" #include "cql3/expr/expression.hh" #include "index/target_parser.hh" +#include "schema/schema.hh" #include "schema/schema_builder.hh" #include "db/view/view.hh" #include "concrete_types.hh" @@ -22,6 +25,8 @@ #include #include +#include +#include namespace secondary_index { @@ -344,4 +349,25 @@ bool secondary_index_manager::is_global_index(const schema& s) const { }); } +std::optional secondary_index_manager::custom_index_class(const schema& s) const { + auto range = _indices | std::views::values; + auto idx = std::ranges::find_if(range, [&s] (const index& i) { + return i.metadata().options().contains(cql3::statements::index_target::custom_index_option_name) && s.cf_name() == index_table_name(i.metadata().name()); + }); + if (idx == std::ranges::end(range)) { + return std::nullopt; + } else { + return (*idx).metadata().options().at(cql3::statements::index_target::custom_index_option_name); + } +} + +std::set secondary_index_manager::supported_custom_classes(const gms::feature_service& fs) const { + using namespace std::literals; + // TODO: when actual vector backend will be added, this will create the set from features + std::set classes = { + "dummy-vector-backend"sv, + }; + return classes; +} + } diff --git a/index/secondary_index_manager.hh b/index/secondary_index_manager.hh index 7961a8bb8725..156b20ad80da 100644 --- a/index/secondary_index_manager.hh +++ b/index/secondary_index_manager.hh @@ -10,11 +10,14 @@ #pragma once +#include "gms/feature_service.hh" #include "schema/schema.hh" #include "data_dictionary/data_dictionary.hh" #include "cql3/statements/index_target.hh" +#include +#include #include namespace cql3::expr { @@ -99,6 +102,8 @@ public: bool is_index(view_ptr) const; bool is_index(const schema& s) const; bool is_global_index(const schema& s) const; + std::optional custom_index_class(const schema& s) const; + std::set supported_custom_classes(const gms::feature_service& fs) const; private: void add_index(const index_metadata& im); }; diff --git a/replica/schema_describe_helper.hh b/replica/schema_describe_helper.hh index cf31f5a49c26..05509f14f495 100644 --- a/replica/schema_describe_helper.hh +++ b/replica/schema_describe_helper.hh @@ -11,6 +11,8 @@ #include "data_dictionary/data_dictionary.hh" #include "index/secondary_index_manager.hh" #include "schema/schema.hh" +#include "seastar/core/sstring.hh" +#include namespace replica { @@ -27,6 +29,10 @@ public: return _db.find_column_family(base_id).get_index_manager().is_index(view_s); } + virtual std::optional custom_index_class(const table_id& base_id, const schema& view_s) const override { + return _db.find_column_family(base_id).get_index_manager().custom_index_class(view_s); + } + virtual schema_ptr find_schema(const table_id& id) const override { return _db.find_schema(id); } diff --git a/schema/schema.cc b/schema/schema.cc index 71237ffb13fb..75dbb0855025 100644 --- a/schema/schema.cc +++ b/schema/schema.cc @@ -927,11 +927,21 @@ sstring schema::get_create_statement(const schema_describe_helper& helper, bool if (is_view()) { if (helper.is_index(view_info()->base_id(), *this)) { auto is_local = !helper.is_global_index(view_info()->base_id(), *this); + auto custom_index_class = helper.custom_index_class(view_info()->base_id(), *this); + + if (custom_index_class) { + os << "CUSTOM "; + } os << "INDEX " << cql3::util::maybe_quote(secondary_index::index_name_from_table_name(cf_name())) << " ON " << cql3::util::maybe_quote(ks_name()) << "." << cql3::util::maybe_quote(view_info()->base_name()); describe_index_columns(os, is_local, *this, helper.find_schema(view_info()->base_id())); + + if (custom_index_class) { + os << " USING \'" << *custom_index_class << "\'"; + } + os << ";\n"; return std::move(os).str(); diff --git a/schema/schema.hh b/schema/schema.hh index ecbb80fe5da2..7d43747bf757 100644 --- a/schema/schema.hh +++ b/schema/schema.hh @@ -541,6 +541,7 @@ class schema_describe_helper { public: virtual bool is_global_index(const table_id& base_id, const schema& view_s) const = 0; virtual bool is_index(const table_id& base_id, const schema& view_s) const = 0; + virtual std::optional custom_index_class(const table_id& base_id, const schema& view_s) const = 0; virtual schema_ptr find_schema(const table_id& id) const = 0; virtual ~schema_describe_helper() = default; }; From c0f3648862fdaf9a065e401c0bb891fa491ecf85 Mon Sep 17 00:00:00 2001 From: smoczy123 Date: Thu, 21 Nov 2024 22:13:32 +0100 Subject: [PATCH 2/3] cql3/statements: set index_options in create_index_statement when custom class is provided Fixed validation, remove some conditions that are not needed now. Validate used supported custom classes set instead of hardcoding --- cql3/statements/create_index_statement.cc | 14 +++++++++----- cql3/statements/index_prop_defs.cc | 14 +------------- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/cql3/statements/create_index_statement.cc b/cql3/statements/create_index_statement.cc index 20f57c02a5ba..f1b449f2a95e 100644 --- a/cql3/statements/create_index_statement.cc +++ b/cql3/statements/create_index_statement.cc @@ -67,13 +67,13 @@ create_index_statement::validate(query_processor& qp, const service::client_stat if (_raw_targets.empty() && !_properties->is_custom) { throw exceptions::invalid_request_exception("Only CUSTOM indexes can be created without specifying a target column"); } - + _properties->validate(); } std::vector<::shared_ptr> create_index_statement::validate_while_executing(data_dictionary::database db) const { auto schema = validation::validate_column_family(db, keyspace(), column_family()); - + auto classes = db.find_column_family(schema).get_index_manager().supported_custom_classes(db.features()); if (schema->is_counter()) { throw exceptions::invalid_request_exception("Secondary indexes are not supported on counter tables"); } @@ -88,7 +88,11 @@ std::vector<::shared_ptr> create_index_statement::validate_while_e } validate_for_local_index(*schema); - + + if (_properties && _properties->custom_class && !classes.contains(*(_properties->custom_class))) { + throw exceptions::invalid_request_exception(format("Non-supported custom class \'{}\' provided", *(_properties->custom_class))); + } + std::vector<::shared_ptr> targets; for (auto& raw_target : _raw_targets) { targets.emplace_back(raw_target->prepare(*schema)); @@ -341,9 +345,9 @@ std::optional create_index_s } index_metadata_kind kind; index_options_map index_options; - if (_properties->is_custom) { - kind = index_metadata_kind::custom; + if (_properties->custom_class || _properties->is_custom) { index_options = _properties->get_options(); + kind = index_metadata_kind::custom; } else { kind = schema->is_compound() ? index_metadata_kind::composites : index_metadata_kind::keys; } diff --git a/cql3/statements/index_prop_defs.cc b/cql3/statements/index_prop_defs.cc index 81fa4c7689d0..1166011104ac 100644 --- a/cql3/statements/index_prop_defs.cc +++ b/cql3/statements/index_prop_defs.cc @@ -22,10 +22,7 @@ void cql3::statements::index_prop_defs::validate() { if (is_custom && !custom_class) { throw exceptions::invalid_request_exception("CUSTOM index requires specifying the index class"); } - - if (!is_custom && custom_class) { - throw exceptions::invalid_request_exception("Cannot specify index class for a non-CUSTOM index"); - } + if (!is_custom && !_properties.empty()) { throw exceptions::invalid_request_exception("Cannot specify options for a non-CUSTOM index"); } @@ -36,15 +33,6 @@ void cql3::statements::index_prop_defs::validate() { db::index::secondary_index::custom_index_option_name)); } - // Currently, Scylla does not support *any* class of custom index - // implementation. If in the future we do (e.g., SASI, or something - // new), we'll need to check for valid values here. - if (is_custom && custom_class) { - throw exceptions::invalid_request_exception( - format("Unsupported CUSTOM INDEX class {}. Note that currently, Scylla does not support SASI or any other CUSTOM INDEX class.", - *custom_class)); - - } } index_options_map From b152bf4053ba492698581bc4ec456d42056a0041 Mon Sep 17 00:00:00 2001 From: Balwancia Date: Mon, 25 Nov 2024 23:10:57 +0100 Subject: [PATCH 3/3] test/boost, cqlpy: add custom index tests Tests on custom index using dummy-vector-backend. --- test/boost/secondary_index_test.cc | 14 ++++++----- test/cqlpy/test_describe.py | 38 +++++++++++++++++++++++------- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/test/boost/secondary_index_test.cc b/test/boost/secondary_index_test.cc index 1700c5828713..4e2052688271 100644 --- a/test/boost/secondary_index_test.cc +++ b/test/boost/secondary_index_test.cc @@ -691,7 +691,7 @@ SEASTAR_TEST_CASE(test_secondary_index_collections) { // combination of parameters related to custom indexes are rejected as well. SEASTAR_TEST_CASE(test_secondary_index_create_custom_index) { return do_with_cql_env_thread([] (cql_test_env& e) { - e.execute_cql("create table cf (p int primary key, a int)").get(); + e.execute_cql("create table cf (p int primary key, a int, b int)").get(); // Creating an index on column a works, obviously. e.execute_cql("create index on cf (a)").get(); // The following is legal syntax on Cassandra, to create a SASI index. @@ -708,11 +708,13 @@ SEASTAR_TEST_CASE(test_secondary_index_create_custom_index) { // "exceptions::invalid_request_exception: CUSTOM index requires // specifying the index class" assert_that_failed(e.execute_cql("create custom index on cf (a)")); - // It's also a syntax error to try to specify a "USING" without - // specifying CUSTOM. We expect the exception: - // "exceptions::invalid_request_exception: Cannot specify index class - // for a non-CUSTOM index" - assert_that_failed(e.execute_cql("create index on cf (a) using 'org.apache.cassandra.index.sasi.SASIIndex'")); + // This is the default syntax for specifying a custom index and we + // 'support' "dummy-vector-backend". This should work. + e.execute_cql("create custom index on cf (a) using 'dummy-vector-backend'").get(); + // It's not a syntax error to try to specify a "USING" without + // specifying CUSTOM. This should work. + e.execute_cql("create index on cf (b) using 'dummy-vector-backend'").get(); + }); } diff --git a/test/cqlpy/test_describe.py b/test/cqlpy/test_describe.py index d2191a941922..5ffbf5fa3a6c 100644 --- a/test/cqlpy/test_describe.py +++ b/test/cqlpy/test_describe.py @@ -476,11 +476,28 @@ def test_desc_index(cql, test_keyspace): create_idx_c = f"CREATE INDEX named_index ON {tbl}(c)" # Only Scylla supports local indexes has_local = is_scylla(cql) + + # Cassandra inserts a space between the table name and parentheses, + # Scylla doesn't. This difference doesn't matter because both are + # valid CQL commands + # Scylla doesn't support sai custom class. + if is_scylla(cql): + maybe_space = '' + custom_class = 'dummy-vector-backend' + else: + maybe_space = ' ' + custom_class = 'sai' + + if has_local: create_idx_ab = f"CREATE INDEX ON {tbl}((a), b)" + create_idx_d = f"CREATE INDEX custom ON {tbl}(c) USING '{custom_class}'" + create_idx_e = f"CREATE CUSTOM INDEX custom1 ON {tbl}(b) USING '{custom_class}'" cql.execute(create_idx_b) cql.execute(create_idx_c) + cql.execute(create_idx_d) + cql.execute(create_idx_e) if has_local: cql.execute(create_idx_ab) @@ -488,20 +505,25 @@ def test_desc_index(cql, test_keyspace): if has_local: ab_desc = cql.execute(f"DESC INDEX {test_keyspace}.{tbl_name}_b_idx_1").one().create_statement c_desc = cql.execute(f"DESC INDEX {test_keyspace}.named_index").one().create_statement - - # Cassandra inserts a space between the table name and parentheses, - # Scylla doesn't. This difference doesn't matter because both are - # valid CQL commands - if is_scylla(cql): - maybe_space = '' - else: - maybe_space = ' ' + d_desc = cql.execute(f"DESC INDEX {test_keyspace}.custom").one().create_statement + e_desc = cql.execute(f"DESC INDEX {test_keyspace}.custom1").one().create_statement + assert f"CREATE INDEX {tbl_name}_b_idx ON {tbl}{maybe_space}(b)" in b_desc if has_local: assert f"CREATE INDEX {tbl_name}_b_idx_1 ON {tbl}((a), b)" in ab_desc assert f"CREATE INDEX named_index ON {tbl}{maybe_space}(c)" in c_desc + assert f"CREATE CUSTOM INDEX custom ON {tbl}{maybe_space}(c) USING '{custom_class}'" in d_desc + assert f"CREATE CUSTOM INDEX custom1 ON {tbl}{maybe_space}(b) USING '{custom_class}'" in e_desc + with pytest.raises(InvalidRequest): + cql.execute(f"CREATE INDEX custom ON {tbl}(b) USING '{custom_class}'") + invalid_custom_class = "invalid.custom.class" + with pytest.raises(InvalidRequest): + cql.execute(f"CREATE CUSTOM INDEX invalid_idx ON {tbl}(b) USING '{invalid_custom_class}'") + with pytest.raises(InvalidRequest): + cql.execute(f"CREATE CUSTOM INDEX no_class_idx ON {tbl}(b)") + def test_desc_index_on_collections(cql, test_keyspace): # In this test, all assertions are in form of # `assert create_stmt in desc or create_stmt.replace("(", " (", 1) in desc`