-
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
Support Index Operations #1459
Support Index Operations #1459
Conversation
Jenkins go |
Unit testing failed. |
src/graph/ShowExecutor.cpp
Outdated
auto items = std::move(resp).value(); | ||
resp_ = std::make_unique<cpp2::ExecutionResponse>(); | ||
std::vector<cpp2::RowValue> rows; | ||
std::vector<std::string> header{"Index ID", "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.
The Index ID should not be known to the user, modify to TagName
, WDYT?
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.
actually it's the index's name :)
Unit testing passed. |
Please resolve the conflicts, thanks. |
} | ||
|
||
auto *mc = ectx()->getMetaClient(); | ||
auto *name = sentence_->indexName(); |
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.
const
is better?
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 don't know what's the practical use ?
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 don't know what's the practical use ?
Actually it's just my code like, here's what I think :
Reason 1 : The methodsentence_->indexName()
return value is a const string *.
Reason 2 : This value is an immutable constant.
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 the index name is immutable. The compiler maybe have some optimization for the const auto*
.
auto cb = [this] (auto &&resp) { | ||
if (!resp.ok()) { | ||
DCHECK(onError_); | ||
onError_(std::move(resp).status()); |
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.
std::forward
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.
std::move
takes an object and allows you to treat it as a temporary.
std::forward
has a single use case: to cast a templated function parameter to the value category the caller used to pass it.
When the parameter is an rvalue, they are treated are the same.
Unit testing passed. |
1 similar comment
Unit testing passed. |
Unit testing passed. |
Conflicts: src/graph/SchemaHelper.cpp src/parser/parser.yy
Unit testing passed. |
1 similar comment
Unit testing passed. |
properties.emplace(std::move(edgeName), std::move(fields)); | ||
return createEdgeIndex(spaceID, | ||
std::move(indexName), | ||
std::move(properties), |
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 we support create index on multi fields? It seems storage only support index on first field?
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.
actually I have update to single schema in another PR
} | ||
|
||
|
||
void DescribeEdgeIndexExecutor::execute() { |
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 we need desc for index, show index
is enough?
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.
SHOW TAG/EDGE INDEXES
will display an index list.
Unit testing passed. |
return edgeName_.get(); | ||
} | ||
|
||
std::vector<std::string> names() const { |
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.
naming columns
is better?
Unit testing failed. |
Unit testing passed. |
Reference document : #1578 |
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.
Well done. The pr lgtm
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
Unit testing passed. |
Unit testing passed. |
close #470 |
relate to #463 |
No description provided.