-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Update parser recursion limit from 50 to 100 #15622
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm thanks @Omega359
@@ -185,3 +185,17 @@ select ammp from a; | |||
|
|||
statement ok | |||
drop table a; | |||
|
|||
# Error number of nested expressions exceeds limit | |||
statement error DataFusion error: SQL error: RecursionLimitExceeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the error looks like
sql parser error: recursion limit exceeded
It is is good to provide a current recursion limit value as long as it is can be configured through datafusion.sql_parser.recursion_limit
parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need 2 tests to guard the current default value, for both increments(error expecting test wouldn't give an error) and decrements(success expecting test would fail)
This PR is incomplete, I discovered that I didn't update the config to reflect the change. Once that change was made the sqllogictests started with a stack overflow. So the change that @alamb thought would help with that doesn't seem to have had the desired results. |
For some reason I can't switch this to draft, can a committer do that for me please? |
Which issue does this PR close?
Rationale for this change
A number of valid tests are failing because the recursion limit is fairly low. See #14091
What changes are included in this PR?
Updated the default recursion limit from 50 to 100.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.