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

Fix get edges transform rule. #4328

Merged
merged 5 commits into from
Jun 21, 2022

Conversation

Shylock-Hg
Copy link
Contributor

@Shylock-Hg Shylock-Hg commented Jun 15, 2022

  1. Input/Ouput variables.
  2. Keep column names of Limit same with input plan node.

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

How do you solve it?

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

Checklist:

Tests:

  • Unit test(positive and negative cases)
  • Function test
  • Performance test
  • N/A

Affects:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatibility (If it breaks the compatibility, please describe it and add the label.)
  • If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

1. Input/Ouput variables.
2. Keep column names of Limit same with input plan node.
@Shylock-Hg Shylock-Hg added the ready-for-testing PR: ready for the CI test label Jun 15, 2022
@Sophie-Xie Sophie-Xie added this to the v3.2.0 milestone Jun 15, 2022
@czpmango
Copy link
Contributor

czpmango commented Jun 16, 2022

Nice fix. But since it is a bug fix, is there any corresponding issue to track? For the current pr, it is also necessary to add test cases to constrain subsequent development.

@wey-gu
Copy link
Contributor

wey-gu commented Jun 16, 2022

Nice fix. But since it is a bug fix, is there a corresponding issue to track? For the current pr, it is also necessary to add test cases to constrain subsequent development.

Thanks, I would like to learn how some of the fixes (addressing my previous faults I think) cover wrong behaviors, too!

Silly questions from my side

  • i.e. I don't understand when should we care about input var, output var, col, and when we don't have to, I need to look into its corresponding nodes' executor code or?
  • and, I don't understand why for instance PushTopN index scan and Push vFilter ScanV are doing erase policy differently(one for current and other for all)

@Shylock-Hg
Copy link
Contributor Author

Nice fix. But since it is a bug fix, is there any corresponding issue to track? For the current pr, it is also necessary to add test cases to constrain subsequent development.

In fact, the CI job in Debug building will report error, but now it always compile to OOM

@Shylock-Hg Shylock-Hg added the cherry-pick-v3.2 PR: need cherry-pick to this version label Jun 17, 2022
@Shylock-Hg
Copy link
Contributor Author

Nice fix. But since it is a bug fix, is there a corresponding issue to track? For the current pr, it is also necessary to add test cases to constrain subsequent development.

Thanks, I would like to learn how some of the fixes (addressing my previous faults I think) cover wrong behaviors, too!

Silly questions from my side

  • i.e. I don't understand when should we care about input var, output var, col, and when we don't have to, I need to look into its corresponding nodes' executor code or?
  • and, I don't understand why for instance PushTopN index scan and Push vFilter ScanV are doing erase policy differently(one for current and other for all)
  1. You can check the constraints I mentioned in Refactor the process of symbols in optimizer. #4146 (comment)
  2. Now there are no differences, two methods to erase will lead to that subplan produced by rule will replace origin.

@codecov-commenter
Copy link

Codecov Report

Merging #4328 (ce28e81) into master (fcbab77) will decrease coverage by 0.00%.
The diff coverage is 77.02%.

@@            Coverage Diff             @@
##           master    #4328      +/-   ##
==========================================
- Coverage   84.88%   84.87%   -0.01%     
==========================================
  Files        1343     1343              
  Lines      133363   133509     +146     
==========================================
+ Hits       113199   113317     +118     
- Misses      20164    20192      +28     
Impacted Files Coverage Δ
src/common/base/ObjectPool.h 100.00% <ø> (ø)
src/common/context/ExpressionContext.h 100.00% <ø> (ø)
src/common/expression/PropertyExpression.h 100.00% <ø> (ø)
src/common/utils/DefaultValueContext.h 0.00% <0.00%> (ø)
src/graph/context/Iterator.h 69.94% <0.00%> (-0.82%) ⬇️
src/graph/context/QueryExpressionContext.h 100.00% <ø> (ø)
...rc/graph/executor/algo/ProduceAllPathsExecutor.cpp 97.67% <ø> (ø)
src/graph/executor/query/InnerJoinExecutor.h 100.00% <ø> (ø)
src/graph/executor/query/JoinExecutor.h 100.00% <ø> (ø)
src/graph/executor/query/LeftJoinExecutor.h 100.00% <ø> (ø)
... and 88 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 0ad5c38...ce28e81. Read the comment docs.

@Sophie-Xie Sophie-Xie merged commit 7be5638 into vesoft-inc:master Jun 21, 2022
@Shylock-Hg Shylock-Hg deleted the fix/rule-get-edges-transform branch June 21, 2022 03:13
nebula-bots pushed a commit that referenced this pull request Jun 23, 2022
1. Input/Ouput variables.
2. Keep column names of Limit same with input plan node.

Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>
Sophie-Xie added a commit that referenced this pull request Jun 27, 2022
1. Input/Ouput variables.
2. Keep column names of Limit same with input plan node.

Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>
Sophie-Xie added a commit that referenced this pull request Jun 27, 2022
* force cache the docker layer (#4331)

* check god role exist when meta init (#4330)

* check god role exist when meta init

* return error if kv fail

Co-authored-by: Doodle <13706157+critical27@users.noreply.github.com>

* Fix object pool mtsafe. (#4332)

* Fix object pool mtsafe.

* Fix lock.

* Fixed web service crash (#4334)

Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>

* Fix get edges transform rule. (#4328)

1. Input/Ouput variables.
2. Keep column names of Limit same with input plan node.

Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>

* fix rc docker (#4336)

* add lock (#4352)

* fix map concurrency issue (#4344)

* fix mutex in map

* add test

* move the order

Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>

* add stats under index conditions (#4353)

Co-authored-by: Harris.Chu <1726587+HarrisChu@users.noreply.github.com>
Co-authored-by: jimingquan <mingquan.ji@vesoft.com>
Co-authored-by: Doodle <13706157+critical27@users.noreply.github.com>
Co-authored-by: shylock <33566796+Shylock-Hg@users.noreply.github.com>
Co-authored-by: dutor <440396+dutor@users.noreply.github.com>
Co-authored-by: panda-sheep <59197347+panda-sheep@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-v3.2 PR: need cherry-pick to this version ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants