alamb commented on code in PR #23649: URL: https://github.com/apache/datafusion/pull/23649#discussion_r3605043896
########## docs/source/library-user-guide/upgrading/55.0.0.md: ########## @@ -650,3 +650,88 @@ months are ignored, as in PostgreSQL. The result keeps the input time's unit unit is truncated -- so `time(s) + interval '1 nanosecond'` is a no-op. See [PR #23279](https://github.com/apache/datafusion/pull/23279) for details. + +### Scalar-subquery state moved from `ExecutionProps` to an explicit `SubqueryContext` parameter + +The `subquery_indexes` and `subquery_results` public fields on Review Comment: FYI @neilconway ########## datafusion-examples/examples/dataframe/cache_factory.rs: ########## @@ -29,6 +29,7 @@ use datafusion::error::Result; use datafusion::execution::context::QueryPlanner; use datafusion::execution::session_state::CacheFactory; use datafusion::execution::{SessionState, SessionStateBuilder}; +use datafusion::logical_expr::execution_props::SubqueryContext; Review Comment: I found it confusing that SubqueryContext is logically part of ExecutionProps (which is supposed to be per-execution state) but SubqueryContext has ########## datafusion/expr/src/execution_props.rs: ########## @@ -169,6 +161,52 @@ impl ExecutionProps { } } +/// Per-plan context used by the physical planner when creating physical Review Comment: Technically speaking I don't think this is "per plan" -- instead it is more like "per plan subtree" ########## datafusion/expr/src/execution_props.rs: ########## @@ -169,6 +161,52 @@ impl ExecutionProps { } } +/// Per-plan context used by the physical planner when creating physical +/// expressions for [`Expr::ScalarSubquery`] nodes that read from a shared +/// [`ScalarSubqueryResults`] container. +/// +/// The physical planner builds this from the set of uncorrelated scalar +/// subqueries it has scheduled for execution. It is then passed explicitly +/// through `create_physical_expr` so that function can find the slot index for +/// each [`Subquery`]. +/// +/// An empty [`SubqueryContext`] (the [`Default`]) is what every +/// non-physical-planner caller passes; if such a caller encounters a scalar +/// subquery, `create_physical_expr` returns a `not_impl_err`. +/// +/// [`Expr::ScalarSubquery`]: crate::Expr::ScalarSubquery +/// [`Subquery`]: crate::logical_plan::Subquery +#[derive(Clone, Debug, Default)] +pub struct SubqueryContext { + indexes: HashMap<crate::logical_plan::Subquery, SubqueryIndex>, Review Comment: I find this somewhat confusing that the subquery context has both planning time info (indexes) and something that is modified during execution time (results) ########## datafusion-examples/examples/query_planning/expr_api.rs: ########## @@ -541,8 +542,12 @@ fn type_coercion_demo() -> Result<()> { // Evaluation with an expression that has not been type coerced cannot succeed. let props = ExecutionProps::default(); - let physical_expr = - datafusion::physical_expr::create_physical_expr(&expr, &df_schema, &props)?; + let physical_expr = datafusion::physical_expr::create_physical_expr( + &expr, + &df_schema, + &props, Review Comment: One question I have is can we possible put the subquery context as a field on ExecutionProps (as it seems specific to each execution -- which is supposed to be the scope of the ExecutionProps) ########## datafusion/expr/src/execution_props.rs: ########## @@ -169,6 +161,52 @@ impl ExecutionProps { } } +/// Per-plan context used by the physical planner when creating physical +/// expressions for [`Expr::ScalarSubquery`] nodes that read from a shared +/// [`ScalarSubqueryResults`] container. +/// +/// The physical planner builds this from the set of uncorrelated scalar +/// subqueries it has scheduled for execution. It is then passed explicitly +/// through `create_physical_expr` so that function can find the slot index for +/// each [`Subquery`]. +/// +/// An empty [`SubqueryContext`] (the [`Default`]) is what every +/// non-physical-planner caller passes; if such a caller encounters a scalar +/// subquery, `create_physical_expr` returns a `not_impl_err`. +/// +/// [`Expr::ScalarSubquery`]: crate::Expr::ScalarSubquery +/// [`Subquery`]: crate::logical_plan::Subquery +#[derive(Clone, Debug, Default)] +pub struct SubqueryContext { + indexes: HashMap<crate::logical_plan::Subquery, SubqueryIndex>, Review Comment: Ah, they actually do: https://github.com/apache/datafusion/blob/44c8305b66652a55aab6f190e5334cdbbd0c2c02/datafusion/physical-expr/src/planner.rs#L612-L614 ########## datafusion/expr/src/execution_props.rs: ########## @@ -169,6 +161,52 @@ impl ExecutionProps { } } +/// Per-plan context used by the physical planner when creating physical +/// expressions for [`Expr::ScalarSubquery`] nodes that read from a shared +/// [`ScalarSubqueryResults`] container. +/// +/// The physical planner builds this from the set of uncorrelated scalar +/// subqueries it has scheduled for execution. It is then passed explicitly +/// through `create_physical_expr` so that function can find the slot index for +/// each [`Subquery`]. +/// +/// An empty [`SubqueryContext`] (the [`Default`]) is what every +/// non-physical-planner caller passes; if such a caller encounters a scalar +/// subquery, `create_physical_expr` returns a `not_impl_err`. +/// +/// [`Expr::ScalarSubquery`]: crate::Expr::ScalarSubquery +/// [`Subquery`]: crate::logical_plan::Subquery +#[derive(Clone, Debug, Default)] +pub struct SubqueryContext { + indexes: HashMap<crate::logical_plan::Subquery, SubqueryIndex>, Review Comment: Also it is not clear to me why lambda expressions don't have the same issue 🤔 -- 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]
