gabotechs commented on code in PR #22340:
URL: https://github.com/apache/datafusion/pull/22340#discussion_r3551382006
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -458,27 +462,25 @@ impl DefaultPhysicalPlanner {
if links.is_empty() {
return self
- .create_initial_plan_inner(logical_plan, session_state)
+ .create_initial_plan_inner(
+ logical_plan,
+ session_state,
+ &SubqueryContext::default(),
+ )
.await;
}
- // Create the shared `ScalarSubqueryResults` container and register
- // it in `ExecutionProps` so that `create_physical_expr` can
resolve
- // `Expr::ScalarSubquery` into `ScalarSubqueryExpr` nodes. We clone
- // the `SessionState` so these are available throughout physical
- // planning without mutating the caller's state.
- //
- // Ideally, the subquery state would live in a dedicated planning
- // context rather than in `ExecutionProps`. It's here because
- // `create_physical_expr` only receives `&ExecutionProps`.
+ // Build a `SubqueryContext` that carries the index map and shared
+ // results container into expression lowering. The context is
+ // threaded explicitly through physical planning rather than being
+ // stashed in `ExecutionProps`, so that the planner does not need
+ // a mutable `SessionState` and external callers of
+ // `create_physical_expr` are unaffected.
let results = ScalarSubqueryResults::new(links.len());
- let mut owned = session_state.clone();
- owned.execution_props_mut().subquery_indexes = index_map;
- owned.execution_props_mut().subquery_results = results.clone();
- let session_state = Cow::Owned(owned);
+ let subquery_ctx = SubqueryContext::new(index_map,
results.clone());
Review Comment:
Given that the `dyn Session` trait has the `.execution_props()` method, and
that `ExecutionProps` implements `Clone`, shouldn't it be possible to do
something like this?
```rust
let mut props = session_state.execution_props().clone();
props.subquery_indexes = index_map;
props.subquery_results = results.clone();
```
And thread around explicitly that cloned+mutated `ExecutionProps` everywhere?
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -458,27 +462,25 @@ impl DefaultPhysicalPlanner {
if links.is_empty() {
return self
- .create_initial_plan_inner(logical_plan, session_state)
+ .create_initial_plan_inner(
+ logical_plan,
+ session_state,
+ &SubqueryContext::default(),
+ )
.await;
}
- // Create the shared `ScalarSubqueryResults` container and register
- // it in `ExecutionProps` so that `create_physical_expr` can
resolve
- // `Expr::ScalarSubquery` into `ScalarSubqueryExpr` nodes. We clone
- // the `SessionState` so these are available throughout physical
- // planning without mutating the caller's state.
- //
- // Ideally, the subquery state would live in a dedicated planning
- // context rather than in `ExecutionProps`. It's here because
- // `create_physical_expr` only receives `&ExecutionProps`.
+ // Build a `SubqueryContext` that carries the index map and shared
+ // results container into expression lowering. The context is
+ // threaded explicitly through physical planning rather than being
+ // stashed in `ExecutionProps`, so that the planner does not need
+ // a mutable `SessionState` and external callers of
+ // `create_physical_expr` are unaffected.
let results = ScalarSubqueryResults::new(links.len());
- let mut owned = session_state.clone();
- owned.execution_props_mut().subquery_indexes = index_map;
- owned.execution_props_mut().subquery_results = results.clone();
- let session_state = Cow::Owned(owned);
+ let subquery_ctx = SubqueryContext::new(index_map,
results.clone());
Review Comment:
I gave it a try in a draft:
- https://github.com/apache/datafusion/pull/23418
And it looks dead simple, but not sure if I'm missing something.
##########
datafusion/physical-expr/src/physical_expr.rs:
##########
@@ -211,10 +233,33 @@ pub fn create_physical_sort_exprs(
exprs: &[SortExpr],
input_dfschema: &DFSchema,
execution_props: &ExecutionProps,
+) -> Result<Vec<PhysicalSortExpr>> {
+ create_physical_sort_exprs_with_subquery_context(
+ exprs,
+ input_dfschema,
+ execution_props,
+ &SubqueryContext::default(),
+ )
+}
+
+/// Create vector of physical sort expressions from a vector of logical
+/// expressions with an explicit [`SubqueryContext`].
+pub fn create_physical_sort_exprs_with_subquery_context(
+ exprs: &[SortExpr],
+ input_dfschema: &DFSchema,
+ execution_props: &ExecutionProps,
+ subquery_ctx: &SubqueryContext,
) -> Result<Vec<PhysicalSortExpr>> {
exprs
Review Comment:
It's not very beautiful to have this two variants of the same function with
different argument count and different name be part of the public API.
It does not scale very well in case newer params need to be added.
I do not have a better suggestion though...
--
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]
