-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Check the index have existed although the name not the same #1695
Conversation
} | ||
|
||
for (size_t i = 0; i < fieldNames.size(); i++) { | ||
if (fieldNames[i] != item.get_fields()[i].get_name()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you ensure the item have the same number fields as the filedNames?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, when the index's field is disaccord, they are not the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So maybe access invalid index on "item.get_fields()[i]"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a checker, if the size of current index is longer than the previous one , it will skip.
while (checkIter->valid()) { | ||
auto val = checkIter->val(); | ||
auto item = MetaServiceUtils::parseIndex(val); | ||
if (item.get_schema_id().getType() != nebula::cpp2::SchemaID::Type::tag_id) { | ||
continue; | ||
} | ||
|
||
for (size_t i = 0; i < fieldNames.size(); i++) { | ||
if (fieldNames[i] != item.get_fields()[i].get_name()) { | ||
break; | ||
} | ||
|
||
if (i == fieldNames.size() - 1) { | ||
LOG(ERROR) << "Tag Index " << indexName << " have existed"; | ||
resp_.set_code(cpp2::ErrorCode::E_EXISTED); | ||
onFinished(); | ||
return; | ||
} | ||
} | ||
checkIter->next(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example , exist index1 (col1, col2), then we want create index2(col1, col2, col4), is it work well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
In the test case single_index
create on name
and multi_index
indexed on name
and email
. It's work.
@@ -43,6 +43,38 @@ void CreateTagIndexProcessor::process(const cpp2::CreateTagIndexReq& req) { | |||
return; | |||
} | |||
|
|||
auto prefix = MetaServiceUtils::indexPrefix(space); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we unify the code.
Codecov Report
@@ Coverage Diff @@
## master #1695 +/- ##
==========================================
+ Coverage 86.90% 87.15% +0.25%
==========================================
Files 636 641 +5
Lines 59819 61163 +1344
==========================================
+ Hits 51984 53307 +1323
- Misses 7835 7856 +21
Continue to review full report at Codecov.
|
@@ -454,5 +454,21 @@ BaseProcessor<RESP>::indexCheck(const std::vector<nebula::cpp2::IndexItem>& item | |||
return cpp2::ErrorCode::SUCCEEDED; | |||
} | |||
|
|||
template<typename RESP> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think better put this function to processor/indexMan/
folder, because it's not the concept of BaseProcessor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check item.get_fields().szie() >= fields.size()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before we check the index have checked the fields size.
It is a bug. We should address it in next version. @darionyaphet Could you rebase it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Co-authored-by: Sophie <[email protected]> Co-authored-by: hs.zhang <[email protected]> Co-authored-by: Sophie <[email protected]>
No description provided.