Skip to content

Commit

Permalink
Suggested changes and tests
Browse files Browse the repository at this point in the history
  • Loading branch information
smoczy123 committed Nov 25, 2024
1 parent a5113d1 commit c087ffe
Show file tree
Hide file tree
Showing 9 changed files with 37 additions and 31 deletions.
13 changes: 8 additions & 5 deletions cql3/statements/create_index_statement.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "cql3/statements/index_prop_defs.hh"
#include "index/secondary_index_manager.hh"
#include "mutation/mutation.hh"
#include "view_info.hh"

#include <boost/range/adaptor/transformed.hpp>
#include <boost/algorithm/string/join.hpp>
Expand Down Expand Up @@ -69,14 +70,12 @@ 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");
}

gms::feature_service fs = gms::feature_service(gms::feature_config_from_db_config(qp.db().get_config()));
_properties->validate(fs);
_properties->validate();
}

std::vector<::shared_ptr<index_target>> 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");
}
Expand All @@ -91,7 +90,11 @@ std::vector<::shared_ptr<index_target>> 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("Non-supported custom class provided");
}

std::vector<::shared_ptr<index_target>> targets;
for (auto& raw_target : _raw_targets) {
targets.emplace_back(raw_target->prepare(*schema));
Expand Down
15 changes: 2 additions & 13 deletions cql3/statements/index_prop_defs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,15 @@
#include "index/secondary_index.hh"
#include "exceptions/exceptions.hh"

void cql3::statements::index_prop_defs::validate(gms::feature_service &fs) {
void cql3::statements::index_prop_defs::validate() {
static std::set<sstring> keywords({ sstring(KW_OPTIONS) });

property_definitions::validate(keywords);

if (is_custom && !custom_class) {
throw exceptions::invalid_request_exception("CUSTOM index requires specifying the index class");
}
if (custom_class && !(fs.supported_custom_classes().contains(*custom_class))) {
throw exceptions::invalid_request_exception("Unsupported custom index class " + *custom_class);
}

if (!is_custom && !_properties.empty()) {
throw exceptions::invalid_request_exception("Cannot specify options for a non-CUSTOM index");
}
Expand All @@ -36,15 +34,6 @@ void cql3::statements::index_prop_defs::validate(gms::feature_service &fs) {
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
Expand Down
2 changes: 1 addition & 1 deletion cql3/statements/index_prop_defs.hh
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public:
bool is_custom = false;
std::optional<sstring> custom_class;

void validate(gms::feature_service &fs);
void validate();
index_options_map get_raw_options();
index_options_map get_options();
};
Expand Down
6 changes: 0 additions & 6 deletions gms/feature_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -396,12 +396,6 @@ future<> feature_service::enable(std::set<std::string_view> list) {
});
}

std::set<std::string_view> feature_service::supported_custom_classes() const {
std::set<std::string_view> classes = {
"dummy-vector-backend"sv,
};

return classes;
}

} // namespace gms
1 change: 0 additions & 1 deletion gms/feature_service.hh
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ public:
future<> enable(std::set<std::string_view> list);
db::schema_features cluster_schema_features() const;
std::set<std::string_view> supported_feature_set() const;
std::set<std::string_view> supported_custom_classes() const;

// Key in the 'system.scylla_local' table, that is used to
// persist enabled features
Expand Down
11 changes: 11 additions & 0 deletions index/secondary_index_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@

#include <boost/range/adaptor/map.hpp>
#include <boost/algorithm/string/predicate.hpp>
#include <set>
#include <string_view>

namespace secondary_index {

Expand Down Expand Up @@ -359,4 +361,13 @@ std::optional<sstring> secondary_index_manager::custom_index_class(const schema&
}
}

std::set<std::string_view> secondary_index_manager::supported_custom_classes(const gms::feature_service& fs) const {
using namespace std::literals;
// TEMP: when actual vector backend will be added, this will create the set from features
std::set<std::string_view> classes = {
"dummy-vector-backend"sv,
};
return classes;
}

}
4 changes: 4 additions & 0 deletions index/secondary_index_manager.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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 <set>
#include <string_view>
#include <vector>

namespace cql3::expr {
Expand Down Expand Up @@ -100,6 +103,7 @@ public:
bool is_index(const schema& s) const;
bool is_global_index(const schema& s) const;
std::optional<sstring> custom_index_class(const schema& s) const;
std::set<std::string_view> supported_custom_classes(const gms::feature_service& fs) const;
private:
void add_index(const index_metadata& im);
};
Expand Down
12 changes: 7 additions & 5 deletions test/boost/secondary_index_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 (a) using 'dummy-vector-backend'").get();

});
}

Expand Down
4 changes: 4 additions & 0 deletions test/cqlpy/test_describe.py
Original file line number Diff line number Diff line change
Expand Up @@ -477,16 +477,19 @@ def test_desc_index(cql, test_keyspace):
has_local = is_scylla(cql)
if has_local:
create_idx_ab = f"CREATE INDEX ON {tbl}((a), b)"
create_idx_d = f"CREATE INDEX custom ON {tbl}(c) USING 'dummy-vector-backend'"

cql.execute(create_idx_b)
cql.execute(create_idx_c)
cql.execute(create_idx_d)
if has_local:
cql.execute(create_idx_ab)

b_desc = cql.execute(f"DESC INDEX {test_keyspace}.{tbl_name}_b_idx").one().create_statement
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
d_desc = cql.execute(f"DESC INDEX {test_keyspace}.custom").one().create_statement

# Cassandra inserts a space between the table name and parentheses,
# Scylla doesn't. This difference doesn't matter because both are
Expand All @@ -500,6 +503,7 @@ def test_desc_index(cql, test_keyspace):
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 INDEX custom ON {tbl}{maybe_space}(c) USING 'dummy-vector-backend'" in d_desc

def test_desc_index_on_collections(cql, test_keyspace):
# In this test, all assertions are in form of
Expand Down

0 comments on commit c087ffe

Please sign in to comment.