Support named windows in OVER (window_definition) clause

[Nikita-str]

Issue #999

Parse window_name in OVER ([window_name] [PARTITION BY ..] [ORDER BY ..] ..)

[alamb]

Thank you for the contribution @Nikita-str -- this makes sense to me

[alamb]

👍

[Nikita-str]

@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]

@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

[Nikita-str]

@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]

@alamb do you suggest to change const DEFAULT_REMAINING_DEPTH: usize = 50; in src\parser\mod.rs to 40 for 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)

[Nikita-str]

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

[alamb]

You are not the first to hit this problem -- I think @universalmind303 saw it in #1144

Thus i think we need to do some real work -- I will try and find time later this week to help

[coveralls]

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

---

💛 - Coveralls

[alamb]

I merged up from main and now this test is passing 🎉

[alamb]

Thanks again @Nikita-str -- this is a very nice contribution