Skip to content

ISSUE-3572 try fix stack overflow#3603

Merged
databend-bot merged 7 commits into
databendlabs:mainfrom
zhang2014:fix/ISSUE-3572
Dec 29, 2021
Merged

ISSUE-3572 try fix stack overflow#3603
databend-bot merged 7 commits into
databendlabs:mainfrom
zhang2014:fix/ISSUE-3572

Conversation

@zhang2014

@zhang2014 zhang2014 commented Dec 22, 2021

Copy link
Copy Markdown
Member

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

fix stack overflow.

Changelog

  • Bug Fix

Related Issues

#3572 Partial fixes(SQL parser may still stack overflow)

Test Plan

Unit Tests

Stateless Tests

@databend-bot

Copy link
Copy Markdown
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@databend-bot databend-bot added pr-bugfix this PR patches a bug in codebase need-review labels Dec 22, 2021
@vercel

vercel Bot commented Dec 22, 2021

Copy link
Copy Markdown

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/databend/databend/8jMkZZ7y1JCYj8k5F5d3kW3JK5UL
✅ Preview: https://databend-git-fork-zhang2014-fix-issue-3572-databend.vercel.app

[Deployment for 4447d08 canceled]

Comment thread common/planners/src/plan_expression_chain.rs
@sundy-li

Copy link
Copy Markdown
Member

Need a test case to cover the issue.

@xudong963 xudong963 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have tried to use maybe_grow() to solve stack overflow in another database, but it can't completely solve the problem, only to prevent panic or limit users' behavior. I think your idea can fully solve stack overflow, nice work!

visitor = arg.accept(visitor)?;
fn visit(mut self, predecessor_expr: &Expression) -> Result<Self> {
let mut stack = vec![RecursionProcessing::Call(predecessor_expr)];
while let Some(element) = stack.pop() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I read correctly, you try to avoid stack overflow by manually replacing the stack with the heap. You are essentially simulating a stack behavior. Maybe we can fully avoid recursing by using iterating? And VecDeque may be a good structure.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe we can fully avoid recursing by using iterating? And VecDeque may be a good structure.

We want to support both preorder traversal(pre_visit) and postorder traversal(post_visit), which makes me think of the stack first, maybe you have a better idea, please let me know. : D

@mergify

mergify Bot commented Dec 23, 2021

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be merged. @zhang2014 please rebase it 🙏

@zhyass

zhyass commented Dec 24, 2021

Copy link
Copy Markdown
Member

Need resolve the conflicts.

@zhang2014

zhang2014 commented Dec 24, 2021

Copy link
Copy Markdown
Member Author

Need resolve the conflicts.

Let's convert PR to draft first and I will continue to this work until it's ready.
BTW: resolve conflicts is hard(because the FunctionFactory::get has some different).

@zhang2014
zhang2014 marked this pull request as draft December 24, 2021 12:40
# Conflicts:
#	common/planners/src/plan_expression.rs
#	common/planners/src/plan_expression_chain.rs
@zhang2014
zhang2014 marked this pull request as ready for review December 29, 2021 08:53
@zhang2014
zhang2014 requested a review from bohutang as a code owner December 29, 2021 08:53
@zhang2014

Copy link
Copy Markdown
Member Author

Need a test case to cover the issue.
Need resolve the conflicts.

@zhyass @sundy-li Done.

@zhang2014
zhang2014 requested a review from sundy-li December 29, 2021 08:56
@codecov-commenter

codecov-commenter commented Dec 29, 2021

Copy link
Copy Markdown

Codecov Report

Merging #3603 (4447d08) into main (b712e6d) will increase coverage by 0%.
The diff coverage is 70%.

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #3603    +/-   ##
======================================
  Coverage     60%     60%            
======================================
  Files        697     705     +8     
  Lines      37230   37787   +557     
======================================
+ Hits       22367   22709   +342     
- Misses     14863   15078   +215     
Impacted Files Coverage Δ
common/planners/src/plan_rewriter.rs 39% <50%> (-4%) ⬇️
common/planners/src/plan_expression_rewriter.rs 63% <60%> (ø)
common/planners/src/plan_expression_common.rs 63% <78%> (+7%) ⬆️
query/src/optimizers/optimizer_constant_folding.rs 85% <81%> (-2%) ⬇️
common/planners/src/plan_expression.rs 81% <100%> (-2%) ⬇️
common/planners/src/plan_expression_chain.rs 68% <100%> (+3%) ⬆️
common/planners/src/plan_expression_visitor.rs 100% <100%> (+14%) ⬆️
.../functions/src/aggregates/aggregate_arg_min_max.rs 58% <0%> (-12%) ⬇️
common/functions/src/aggregates/aggregate_sum.rs 85% <0%> (-4%) ⬇️
...ns/src/aggregates/aggregate_combinator_distinct.rs 68% <0%> (-4%) ⬇️
... and 34 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 0ef869a...4447d08. Read the comment docs.

@databend-bot

Copy link
Copy Markdown
Member

Wait for another reviewer approval

@databend-bot

Copy link
Copy Markdown
Member

CI Passed
Reviewers Approved
Let's Merge
Thank you for the PR @zhang2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix this PR patches a bug in codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants