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 two crash issues caused by the optimizer's filter push-down rules #4864

Merged
merged 1 commit into from
Nov 22, 2022

Conversation

xtcyclist
Copy link
Contributor

@xtcyclist xtcyclist commented Nov 11, 2022

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Close https://github.com/vesoft-inc/nebula-ent/issues/1580
Close https://github.com/vesoft-inc/nebula-ent/issues/1683

Description:

  • colNames[i] runs out-of-range in PrunPropertiesVisitor.cpp, caused by errors in PushFilterDownAggregateRule & PushFilterDownFilterRule.
  • colNames becomes larger than groupItems in PushFilterDownAggregateRule, causing groupItems[i] to run out-of-range.

How do you solve it?

  • Check range sizes in for loop in PrunPropertiesVisitor.cpp.
  • Found the reason why the sizes differ in PushFilterDownAggregateRule and PushFilterDownProjectRule. Try fixing the exchange of operators.

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

Design-wise, these two structures that shall have the same size belong to different classes: one in the base class, the other in a derived class. They are manipulated independently in many places, directly or indirecly, causing their sizes to differ. If these structures have to be declared in different classes, maybe we could try grouping modifications on them together in the same method. This may prevent such issues at the first place.

Checklist:

Tests:

  • Unit test(positive and negative cases)
  • Function test
  • Performance test
  • TCK

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 .....

@xtcyclist xtcyclist added ready-for-testing PR: ready for the CI test cherry-pick-v3.3 PR: need cherry-pick to this version labels Nov 11, 2022
@xtcyclist xtcyclist requested review from jievince, nevermore3, codesigner and czpmango and removed request for jievince November 11, 2022 10:15
@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2022

Codecov Report

Base: 76.78% // Head: 76.85% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (0649363) compared to base (6f240e3).
Patch coverage: 74.85% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4864      +/-   ##
==========================================
+ Coverage   76.78%   76.85%   +0.06%     
==========================================
  Files        1102     1102              
  Lines       80953    81067     +114     
==========================================
+ Hits        62160    62302     +142     
+ Misses      18793    18765      -28     
Impacted Files Coverage Δ
src/graph/context/Symbols.cpp 91.78% <ø> (ø)
src/graph/executor/admin/SubmitJobExecutor.cpp 56.94% <ø> (ø)
.../graph/executor/algo/MultiShortestPathExecutor.cpp 98.55% <ø> (ø)
...timizer/rule/OptimizeEdgeIndexScanByFilterRule.cpp 87.67% <ø> (-6.93%) ⬇️
src/graph/planner/Planner.h 100.00% <ø> (ø)
src/graph/planner/match/MatchPathPlanner.cpp 98.52% <ø> (ø)
src/graph/planner/plan/PlanNode.h 89.42% <ø> (-0.40%) ⬇️
src/graph/validator/ACLValidator.h 89.65% <ø> (ø)
src/graph/validator/MatchValidator.cpp 89.39% <ø> (+1.32%) ⬆️
src/meta/processors/admin/AdminClient.cpp 63.99% <ø> (ø)
... and 63 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@xtcyclist xtcyclist force-pushed the fix_ent#1580 branch 2 times, most recently from 15a6ec4 to cceb040 Compare November 15, 2022 05:40
@xtcyclist xtcyclist added ready for review and removed cherry-pick-v3.3 PR: need cherry-pick to this version labels Nov 15, 2022
src/graph/planner/plan/PlanNode.cpp Outdated Show resolved Hide resolved
src/graph/visitor/PropertyTrackerVisitor.cpp Outdated Show resolved Hide resolved
src/graph/visitor/PrunePropertiesVisitor.cpp Show resolved Hide resolved
@xtcyclist xtcyclist changed the title Fix crash due to unequal sizes of columns and columns' names in project. Fix crash and other issues caused by the optimizer's filter push-down rules Nov 17, 2022
@xtcyclist xtcyclist force-pushed the fix_ent#1580 branch 2 times, most recently from 0f1c5f9 to a598f24 Compare November 17, 2022 03:41
@xtcyclist xtcyclist force-pushed the fix_ent#1580 branch 3 times, most recently from 400a7eb to 3f35ce4 Compare November 18, 2022 13:23
@xtcyclist xtcyclist force-pushed the fix_ent#1580 branch 2 times, most recently from 0649363 to 89618a5 Compare November 21, 2022 15:33
@xtcyclist
Copy link
Contributor Author

xtcyclist commented Nov 21, 2022

I added the fix (PushFilterDownAggregateRule.cpp:97) to another issue (https://github.com/vesoft-inc/nebula-ent/issues/1683) in this PR, since they are concerning the same code segment in PushFilterDownAggregateRule.cpp.

@xtcyclist xtcyclist changed the title Fix crash and other issues caused by the optimizer's filter push-down rules Fix two crash issues caused by the optimizer's filter push-down rules Nov 21, 2022
Copy link
Contributor

@yixinglu yixinglu left a comment

Choose a reason for hiding this comment

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

Excellent!!

@Sophie-Xie Sophie-Xie merged commit f2cee66 into vesoft-inc:master Nov 22, 2022
@xtcyclist xtcyclist deleted the fix_ent#1580 branch June 2, 2023 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants