KassaSana opened a new pull request, #25807:
URL: https://github.com/apache/datafusion/pull/25807
## Which issue does this PR close?
- Closes #24913.
## Rationale for this change
`datafusion.sql_parser.recursion_limit` has no effect in `datafusion-cli`.
After
`SET datafusion.sql_parser.recursion_limit = 200` (or
`DATAFUSION_SQL_PARSER_RECURSION_LIMIT=200`),
a query with 60 nested `abs(...)` calls still fails with
`RecursionLimitExceeded (current limit: 51)` via `-c`, `-f` and the
interactive shell,
while the same query works through `SessionContext::sql`. Lowering the limit
is ignored too.
The CLI parses statements itself instead of going through
`SessionState::sql_to_statement`,
and only reads the dialect from the session config. There are two parse
sites:
- `exec_and_print` (`-c`, `-f`, rc files, and REPL execution)
- `CliHelper::validate_input`, the REPL's rustyline validator, which rejects
the line
before it is executed
The earlier attempt in #24914 fixed only the first one, so the REPL stayed
broken.
## What changes are included in this PR?
- `exec_and_print` parses with `DFParserBuilder::with_recursion_limit` using
the session's
limit, the same way `SessionState::sql_to_statement` does.
- `CliHelper` gets a `recursion_limit` (default taken from
`SqlParserOptions`) and a new
`set_recursion_limit` method. `exec_from_repl` sets it when the REPL
starts and after each
statement, next to the existing `set_dialect` call. `CliHelper::new` is
unchanged, so
this is additive.
Not changed:
- `is_open_quote_for_location` in `helper.rs` (tab completion for `LOCATION
'...`) still
uses the default parser. It already ignores the dialect, and a parse
failure there only
means no filename completion.
- A single `-c` string or REPL line containing `SET ...; <query>` is parsed
as a whole
before the `SET` runs, so the new limit does not apply to the query in
that same string.
This matches how the dialect already behaves. Separate `-c` arguments,
file lines, or
REPL entries work.
Question for reviewers: I added a narrow `set_recursion_limit`. Would you
prefer a
`set_parser_options(&SqlParserOptions)` that also replaces `set_dialect`, so
future parser
options can't drift the same way?
Something I noticed and did not special-case: with a limit of 1 or 2, even a
`SET`
statement fails to parse, so in the REPL you have to restart to recover (3
and above is
fine). `SessionContext::sql` behaves the same way; the CLI just used to
ignore the setting.
## What is the testing strategy for this PR?
- New `cli_quick_test` case `recursion_limit` with
`tests/sql/recursion_limit.sql`: raises
the limit and runs a 60-deep query, then lowers it to 5 and runs a 10-deep
query. On
`main` the snapshot shows the reverse (the deep query fails at 51, the
shallow one
succeeds).
- New `helper.rs` unit test `sql_recursion_limit`, modelled on
`sql_dialect`, for the REPL
validator.
- Manually checked a debug build with `-c`, the env var, and the interactive
shell (via
`script` for a pty).
Commands run:
```bash
cargo test -p datafusion-cli --lib helper::tests
cargo test -p datafusion-cli --test cli_integration cli_quick_test
cargo test -p datafusion-cli
cargo fmt --all
cargo clippy -p datafusion-cli --all-targets --all-features -- -D warnings
```
## Are there any user-facing changes?
Yes: `datafusion.sql_parser.recursion_limit` is now respected by
`datafusion-cli`.
The only API change is the new public `CliHelper::set_recursion_limit`
method (additive).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
