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

Fix the lookup limit result. #3171

Merged
merged 4 commits into from
Oct 21, 2021
Merged

Fix the lookup limit result. #3171

merged 4 commits into from
Oct 21, 2021

Conversation

Shylock-Hg
Copy link
Contributor

@Shylock-Hg Shylock-Hg added ready-for-testing PR: ready for the CI test type/bug Type: something is unexpected labels Oct 21, 2021
@Shylock-Hg Shylock-Hg added the doc affected PR: improvements or additions to documentation label Oct 21, 2021
@Sophie-Xie Sophie-Xie added the cherry-pick-v2.6 PR: need cherry-pick to this version label Oct 21, 2021
bright-starry-sky
bright-starry-sky previously approved these changes Oct 21, 2021
View reviewed changes
Copy link
Contributor

@bright-starry-sky bright-starry-sky 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.

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2021 •
edited
Loading

Codecov Report

Merging #3171 (707f1cd) into master (50b1760) will increase coverage by 0.11%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master    #3171      +/-   ##
==========================================
+ Coverage   85.17%   85.29%   +0.11%     
==========================================
  Files        1294     1294              
  Lines      118261   118250      -11     
==========================================
+ Hits       100733   100860     +127     
+ Misses      17528    17390     -138
Impacted Files Coverage Δ
src/graph/validator/BalanceValidator.cpp 0.00% <ø> (ø)
src/graph/validator/IngestValidator.cpp 0.00% <ø> (ø)
src/graph/planner/ngql/LookupPlanner.cpp 100.00% <100.00%> (ø)
src/parser/GraphScanner.h 91.46% <0.00%> (-2.29%) ⬇️
src/common/expression/LogicalExpression.cpp 91.66% <0.00%> (-1.73%) ⬇️
src/common/fs/FileUtils.cpp 74.66% <0.00%> (-1.36%) ⬇️
src/kvstore/DiskManager.cpp 79.01% <0.00%> (-1.24%) ⬇️
src/kvstore/wal/FileBasedWal.cpp 72.03% <0.00%> (-0.48%) ⬇️
src/storage/test/StatsTaskTest.cpp 95.51% <0.00%> (-0.45%) ⬇️
src/clients/meta/MetaClient.cpp 75.15% <0.00%> (+0.07%) ⬆️
... and 40 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 50b1760...f59f62f. Read the comment docs.

czpmango
czpmango reviewed Oct 21, 2021
View reviewed changes
Copy link
Contributor

@czpmango czpmango left a comment

Choose a reason for hiding this comment

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

Are LOOKUP ON like WHERE like.likeness == 90 yield like.likeness limit 3 and LOOKUP ON like WHERE like.likeness == 90 yield like.likeness | limit 3 the same?

@Shylock-Hg
Copy link
Contributor Author

Are LOOKUP ON like WHERE like.likeness == 90 yield like.likeness limit 3 and LOOKUP ON like WHERE like.likeness == 90 yield like.likeness | limit 3 the same?

Yes.

@Shylock-Hg Shylock-Hg requested a review from czpmango October 21, 2021 06:11
@czpmango
Copy link
Contributor

Are LOOKUP ON like WHERE like.likeness == 90 yield like.likeness limit 3 and LOOKUP ON like WHERE like.likeness == 90 yield like.likeness | limit 3 the same?

Yes.

Since they are the same, why do we need to design such syntax?
If so, does other ngql also need to support similar syntax, such as go from "Tim Duncan" over like limit 3?

czpmango
czpmango approved these changes Oct 21, 2021
View reviewed changes
Copy link
Contributor

@czpmango czpmango left a comment

Choose a reason for hiding this comment

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

LGTM

@yixinglu yixinglu merged commit 9f8802d into vesoft-inc:master Oct 21, 2021
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Oct 21, 2021
@Shylock-Hg @yixinglu
Fix the lookup limit result (vesoft-inc#3171)
7788ebc 
* Fix the lookup limit result.

* Stablize the limit cast in multiple storage nodes.

* Remove limit clause of lookup.
critical27 pushed a commit that referenced this pull request Oct 22, 2021
@yixinglu @czpmango @Shylock-Hg
Cherry pick V2.6.0 1021 (#3177)
1a82164 
* Fix the interval time of memory checking in graph background thread (#3175)

* Move ScopedTimer from graph to common module

* fix PushLimitDownProjRule (#3167)

* Fix the lookup limit result (#3171)

* Fix the lookup limit result.

* Stablize the limit cast in multiple storage nodes.

* Remove limit clause of lookup.

Co-authored-by: kyle.cao <[email protected]>
Co-authored-by: Shylock Hg <[email protected]>
@Shylock-Hg Shylock-Hg mentioned this pull request Oct 22, 2021
Merged
@jamieliu1023 jamieliu1023 mentioned this pull request Nov 1, 2021
Closed
@Shylock-Hg Shylock-Hg deleted the correct/fix-lookup-limit branch February 9, 2022 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@czpmango czpmango czpmango approved these changes

@yixinglu yixinglu yixinglu approved these changes

@CPWstatic CPWstatic Awaiting requested review from CPWstatic

@critical27 critical27 Awaiting requested review from critical27

@randomJoe211 randomJoe211 Awaiting requested review from randomJoe211

@bright-starry-sky bright-starry-sky Awaiting requested review from bright-starry-sky

Assignees
No one assigned
Labels
cherry-pick-v2.6 PR: need cherry-pick to this version doc affected PR: improvements or additions to documentation ready-for-testing PR: ready for the CI test type/bug Type: something is unexpected
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent LIMIT operations
7 participants
@Shylock-Hg @codecov-commenter @czpmango @yixinglu @bright-starry-sky @randomJoe211 @Sophie-Xie