sqlparser-rs
Named windows in `OVER (window_definition)` clause
Issue #999
Parse window_name in OVER ([window_name] [PARTITION BY ..] [ORDER BY ..] ..)
@alamb first of all, should I do something with parse_deeply_nested_parens_hits_recursion_limits test? Because locally it's failed with stack overflow even in main branch. All others test result is ok.
@alamb first of all, should I do something with parse_deeply_nested_parens_hits_recursion_limits test? Because locally it's failed with stack overflow even in main branch. All others test result is ok.
I think we may need to update the default limit to have a lower limit (it is right on the edge of stack limit in the debug build, and so any change to the stack size leads to a failure)
Maybe we can do it in a separate PR -- any chance you are willing to do so? If not I will try to do so but it may be several days
@alamb do you suggest to change const DEFAULT_REMAINING_DEPTH: usize = 50; in src\parser\mod.rs to 40 for example?
Maybe it's better to wrap this test code into new thread with bigger stack size and then wait until the thread finished?:
const KB: usize = 1024;
const MB: usize = 1024 * KB;
std::thread::Builder::new().stack_size(8 * MB).spawn(move || {
// prev code
}).unwrap().join().unwrap();
@alamb do you suggest to change
const DEFAULT_REMAINING_DEPTH: usize = 50;insrc\parser\mod.rsto40for example?
Yes, this is basically my suggestion
The rationale is that doing so will prevent stack overflows / panics for debug builds.
Another thing we can do is to see if we can reduce the stack frame size for some of the intermediate function calls (reduce stack frame means make fewer local variables)
Maybe we can do it in a separate PR -- any chance you are willing to do so?
Ok, I will do it, ~~at least by wrapping in new thread.
If I solve it by wrapping then I would to wrap all tests that assert ParserError::RecursionLimitExceeded to prevent future the same errors with stack overflow at least in already existed tests, ok?~~
Idea with wrapping is bad because it only handles the tests situation and when RecursionLimitExceeded situation will happen during development with using the crate, the user will get stack overflow
You are not the first to hit this problem -- I think @universalmind303 saw it in https://github.com/sqlparser-rs/sqlparser-rs/pull/1144
Thus i think we need to do some real work -- I will try and find time later this week to help
Pull Request Test Coverage Report for Build 8588647268
Details
- 29 of 29 (100.0%) changed or added relevant lines in 4 files are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage increased (+0.02%) to 87.921%
| Totals | |
|---|---|
| Change from base Build 8588627223: | 0.02% |
| Covered Lines: | 20657 |
| Relevant Lines: | 23495 |
