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

Support GO M TO N STEPS which record the data in go traversal. #2091

Merged
merged 10 commits into from May 22, 2020
Merged

Support GO M TO N STEPS which record the data in go traversal. #2091

merged 10 commits into from May 22, 2020

Conversation

Shylock-Hg
Copy link
Contributor

@Shylock-Hg Shylock-Hg commented Apr 23, 2020

What changes were proposed in this pull request?

Support the grammar GO [[<M> TO] <N> STEPS] to collect the data in GO traversal.
If can't reach N steps, return empty now.(Will be change in #2148

Why are the changes needed?

Does this PR introduce any user-facing change?

Yes, @Amber1990Zhang

How was this patch tested?

Checklist

  • I've run the tests to see all new and existing tests pass
  • If this Pull Request resolves an issue, I linked to the issue in the text above
  • I've informed the technical writer about the documentation change if necessary

@Shylock-Hg Shylock-Hg added feature ready-for-testing PR: ready for the CI test labels Apr 23, 2020
@Amber1990Zhang Amber1990Zhang added the doc affected PR: improvements or additions to documentation label Apr 24, 2020
@codecov-io
Copy link

codecov-io commented Apr 26, 2020

Codecov Report

Merging #2091 into master will increase coverage by 0.10%.
The diff coverage is 85.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2091      +/-   ##
==========================================
+ Coverage   86.70%   86.81%   +0.10%     
==========================================
  Files         641      641              
  Lines       61190    61523     +333     
==========================================
+ Hits        53056    53411     +355     
+ Misses       8134     8112      -22     
Impacted Files Coverage Δ
src/common/encryption/Base64.cpp 0.00% <0.00%> (ø)
src/common/http/HttpClient.cpp 42.85% <0.00%> (ø)
src/graph/CloudAuthenticator.cpp 0.00% <0.00%> (ø)
src/graph/GraphService.h 100.00% <ø> (ø)
src/meta/client/MetaClient.cpp 83.23% <0.00%> (ø)
src/meta/client/MetaClient.h 100.00% <ø> (ø)
src/parser/Clauses.cpp 26.20% <0.00%> (ø)
src/graph/test/TestBase.h 70.00% <50.00%> (-0.34%) ⬇️
src/graph/GraphService.cpp 73.46% <55.55%> (ø)
src/common/permission/PermissionManager.cpp 75.00% <60.00%> (ø)
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 311bed3...c931d6c. Read the comment docs.

@Shylock-Hg Shylock-Hg requested a review from CPWstatic May 6, 2020 06:16
@Shylock-Hg Shylock-Hg requested a review from monadbobo May 18, 2020 06:52
@codecov-commenter
Copy link

codecov-commenter commented May 20, 2020

Codecov Report

Merging #2091 into master will increase coverage by 0.04%.
The diff coverage is 84.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2091      +/-   ##
==========================================
+ Coverage   86.84%   86.88%   +0.04%     
==========================================
  Files         641      641              
  Lines       61651    61983     +332     
==========================================
+ Hits        53541    53856     +315     
- Misses       8110     8127      +17     
Impacted Files Coverage Δ
src/meta/MetaServiceUtils.h 100.00% <ø> (ø)
src/meta/processors/BaseProcessor.h 81.39% <ø> (ø)
.../meta/processors/schemaMan/CreateEdgeProcessor.cpp 74.74% <ø> (ø)
...c/meta/processors/schemaMan/CreateTagProcessor.cpp 76.00% <ø> (ø)
src/parser/Clauses.cpp 26.20% <0.00%> (ø)
src/meta/MetaServiceUtils.cpp 86.91% <36.36%> (-5.95%) ⬇️
src/graph/test/TestBase.h 70.00% <50.00%> (-0.34%) ⬇️
...c/meta/processors/schemaMan/AlterEdgeProcessor.cpp 77.77% <70.00%> (-1.91%) ⬇️
...rc/meta/processors/schemaMan/AlterTagProcessor.cpp 77.77% <70.00%> (-1.91%) ⬇️
src/graph/SchemaHelper.cpp 86.84% <76.56%> (-0.31%) ⬇️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8a0030...f62dc23. Read the comment docs.

Copy link
Contributor

@laura-ding laura-ding left a comment

Choose a reason for hiding this comment

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

How about only save the data which we hope? now it may cost much memory.

return;
}
starts_ = std::move(status).value();
auto dsts = getDstIdsFromResps(records_.end() - 1, records_.end());
Copy link
Contributor

Choose a reason for hiding this comment

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

modify records_.end() - 1 to records_.rbegin()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's better clear.

return;
}

void GoExecutor::onVertexProps(RpcResponse &&rpcResp) {
UNUSED(rpcResp);
}

StatusOr<std::vector<VertexID>> GoExecutor::getDstIdsFromResp(RpcResponse &rpcResp) const {
std::vector<VertexID> GoExecutor::getDstIdsFromResps(std::vector<RpcResponse>::iterator begin,
Copy link
Contributor

Choose a reason for hiding this comment

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

How about std::vector<VertexID> GoExecutor::getDstIdsFromResps(uint32_t beginPos) the beginPos is the first element of recordFrom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think iterator is better.

@Shylock-Hg
Copy link
Contributor Author

Shylock-Hg commented May 21, 2020

How about only save the data which we hope? now it may cost much memory.

It's to unify the process. Won't record the unused properties.
And another I want to avoid once extra copy the response.

@dutor dutor added ready-for-testing PR: ready for the CI test and removed ready-for-testing PR: ready for the CI test labels May 22, 2020
Copy link
Contributor

@dutor dutor left a comment

Choose a reason for hiding this comment

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

LGTM.

@dutor dutor merged commit 7eaa5fb into vesoft-inc:master May 22, 2020
jude-zhu pushed a commit to jude-zhu/nebula that referenced this pull request May 22, 2020
…oft-inc#2091)

* Support `GO M TO N STEPS` which record the data in go traversal.

* Add the parser testing.

* Put the variable to cross all loop.

* Add test.

* Comfirm the bidirectionally cases.

* Add cases for over *.

* Let the intermidiate response return the properies if specified.
dutor pushed a commit that referenced this pull request May 22, 2020
…) (#2136)

* Support `GO M TO N STEPS` which record the data in go traversal.

* Add the parser testing.

* Put the variable to cross all loop.

* Add test.

* Comfirm the bidirectionally cases.

* Add cases for over *.

* Let the intermidiate response return the properies if specified.

Co-authored-by: Shylock Hg <33566796+Shylock-Hg@users.noreply.github.com>
@Amber1990Zhang
Copy link
Contributor

doc done

tong-hao pushed a commit to tong-hao/nebula that referenced this pull request Jun 1, 2021
…oft-inc#2091)

* Support `GO M TO N STEPS` which record the data in go traversal.

* Add the parser testing.

* Put the variable to cross all loop.

* Add test.

* Comfirm the bidirectionally cases.

* Add cases for over *.

* Let the intermidiate response return the properies if specified.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc affected PR: improvements or additions to documentation 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