Feature/index scan limit push down #2823

Shylock-Hg · 2021-09-08T08:32:53Z

Require #2796

Sub job of #2602

…t-push-down

…ula into feature/node-polymorphsim

…t-push-down

…ature/index-scan-limit-push-down

bright-starry-sky · 2021-09-15T01:39:49Z

please resolve the conflicts.

…ature/index-scan-limit-push-down

Shylock-Hg · 2021-09-15T05:56:15Z

please resolve the conflicts.

Done.

src/graph/optimizer/rule/PushLimitDownIndexScanRule.cpp

bright-starry-sky · 2021-09-16T02:35:10Z

I didn't find the keyword LIMIT in lookup_sentence, did you solve it via other way?

Shylock-Hg · 2021-09-16T02:39:27Z

I didn't find the keyword LIMIT in lookup_sentence, did you solve it via other way?

In the compound sentence lookup .. | limit 3, 5

bright-starry-sky · 2021-09-16T03:06:56Z

I didn't find the keyword LIMIT in lookup_sentence, did you solve it via other way?

In the compound sentence lookup .. | limit 3, 5

I see. our ideas seem to diverge，let me explain what I think.I'm not sure what I think is right. It's just a discussion.
for example :
part1 ---> 10 rows;
part2 ---> 10 rows;

lookup tag ... limit 5 ---> The number of rows returned should be 5 + 5 == 10 (part1 = 5 rows, part2 = 5 rows)
lookup tag ... | limit 5 ---> The number of rows returned should be 5 (part1 = 10 rows, part2 = 10 rows, need to truncate it to 5 rows at the graph layer)
lookup tag ... limit 5 | limit 6 ---> The number of rows returned should be 5 + 5 == 10 (part1 = 5 rows, part2 = 5 rows. need to truncate it to 6 rows at the graph layer)

Shylock-Hg · 2021-09-16T08:18:12Z

I didn't find the keyword LIMIT in lookup_sentence, did you solve it via other way?

In the compound sentence lookup .. | limit 3, 5

I see. our ideas seem to diverge，let me explain what I think.I'm not sure what I think is right. It's just a discussion.
for example :
part1 ---> 10 rows;
part2 ---> 10 rows;

lookup tag ... limit 5 ---> The number of rows returned should be 5 + 5 == 10 (part1 = 5 rows, part2 = 5 rows)

lookup tag ... | limit 5 ---> The number of rows returned should be 5 (part1 = 10 rows, part2 = 10 rows, need to truncate it to 5 rows at the graph layer)

lookup tag ... limit 5 | limit 6 ---> The number of rows returned should be 5 + 5 == 10 (part1 = 5 rows, part2 = 5 rows. need to truncate it to 6 rows at the graph layer)

Fixed.

yixinglu · 2021-09-22T03:03:18Z

@CPWstatic @yixinglu, PTAL.

@critical27 @bright-starry-sky

I don't agree with the changes about the Plan Node kind, this new definition is quite ugly. I prefer to meet the needs by enhancing the flexibility of pattern, rather than by modifying the way plan node's kind is defined.

Shylock-Hg · 2021-09-22T03:06:53Z

@CPWstatic @yixinglu, PTAL.

@critical27 @bright-starry-sky

I don't agree with the changes about the Plan Node kind, this new definition is quite ugly. I prefer to meet the needs by enhancing the flexibility of pattern, rather than by modifying the way plan node's kind is defined.

How to enhance the pattern?

yixinglu · 2021-09-22T11:54:05Z

@CPWstatic @yixinglu, PTAL.

@critical27 @bright-starry-sky
I don't agree with the changes about the Plan Node kind, this new definition is quite ugly. I prefer to meet the needs by enhancing the flexibility of pattern, rather than by modifying the way plan node's kind is defined.

How to enhance the pattern?

Try one rule to match multiple patterns

yixinglu · 2021-09-22T11:58:39Z

src/graph/optimizer/rule/PushLimitDownIndexScanRule.cpp

+  static Pattern pattern = Pattern::create(
+      graph::PlanNode::Kind::kLimit,
+      {Pattern::create(graph::PlanNode::Kind::kProject,
+                       {Pattern::create(graph::PlanNode::Kind::kIndexScan, {}, true)})});


here should not be one opt rule, but should be split into two rules: limit <-> project, limit -> indexscan

Why need limit->indexscan?

Push limit down indexscan

The sentence only generate limit->project->indexscan plan

You should consider the optimization rules from the perspective of the optimizer, not just for this case.

We don't need to match the nonexistent pattern.

Shylock-Hg · 2021-09-23T09:47:00Z

@CPWstatic @yixinglu, PTAL.

