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

Feature/scan multiple parts #3262

Merged
merged 17 commits into from
Nov 11, 2021

Conversation

Shylock-Hg
Copy link
Contributor

@Shylock-Hg Shylock-Hg commented Nov 3, 2021

What type of PR is this?

  • bug
  • feature

Which issue(s) this PR fixes:

Subjob of #3106
(If it is requirement, issue(s) number must be listed.)

What this PR does / why we need it?

Extend the Scan storage interface to support scan multiple partitions. And ScanVertex will return kVid as first column to keep same with GetProp.

Special notes for your reviewer, ex. impact of this fix, etc:

Additional context:

@jievince @Nicole00 The scan interface need update.

Checklist:

  • Documentation affected (If need to modify document, please label it.)
  • Incompatible (If it is incompatile, please describle it and label it.)
  • Need to cherry pick (If need to cherry pick to some branchs, please label the destination version(s).)
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory

Release notes:

Please confirm whether to reflect in release notes and how to describe:

                                                            `

@Shylock-Hg Shylock-Hg added the ready-for-testing PR: ready for the CI test label Nov 3, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2021

Codecov Report

Merging #3262 (223bb5c) into master (58f0b44) will decrease coverage by 0.01%.
The diff coverage is 87.91%.

❗ Current head 223bb5c differs from pull request most recent head 4601449. Consider uploading reports for the commit 4601449 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3262      +/-   ##
==========================================
- Coverage   85.24%   85.23%   -0.02%     
==========================================
  Files        1295     1305      +10     
  Lines      118190   120394    +2204     
==========================================
+ Hits       100748   102612    +1864     
- Misses      17442    17782     +340     
Impacted Files Coverage Δ
src/clients/storage/GraphStorageClient.cpp 73.11% <0.00%> (-3.24%) ⬇️
src/clients/storage/GraphStorageClient.h 100.00% <ø> (ø)
src/clients/storage/StorageClientBase-inl.h 72.34% <0.00%> (-5.83%) ⬇️
src/clients/storage/StorageClientBase.h 65.57% <ø> (+4.03%) ⬆️
src/storage/ExprVisitorBase.h 0.00% <0.00%> (ø)
src/storage/exec/RelNode.h 79.06% <ø> (-18.61%) ⬇️
src/storage/exec/StorageIterator.h 94.33% <ø> (+5.45%) ⬆️
src/storage/query/ScanEdgeProcessor.h 100.00% <ø> (ø)
src/storage/query/ScanVertexProcessor.h 100.00% <ø> (ø)
src/storage/test/LookupIndexTest.cpp 100.00% <ø> (ø)
... and 64 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 1d0c583...4601449. Read the comment docs.

Copy link
Contributor

@critical27 critical27 left a comment

Choose a reason for hiding this comment

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

Good job, clean code. Only one question, others LGTM.

src/storage/exec/ScanNode.h Outdated Show resolved Hide resolved
@@ -557,24 +557,29 @@ struct LookupAndTraverseRequest {
* End of Index section
*/

struct ScanCursor {
3: bool has_next,
Copy link
Contributor

Choose a reason for hiding this comment

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

The has_next seems useless, just judge by next_cursor is enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I just keep the origin fields.

src/interface/storage.thrift Show resolved Hide resolved
@Shylock-Hg Shylock-Hg requested a review from critical27 November 4, 2021 08:32
@Shylock-Hg Shylock-Hg added this to the v3.0.0 milestone Nov 4, 2021
src/storage/exec/ScanNode.h Show resolved Hide resolved
src/storage/exec/ScanNode.h Outdated Show resolved Hide resolved
@Shylock-Hg Shylock-Hg requested a review from critical27 November 9, 2021 08:33
Copy link
Contributor

@critical27 critical27 left a comment

Choose a reason for hiding this comment

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

Good job, thx!

@cangfengzhs
Copy link
Contributor

Overall it's ok. There is a small problem. I think from the perspective of a volcano model, only the leaf nodes TagNode/EdgeNode need to access kvstore, so cursor should be directly assigned to TagNode/EdgeNode instead of directly accessing kvstore in ScanNode and then key/value is given to TagNode /EdgeNode wraps once. Of course, this has something to do with the inadequate standards of our current implementation plan, which can be modified later.

@Shylock-Hg
Copy link
Contributor Author

Overall it's ok. There is a small problem. I think from the perspective of a volcano model, only the leaf nodes TagNode/EdgeNode need to access kvstore, so cursor should be directly assigned to TagNode/EdgeNode instead of directly accessing kvstore in ScanNode and then key/value is given to TagNode /EdgeNode wraps once. Of course, this has something to do with the inadequate standards of our current implementation plan, which can be modified later.

Yes, the ScanNode is too heavy, refactor in volcano model maybe is better.

@critical27 critical27 merged commit 8e0028f into vesoft-inc:master Nov 11, 2021
@Sophie-Xie Sophie-Xie removed the ready-for-testing PR: ready for the CI test label Nov 12, 2021
@Shylock-Hg Shylock-Hg deleted the feature/scan_multiple_parts branch February 9, 2022 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants