alamb commented on code in PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#discussion_r2026999963
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -290,12 +290,31 @@ impl<'a> TypeCoercionRewriter<'a> {
right: Expr,
right_schema: &DFSchema,
) -> Result<(Expr, Expr)> {
- let (left_type, right_type) = BinaryTypeCoercer::new(
- &left.get_type(left_schema)?,
- &op,
- &right.get_type(right_schema)?,
- )
- .get_input_types()?;
+ let left_type = left.get_type(left_schema)?;
+ let right_type = right.get_type(right_schema)?;
+
+ match (&left, &right) {
Review Comment:
I am surprise this code is only triggered for literals -- I think coercion
is supposed to happen based on data types not on expressions. Among other
things, this code won't handle expressions (`date_col = '2025'||'-'||'02'` for
example)
I think we should change the base coercion rules for types
##########
datafusion/sqllogictest/test_files/push_down_filter.slt:
##########
@@ -230,19 +230,19 @@ logical_plan TableScan: t projection=[a],
full_filters=[t.a != Int32(100)]
query TT
explain select a from t where a = '99999999999';
----
-logical_plan TableScan: t projection=[a], full_filters=[CAST(t.a AS Utf8) =
Utf8("99999999999")]
+
Review Comment:
Let's file a ticket about the explain plan being missing
##########
datafusion/sqllogictest/test_files/push_down_filter.slt:
##########
@@ -230,19 +230,19 @@ logical_plan TableScan: t projection=[a],
full_filters=[t.a != Int32(100)]
query TT
explain select a from t where a = '99999999999';
----
-logical_plan TableScan: t projection=[a], full_filters=[CAST(t.a AS Utf8) =
Utf8("99999999999")]
+
# The predicate should still have the column cast when the value is a NOT
valid i32
query TT
explain select a from t where a = '99.99';
----
-logical_plan TableScan: t projection=[a], full_filters=[CAST(t.a AS Utf8) =
Utf8("99.99")]
Review Comment:
I think the comments in this file are now out of date -- so perhaps we can
update them
##########
datafusion/sqllogictest/test_files/push_down_filter.slt:
##########
@@ -230,19 +230,19 @@ logical_plan TableScan: t projection=[a],
full_filters=[t.a != Int32(100)]
query TT
explain select a from t where a = '99999999999';
----
Review Comment:
Yeah, it is kind of wierd -- it would be nice to have at least some message
in the explain plan if there was an error
Perhaps we can file a ticket to track it
I think we should update the test to just run the query (and expect an
error) rather than `EXPLAIN` it
##########
datafusion/sqllogictest/test_files/push_down_filter.slt:
##########
@@ -230,19 +230,19 @@ logical_plan TableScan: t projection=[a],
full_filters=[t.a != Int32(100)]
query TT
explain select a from t where a = '99999999999';
----
Review Comment:
BTW this is a new test added last week:
https://github.com/apache/datafusion/blame/190634bee1093d9d71786aa9c98ec207be05ea72/datafusion/sqllogictest/test_files/push_down_filter.slt#L231
- in https://github.com/apache/datafusion/pull/15110
--
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]
