Conversation
Pull Request Test Coverage Report for Build 3689510617
💛 - Coveralls |
alamb
left a comment
There was a problem hiding this comment.
https://github.com/sqlparser-rs/sqlparser-rs/pull/751/files?w=1
This PR Is very compelling to me as it does not require many intrusive changes. Thank you @step-baby
I will look into the proposed stacker dependency a bit prior to approving this PR, but it is looking good.
@AugustoFKL do you have any thoughts?
|
Stacker looks quite impressive: https://crates.io/crates/stacker |
AugustoFKL
left a comment
There was a problem hiding this comment.
@alamb really awesome. Never thought about that.
Althought, stackoverflow is dependant on the current architecture support, right? A machine with 2 GB of RAM would have this problem more usually than one with 32GB. Am I missing something?
Not sure if the depth being static is perfect, but seems ok for now IMHO.
src/ast/mod.rs
Outdated
| } else { | ||
| write!(f, "EXPLAIN ")?; | ||
| } | ||
| stacker::maybe_grow(2 * 1024 * 1024, 5 * 1024 * 1024, || { |
There was a problem hiding this comment.
I was worried about this indent change because it will conflict with every other PR.
So I pushed 853a890 as a way to make this diff smaller and shrink the merge conflict problem
|
There is some issue building with no-std which I will try and resolve later this week |
stacks are normally much smaller than available memory -- specifically I think the default is like 8MB or so on Linux. |
6f2a1b2 to
853a890
Compare
alamb
left a comment
There was a problem hiding this comment.
I think we need to check the stack size at each expr boundary or something -- I don't think this PR implements overflow checking in the way intended, sadly
| } | ||
|
|
||
| let statement = parser.parse_statement()?; | ||
| let statement = stacker::maybe_grow(2 * 1024 * 1024, 5 * 1024 * 1024, || { |
There was a problem hiding this comment.
So looking at this code more carefully, I am not sure exactly how it is working -- specifically this call to maybe_grow grows the stack if there is less then 2MB remaining
In fact the test passes even if I remove the maybe_grow around this call (it fails if i remove the check on the impl Display)
There was a problem hiding this comment.
diff --git a/src/parser.rs b/src/parser.rs
index 5fd7af2..cff2281 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -161,9 +161,7 @@ impl<'a> Parser<'a> {
return parser.expected("end of statement", parser.peek_token());
}
- let statement = stacker::maybe_grow(2 * 1024 * 1024, 5 * 1024 * 1024, || {
- parser.parse_statement()
- })?;
+ let statement = parser.parse_statement()?;
stmts.push(statement);
expecting_statement_delimiter = true;
}And then
cd /Users/alamb/Software/sqlparser-rs/ && RUST_BACKTRACE=1 CARGO_TARGET_DIR=/Users/alamb/Software/target-df cargo test -- overflow
Finished test [unoptimized + debuginfo] target(s) in 0.04s
Running unittests src/lib.rs (/Users/alamb/Software/target-df/debug/deps/sqlparser-d27a7d1b89bd35ed)
running 1 test
test parser::tests::test_stack_overflow ... ok🤔
|
Hi @alamb . Is there any better proposal? We urgently need this function. |
|
@baoyachi and @step-baby -- here is my alternate proposal: #764 |
|
merged alternate proposal #764 - thanks for sparking this discussion @step-baby |
Closes #305