[Proposal] Propagate ExecutionOptions into Physical BinaryExpr
- Dominant language
- Rust
- Stars
- 9.3k
- Forks
- 2.4k
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 344
Description
## Proposed Title
Propagate `ExecutionOptions` (ConfigOptions) into physical `BinaryExpr` / `PhysicalExpr` evaluation (needed for configurable `preselection_threshold`)
## Description (paste into GitHub issue)
### Background
PR #19420 adds a `preselection_threshold` knob to `BinaryExpr` to improve short-circuit evaluation for `AND` when the RHS is expensive (or may error if evaluated unnecessarily):
https://github.com/apache/datafusion/pull/19420
In the PR discussion, it was noted that there is currently **no way to read `ExecutionOptions` inside `PhysicalExpr::evaluate`**, which blocks wiring this knob to a user-facing/session-level configuration:
https://github.com/apache/datafusion/pull/19420#issuecomment-3724107305
### Problem
`PhysicalExpr::evaluate(&RecordBatch)` does not take any session/task context, so built-in physical expressions (such as `BinaryExpr`) cannot consult `ExecutionOptions` at evaluation time.
While `datafusion_physical_expr::create_physical_expr` receives `ExecutionProps` (which can carry a snapshot of `ConfigOptions`/`ExecutionOptions`), today that snapshot is only used for some expressions (for example, `ScalarFunctionExpr` captures `Arc`). The `Expr::BinaryExpr` conversion path does not propagate any `ExecutionOptions`-derived settings into the created physical `BinaryExpr`, so the new behavior can’t be configured via `ExecutionOptions`.
**Relevant code pointers**
- `PhysicalExpr` trait / `evaluate` signature: `datafusion/physical-expr-common/src/physical_expr.rs`
- logical → physical expr conversion: `datafusion/physical-expr/src/planner.rs` (`Expr::BinaryExpr` match arm)
- physical `BinaryExpr`: `datafusion/physical-expr/src/expressions/binary.rs`
- `ExecutionProps` and `config_options`: `datafusion/expr/src/execution_props.rs`
### Proposal (minimal / non-breaking)
Capture the needed option at **plan time**, and store it in the physical `BinaryExpr`, rather than trying to read session config during `evaluate`.
Concretely:
1. Add a new `ExecutionOptions` field, e.g. `binary_expr_preselection_threshold: f32` (default `0.2`).
2. In `datafusion/physical-expr/src/planner.rs` when converting `Expr::BinaryExpr`, read the option from `execution_props.config_options` (fallback to default when `None`), and set it on the constructed physical `BinaryExpr` (e.g. via `BinaryExpr::with_preselection_threshold(threshold)` or a `binary_with_options(...)` constructor).
This matches the existing pattern used by `ScalarFunctionExpr` (which captures `Arc` during physical expr planning).
### Alternatives / open questions
- More general solution: introduce a lightweight “physical expression config” object (or pass `Arc`) that can be captured by any `PhysicalExpr` that needs it.
- More invasive: change `PhysicalExpr::evaluate` to accept a `TaskContext` (or similar) so evaluation can consult session options dynamically. This seems API-breaking and likely too broad for the `BinaryExpr` use case.
### Expected outcome
- Users can configure `BinaryExpr` short-circuit preselection via `SessionConfig` / SQL `SET`.
- `EXPLAIN` / debug output reflects the configured threshold.
- No API-breaking changes required for `PhysicalExpr::evaluate`.
Contributor guide
Research direction
Start by reading ExecutionProps in datafusion/expr/src/execution_props.rs, then trace the Expr::BinaryExpr conversion in datafusion/physical-expr/src/planner.rs into datafusion/physical-expr/src/expressions/binary.rs. Done means the configured threshold is captured at plan time, defaults correctly, is exposed through SessionConfig or SQL SET, and appears in EXPLAIN or debug output without changing PhysicalExpr::evaluate.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend, backend-api-design
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100