timsaucer opened a new pull request, #22340: URL: https://github.com/apache/datafusion/pull/22340
## Which issue does this PR close? - part of https://github.com/apache/datafusion/issues/23348 None, but it is a precursor to making `QueryPlanner`/`PhysicalPlanner` take `&dyn Session` instead of `&SessionState`, so we can expose the query planner to FFI properly (similar to https://github.com/apache/datafusion/pull/22151). ## Rationale for this change "Expression lowering" here means turning a logical `Expr` into a physical `Arc<dyn PhysicalExpr>` — the job of `create_physical_expr` in `physical-expr/src/planner.rs`. That function only receives `&ExecutionProps`, so any state it needs has to arrive through `ExecutionProps`. Uncorrelated scalar subqueries need such state: the `Expr::ScalarSubquery` branch of `create_physical_expr` has to look up which result slot a given subquery maps to. Today `DefaultPhysicalPlanner::create_initial_plan` supplies that by cloning the `SessionState` and mutating the embedded `ExecutionProps`: ```rust let mut owned = session_state.clone(); owned.execution_props_mut().subquery_indexes = index_map; owned.execution_props_mut().subquery_results = results.clone(); ``` The block comment at that write site already flags this as a hack that should live in a dedicated planning context. It matters beyond aesthetics because **this exact clone-and-mutate pattern is impossible once the planner takes `&dyn Session`**: - `Session` is a trait object with no `clone()`, so `session_state.clone()` has no equivalent — you cannot cheaply produce an owned copy to mutate. - `Session::execution_props(&self)` returns `&ExecutionProps` immutably by design (it is called from `&self` `TableProvider` hot paths), so there is no `execution_props_mut()` to write into. So the subquery state cannot ride inside `ExecutionProps` under `&dyn Session`. It has to be threaded explicitly instead. This PR does that threading now, decoupled from the larger `&dyn Session` change. ## What changes are included in this PR? - Adds `SubqueryContext` in `datafusion-expr` — a small carrier bundling the `Subquery -> SubqueryIndex` map and the shared `ScalarSubqueryResults`. - Threads `&SubqueryContext` explicitly from `DefaultPhysicalPlanner` down into expression lowering, replacing the `ExecutionProps` side channel. - Removes the `SessionState::clone()` + `execution_props_mut()` mutation at the write site. - Removes the `subquery_indexes` and `subquery_results` fields from `ExecutionProps` (breaking — see user-facing changes). ### Why add `_with_subquery_context` variants instead of changing the existing signatures? Adding a 4th `subquery_ctx` parameter to `create_physical_expr` is a cleaner end state but has the wrong cost: - `create_physical_expr` has ~92 callers in this repo plus downstream crates (datafusion-comet, ballista, datafusion-python, custom planners). All would have to pass the new argument. - None of them need scalar-subquery support. Only one read site cares — the `Expr::ScalarSubquery` branch — and it is only reached when the caller has registered uncorrelated subqueries, which only the physical planner does. So each existing entry point keeps its signature and delegates to a new `_with_subquery_context` twin that passes `SubqueryContext::default()`. External callers behave exactly as before (a scalar subquery lowers to `not_impl_err`, matching today's behavior when the map was empty); only the physical planner uses the new variants. The trade-off is one extra public function per entry point in exchange for zero breakage of the existing signatures. ## Are these changes tested? Existing test coverage (including the `subquery` sqllogictests) exercises the changed path. No behavior change is intended. ## Are there any user-facing changes? **Breaking:** the `subquery_indexes` and `subquery_results` public fields are removed from `datafusion_expr::execution_props::ExecutionProps`. These fields shipped in `54.0.0` (added in #21240), so this is a breaking change and is documented in the [55.0.0 upgrade guide](docs/source/library-user-guide/upgrading/55.0.0.md). Only code that read or wrote those fields directly is affected; in practice only the physical planner populated them. Callers migrate by building a `SubqueryContext` and using the `_with_subquery_context` lowering entry points (see the upgrade guide for a before/after example). **Additive, non-breaking:** for each existing public lowering entry point in `datafusion-physical-expr` and `datafusion::physical_planner` (`create_physical_expr`, `create_physical_exprs`, `create_physical_sort_expr[s]`, `create_physical_partitioning`, `create_window_expr[_with_name]`) there is now a `_with_subquery_context` sibling. `LoweredAggregateBuilder` gains a `with_subquery_context` method, and `SubqueryContext` is a new public type. Every original function keeps its signature and delegates with `SubqueryContext::default()`. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
