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

Lookup limit push down #2796

Merged
merged 5 commits into from
Sep 15, 2021
Merged

Lookup limit push down #2796

merged 5 commits into from
Sep 15, 2021

Conversation

bright-starry-sky
Copy link
Contributor

No description provided.

@bright-starry-sky bright-starry-sky added the wip Solution: work in progress label Sep 6, 2021
@Shylock-Hg
Copy link
Contributor

And you should add a test case.

@Shylock-Hg Shylock-Hg mentioned this pull request Sep 8, 2021
Closed
@bright-starry-sky bright-starry-sky added ready-for-testing PR: ready for the CI test and removed wip Solution: work in progress labels Sep 9, 2021
@bright-starry-sky bright-starry-sky changed the title [DRAFT] lookup limit push down Lookup limit push down Sep 9, 2021
@bright-starry-sky
Copy link
Contributor Author

Addressed comments.

@bright-starry-sky
Copy link
Contributor Author

Related #2602

panda-sheep
panda-sheep previously approved these changes Sep 14, 2021
View reviewed changes
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 job！

src/clients/storage/GraphStorageClient.cpp Show resolved Hide resolved
src/storage/index/LookupBaseProcessor.h Show resolved Hide resolved
@bright-starry-sky
Copy link
Contributor Author

Corrected comments. thanks @panda-sheep

panda-sheep
panda-sheep previously approved these changes Sep 14, 2021
View reviewed changes
@bright-starry-sky
Copy link
Contributor Author

rebase and resolve conflict

critical27
critical27 approved these changes Sep 15, 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.

Generally LGTM, wait graph PR is ready.

@bright-starry-sky bright-starry-sky merged commit dbea9c7 into vesoft-inc:master Sep 15, 2021
@bright-starry-sky bright-starry-sky deleted the push_dowm_limit_sample branch September 15, 2021 01:37
@bright-starry-sky
Copy link
Contributor Author

related #2823

@cangfengzhs
Copy link
Contributor

The limit must be the top node of the execution plan, otherwise the size of the final dataset will be smaller than the limit count due to the filter (and possibly dedup) discarding part of the data.

@Shylock-Hg
Copy link
Contributor

The limit must be the top node of the execution plan, otherwise the size of the final dataset will be smaller than the limit count due to the filter (and possibly dedup) discarding part of the data.

Yes the limit push down can't over the filter operation.

@bright-starry-sky
Copy link
Contributor Author

The limit must be the top node of the execution plan, otherwise the size of the final dataset will be smaller than the limit count due to the filter (and possibly dedup) discarding part of the data.

Good point. Thanks for your remind.
The limit can't cover filter, ttl and dedup. I'll deal with it later.

@bright-starry-sky bright-starry-sky mentioned this pull request Sep 18, 2021
Merged
@jamieliu1023 jamieliu1023 mentioned this pull request Sep 18, 2021
Closed
@bright-starry-sky bright-starry-sky mentioned this pull request Sep 27, 2021
Closed
@Sophie-Xie Sophie-Xie added this to the v2.6.0 milestone Sep 27, 2021
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Sep 14, 2023
@Aiee
[LM] Set readFromCache_ to true if the host is not the leader (veso…
ecacd63 
…ft-inc#2796)

Fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shylock-Hg Shylock-Hg Shylock-Hg left review comments

@critical27 critical27 critical27 approved these changes

@panda-sheep panda-sheep panda-sheep approved these changes

Assignees
No one assigned
Labels
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.
6 participants
@bright-starry-sky @Shylock-Hg @cangfengzhs @critical27 @panda-sheep @Sophie-Xie