@critical27 @bright-starry-sky
I don't agree with the changes about the Plan Node kind, this new definition is quite ugly. I prefer to meet the needs by enhancing the flexibility of pattern, rather than by modifying the way plan node's kind is defined.

How to enhance the pattern?

Try one rule to match multiple patterns

I think it's ok. We could refactor when get better way in future.

bright-starry-sky · 2021-09-27T02:57:48Z

What's new advances ? @Shylock-Hg @yixinglu .
If some differences require further discussion. would be better to split the current PR into two?

…ature/index-scan-limit-push-down

yixinglu · 2021-09-27T07:43:42Z

What's new advances ? @Shylock-Hg @yixinglu .
If some differences require further discussion. would be better to split the current PR into two?

I don’t accept refactoring in the future. If don’t want to optimize the pattern this time, then implement a few more similar optimization rules.

Shylock-Hg · 2021-09-27T08:00:53Z

What's new advances ? @Shylock-Hg @yixinglu .
If some differences require further discussion. would be better to split the current PR into two?

I don’t accept refactoring in the future. If don’t want to optimize the pattern this time, then implement a few more similar optimization rules.

In fact, I'm not sure about refactoring. Match variants at once is reasonable demand in this optimizer. Write multiple almost similar rules is not well.

codecov-commenter · 2021-09-27T10:35:13Z

Codecov Report

Merging #2823 (4b995d8) into master (dddf54e) will decrease coverage by 0.75%.
The diff coverage is 61.94%.

@@            Coverage Diff             @@
##           master    #2823      +/-   ##
==========================================
- Coverage   85.47%   84.71%   -0.76%     
==========================================
  Files        1229     1232       +3     
  Lines      110375   110924     +549     
==========================================
- Hits        94341    93974     -367     
- Misses      16034    16950     +916

Impacted Files	Coverage Δ
src/clients/meta/MetaClient.h	`92.30% <ø> (ø)`
src/clients/storage/GraphStorageClient.h	`100.00% <ø> (ø)`
src/common/utils/NebulaKeyUtils.cpp	`89.03% <0.00%> (-4.89%)`	⬇️
src/common/utils/NebulaKeyUtils.h	`78.02% <ø> (ø)`
src/graph/executor/query/IndexScanExecutor.cpp	`88.88% <ø> (ø)`
src/graph/optimizer/OptRule.h	`100.00% <ø> (ø)`
src/graph/planner/plan/Admin.cpp	`54.23% <0.00%> (-2.40%)`	⬇️
src/graph/planner/plan/Scan.h	`74.13% <0.00%> (-25.87%)`	⬇️
src/graph/service/PermissionCheck.cpp	`83.72% <ø> (+2.32%)`	⬆️
src/kvstore/KVEngine.h	`100.00% <ø> (ø)`
... and 137 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 073fb37...4b995d8. Read the comment docs.

Shylock-Hg · 2021-09-28T11:28:45Z

Reopen in #2966

Shylock-Hg added 10 commits September 7, 2021 10:32

Add the polymorphsim mechanism for plan node.

6763494

Merge branch 'master' into feature/node-polymorphsim

0cd521a

Fix the typo.

2ba7820

Merge branch 'feature/node-polymorphsim' into feature/index-scan-limi…

5f900d9

…t-push-down

Correct the singleton mt-safe.

3f78095

Merge branch 'feature/node-polymorphsim' of github.com:Shylock-Hg/neb…

12b5c3a

…ula into feature/node-polymorphsim

Merge branch 'feature/node-polymorphsim' into feature/index-scan-limi…

eec2f75

…t-push-down

Push limit to IndexScan.

30f68e5

Merge branch 'master' of https://github.com/vesoft-inc/nebula into fe…

91e807e

…ature/index-scan-limit-push-down

Push down to storage.

03b70a1

Shylock-Hg requested review from a team September 8, 2021 08:33

bright-starry-sky mentioned this pull request Sep 15, 2021

Lookup limit push down #2796

Merged

Shylock-Hg added 2 commits September 15, 2021 11:57

Merge branch 'master' into feature/index-scan-limit-push-down

017356a

Merge branch 'master' of https://github.com/vesoft-inc/nebula into fe…

2fb34dd

…ature/index-scan-limit-push-down

Merge branch 'master' into feature/index-scan-limit-push-down

900b88d

Shylock-Hg added the ready-for-testing PR: ready for the CI test label Sep 15, 2021

bright-starry-sky reviewed Sep 15, 2021

View reviewed changes

src/graph/optimizer/rule/PushLimitDownIndexScanRule.cpp Show resolved Hide resolved

Merge branch 'master' into feature/index-scan-limit-push-down

eb708d0

Merge branch 'master' into feature/index-scan-limit-push-down

e910d28

