timsaucer commented on code in PR #23649:
URL: https://github.com/apache/datafusion/pull/23649#discussion_r3598303201
##########
datafusion/expr/src/execution_props.rs:
##########
@@ -90,8 +84,6 @@ impl ExecutionProps {
alias_generator: Arc::new(AliasGenerator::new()),
config_options: None,
var_providers: None,
- subquery_indexes: HashMap::new(),
- subquery_results: ScalarSubqueryResults::default(),
Review Comment:
This is the core of the change to this PR. We move these values out of the
`ExecutionProps` and into their own struct, `SubqueryContext`
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -458,27 +484,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 calls that create physical expressions.
+ // 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);
Review Comment:
This is the work around we are removing.
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -3875,6 +4014,29 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn scalar_subquery_in_extension_expr_plans() -> Result<()> {
+ let subquery = LogicalPlanBuilder::empty(true)
+ .project(vec![lit(42_i32)])?
+ .build()?;
+ let logical_plan = LogicalPlan::Extension(Extension {
+ node: Arc::new(NoOpExtensionNode {
+ expressions: vec![scalar_subquery(Arc::new(subquery))],
+ ..Default::default()
+ }),
+ });
+ let planner =
DefaultPhysicalPlanner::with_extension_planners(vec![Arc::new(
+ ExpressionExtensionPlanner,
+ )]);
+
+ let plan = planner
+ .create_physical_plan(&logical_plan, &make_session_state())
+ .await?;
+
+ assert_contains!(format!("{plan:?}"), "ScalarSubqueryExec");
+ Ok(())
+ }
+
Review Comment:
This test is necessary and demonstrates extension nodes that need the
subquery context piped through properly.
--
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]
