You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2026/06/20 18:20:58 UTC
[PR] fix: prevent unparser stack overflow on deeply nested expressions [datafusion]
adriangb opened a new pull request, #23058:
URL: https://github.com/apache/datafusion/pull/23058
## Which issue does this PR close?
- Closes #23056.
## Rationale for this change
`Unparser::expr_to_sql` overflows the OS stack on deeply nested expressions, and — surprisingly — it does so **even with the `recursive_protection` feature enabled** (the default for the `datafusion` crate).
While investigating I found the root cause is subtler than "a function is missing `#[recursive]`":
`expr_to_sql_inner` **is** already annotated and **is** re-entered on the function-argument / dialect-override paths, so the trampoline does fire. The real problem is that the `recursive` crate's default red zone is **128 KiB**, but a single `expr_to_sql_inner` stack frame in debug builds is **~130 KiB** — *larger than the red zone*. `stacker::maybe_grow` only grows when `remaining < red_zone`, so a frame can pass the check and then overflow before the next checkpoint. The unparser installed **no `StackGuard`**, unlike the planner, which already does exactly this in `query.rs`:
```rust
// query.rs
let _guard = StackGuard::new(256 * 1024);
```
with a comment noting the same debug-frame-size issue (PR #13310).
Secondary leaks: several internal recursion sites recursed through the public, un-annotated `expr_to_sql` (function arguments, `make_array`, `array_element`, `named_struct`, `map`, and the `array_has` / `date_part` dialect overrides), and `remove_unnecessary_nesting` (pretty mode) was not annotated at all — it would be the next overflow site once the first is fixed.
## What changes are included in this PR?
The recursive design is kept (an iterative rewrite of the tree-building unparser would be a large, high-risk change for no benefit over the trampoline). The fix mirrors the planner:
- Install `StackGuard::new(256 * 1024)` once at the top-level `expr_to_sql`.
- Add an annotated `pub(crate) expr_to_sql_with_nesting` recursion core that all internal recursion sites call. This keeps pretty-mode behavior identical (each level still runs `remove_unnecessary_nesting`) while making every nesting level a stack-growth checkpoint.
- Annotate `remove_unnecessary_nesting` with `#[recursive]`.
The PR is split into two commits for review: the failing regression test first, then the fix.
## Are these changes tested?
Yes. `test_deeply_nested_expr_does_not_overflow_stack` (gated on `recursive_protection`) unparses a depth-2000 `array_has` chain on `PostgreSqlDialect` and a depth-2000 binary chain on a 2 MiB thread. It aborts the process on `main` and passes with the fix. I verified empirically that with the default 128 KiB red zone the test still aborts, and that 256 KiB carries both paths past depth 5000 on a realistic thread.
Note: the guarantee holds on realistically-sized thread stacks (≥1–2 MiB). The 512 KiB-stack figure in the issue's repro is too small to bootstrap the heavier `array_has` override path even after the fix, but that is the same memory floor the planner side has and is not a configuration used by DataFusion in practice.
## Are there any user-facing changes?
No public API changes. Deeply nested expressions that previously aborted the process now unparse successfully when `recursive_protection` is enabled.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01QyHc4z88pC8nQLpFx7z2vM
--
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: prevent unparser stack overflow on deeply nested expressions [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #23058:
URL: https://github.com/apache/datafusion/pull/23058#discussion_r3451348221
##########
datafusion/sql/src/unparser/expr.rs:
##########
@@ -3278,6 +3302,61 @@ mod tests {
Ok(())
}
+ /// Regression test for https://github.com/apache/datafusion/issues/23056
+ ///
+ /// Deeply-nested expressions whose unparse path routes through scalar
+ /// function arguments and dialect scalar-function overrides used to
+ /// overflow the OS stack even with `recursive_protection` enabled,
+ /// because the per-level stack cost of those paths exceeds the default
+ /// `recursive` red zone and the unparser installed no [`StackGuard`].
+ ///
+ /// This test only asserts the protected behavior, so it is gated on the
+ /// `recursive_protection` feature. Without that feature the unparser is
+ /// not stack-safe by design and a deep enough expression will overflow.
+ #[cfg(feature = "recursive_protection")]
+ #[test]
+ fn test_deeply_nested_expr_does_not_overflow_stack() {
+ // Far deeper than the ~60 levels that overflow without protection, but
+ // bounded so the trampoline's heap stacks stay reasonable in debug.
+ const DEPTH: usize = 2_000;
+
+ // Run on an explicit, realistically-sized thread stack. The work is
+ // performed on a spawned thread so an overflow (in the unfixed code)
+ // aborts the process and fails the test deterministically rather than
+ // depending on the harness thread's stack size.
+ let handle = std::thread::Builder::new()
+ .stack_size(2 * 1024 * 1024)
+ .spawn(|| {
+ // 1. Linear chain through a dialect scalar-function override:
+ // array_has(array_has(... array_has(col, 'x') ...), 'x').
+ // PostgreSqlDialect unparses array_has via array_has_to_sql_any,
+ // which recurses back into the unparser for each argument.
+ let mut nested_fn: Expr = col("c");
+ for _ in 0..DEPTH {
+ nested_fn = array_has(nested_fn, lit("x"));
+ }
+ let pg = PostgreSqlDialect {};
+ Unparser::new(&pg)
+ .expr_to_sql(&nested_fn)
+ .expect("deeply nested scalar function should unparse");
+
+ // 2. Linear chain of plain binary operators, exercising the
+ // inner -> inner recursion on the default dialect.
+ let mut nested_binary: Expr = col("c");
+ for _ in 0..DEPTH {
+ nested_binary = nested_binary + lit(1);
+ }
+ Unparser::default()
Review Comment:
Nice regression test. It covers the default unparse path well, but it does not exercise the pretty-print path that now depends on `remove_unnecessary_nesting` being recursive.
Would it make sense to also unparse the deeply nested binary expression with `Unparser::default().with_pretty(true)`? That would help lock down the second recursion site that this PR fixes and make future regressions a little easier to catch.
--
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: prevent unparser stack overflow on deeply nested expressions [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #23058:
URL: https://github.com/apache/datafusion/pull/23058#issuecomment-4767355957
Thanks for the review @kosiew!
--
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: prevent unparser stack overflow on deeply nested expressions [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #23058:
URL: https://github.com/apache/datafusion/pull/23058#issuecomment-4766649914
@adriangb
Thanks for the ping.
I will review this after you resolve the merge conflict.
--
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: prevent unparser stack overflow on deeply nested expressions [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #23058:
URL: https://github.com/apache/datafusion/pull/23058#discussion_r3451496415
##########
datafusion/sql/src/unparser/expr.rs:
##########
@@ -3278,6 +3302,61 @@ mod tests {
Ok(())
}
+ /// Regression test for https://github.com/apache/datafusion/issues/23056
+ ///
+ /// Deeply-nested expressions whose unparse path routes through scalar
+ /// function arguments and dialect scalar-function overrides used to
+ /// overflow the OS stack even with `recursive_protection` enabled,
+ /// because the per-level stack cost of those paths exceeds the default
+ /// `recursive` red zone and the unparser installed no [`StackGuard`].
+ ///
+ /// This test only asserts the protected behavior, so it is gated on the
+ /// `recursive_protection` feature. Without that feature the unparser is
+ /// not stack-safe by design and a deep enough expression will overflow.
+ #[cfg(feature = "recursive_protection")]
+ #[test]
+ fn test_deeply_nested_expr_does_not_overflow_stack() {
+ // Far deeper than the ~60 levels that overflow without protection, but
+ // bounded so the trampoline's heap stacks stay reasonable in debug.
+ const DEPTH: usize = 2_000;
+
+ // Run on an explicit, realistically-sized thread stack. The work is
+ // performed on a spawned thread so an overflow (in the unfixed code)
+ // aborts the process and fails the test deterministically rather than
+ // depending on the harness thread's stack size.
+ let handle = std::thread::Builder::new()
+ .stack_size(2 * 1024 * 1024)
+ .spawn(|| {
+ // 1. Linear chain through a dialect scalar-function override:
+ // array_has(array_has(... array_has(col, 'x') ...), 'x').
+ // PostgreSqlDialect unparses array_has via array_has_to_sql_any,
+ // which recurses back into the unparser for each argument.
+ let mut nested_fn: Expr = col("c");
+ for _ in 0..DEPTH {
+ nested_fn = array_has(nested_fn, lit("x"));
+ }
+ let pg = PostgreSqlDialect {};
+ Unparser::new(&pg)
+ .expr_to_sql(&nested_fn)
+ .expect("deeply nested scalar function should unparse");
+
+ // 2. Linear chain of plain binary operators, exercising the
+ // inner -> inner recursion on the default dialect.
+ let mut nested_binary: Expr = col("c");
+ for _ in 0..DEPTH {
+ nested_binary = nested_binary + lit(1);
+ }
+ Unparser::default()
Review Comment:
Thanks, added in d57542c
--
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: prevent unparser stack overflow on deeply nested expressions [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #23058:
URL: https://github.com/apache/datafusion/pull/23058
--
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