Skip to content
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

Allow setting the recursion limit for sql parsing #14756

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cetra3
Copy link
Contributor

@cetra3 cetra3 commented Feb 19, 2025

Which issue does this PR close?

No issue, just running into this in production.

Rationale for this change

At the moment there isn't a clean way to set the recursion limit on the sql parser.

What changes are included in this PR?

This PR allows a config option, datafusion.sql_parser.recursion_limit which allows overriding the recursion limit.

Are these changes tested?

No, it's just a config option.

Are there any user-facing changes?

There will be some changes to public facing API. I'm also not sure if this is the best approach here, as DFParser might need a deeper refactor to keep things clean.

@github-actions github-actions bot added sql SQL Planner core Core DataFusion crate common Related to common crate labels Feb 19, 2025
@jatin510
Copy link
Contributor

I see that a recursion limit of 50 is introduced for SQL parsing.
Is this based on specific performance benchmarks or potential stack overflow risks?
Also, is there any reference to similar constraints in databases like PostgreSQL, or is this a DataFusion-specific safeguard?

@cetra3

@cetra3
Copy link
Contributor Author

cetra3 commented Feb 19, 2025

@jatin510 This number is actually the default in sqlparser: https://github.com/apache/datafusion-sqlparser-rs/blob/b482562618caa3efa89c2f42f87472b00a270926/src/parser/mod.rs#L187

So it will essentially be the exact same behaviour, but configurable.

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Feb 20, 2025
@cetra3 cetra3 force-pushed the sql_recursion_limit_config branch 2 times, most recently from 9ef87d1 to fd42361 Compare February 21, 2025 04:05
@cetra3 cetra3 force-pushed the sql_recursion_limit_config branch from fd42361 to b29e5e2 Compare February 21, 2025 04:26
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Feb 21, 2025
@adriangb
Copy link
Contributor

I'll note that we encountered this when parsing a dynamically generated expression containing a lot of ANDs and ORs. I believe these sorts of expressions are parsed recursively, so it's pretty easy to hit 50 levels of recursion if each AND is a level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate core Core DataFusion crate documentation Improvements or additions to documentation sql SQL Planner sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants