adriangb commented on code in PR #20269:
URL: https://github.com/apache/datafusion/pull/20269#discussion_r2888859227
##########
datafusion/physical-expr/src/expressions/cast_column.rs:
##########
@@ -177,6 +179,24 @@ impl PhysicalExpr for CastColumnExpr {
)))
}
+ /// A [`CastColumnExpr`] preserves the ordering of its child if the cast
is done
+ /// under the same datatype family.
Review Comment:
Do we have a high level SLT test for this? I'm afraid that as we refactor to
e.g. fold CastColumnExpr into CastExpr we'll loose this property (which seems
very valuable).
##########
datafusion/physical-expr/src/equivalence/properties/mod.rs:
##########
@@ -853,6 +853,18 @@ impl EquivalenceProperties {
sort_expr.options,
));
}
+ } else if let Some(cast_expr) =
+ r_expr.as_any().downcast_ref::<CastColumnExpr>()
Review Comment:
I think this is the unfortunate part of having two cast expressions: we have
to handle both in multiple places.
@paleolimbot this makes me think that even customizing casts with a cast
registry that chooses a different cast expression will be problematic. It
almost feels like we either need to (1) have a single `Cast` expression that
internally dispatches (so code snippets like this can match on a single
expression) or (2) have some way to determine "is this a cast expression?". To
complicate matters further: do all cast expressions share the same properties
e.g. preserving order
([see](https://github.com/apache/datafusion/pull/20269/changes#r2888859227))
--
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]
