-
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
Lookup sentence #1705
Lookup sentence #1705
Conversation
Ready to review. Thanks. |
Status LookupExecutor::prepareFrom() { | ||
from_ = sentence_->from(); | ||
auto *mc = ectx()->getMetaClient(); | ||
auto tagResult = mc->getTagIDByNameFromCache(spaceId_, *from_); |
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 have forbidden same name in tag and edge?
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 have forbidden same name in tag and edge?
Yes, the tag name and edge names are unique. so it's ok.
Addressed dangleptr's comments |
Add expression check to avoid memory leak |
traversalExpression(left); | ||
traversalExpression(right); | ||
break; | ||
optimizedPolicy_ = false; |
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.
Why exit?
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.
Index (col1, col2)
For the expression "co1 > 1 and col1 < 10 and col2 > 5", the index can't be used?
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.
This index can be used. This is a standard logical expression.
for example in my test :
CREATE EDGE lookup_edge_1(col1 int, col2 int, col3 int);
CREATE EDGE INDEX e_index_1 ON lookup_edge_1(col1, col2, col3);
LOOKUP ON lookup_edge_1 WHERE lookup_edge_1.col1 >1 and lookup_edge_1.col1 < 3 and lookup_edge_1.col2 > 1;
Execution succeeded (Time spent: 2.461/4.992 ms)
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.
Got 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 now. Well done.
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.
Orz, brilliant, LGTM.
There are a little suggestions internally.
src/graph/test/LookupTest.cpp
Outdated
{200}, | ||
}; | ||
ASSERT_TRUE(verifyResult(resp, expected)); | ||
ASSERT_TRUE(verifyResult(resp, expected)); |
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.
Redundant
status = checkIfGraphSpaceChosen(); | ||
if (!status.ok()) { | ||
break; | ||
} |
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.
a suggestion, line 24 and line 27 move here
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.
good suggestion.
src/graph/LookupExecutor.cpp
Outdated
} else { | ||
result.emplace_back("VertexID"); | ||
} | ||
result.reserve(yields_.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.
line 393 has reserved
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.
Sharp eyes
…xPrefix from index key. 3: Removed IndexItem from all thrift requests. 3: Moved hint from storage.thrift to common.thrift
…, 3, Remove IndexOptimizer class, 4, Add testcases
c2519a8
Addressed panda-sheep's comment. |
close #465 |
This PR based on PR #1437.