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.

Sign up for GitHub

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

Fix if-not-exists behavior of create index statements #4705

Merged
merged 4 commits into from
Oct 13, 2022
Merged

Fix if-not-exists behavior of create index statements #4705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
628b5a9
fix create index if not exists behavior
czpmango Oct 10, 2022
4ec0231
Merge branch 'master' into fix/create-index-if-not-exists
Sophie-Xie Oct 11, 2022
8a60842
Merge branch 'master' into fix/create-index-if-not-exists
czpmango Oct 13, 2022
8979834
Merge branch 'master' into fix/create-index-if-not-exists
critical27 Oct 13, 2022
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
13 changes: 11 additions & 2 deletions src/meta/processors/index/CreateEdgeIndexProcessor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ void CreateEdgeIndexProcessor::process(const cpp2::CreateEdgeIndexReq& req) {
const auto& indexName = req.get_index_name();
auto& edgeName = req.get_edge_name();
const auto& fields = req.get_fields();
auto ifNotExists = req.get_if_not_exists();

std::set<std::string> columnSet;
for (const auto& field : fields) {
Expand All @@ -41,7 +42,7 @@ void CreateEdgeIndexProcessor::process(const cpp2::CreateEdgeIndexReq& req) {
// check if the space already exist index has the same index name
auto ret = getIndexID(space, indexName);
if (nebula::ok(ret)) {
if (req.get_if_not_exists()) {
if (ifNotExists) {
handleErrorCode(nebula::cpp2::ErrorCode::SUCCEEDED);
} else {
LOG(INFO) << "Create Edge Index Failed: " << indexName << " has existed";
Expand Down Expand Up @@ -96,7 +97,15 @@ void CreateEdgeIndexProcessor::process(const cpp2::CreateEdgeIndexReq& req) {
}

if (checkIndexExist(fields, item)) {
resp_.code_ref() = nebula::cpp2::ErrorCode::E_EXISTED;
if (ifNotExists) {
resp_.code_ref() = nebula::cpp2::ErrorCode::SUCCEEDED;
cpp2::ID thriftID;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use tools like resp_.id_ref() = to(edgeIndex, EntryType::INDEX); ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just extract the id from the index item.

// Fill index id to avoid broken promise
thriftID.index_id_ref() = item.get_index_id();
resp_.id_ref() = thriftID;
Copy link
Contributor

@panda-sheep panda-sheep Oct 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It feels a little weird this way

  1. The first ifNotExists in line 45 is to judge whether the index name exists or not.

2 This place is to judge whether there are other indexes that contain this fields
for example

create edge e(a int, b int);
create edge index e_index on e(a, b)
create edge index if not exists e_index_2 on e(a);

When execute create edge index if not exists e_index_2 on e(a); ，this returns a creation index success and returns the indexId of the first index.
There is actually only one index e_index

Copy link
Contributor

Choose a reason for hiding this comment

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

It shows that the index creation is successful, but when show, the index name does not exist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, this is the point just as you said.
IMO, index name is a redundant design.
A better syntax might be this:

create edge index on e(a);
create edge index if not exists on e(a);
rebuild edge index on e(a);

In this way, the implementation layer does not need to verify the index name(line 45).
Considering compatibility, this pr is not going to do major refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

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

@czpmango @MuYiYong How is our GQL version index implemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should try to avoid similar problems in GQL version.

Copy link
Contributor

Choose a reason for hiding this comment

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

5.0 index design needs to consider this problem, thx @MuYiYong

} else {
resp_.code_ref() = nebula::cpp2::ErrorCode::E_EXISTED;
}
onFinished();
return;
}
Expand Down
13 changes: 11 additions & 2 deletions src/meta/processors/index/CreateTagIndexProcessor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ void CreateTagIndexProcessor::process(const cpp2::CreateTagIndexReq& req) {
const auto& indexName = req.get_index_name();
auto& tagName = req.get_tag_name();
const auto& fields = req.get_fields();
auto ifNotExists = req.get_if_not_exists();

std::set<std::string> columnSet;
for (const auto& field : fields) {
Expand All @@ -42,7 +43,7 @@ void CreateTagIndexProcessor::process(const cpp2::CreateTagIndexReq& req) {
// check if the space has the index with the same name
auto ret = getIndexID(space, indexName);
if (nebula::ok(ret)) {
if (req.get_if_not_exists()) {
if (ifNotExists) {
handleErrorCode(nebula::cpp2::ErrorCode::SUCCEEDED);
} else {
LOG(INFO) << "Create Tag Index Failed: " << indexName << " has existed";
Expand Down Expand Up @@ -96,7 +97,15 @@ void CreateTagIndexProcessor::process(const cpp2::CreateTagIndexReq& req) {
}

if (checkIndexExist(fields, item)) {
resp_.code_ref() = nebula::cpp2::ErrorCode::E_EXISTED;
if (ifNotExists) {
resp_.code_ref() = nebula::cpp2::ErrorCode::SUCCEEDED;
cpp2::ID thriftID;
// Fill index id to avoid broken promise
thriftID.index_id_ref() = item.get_index_id();
resp_.id_ref() = thriftID;
} else {
resp_.code_ref() = nebula::cpp2::ErrorCode::E_EXISTED;
}
onFinished();
return;
}
Expand Down
10 changes: 10 additions & 0 deletions tests/tck/features/index/Index.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ Feature: IndexTest_Vid_String
CREATE TAG INDEX single_tag_index ON tag_1(col2);
"""
Then the execution should be successful
When executing query:
"""
CREATE TAG INDEX IF NOT EXISTS single_tag_index_1 ON tag_1(col2);
"""
Then the execution should be successful
When executing query:
"""
CREATE TAG INDEX duplicate_tag_index_1 ON tag_1(col2);
Expand Down Expand Up @@ -155,6 +160,11 @@ Feature: IndexTest_Vid_String
CREATE EDGE INDEX single_edge_index ON edge_1(col2);
"""
Then the execution should be successful
When executing query:
"""
CREATE EDGE INDEX IF NOT EXISTS single_edge_index_1 ON edge_1(col2);
"""
Then the execution should be successful
When executing query:
"""
CREATE EDGE INDEX duplicate_edge_1_index ON edge_1(col2)
Expand Down