Sophie-Xie requested review from CPWstatic and removed request for a team September 16, 2021 05:06

Limit clause for lookup sentence.

a5534ea

Merge branch 'master' into feature/index-scan-limit-push-down

fa879b7

Shylock-Hg requested a review from czpmango September 17, 2021 03:28

critical27 previously approved these changes Sep 18, 2021

View reviewed changes

Sophie-Xie added this to the v2.6.0 milestone Sep 22, 2021

yixinglu reviewed Sep 22, 2021

View reviewed changes

Shylock-Hg requested a review from yixinglu September 23, 2021 02:17

Sophie-Xie removed the request for review from CPWstatic September 23, 2021 07:04

Shylock-Hg added the doc affected PR: improvements or additions to documentation label Sep 26, 2021

Add method to check base type.

94055ca

Shylock-Hg dismissed stale reviews from critical27 and bright-starry-sky via 94055ca September 27, 2021 03:30

Shylock-Hg added 3 commits September 27, 2021 13:48

Merge branch 'master' into feature/index-scan-limit-push-down

b15ad09

Merge branch 'master' of https://github.com/vesoft-inc/nebula into fe…

6a5fb69

…ature/index-scan-limit-push-down

Reserve space for query kind.

4b995d8

Shylock-Hg requested review from bright-starry-sky and critical27 September 27, 2021 07:01

bright-starry-sky mentioned this pull request Sep 27, 2021

[Feature] index scan limit push down #2950

Closed

Sophie-Xie added the need to discuss Solution: issue or PR without a clear conclusion on whether to handle it label Sep 27, 2021

Shylock-Hg linked an issue Sep 28, 2021 that may be closed by this pull request

[Feature] index scan limit push down #2950

Closed

Sophie-Xie removed the need to discuss Solution: issue or PR without a clear conclusion on whether to handle it label Sep 28, 2021

Shylock-Hg closed this Sep 28, 2021

Shylock-Hg deleted the feature/index-scan-limit-push-down branch February 9, 2022 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/index scan limit push down #2823

Feature/index scan limit push down #2823

Shylock-Hg commented Sep 8, 2021

bright-starry-sky commented Sep 15, 2021

Shylock-Hg commented Sep 15, 2021

bright-starry-sky commented Sep 16, 2021

Shylock-Hg commented Sep 16, 2021

bright-starry-sky commented Sep 16, 2021

Shylock-Hg commented Sep 16, 2021

yixinglu commented Sep 22, 2021

Shylock-Hg commented Sep 22, 2021

yixinglu commented Sep 22, 2021

yixinglu Sep 22, 2021

Shylock-Hg Sep 23, 2021

yixinglu Sep 23, 2021

Shylock-Hg Sep 24, 2021

yixinglu Sep 27, 2021

Shylock-Hg Sep 27, 2021

Shylock-Hg commented Sep 23, 2021 •

edited

Loading

bright-starry-sky commented Sep 27, 2021

yixinglu commented Sep 27, 2021 •

edited

Loading

Shylock-Hg commented Sep 27, 2021 •

edited

Loading

codecov-commenter commented Sep 27, 2021

Shylock-Hg commented Sep 28, 2021

Feature/index scan limit push down #2823

Feature/index scan limit push down #2823

Conversation

Shylock-Hg commented Sep 8, 2021

bright-starry-sky commented Sep 15, 2021

Shylock-Hg commented Sep 15, 2021

bright-starry-sky commented Sep 16, 2021

Shylock-Hg commented Sep 16, 2021

bright-starry-sky commented Sep 16, 2021

Shylock-Hg commented Sep 16, 2021

yixinglu commented Sep 22, 2021

Shylock-Hg commented Sep 22, 2021

yixinglu commented Sep 22, 2021

yixinglu Sep 22, 2021

Choose a reason for hiding this comment

Shylock-Hg Sep 23, 2021

Choose a reason for hiding this comment

yixinglu Sep 23, 2021

Choose a reason for hiding this comment

Shylock-Hg Sep 24, 2021

Choose a reason for hiding this comment

yixinglu Sep 27, 2021

Choose a reason for hiding this comment

Shylock-Hg Sep 27, 2021

Choose a reason for hiding this comment

Shylock-Hg commented Sep 23, 2021 • edited Loading

bright-starry-sky commented Sep 27, 2021

yixinglu commented Sep 27, 2021 • edited Loading

Shylock-Hg commented Sep 27, 2021 • edited Loading

codecov-commenter commented Sep 27, 2021

Codecov Report

Shylock-Hg commented Sep 28, 2021

Shylock-Hg commented Sep 23, 2021 •

edited

Loading

yixinglu commented Sep 27, 2021 •

edited

Loading

Shylock-Hg commented Sep 27, 2021 •

edited

Loading