Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added handling of the USING clause in CREATE INDEX for dummy-vector-backend #6

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions cql3/statements/create_index_statement.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: unnecessary whitespace change

_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());
smoczy123 marked this conversation as resolved.
Show resolved Hide resolved
if (schema->is_counter()) {
throw exceptions::invalid_request_exception("Secondary indexes are not supported on counter tables");
}
Expand All @@ -88,7 +88,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(format("Non-supported custom class \'{}\' provided", *(_properties->custom_class)));
}

std::vector<::shared_ptr<index_target>> targets;
for (auto& raw_target : _raw_targets) {
targets.emplace_back(raw_target->prepare(*schema));
Expand Down Expand Up @@ -341,9 +345,9 @@ std::optional<create_index_statement::base_schema_with_new_index> 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;
}
Expand Down
14 changes: 1 addition & 13 deletions cql3/statements/index_prop_defs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand All @@ -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));

}
Comment on lines -39 to -47
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen if you try to create a CUSTOM index, without specifying any custom class? I'm not sure it should be allowed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not allowed, it's checked in test_secondary_index_create_custom_index

}

index_options_map
Expand Down
26 changes: 26 additions & 0 deletions index/secondary_index_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,25 @@
* SPDX-License-Identifier: (AGPL-3.0-or-later and Apache-2.0)
*/

#include <optional>
#include <ranges>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index: function that returns the custom class of an index, set of sup…

…posted custom classes

This commit is lacking motivation. I'm reading this as the first commit and don't understand why you need to introduce those functions.

I believe it would be better to introduce those functions in the same commit where you use it. Then, it won't be necessary to provide motivation - it should be clear from the contents of the commit that those helper functions support the goal that the commit strives to achieve.

#include <seastar/core/shared_ptr.hh>

#include "index/secondary_index_manager.hh"

#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"
#include "db/tags/extension.hh"

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

namespace secondary_index {

Expand Down Expand Up @@ -344,4 +349,25 @@ bool secondary_index_manager::is_global_index(const schema& s) const {
});
}

std::optional<sstring> 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<std::string_view> 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<std::string_view> classes = {
"dummy-vector-backend"sv,
};
return classes;
}

}
5 changes: 5 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 @@ -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<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
6 changes: 6 additions & 0 deletions replica/schema_describe_helper.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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 <optional>

namespace replica {

Expand All @@ -27,6 +29,10 @@ public:
return _db.find_column_family(base_id).get_index_manager().is_index(view_s);
}

virtual std::optional<sstring> 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);
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: one newline too many.

virtual schema_ptr find_schema(const table_id& id) const override {
return _db.find_schema(id);
}
Expand Down
10 changes: 10 additions & 0 deletions schema/schema.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions schema/schema.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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<sstring> 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;
};
Expand Down
14 changes: 8 additions & 6 deletions test/boost/secondary_index_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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();

});
}

Expand Down
38 changes: 30 additions & 8 deletions test/cqlpy/test_describe.py
Original file line number Diff line number Diff line change
Expand Up @@ -476,32 +476,54 @@ 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)

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

# 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see some more tests for the validation logic of CREATE (CUSTOM) INDEX. For example:

  • USING custom_class but with CREATE INDEX and not CREATE CUSTOM INDEX
  • Passing custom class that is not supported by Scylla
  • CUSTOM index without any custom class being provided


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`
Expand Down
Loading