-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Pass execution props to scalar subqueries #23622
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
timsaucer
wants to merge
2
commits into
apache:gabotechs/make-query-planner-accept-dyn-session
from
timsaucer:test/pr-23418-extension-scalar-subquery
+99
−11
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -143,6 +143,35 @@ pub trait PhysicalPlanner: Send + Sync { | |
| ) -> Result<Arc<dyn PhysicalExpr>>; | ||
| } | ||
|
|
||
| /// A planner adapter that lowers expressions with the properties for the plan | ||
| /// currently being built rather than the properties stored on the session. | ||
| struct PhysicalPlannerWithProps<'a> { | ||
| inner: &'a dyn PhysicalPlanner, | ||
| execution_props: &'a ExecutionProps, | ||
| } | ||
|
|
||
| #[async_trait] | ||
| impl PhysicalPlanner for PhysicalPlannerWithProps<'_> { | ||
| async fn create_physical_plan( | ||
| &self, | ||
| logical_plan: &LogicalPlan, | ||
| session_state: &SessionState, | ||
| ) -> Result<Arc<dyn ExecutionPlan>> { | ||
| self.inner | ||
| .create_physical_plan(logical_plan, session_state) | ||
| .await | ||
| } | ||
|
|
||
| fn create_physical_expr( | ||
| &self, | ||
| expr: &Expr, | ||
| input_dfschema: &DFSchema, | ||
| _session_state: &SessionState, | ||
| ) -> Result<Arc<dyn PhysicalExpr>> { | ||
| create_physical_expr(expr, input_dfschema, self.execution_props) | ||
| } | ||
| } | ||
|
|
||
| /// This trait exposes the ability to plan an [`ExecutionPlan`] out of a [`LogicalPlan`]. | ||
| #[async_trait] | ||
| pub trait ExtensionPlanner { | ||
|
|
@@ -469,8 +498,8 @@ impl DefaultPhysicalPlanner { | |
| // 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. | ||
| // the session's `ExecutionProps` 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 | ||
|
|
@@ -694,13 +723,18 @@ impl DefaultPhysicalPlanner { | |
| Arc::clone(res.plan()) | ||
| } else { | ||
| let mut maybe_plan = None; | ||
| for planner in &self.extension_planners { | ||
| let planner = PhysicalPlannerWithProps { | ||
| inner: self, | ||
| execution_props, | ||
| }; | ||
| for extension_planner in &self.extension_planners { | ||
| if maybe_plan.is_some() { | ||
| break; | ||
| } | ||
|
|
||
| maybe_plan = | ||
| planner.plan_table_scan(self, scan, session_state).await?; | ||
| maybe_plan = extension_planner | ||
| .plan_table_scan(&planner, scan, session_state) | ||
| .await?; | ||
| } | ||
|
|
||
| let plan = match maybe_plan { | ||
|
|
@@ -1819,15 +1853,19 @@ impl DefaultPhysicalPlanner { | |
| LogicalPlan::Extension(Extension { node }) => { | ||
| let mut maybe_plan = None; | ||
| let children = children.vec(); | ||
| for planner in &self.extension_planners { | ||
| let planner = PhysicalPlannerWithProps { | ||
| inner: self, | ||
| execution_props, | ||
| }; | ||
| for extension_planner in &self.extension_planners { | ||
| if maybe_plan.is_some() { | ||
| break; | ||
| } | ||
|
|
||
| let logical_input = node.inputs(); | ||
| maybe_plan = planner | ||
| maybe_plan = extension_planner | ||
| .plan_extension( | ||
| self, | ||
| &planner, | ||
| node.as_ref(), | ||
| &logical_input, | ||
| &children, | ||
|
|
@@ -3260,6 +3298,7 @@ mod tests { | |
| Accumulator, AggregateUDF, AggregateUDFImpl, ExprFunctionExt, LogicalPlanBuilder, | ||
| Partitioning as LogicalPartitioning, RangePartitioning, Signature, TableSource, | ||
| UserDefinedLogicalNodeCore, Volatility, WindowFunctionDefinition, col, lit, | ||
| scalar_subquery, | ||
| }; | ||
| use datafusion_functions_aggregate::count::{count_all, count_udaf}; | ||
| use datafusion_functions_aggregate::expr_fn::sum; | ||
|
|
@@ -3887,6 +3926,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_initial_plan(&logical_plan, &make_session_state()) | ||
| .await?; | ||
|
|
||
| assert_contains!(format!("{plan:?}"), "ScalarSubqueryExec"); | ||
| Ok(()) | ||
| } | ||
|
Comment on lines
+3929
to
+3950
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you run this test without the other changes, you will get a failure. |
||
|
|
||
| #[tokio::test] | ||
| async fn error_during_extension_planning() { | ||
| let session_state = make_session_state(); | ||
|
|
@@ -4416,6 +4478,7 @@ mod tests { | |
| #[derive(PartialEq, Eq, Hash)] | ||
| struct NoOpExtensionNode { | ||
| schema: DFSchemaRef, | ||
| expressions: Vec<Expr>, | ||
| } | ||
|
|
||
| impl Default for NoOpExtensionNode { | ||
|
|
@@ -4428,6 +4491,7 @@ mod tests { | |
| ) | ||
| .unwrap(), | ||
| ), | ||
| expressions: vec![], | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -4460,7 +4524,7 @@ mod tests { | |
| } | ||
|
|
||
| fn expressions(&self) -> Vec<Expr> { | ||
| vec![] | ||
| self.expressions.clone() | ||
| } | ||
|
|
||
| fn fmt_for_explain(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
|
|
@@ -4469,10 +4533,13 @@ mod tests { | |
|
|
||
| fn with_exprs_and_inputs( | ||
| &self, | ||
| _exprs: Vec<Expr>, | ||
| exprs: Vec<Expr>, | ||
| _inputs: Vec<LogicalPlan>, | ||
| ) -> Result<Self> { | ||
| unimplemented!("NoOp"); | ||
| Ok(Self { | ||
| schema: Arc::clone(&self.schema), | ||
| expressions: exprs, | ||
| }) | ||
| } | ||
|
|
||
| fn supports_limit_pushdown(&self) -> bool { | ||
|
|
@@ -4548,6 +4615,27 @@ mod tests { | |
| } | ||
| } | ||
|
|
||
| struct ExpressionExtensionPlanner; | ||
|
|
||
| #[async_trait] | ||
| impl ExtensionPlanner for ExpressionExtensionPlanner { | ||
| async fn plan_extension( | ||
| &self, | ||
| planner: &dyn PhysicalPlanner, | ||
| node: &dyn UserDefinedLogicalNode, | ||
| _logical_inputs: &[&LogicalPlan], | ||
| _physical_inputs: &[Arc<dyn ExecutionPlan>], | ||
| session_state: &SessionState, | ||
| ) -> Result<Option<Arc<dyn ExecutionPlan>>> { | ||
| for expr in node.expressions() { | ||
| planner.create_physical_expr(&expr, node.schema(), session_state)?; | ||
| } | ||
| Ok(Some(Arc::new(NoOpExecutionPlan::new(Arc::clone( | ||
| node.schema().inner(), | ||
| ))))) | ||
| } | ||
| } | ||
|
|
||
| // Produces an execution plan where the schema is mismatched from | ||
| // the logical plan node. | ||
| struct BadExtensionPlanner {} | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the key to this PR. By intercepting the calls to
create_physical_exprand making sure they get the execution props we support the scalar subqueries instead of relying on getting them from the session, which in this PR no longer has the correct props.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I didn't realize we'd also need this. This starts to feel a bit more hacky...