Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions datafusion/core/tests/tpcds_planning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,6 @@ async fn tpcds_logical_q63() -> Result<()> {
create_logical_plan(63).await
}

#[ignore] // thread 'q64' has overflowed its stack]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these tests will likely still fail due to #4786 but I figured I would let the CI run and see if we can squeek by

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#[tokio::test]
Comment thread
alamb marked this conversation as resolved.
async fn tpcds_logical_q64() -> Result<()> {
create_logical_plan(64).await
Expand Down Expand Up @@ -851,7 +850,6 @@ async fn tpcds_physical_q63() -> Result<()> {
create_physical_plan(63).await
}

#[ignore] // thread 'q64' has overflowed its stack
#[tokio::test]
Comment thread
alamb marked this conversation as resolved.
async fn tpcds_physical_q64() -> Result<()> {
create_physical_plan(64).await
Expand Down
42 changes: 38 additions & 4 deletions datafusion/sql/src/planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1839,6 +1839,37 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
sql: SQLExpr,
schema: &DFSchema,
planner_context: &mut PlannerContext,
) -> Result<Expr> {
// Workaround for https://github.com/apache/arrow-datafusion/issues/4065
//
// Minimize stack space required in debug builds to plan
// deeply nested binary operators by keeping the stack space
// needed for sql_expr_to_logical_expr minimal for BinaryOp
//
// The reason this reduces stack size in debug builds is
// explained in the "Technical Backstory" heading of
// https://github.com/apache/arrow-datafusion/pull/1047
//
// A likely better way to support deeply nested expressions
// would be to avoid recursion all together and use an
// iterative algorithm.
match sql {
SQLExpr::BinaryOp { left, op, right } => {
self.parse_sql_binary_op(*left, op, *right, schema, planner_context)
}
// since this function requires more space per frame
// avoid calling it for binary ops
_ => self.sql_expr_to_logical_expr_internal(sql, schema, planner_context),
}
}

/// Internal implementation. Use
/// [`Self::sql_expr_to_logical_expr`] to plan exprs.
fn sql_expr_to_logical_expr_internal(
&self,
sql: SQLExpr,
schema: &DFSchema,
planner_context: &mut PlannerContext,
) -> Result<Expr> {
match sql {
SQLExpr::Value(value) => {
Expand Down Expand Up @@ -1976,10 +2007,13 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {


SQLExpr::BinaryOp {
left,
op,
right,
} => self.parse_sql_binary_op(*left, op, *right, schema, planner_context),
..
} => {
Err(DataFusionError::Internal(
"binary_op should be handled by sql_expr_to_logical_expr.".to_string()
))
}


#[cfg(feature = "unicode_expressions")]
SQLExpr::Substring {
Expand Down