Copilot commented on code in PR #23989:
URL: https://github.com/apache/datafusion/pull/23989#discussion_r3703868800
##########
docs/source/library-user-guide/upgrading/55.0.0.md:
##########
@@ -774,6 +779,13 @@ Convenience methods such as
`SessionContext::create_physical_expr` and
parameter and forward it.
- Code that read or wrote `execution_props.subquery_indexes` /
`execution_props.subquery_results`: build a `PhysicalPlanningContext`
instead.
+- 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`.
Review Comment:
This migration note is internally inconsistent: it says “nothing to migrate”
but then points to replacement APIs on `PhysicalPlanningContext`. Consider
rewording to clarify the two cases: (1) most callers don’t need to manually set
lambda qualifiers anymore, but (2) callers that *previously* read or wrote
these `ExecutionProps` APIs should remove that usage (and, if still needed, use
the `PhysicalPlanningContext` equivalents).
##########
datafusion/expr/src/physical_planning_context.rs:
##########
@@ -70,6 +84,27 @@ impl PhysicalPlanningContext {
pub fn results(&self) -> &ScalarSubqueryResults {
&self.results
}
+
+ /// Adds a mapping for each variable to the given qualifier. Existing
+ /// variables with conflicting names get shadowed
Review Comment:
The rustdoc sentence is missing terminal punctuation and reads grammatically
incomplete. Consider changing to a complete sentence (e.g., end with a period
and use “are shadowed”).
##########
datafusion/physical-expr/src/planner.rs:
##########
@@ -612,15 +614,15 @@ pub fn create_physical_expr(
input_dfschema.metadata().clone(),
)?;
- let execution_props = execution_props
+ let planning_ctx = planning_ctx
.clone()
.with_qualified_lambda_variables(&qualifier,
&lambda.params);
Review Comment:
`planning_ctx.clone()` will clone the entire `lambda_variable_qualifier:
HashMap<...>` each time a lambda body is planned (even though `indexes` is now
cheap via `Arc`). If nested lambdas or wide scopes are common, this can become
O(n) per nesting level. Consider representing lambda-variable qualifiers as a
scope stack (e.g., persistent structure / parent pointer) so adding a new scope
is O(k) for new params while lookups walk outward, avoiding full-map clones.
--
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]
