Repository navigation
Conversation
…ating input `datafusion-cli` parsed statements with the parser's built-in recursion limit, so `datafusion.sql_parser.recursion_limit` had no effect in `-c`, `-f`, or the REPL. Parse with the session's limit in `exec_and_print`, and keep the limit used by the REPL's input validator in sync with the session, like the dialect. Closes apache#24913.
|
Hi @kumarUjjawal, whenever you have a moment, could you (or another committer) approve the CI run for this one? Thanks! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25807 +/- ##
==========================================
+ Coverage 82.51% 82.73% +0.21%
==========================================
Files 1141 1147 +6
Lines 439780 449509 +9729
Branches 439780 449509 +9729
==========================================
+ Hits 362888 371882 +8994
- Misses 54950 54951 +1
- Partials 21942 22676 +734 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @KassaSana for working on this.
Left one comment please take a look.
| // 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()); |
There was a problem hiding this 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.
There was a problem hiding this 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 91f61d5. 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.
The REPL refreshed the input validator's dialect and recursion limit only after SQL input. A `\i` file can change `datafusion.sql_parser.recursion_limit` without going through that path, so the validator kept the old limit and held valid input as incomplete. Refresh the validator before every read instead.
Which issue does this PR close?
datafusion-cliignoresdatafusion.sql_parser.recursion_limit#24913.Rationale for this change
datafusion.sql_parser.recursion_limithas no effect indatafusion-cli. AfterSET datafusion.sql_parser.recursion_limit = 200(orDATAFUSION_SQL_PARSER_RECURSION_LIMIT=200),a query with 60 nested
abs(...)calls still fails withRecursionLimitExceeded (current limit: 51)via-c,-fand 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 linebefore 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_printparses withDFParserBuilder::with_recursion_limitusing the session'slimit, the same way
SessionState::sql_to_statementdoes.CliHelpergets arecursion_limit(default taken fromSqlParserOptions) and a newset_recursion_limitmethod.exec_from_replsets it when the REPL starts and after eachstatement, next to the existing
set_dialectcall.CliHelper::newis unchanged, sothis is additive.
Not changed:
is_open_quote_for_locationinhelper.rs(tab completion forLOCATION '...) stilluses the default parser. It already ignores the dialect, and a parse failure there only
means no filename completion.
-cstring or REPL line containingSET ...; <query>is parsed as a wholebefore the
SETruns, so the new limit does not apply to the query in that same string.This matches how the dialect already behaves. Separate
-carguments, file lines, orREPL entries work.
Question for reviewers: I added a narrow
set_recursion_limit. Would you prefer aset_parser_options(&SqlParserOptions)that also replacesset_dialect, so future parseroptions can't drift the same way?
Something I noticed and did not special-case: with a limit of 1 or 2, even a
SETstatement fails to parse, so in the REPL you have to restart to recover (3 and above is
fine).
SessionContext::sqlbehaves the same way; the CLI just used to ignore the setting.What is the testing strategy for this PR?
cli_quick_testcaserecursion_limitwithtests/sql/recursion_limit.sql: raisesthe limit and runs a 60-deep query, then lowers it to 5 and runs a 10-deep query. On
mainthe snapshot shows the reverse (the deep query fails at 51, the shallow onesucceeds).
helper.rsunit testsql_recursion_limit, modelled onsql_dialect, for the REPLvalidator.
-c, the env var, and the interactive shell (viascriptfor a pty).\i(rustyline needs a terminal, so this is not ansqllogictest):
\ia file that sets the limit to 100, then run a 60-deep query. Beforethe follow-up commit the query was held as incomplete and never ran; after it, the query
runs. A
SETtyped directly and the default-limit rejection behave as before.Commands run:
Are there any user-facing changes?
Yes:
datafusion.sql_parser.recursion_limitis now respected bydatafusion-cli.The only API change is the new public
CliHelper::set_recursion_limitmethod (additive).