gabotechs commented on code in PR #25418:
URL: https://github.com/apache/datafusion/pull/25418#discussion_r4045834392
##########
datafusion/physical-expr/src/planner.rs:
##########
@@ -667,27 +667,24 @@ pub fn create_physical_expr(
let schema_field = input_dfschema.field(index);
- // LambdaVariable.field will be made optional as in
Expr::Placeholder
- // and only LambdaVariable.name used, and field.name ignored,
- // so they're not enforced to match for logical expressions
- // Rename the field to match the schema one and use it's PartialEq
impl instead
- // of checking property by property and fail if new properties
get's added to it.
- // While not necessary, the sql planner does create lambda vars
with matching names,
- // so this shouldn't allocate with a lambda var from it
- let renamed_field = Arc::clone(field).renamed(name);
-
- if &renamed_field != schema_field {
- return plan_err!(
- "LambdaVariable field and schema field mismatch {} != {}",
- renamed_field,
- schema_field
+ let variable: Arc<dyn PhysicalExpr> = Arc::new(
+ expressions::LambdaVariable::new(index,
Arc::clone(schema_field)),
+ );
+
+ // The lambda body was coerced against `field`, while the array
bound at
+ // runtime has the type derived into the planning schema. Enforce
`field`
+ // with a cast, the way scalar function arguments are cast to
their coerced
+ // types, so a physical encoding such as a dictionary or a string
view does
+ // not have to match what the plan was built with.
+ if field.data_type() != schema_field.data_type() {
+ return expressions::cast(
+ variable,
+ input_dfschema.inner(),
+ field.data_type().clone(),
);
Review Comment:
This sounds like a type coercion concern, which is handled today by this
code:
https://github.com/apache/datafusion/blob/main/datafusion/optimizer/src/analyzer/type_coercion.rs#L176-L176
By doing this cast during physical planning, I imagine it'd be invisible to
logical plans, so people rendering the logical plan will miss that there's
going to be a cast here.
Have you considered placing this type coercion in the `TypeCoercion`
analyzer rule? or is this type mismatch something that somehow just becomes
visible at the physical plan level.
--
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]
