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 crash when the expression exceed the depth #3429

Closed

Conversation

heroicNeZha
Copy link
Contributor

@heroicNeZha heroicNeZha commented Dec 8, 2021

What type of PR is this?

  • bug
  • feature
  • enhancement

What does this PR do?

fix crash When the expression's depth exceeds the maximum limit
(The depth is related to the stack size. This pr limited 815 is calculated based on the stack size of the development machine)

Which issue(s)/PR(s) this PR relates to?

fixed #3393

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

Additional context:

Checklist:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatible (If it is incompatible, please describe it and add corresponding label.)
  • Need to cherry-pick (If need to cherry-pick to some branches, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to reflect in release notes and how to describe:

                                                            `

@heroicNeZha heroicNeZha added the ready-for-testing PR: ready for the CI test label Dec 8, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #3429 (f3067fc) into master (ab73b4c) will increase coverage by 0.05%.
The diff coverage is 99.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3429      +/-   ##
==========================================
+ Coverage   85.23%   85.28%   +0.05%     
==========================================
  Files        1277     1278       +1     
  Lines      119088   119303     +215     
==========================================
+ Hits       101502   101747     +245     
+ Misses      17586    17556      -30     
Impacted Files Coverage Δ
src/graph/optimizer/rule/CollapseProjectRule.cpp 98.24% <ø> (ø)
...timizer/rule/OptimizeEdgeIndexScanByFilterRule.cpp 90.54% <ø> (ø)
...ptimizer/rule/OptimizeTagIndexScanByFilterRule.cpp 90.27% <ø> (ø)
...h/optimizer/rule/PushLimitDownGetNeighborsRule.cpp 93.54% <ø> (ø)
...timizer/rule/PushStepLimitDownGetNeighborsRule.cpp 93.75% <ø> (ø)
...imizer/rule/PushStepSampleDownGetNeighborsRule.cpp 93.93% <ø> (ø)
src/graph/planner/plan/Query.h 97.61% <ø> (+0.16%) ⬆️
src/graph/util/FTIndexUtils.cpp 4.12% <ø> (ø)
src/graph/validator/FindPathValidator.cpp 84.78% <ø> (ø)
src/graph/validator/MutateValidator.cpp 94.11% <ø> (+0.05%) ⬆️
... and 63 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 d42203d...f3067fc. Read the comment docs.

@Shylock-Hg
Copy link
Contributor

And maybe not all expression will be checked by DeduceTypeVisitor.

@heroicNeZha
Copy link
Contributor Author

And maybe not all expression will be checked by DeduceTypeVisitor.

The DeduceTypeVisitor extends pure virtual class ExprVisitor which seems contains all expressions

And I didn't check non-recursive expressions in DeduceTypeVisitor

@Shylock-Hg
Copy link
Contributor

According to the Single Duty Principle, I think you should write a new visitor instead of do it in DeduceTypeVisitor.

@heroicNeZha heroicNeZha force-pushed the limit_depth_of_expression branch 2 times, most recently from 06bf960 to f429029 Compare December 13, 2021 06:41
@critical27
Copy link
Contributor

Generally LGTM

src/graph/visitor/CheckDepthVisitor.cpp Outdated Show resolved Hide resolved
src/graph/visitor/CheckDepthVisitor.h Outdated Show resolved Hide resolved
tests/tck/features/expression/Depth.feature Outdated Show resolved Hide resolved
src/graph/visitor/CheckDepthVisitor.h Outdated Show resolved Hide resolved
checkDepth();
SCOPE_EXIT { recoverDepth(); };
if (!ok()) return;
for (auto &arg : expr->args()->args()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider use any_of

void CheckDepthVisitor::visit(SubscriptExpression *expr) {
checkDepth();
SCOPE_EXIT { recoverDepth(); };
if (!ok()) return;
Copy link
Contributor

@jiayuehua jiayuehua Dec 27, 2021

Choose a reason for hiding this comment

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

consider refactor to two function that return bool and use: fa() && fb();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this pr is closed

@heroicNeZha heroicNeZha deleted the limit_depth_of_expression branch December 27, 2021 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit the depth of the expression, report an error when it exceeds but not crash
7 participants