refactor: move lambda variable scope into Physical Planning Context - #23989
refactor: move lambda variable scope into Physical Planning Context#23989sweb wants to merge 2 commits into
Conversation
|
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23989 +/- ##
==========================================
- Coverage 80.87% 80.87% -0.01%
==========================================
Files 1101 1101
Lines 375765 375781 +16
Branches 375765 375781 +16
==========================================
+ Hits 303915 303917 +2
- Misses 53747 53755 +8
- Partials 18103 18109 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0bab196 to
3b38dfa
Compare
|
@timsaucer may I ask you for a review? Thank you! |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Refactors physical planning to move lambda-variable qualifier state out of ExecutionProps into PhysicalPlanningContext, aligning it with prior scalar-subquery state refactors.
Changes:
- Move lambda-variable qualifier state from
ExecutionPropstoPhysicalPlanningContext. - Put
PhysicalPlanningContext::indexesbehindArcto avoid deep copies when cloning per lambda body. - Update docs and add a unit test for lambda variable shadowing across nested scopes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| docs/source/library-user-guide/upgrading/55.0.0.md | Documents the breaking API move from ExecutionProps to PhysicalPlanningContext (now including lambda-variable qualifier state). |
| datafusion/physical-expr/src/planner.rs | Switches lambda-variable qualifier lookup/propagation to use PhysicalPlanningContext. |
| datafusion/expr/src/physical_planning_context.rs | Adds lambda-variable qualifier tracking to the planning context; wraps subquery indexes in Arc; adds unit test. |
| datafusion/expr/src/execution_props.rs | Removes lambda-variable qualifier state and helper API from ExecutionProps; updates Debug test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - Code that read `execution_props.lambda_variable_qualifier` or called | ||
| `ExecutionProps::with_qualified_lambda_variables`: nothing to migrate. | ||
| `create_physical_expr` populates the lambda qualifiers itself as it descends | ||
| into lambda bodies, so callers planning a `HigherOrderFunction` do not need to | ||
| do anything. The equivalent state now lives on `PhysicalPlanningContext`, | ||
| reachable via `PhysicalPlanningContext::lambda_variable_qualifier` and | ||
| `PhysicalPlanningContext::with_qualified_lambda_variables`. |
| /// Adds a mapping for each variable to the given qualifier. Existing | ||
| /// variables with conflicting names get shadowed |
| let planning_ctx = planning_ctx | ||
| .clone() | ||
| .with_qualified_lambda_variables(&qualifier, &lambda.params); |
Which issue does this PR close?
Rationale for this change
This is a clean up following #23649
What changes are included in this PR?
Moves the lambda-variable scope state out of
ExecutionPropsand intoPhysicalPlanningContext, following the same pattern as the scalar-subquery state in refactor: passPhysicalPlanningContextexplicitly through planner traits #23649.Puts
PhysicalPlanningContext::indexesbehind anArcso the new per-lambda-body clone doesn't deep-copy the subquery index map.PhysicalPlanningContext::new's signature is unchanged.Are these changes tested?
lambda_variables_shadow_outer_scopeunit test inphysical_planning_context.rscoverswith_qualified_lambda_variables/lambda_variable_qualifierdirectly, including an inner lambda shadowing an outer parameter name.Are there any user-facing changes?
this is a breaking change for lib users.