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

Lookup sentence #1705

Merged
merged 27 commits into from
Feb 19, 2020
Merged

Lookup sentence #1705

merged 27 commits into from
Feb 19, 2020

Conversation

bright-starry-sky
Copy link
Contributor

This PR based on PR #1437.

@bright-starry-sky
Copy link
Contributor Author

Ready to review. Thanks.

src/graph/LookupExecutor.cpp Outdated Show resolved Hide resolved
src/graph/test/LookupTestBase.h Show resolved Hide resolved
src/graph/test/LookupTest.cpp Show resolved Hide resolved
src/graph/LookupExecutor.cpp Show resolved Hide resolved
src/graph/LookupExecutor.cpp Outdated Show resolved Hide resolved
src/graph/LookupExecutor.cpp Outdated Show resolved Hide resolved
Status LookupExecutor::prepareFrom() {
from_ = sentence_->from();
auto *mc = ectx()->getMetaClient();
auto tagResult = mc->getTagIDByNameFromCache(spaceId_, *from_);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

src/graph/LookupExecutor.cpp Outdated Show resolved Hide resolved
src/graph/LookupExecutor.cpp Show resolved Hide resolved
src/graph/LookupExecutor.cpp Outdated Show resolved Hide resolved
src/graph/LookupExecutor.cpp Outdated Show resolved Hide resolved
src/graph/LookupExecutor.cpp Outdated Show resolved Hide resolved
src/graph/LookupExecutor.cpp Show resolved Hide resolved
src/graph/LookupExecutor.cpp Outdated Show resolved Hide resolved
@bright-starry-sky
Copy link
Contributor Author

Addressed dangleptr's comments

@bright-starry-sky
Copy link
Contributor Author

Add expression check to avoid memory leak

src/graph/LookupExecutor.cpp Outdated Show resolved Hide resolved
src/graph/LookupExecutor.cpp Outdated Show resolved Hide resolved
src/graph/LookupExecutor.cpp Outdated Show resolved Hide resolved
src/graph/LookupExecutor.cpp Outdated Show resolved Hide resolved
traversalExpression(left);
traversalExpression(right);
break;
optimizedPolicy_ = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why exit?

Copy link
Contributor

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?

Copy link
Contributor Author

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it

src/graph/LookupExecutor.cpp Show resolved Hide resolved
src/graph/LookupExecutor.cpp Outdated Show resolved Hide resolved
dangleptr
dangleptr previously approved these changes Feb 18, 2020
Copy link
Contributor

@dangleptr dangleptr left a 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.

panda-sheep
panda-sheep previously approved these changes Feb 19, 2020
Copy link
Contributor

@panda-sheep panda-sheep left a 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.

{200},
};
ASSERT_TRUE(verifyResult(resp, expected));
ASSERT_TRUE(verifyResult(resp, expected));
Copy link
Contributor

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;
}
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good suggestion.

} else {
result.emplace_back("VertexID");
}
result.reserve(yields_.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

line 393 has reserved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sharp eyes

bright-starry-sky added 3 commits February 19, 2020 16:27
…xPrefix from index key. 3: Removed IndexItem from all thrift requests. 3: Moved hint from storage.thrift to common.thrift
@bright-starry-sky
Copy link
Contributor Author

Addressed panda-sheep's comment.

@dangleptr dangleptr merged commit 73baf2a into vesoft-inc:master Feb 19, 2020
@bright-starry-sky bright-starry-sky deleted the lookup_sentence branch February 19, 2020 09:52
@jude-zhu
Copy link
Contributor

close #465

tong-hao pushed a commit to tong-hao/nebula that referenced this pull request Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants