apache / apache/datafusion

Flatter representation of deeply nested `AND` / `OR` Expressions

Open
#23,264 10 comments 1 reaction 0 assignees View on GitHub
enhancement
Dominant language
Rust
Stars
9.3k
Forks
2.4k
Avg merge
3d 7h
Merged PRs (30d)
344

Description

### Is your feature request related to a problem or challenge?

DataFusion represents arbitrary binary Operator trees like
```sql
(a OR (b OR (c OR (d OR ...)))
```

Using a binary tree structure called `BinaryExpr`:

https://github.com/apache/datafusion/blob/2da88876f6e764ca3052c92b9d1783929d13d7bf/datafusion/expr/src/expr.rs#L770-L779

There are also many places in the codebase where recursive algorithms are used to walk over Expr trees,

An example of this is `Treenode::apply`: https://github.com/apache/datafusion/blob/0838a4ddb902535b0e95a1c5a254be7e9c7fe9bf/datafusion/common/src/tree_node.rs#L196-L209

The stack depth is a function of the nesting depth of the expression, which means that

> [!IMPORTANT]
> In some cases processing deeply nested expressions can lean to a stack overflow (and thus a process `abort`)

This shows up in practice from Machine-generated SQL/filters with thousands of OR/AND terms (e.g. IN lists rewritten to OR, or large predicate pushdown

This representation has served us quite well, but we do have several mitigations

1. [`recursive` crate](https://crates.io/crates/recursive) + recursive_protection feature (segmented stack growth)

The primary mitigation. Recursive TreeNode drivers are annotated `#[cfg_attr(feature = "recursive_protection", recursive::recursive)]`, which uses the recursive crate to grow the stack on the heap when it runs low, instead of overflowing. See the TreeNode walkers (6 sites — apply/transform_down/transform_up/rewrite/etc.): https://github.com/apache/datafusion/blob/0838a4ddb902535b0e95a1c5a254be7e9c7fe9bf/datafusion/common/src/tree_node.rs#L196-L209

2. SQL parser recursion limit (bounded recursion → error, not crash)

The [sqlparser crate ](https://crates.io/crates/sqlparser) itself caps nesting depth (default 50) and returns RecursionLimitExceeded rather than overflowing the stack.

3. Explicit-stack (iterative) SQL→Expr conversion
- See issue #1444

We also hit this a bunch when converting a [sqlparser](https://crates.io/crates/sqlparser) binary-operator tree into `Expr` and have a stack based conversion approach (see https://github.com/apache/datafusion/blob/0838a4ddb902535b0e95a1c5a254be7e9c7fe9bf/datafusion/sql/src/expr/mod.rs#L76-L120)

4. Out-of-line match arms in the expression simplifier

Large match arms are extracted into separate functions specifically to shrink per-frame stack usage and avoid overflow during simplification.

- https://github.com/apache/datafusion/blob/0838a4ddb902535b0e95a1c5a254be7e9c7fe9bf/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs#L2335-L2341

Recently people seem to be hitting recursion limits again and trying to put in patches, such as
* https://github.com/apache/datafusion/pull/23198 from @mdashti
* https://github.com/apache/datafusion/pull/22943 from @nathanb

### Describe the solution you'd like

I would like a more fool proof way to avoid stack overflow issues

### Describe alternatives you've considered

## Option 1: n-ary expressions

The approach taken by DuckDb Mysql and Postgres is to represent `AND` and `OR` as n-ary rather than binary (2)

So that would be something like this (could also have an enum for And/Or)
```rust
enum ConjunctionType { And, Or }
/// AND/OR of some number of expressions
struct Conjunction {
conjunction_type: ConjunctionType,
/// Logically `exprs[0] AND (exprs[1] AND (exprs[2] AND ...`
exprs : Vec.
}
```

And then
```rust
/// extend the Expr enum
enum Expr {
...
// AND
Conjunction(Conjunction),
...
}
```

### Examples:

DuckDB uses ConjunctionExpression: https://github.com/duckdb/duckdb/blob/b8a06e4a22672e254cd0baa68a3dbed2eb51c56e/src/include/duckdb/parser/expression/conjunction_expression.hpp#L17-L27

Postrgres uses BoolExpr https://github.com/postgres/postgres/blob/REL_17_0/src/include/nodes/primnodes.h#L929-L942

Mysql uses [`Item_cond_and` / `Item_cond_or`](https://github.com/mysql/mysql-server/blob/mysql-8.0.40/sql/item_cmpfunc.h#L2711-L2768) (based on [Item_cond base which stores args as a List list](https://github.com/mysql/mysql-server/blob/mysql-8.0.40/sql/item_cmpfunc.h#L2421-L2447);

### Cons
This would be a major downstream API change
Also unless we removed Operator::And / Operator::Or we would potentially have multiple ways to express the same logical expression

## Keep Binary Expr, have some sort of "balance" operation
Another approach taken by spark is to use Binary expressions: https://github.com/apache/spark/blob/fa33ea000a0bda9e5a3fa1af98e8e85b8cc5e4d4/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala#L828

[`buildBalancedPredicate`](https://github.com/apache/spark/blob/fa33ea000a0bda9e5a3fa1af98e8e85b8cc5e4d4/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala#L160-L184) rebuilds a predicate list into a height-balanced tree in
- https://github.com/apache/spark/pull/31724

### Additional context

Recent PRs
* https://github.com/apache/datafusion/pull/23198 from @mdashti
* https://github.com/apache/datafusion/pull/22943 from @nathanb

🎯 Umbrella / strategic (open)

- #16788 — the epic to explore iterative rewrites / Box-ing / n-ary to remove the need for #[recursive] and regain the 1–2% planning overhead.
- #14857
- #19787
- #14118

🐛 Open stack-overflow bugs (motivating cases)

- #22229
- #22936
- #8900
- #23246

📜 Closed — the history that motivated the current mitigations

- #1444
- #1434
- #9375
- #11102
- #16030
- #4066
- #3968
- #23056
- #9573
- #9373
- #15087
- TPC-DS planning overflows: #6277, #6264, #6040, #4786, #4065, #8837

Contributor guide

Open the contributing guide

Research direction

Start with datafusion/expr/src/expr.rs and datafusion/common/src/tree_node.rs to understand the current BinaryExpr representation and recursive walkers. Read the SQL-to-Expr conversion in datafusion/sql/src/expr/mod.rs and the simplifier entry points in datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs. Done requires a concrete, agreed approach for deeply nested AND/OR expressions that addresses stack safety and the downstream API impact.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust, sql
Domain
backend, data-engineering, databases
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.