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.

Sign up for GitHub

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

Jump to bottom

Feature/index scan limit multiple rules #2966

Merged
merged 3 commits into from
Sep 29, 2021
Merged

Feature/index scan limit multiple rules #2966

merged 3 commits into from
Sep 29, 2021

Conversation

Shylock-Hg
Copy link
Contributor

Close #2950

@Shylock-Hg Shylock-Hg added ready-for-testing PR: ready for the CI test doc affected PR: improvements or additions to documentation labels Sep 28, 2021
@Shylock-Hg Shylock-Hg mentioned this pull request Sep 28, 2021
Closed
@Sophie-Xie Sophie-Xie added this to the v2.6.0 milestone Sep 28, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #2966 (22f0c44) into master (dddf54e) will decrease coverage by 1.11%.
The diff coverage is 51.53%.

❗ Current head 22f0c44 differs from pull request most recent head 5b1f1ca. Consider uploading reports for the commit 5b1f1ca to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2966      +/-   ##
==========================================
- Coverage   85.47%   84.35%   -1.12%     
==========================================
  Files        1229     1264      +35     
  Lines      110375   112266    +1891     
==========================================
+ Hits        94341    94707     +366     
- Misses      16034    17559    +1525
Impacted Files Coverage Δ
src/clients/meta/MetaClient.h 92.30% <ø> (ø)
src/clients/storage/GraphStorageClient.h 100.00% <ø> (ø)
src/clients/storage/InternalStorageClient.cpp 0.00% <0.00%> (ø)
src/clients/storage/InternalStorageClient.h 0.00% <ø> (-100.00%) ⬇️
src/clients/storage/StorageClientBase.h 59.32% <ø> (+6.94%) ⬆️
src/common/utils/NebulaKeyUtils.cpp 89.03% <0.00%> (-4.89%) ⬇️
src/common/utils/NebulaKeyUtils.h 80.68% <ø> (+2.65%) ⬆️
src/graph/executor/mutate/InsertExecutor.cpp 100.00% <ø> (ø)
src/graph/executor/query/IndexScanExecutor.cpp 88.88% <ø> (ø)
src/graph/planner/SequentialPlanner.cpp 100.00% <ø> (ø)
... and 262 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 c444b0f...5b1f1ca. Read the comment docs.

yixinglu
yixinglu reviewed Sep 28, 2021
View reviewed changes
tests/tck/features/lookup/LookUpLimit.feature
| id | name | dependencies | operator info |
| 4 | DataCollect | 5 | |
| 5 | Sort | 6 | |
| 6 | Project | 7 | |
Copy link
Contributor

Choose a reason for hiding this comment

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

The limit is missing in the graph layer

Copy link
Contributor

Choose a reason for hiding this comment

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

The limit is missing in the graph layer

I think ok, limit node should not appear in the graph layer for this query. WDYT?
lookup on tag where .. limit | limit should appear both storage and graph layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The limit clause of lookup is different with individual limit sentence. It means limit by partition.

@bright-starry-sky
Copy link
Contributor

Generally looks good for me. PTAL @yixinglu @CPWstatic

critical27
critical27 approved these changes Sep 29, 2021
View reviewed changes
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.

LGTM, quick one

@Shylock-Hg Shylock-Hg requested a review from yixinglu September 29, 2021 02:17
@bright-starry-sky bright-starry-sky merged commit b685bcf into vesoft-inc:master Sep 29, 2021
@jamieliu1023 jamieliu1023 mentioned this pull request Oct 2, 2021
Closed
@Shylock-Hg Shylock-Hg deleted the feature/index-scan-limit-multiple-rules branch February 9, 2022 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bright-starry-sky bright-starry-sky bright-starry-sky approved these changes

@critical27 critical27 critical27 approved these changes

@CPWstatic CPWstatic Awaiting requested review from CPWstatic

@yixinglu yixinglu Awaiting requested review from yixinglu

Assignees
No one assigned
Labels
doc affected PR: improvements or additions to documentation ready-for-testing PR: ready for the CI test
Projects
None yet
Milestone
v2.6.0
Development

Successfully merging this pull request may close these issues.

[Feature] index scan limit push down
6 participants
@Shylock-Hg @codecov-commenter @bright-starry-sky @yixinglu @critical27 @Sophie-Xie