You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "KassaSana (via GitHub)" <gi...@apache.org> on 2026/09/27 16:20:47 UTC
[PR] fix(cli): respect `sql_parser.recursion_limit` when parsing and validating input [datafusion]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(cli): respect `sql_parser.recursion_limit` when parsing and validating input [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #25807:
URL: https://github.com/apache/datafusion/pull/25807#discussion_r4373905527
##########
datafusion-cli/tests/sql/recursion_limit.sql:
##########
Review Comment:
Could we add these statements to the existing `tests/sql/select.sql` fixture and update `cli_quick_test@files.snap`? That would cover the same behavior through `exec_from_files` without needing a separate SQL file, test case, and snapshot.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(cli): respect `sql_parser.recursion_limit` when parsing and validating input [datafusion]
Posted by "KassaSana (via GitHub)" <gi...@apache.org>.
KassaSana commented on PR #25807:
URL: https://github.com/apache/datafusion/pull/25807#issuecomment-6406112476
Hi @kumarUjjawal, I merged the latest `main` into this branch. There were no conflicts, and the diff is unchanged (the `\i` fix from 91f61d5 is still in place). Could you (or another committer) approve the CI run when you get a chance, and take another look if the `\i` change looks good to you? Thanks!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(cli): respect `sql_parser.recursion_limit` when parsing and validating input [datafusion]
Posted by "KassaSana (via GitHub)" <gi...@apache.org>.
KassaSana commented on code in PR #25807:
URL: https://github.com/apache/datafusion/pull/25807#discussion_r4374535720
##########
datafusion-cli/tests/sql/recursion_limit.sql:
##########
Review Comment:
Done in 4648c72. I moved the statements into `tests/sql/select.sql` after the existing `select 1;` and updated `cli_quick_test@files.snap`. The separate fixture, test case, and snapshot are removed.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(cli): respect `sql_parser.recursion_limit` when parsing and validating input [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25807:
URL: https://github.com/apache/datafusion/pull/25807#issuecomment-5965002100
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25807?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 82.51%. Comparing base ([`f029b94`](https://app.codecov.io/gh/apache/datafusion/commit/f029b944c8d2f8e70f0c213f87160c12ddad6a09?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`86d94ee`](https://app.codecov.io/gh/apache/datafusion/commit/86d94eef627b1118088b0029c5dbc2eee136d9c0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 83 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25807 +/- ##
==========================================
- Coverage 82.51% 82.51% -0.01%
==========================================
Files 1141 1141
Lines 439780 439780
Branches 439780 439780
==========================================
- Hits 362888 362880 -8
- Misses 54950 54956 +6
- Partials 21942 21944 +2
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25807?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(cli): respect `sql_parser.recursion_limit` when parsing and validating input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25807:
URL: https://github.com/apache/datafusion/pull/25807#discussion_r4393885938
##########
datafusion-cli/src/exec.rs:
##########
@@ -126,13 +126,24 @@ pub async fn exec_from_repl(
print_options: &mut PrintOptions,
) -> rustyline::Result<()> {
let mut rl = Editor::new()?;
- rl.set_helper(Some(CliHelper::new(
- &ctx.task_ctx().session_config().options().sql_parser.dialect,
- print_options.color,
- )));
+ {
+ let task_ctx = ctx.task_ctx();
Review Comment:
is there some way to avoid copy/pasting the same code 3 times in this code? Perhaps making a helper function or somethig?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(cli): respect `sql_parser.recursion_limit` when parsing and validating input [datafusion]
Posted by "KassaSana (via GitHub)" <gi...@apache.org>.
KassaSana commented on PR #25807:
URL: https://github.com/apache/datafusion/pull/25807#issuecomment-5963263830
Hi @kumarUjjawal, whenever you have a moment, could you (or another committer) approve the CI run for this one? Thanks!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(cli): respect `sql_parser.recursion_limit` when parsing and validating input [datafusion]
Posted by "KassaSana (via GitHub)" <gi...@apache.org>.
KassaSana commented on code in PR #25807:
URL: https://github.com/apache/datafusion/pull/25807#discussion_r4394652093
##########
datafusion-cli/src/exec.rs:
##########
@@ -126,13 +126,24 @@ pub async fn exec_from_repl(
print_options: &mut PrintOptions,
) -> rustyline::Result<()> {
let mut rl = Editor::new()?;
- rl.set_helper(Some(CliHelper::new(
- &ctx.task_ctx().session_config().options().sql_parser.dialect,
- print_options.color,
- )));
+ {
+ let task_ctx = ctx.task_ctx();
Review Comment:
Thanks! Done in 7a6e21d. I replaced `set_recursion_limit` with `CliHelper::set_parser_options(&SqlParserOptions)`, which sets both dialect and recursion limit, so the REPL loop now syncs with a single call. The extra setup block before the loop is gone (back to the original `CliHelper::new`), since the loop syncs before the first read anyway. `exec_and_print` still reads the options once to build the parser.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(cli): respect `sql_parser.recursion_limit` when parsing and validating input [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #25807:
URL: https://github.com/apache/datafusion/pull/25807#discussion_r4171500448
##########
datafusion-cli/src/exec.rs:
##########
@@ -183,10 +186,12 @@ pub async fn exec_from_repl(
continue
},
}
- // dialect might have changed
- rl.helper_mut().unwrap().set_dialect(
- &ctx.task_ctx().session_config().options().sql_parser.dialect,
- );
+ // dialect or recursion limit might have changed
+ let task_ctx = ctx.task_ctx();
+ let sql_parser = &task_ctx.session_config().options().sql_parser;
+ let helper = rl.helper_mut().unwrap();
+ helper.set_dialect(&sql_parser.dialect);
+ helper.set_recursion_limit(sql_parser.recursion_limit.get());
Review Comment:
If a file run with `\i` changes the recursion limit, the REPL helper keeps its previous limit. `Command::Include` runs SQL through `exec_from_lines`, but this refresh runs only in the SQL-input branch. After an included file raises the limit to 100, the validator still rejects a 60-deep query at the old limit of 51. Refresh the helper after backslash commands too.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(cli): respect `sql_parser.recursion_limit` when parsing and validating input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #25807:
URL: https://github.com/apache/datafusion/pull/25807
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(cli): respect `sql_parser.recursion_limit` when parsing and validating input [datafusion]
Posted by "KassaSana (via GitHub)" <gi...@apache.org>.
KassaSana commented on code in PR #25807:
URL: https://github.com/apache/datafusion/pull/25807#discussion_r4213938162
##########
datafusion-cli/src/exec.rs:
##########
@@ -183,10 +186,12 @@ pub async fn exec_from_repl(
continue
},
}
- // dialect might have changed
- rl.helper_mut().unwrap().set_dialect(
- &ctx.task_ctx().session_config().options().sql_parser.dialect,
- );
+ // dialect or recursion limit might have changed
+ let task_ctx = ctx.task_ctx();
+ let sql_parser = &task_ctx.session_config().options().sql_parser;
+ let helper = rl.helper_mut().unwrap();
+ helper.set_dialect(&sql_parser.dialect);
+ helper.set_recursion_limit(sql_parser.recursion_limit.get());
Review Comment:
Good catch, thanks. You're right: the helper only got refreshed after SQL input, so a `\i` that raised the limit never reached it.
Fixed in 91f61d594. The sync now runs at the top of the REPL loop, right before each `readline`, so SQL, `\i` and the other backslash commands are all covered. I removed the old post-SQL refresh, since nothing reads the helper between there and the next read. The `continue` in the Ctrl-C arm went too, since clippy flagged it as redundant.
I reproduced it through a pty on the previous commit (`\i` setting the limit to 100, then a 60-deep query: held as incomplete, never ran). On the fix it runs. A directly typed `SET` and the default-limit rejection behave as before.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(cli): respect `sql_parser.recursion_limit` when parsing and validating input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25807:
URL: https://github.com/apache/datafusion/pull/25807#issuecomment-6480500575
I guess it is ok -- thank you
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org