Allow setting the recursion limit for sql parsing

[cetra3]

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.

[jatin510]

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]

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

[adriangb]

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.

[alamb]

Thanks @cetra3 -- this change makes sense to me. Thank you 🙏

To merge this PR I think we should:

1. try and improve the API (I left comments about this)
2. Add a test (so that if we break this in the future accidentally we'll know)

A good test would be a sqllogictest that runs a query that passes and then cranks the limit down to something low and then runs the same query again and show it errors

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.

@adriangb I think you are saying that this is a good change, right?

[alamb]

The API for DFParser is already somewhat tough.

Rather than adding a new method here, could you make this a builder style instead, so ths would look something like this?

Suggested change

	let mut statements = DFParser::parse_sql_with_dialect_limit(
	let mut statements = DFParser::new_with_dialect(sql, dialect.with_ref())
	.with_recursion_limit(recursion_limit)
	.parse_statements()

[cetra3]

I've adjusted the PR to include a new DFParserBuilder that accomplishes this, but I've tried to keep the original methods as close as possible for backwards compat

[alamb]

I verified this is the same default:
https://github.com/apache/datafusion-sqlparser-rs/blob/648efd7057d63c65b53eddc3d05cc89d5697d85c/src/parser/mod.rs#L187

[adriangb]

@adriangb I think you are saying that this is a good change, right?

Yes I was just explaining why this is necessary - it's not immediately obvious how parsing SQL can be so recursive.

[cetra3]

@alamb I've added a test and a builder struct, let me know if you want further changes

[alamb]

Thank you @cetra3

In order to try and minimize downstream API breakages following https://datafusion.apache.org/library-user-guide/api-health.html I think we should

1. put back DFParser::new() and DFParser::new_with_dialect marked at deprecated
2. Adding some doc strings and examples to DFParserBuilder

Here is a proposed PR with those changes to your fork:

• pydantic#8

Clicked wrong button

[alamb]

Thanks again @cetra3