refactor: pass SubqueryContext explicitly through planner traits#23649
Draft
timsaucer wants to merge 4 commits into
Draft
refactor: pass SubqueryContext explicitly through planner traits#23649timsaucer wants to merge 4 commits into
timsaucer wants to merge 4 commits into
Conversation
AI Disclosure: This code was written in part by an AI agent.
github-actions
Bot
added
documentation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Replace broad uses of expression lowering with explicit language about creating physical expressions, and correct the migration guide PR link. AI Disclosure: This code was written in part by an AI agent.
timsaucer
commented
Jul 16, 2026
timsaucer
left a comment
timsaucer
left a comment
Member
Author
There was a problem hiding this comment.
I've tried to mark all of the places in the code that I think are significant, which are few. The rest is just adding in an extra parameter to function calls and plumbing that through as necessary.
Comment on lines
-475
to
-478
| 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); |
Member
Author
There was a problem hiding this comment.
This is the work around we are removing.
Comment on lines
+4017
to
+4039
| #[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(()) | ||
| } | ||
|
|
Member
Author
There was a problem hiding this comment.
This test is necessary and demonstrates extension nodes that need the subquery context piped through properly.
Comment on lines
-93
to
-94
| subquery_indexes: HashMap::new(), | ||
| subquery_results: ScalarSubqueryResults::default(), |
Member
Author
There was a problem hiding this 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
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Which issue does this PR close?
Rationale for this change
When we turn logical plans into physical plans, we have a work around to get the context for scalar subqueries. Specifically we clone the session and mutate the
execution_propswhere the subquery context currently sits. This is even called out in the code with comments:The real reason for this is that we want to expose the
QueryPlannervia FFI and currently it depends upon downcasts fromSessiontoSessionState. That does not work for the FFI crate where we have a differentSessionimplementation. Since we cannot do the downcast, we cannot use the work around in the current code.This PR is designed to plumb through the required context properly rather than rely on manipulating the execution properties.
What changes are included in this PR?
The major change here is to remove
subquery_indexesandsubquery_resultsfromExecutionPropsand put them into their own structSubqueryContext. Everything else is just refactoring and plumbing to align around this split of the data.Are these changes tested?
Are there any user-facing changes?
Yes, this adds a single parameter into
create_physical_expr, the subquery context. The migration guide demonstrates how to add a default context in for users, if necessary.