Fix Stack overflow in sql planning in debug builds #4779
Conversation
| })) | ||
| } | ||
| } | ||
| SQLExpr::Identifier(id) => self.sql_identifier_to_expr(id), |
There was a problem hiding this comment.
I just applied this pattern several times: extract code into a separate function
|
|
||
| SQLExpr::Function(mut function) => { | ||
| let name = if function.name.0.len() > 1 { |
There was a problem hiding this comment.
I think the expanded handling of SQLExpr::Function is likely the thing that resulted in significantly larger stacks and thus overflows
| @@ -2375,6 +2072,395 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |||
| } | |||
| } | |||
|
|
|||
| fn sql_function_to_expr( | |||
There was a problem hiding this comment.
All this code was simply moved from above
|
https://github.com/apache/arrow-datafusion/actions/runs/3812581550/jobs/6485893384 😢 still overflows on windows |
|
Looks like q64 needs some more work -- will do so |
|
I have one more PR that should avoid stack overflows while planning (SQL --> LogicalPlan) but then planning overflows at a later step -- see #4786 |
|
Benchmark runs are scheduled for baseline = 9a8190a and contender = 0d6d371. 0d6d371 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #4065 from @andygrove
Closes #4728 from @Jefffrey
Also reported by @maxburke in slack
Rationale for this change
Debug builds are overflowing the stack causing pain and suffering for users.
What changes are included in this PR?
SqlToRel::sql_expr_to_logical_exprinto separate functionsThe reason this reduces stack size in debug builds is explained in the "Technical Backstory" heading of #1047
Are these changes tested?
Covered by existing coverage
Are there any user-facing changes?
None intended (other than avoiding stack overflows)