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

Adjust get neighbor plan #3458

Merged
merged 6 commits into from
Dec 29, 2021

Conversation

liuyu85cn
Copy link
Contributor

What type of PR is this?

  • bug
  • [ X] feature
  • enhancement

What does this PR do?

Which issue(s)/PR(s) this PR relates to?

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

Additional context:

Checklist:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatible (If it is incompatible, please describe it and add corresponding label.)
  • Need to cherry-pick (If need to cherry-pick to some branches, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

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

                                                            `

@liuyu85cn liuyu85cn marked this pull request as ready for review December 14, 2021 08:30
@liuyu85cn liuyu85cn added the ready-for-testing PR: ready for the CI test label Dec 20, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2021

Codecov Report

Merging #3458 (7f533b5) into master (ab73b4c) will increase coverage by 0.00%.
The diff coverage is 90.08%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #3458     +/-   ##
=========================================
  Coverage   85.23%   85.23%             
=========================================
  Files        1277     1308     +31     
  Lines      119088   122744   +3656     
=========================================
+ Hits       101502   104622   +3120     
- Misses      17586    18122    +536     
Impacted Files Coverage Δ
src/clients/meta/MetaClient.cpp 74.73% <0.00%> (-1.19%) ⬇️
src/graph/optimizer/rule/CollapseProjectRule.cpp 98.24% <ø> (ø)
...timizer/rule/OptimizeEdgeIndexScanByFilterRule.cpp 90.54% <ø> (ø)
...ptimizer/rule/OptimizeTagIndexScanByFilterRule.cpp 94.59% <ø> (+4.31%) ⬆️
...h/optimizer/rule/PushLimitDownGetNeighborsRule.cpp 93.75% <ø> (+0.20%) ⬆️
...timizer/rule/PushStepLimitDownGetNeighborsRule.cpp 94.11% <ø> (+0.36%) ⬆️
...imizer/rule/PushStepSampleDownGetNeighborsRule.cpp 93.93% <ø> (ø)
src/graph/util/FTIndexUtils.cpp 4.12% <ø> (ø)
src/graph/validator/FindPathValidator.cpp 84.78% <ø> (ø)
src/graph/validator/MutateValidator.cpp 94.17% <ø> (+0.11%) ⬆️
... and 255 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 6300233...7f533b5. 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.

Generally LGTM~

src/storage/exec/FilterNode.h Outdated Show resolved Hide resolved
src/storage/exec/GetNeighborsNode.h Show resolved Hide resolved
src/storage/exec/MultiTagNode.h Show resolved Hide resolved
src/storage/CommonUtils.h Outdated Show resolved Hide resolved
src/storage/query/GetNeighborsProcessor.cpp Show resolved Hide resolved
src/storage/test/GetNeighborsTest.cpp Outdated Show resolved Hide resolved
src/storage/test/GetNeighborsTest.cpp Outdated Show resolved Hide resolved
src/storage/exec/MultiTagNode.h Outdated Show resolved Hide resolved
wenhaocs
wenhaocs previously approved these changes Dec 24, 2021
Copy link
Contributor

@wenhaocs wenhaocs left a comment

Choose a reason for hiding this comment

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

Just some minor issues. Just want to make sure the logic is "if not edge specified, return only the vertex properties". Can you also give an update of the current behavior of GetNeighbors in the description of this PR and GetNeighborsProcessor.cpp ?

@critical27 critical27 added this to the v3.0.0 milestone Dec 28, 2021
@liuyu85cn liuyu85cn force-pushed the adjust-get-neighbor-plan branch from 80672f4 to 88b1ebc Compare December 29, 2021 03:38
critical27
critical27 previously approved these changes Dec 29, 2021
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.

Well done~

panda-sheep
panda-sheep previously approved these changes Dec 29, 2021
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.

Good!

@liuyu85cn liuyu85cn dismissed stale reviews from panda-sheep and critical27 via 2039e09 December 29, 2021 05:57
+----+---+
|TagNodes|
+--------+

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants