peter-toth commented on code in PR #10356:
URL: https://github.com/apache/datafusion/pull/10356#discussion_r1589043927
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -99,25 +102,75 @@ fn analyze_internal(
// select t2.c2 from t1 where t1.c1 in (select t2.c1 from t2 where
t2.c2=t1.c3)
schema.merge(external_schema);
- let mut expr_rewrite = TypeCoercionRewriter { schema: &schema };
-
- let new_expr = plan
- .expressions()
- .into_iter()
- .map(|expr| {
- // ensure aggregate names don't change:
- // https://github.com/apache/datafusion/issues/3555
- rewrite_preserving_name(expr, &mut expr_rewrite)
- })
- .collect::<Result<Vec<_>>>()?;
-
- plan.with_new_exprs(new_expr, new_inputs)
+ let mut expr_rewrite = TypeCoercionRewriter::new(&schema);
+
+ let name_preserver = NamePreserver::new(&plan);
+ // apply coercion rewrite all expressions in the plan indivdually
+ plan.map_expressions(|expr| {
+ let original_name = name_preserver.save(&expr)?;
+ expr.rewrite(&mut expr_rewrite)?
+ .map_data(|expr| original_name.restore(expr))
+ })?
+ // coerce join expressions specially
+ .map_data(|plan| expr_rewrite.coerce_joins(plan))?
+ // recompute the schema after the expressions have been rewritten as the
types may have changed
+ .map_data(|plan| plan.recompute_schema())
Review Comment:
Ah ok, it seems there are many unnecessary `Transformed::yes`s in the
current code. But false positive `transformed`s doesn't cause any issue...
Sure, a follow-up PR sounds good, I agree that this PR already looks really
nice!
--
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]
