Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-06-23 Thread via GitHub 


github-actions[bot] closed pull request #15482: fix!: incorrect coercion when 
comparing with string literals
URL: https://github.com/apache/datafusion/pull/15482


-- 
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]

Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-06-15 Thread via GitHub 


github-actions[bot] commented on PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#issuecomment-2974908124

   Thank you for your contribution. Unfortunately, this pull request is stale 
because it has been open 60 days with no activity. Please remove the stale 
label or comment or this will be closed in 7 days.


-- 
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]

Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-04-10 Thread via GitHub 


alamb commented on code in PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#discussion_r2038079988


##
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 = '999';
 

Review Comment:
   (in case anyone else find this, you can get the full plan via `EXPLAIN 
FORMAT INDENT ...`



-- 
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]

Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-04-09 Thread via GitHub 


alan910127 commented on code in PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#discussion_r2029954233


##
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 = '999';
 

Review Comment:
   EDIT: `EXPLAIN` works (only outputs physical plan) in the latest main 
branch, but I'm not sure if this behavior is expected :thinking:
   
   ```sql
   > create table t as select CAST(123 AS int) a;
   0 row(s) fetched.
   Elapsed 0.021 seconds.
   
   > select * from t where a = '99';
   type_coercion
   caused by
   Error during planning: Cannot coerce '99' to type 'Int32'
   > explain select * from t where a = '99';
   +---+---+
   | plan_type | plan  |
   +---+---+
   | physical_plan | ┌───┐ |
   |   | │CoalesceBatchesExec│ |
   |   | │   │ |
   |   | │ target_batch_size:│ |
   |   | │8192   │ |
   |   | └─┬─┘ |
   |   | ┌─┴─┐ |
   |   | │ FilterExec│ |
   |   | │   │ |
   |   | │ predicate:│ |
   |   | │   a = 99  │ |
   |   | └─┬─┘ |
   |   | ┌─┴─┐ |
   |   | │   DataSourceExec  │ |
   |   | │   │ |
   |   | │ bytes: 160│ |
   |   | │   format: memory  │ |
   |   | │  rows: 1  │ |
   |   | └───┘ |
   |   |   |
   +---+---+
   ```
   
   > Perhaps we can file a ticket to track it
   
   ~Issue created: #15598~



-- 
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]

Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-04-08 Thread via GitHub 


alan910127 commented on code in PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#discussion_r2029954233


##
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 = '999';
 

Review Comment:
   EDIT: @alamb `EXPLAIN` works (only outputs physical plan) in the latest main 
branch, but I'm not sure if this behavior is expected :thinking:
   
   ```sql
   > create table t as select CAST(123 AS int) a;
   0 row(s) fetched.
   Elapsed 0.021 seconds.
   
   > select * from t where a = '99';
   type_coercion
   caused by
   Error during planning: Cannot coerce '99' to type 'Int32'
   > explain select * from t where a = '99';
   +---+---+
   | plan_type | plan  |
   +---+---+
   | physical_plan | ┌───┐ |
   |   | │CoalesceBatchesExec│ |
   |   | │   │ |
   |   | │ target_batch_size:│ |
   |   | │8192   │ |
   |   | └─┬─┘ |
   |   | ┌─┴─┐ |
   |   | │ FilterExec│ |
   |   | │   │ |
   |   | │ predicate:│ |
   |   | │   a = 99  │ |
   |   | └─┬─┘ |
   |   | ┌─┴─┐ |
   |   | │   DataSourceExec  │ |
   |   | │   │ |
   |   | │ bytes: 160│ |
   |   | │   format: memory  │ |
   |   | │  rows: 1  │ |
   |   | └───┘ |
   |   |   |
   +---+---+
   ```
   
   > Perhaps we can file a ticket to track it
   
   ~Issue created: #15598~



-- 
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]

Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-04-05 Thread via GitHub 


alan910127 commented on code in PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#discussion_r2029961032


##
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 think both are valid -- duckdb supports something like `select int_col = 
'12'||'3'`, while postgres returns an error since it only treats string 
literals as `unknown` type (thus the filter was `int = text` => error). 
However, I think supporting expressions makes more sense semantically (I 
believe we don't have a concept like the `unknown` type?).



-- 
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]

Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-04-05 Thread via GitHub 


alan910127 commented on code in PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#discussion_r2029954233


##
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 = '999';
 

Review Comment:
   EDIT: `EXPLAIN` works in the latest main branch, but I'm not sure if this 
behavior is expected :thinking:
   
   ```sql
   > create table t as select CAST(123 AS int) a;
   0 row(s) fetched.
   Elapsed 0.021 seconds.
   
   > select * from t where a = '99';
   type_coercion
   caused by
   Error during planning: Cannot coerce '99' to type 'Int32'
   > explain select * from t where a = '99';
   +---+---+
   | plan_type | plan  |
   +---+---+
   | physical_plan | ┌───┐ |
   |   | │CoalesceBatchesExec│ |
   |   | │   │ |
   |   | │ target_batch_size:│ |
   |   | │8192   │ |
   |   | └─┬─┘ |
   |   | ┌─┴─┐ |
   |   | │ FilterExec│ |
   |   | │   │ |
   |   | │ predicate:│ |
   |   | │   a = 99  │ |
   |   | └─┬─┘ |
   |   | ┌─┴─┐ |
   |   | │   DataSourceExec  │ |
   |   | │   │ |
   |   | │ bytes: 160│ |
   |   | │   format: memory  │ |
   |   | │  rows: 1  │ |
   |   | └───┘ |
   |   |   |
   +---+---+
   ```
   
   > Perhaps we can file a ticket to track it
   
   ~Issue created: #15598~



-- 
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]

Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-04-05 Thread via GitHub 


alan910127 commented on code in PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#discussion_r2029954233


##
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 = '999';
 

Review Comment:
   > Perhaps we can file a ticket to track it
   
   Issue created: #15598 



-- 
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]

Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-04-05 Thread via GitHub 


gabotechs commented on code in PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#discussion_r2024939228


##
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 = '999';
 

Review Comment:
   I imagine that instead of returning an empty plan, we should be expecting a 
runtime error here, something like
   ```suggestion
   statement error Cannot coerce '...' to type '...'
   explain select a from t where a = '999';
   ```
   I do not see any instance in the sql logic tests that are actually expecting 
an empty plan upon an error.



##
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 = '999';
 
-logical_plan TableScan: t projection=[a], full_filters=[CAST(t.a AS Utf8) = 
Utf8("999")]
+

Review Comment:
   I can confirm that Postgres is also not able to plan for this query for the 
same reason: [link to Postgres 
fiddle](https://www.db-fiddle.com/f/4jyoMCicNSZpjMt4jFYoz5/16528)



-- 
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]

Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-04-04 Thread via GitHub 


alan910127 commented on code in PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#discussion_r2025222514


##
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 = '999';
 

Review Comment:
   I think this is likely an EXPLAIN-related issue, but returning an error when 
no plan could be generated seems like a reasonable approach. I'm not entirely 
sure why it's implemented this way, maybe we should call for help :laughing:



-- 
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]

Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-04-03 Thread via GitHub 


alamb commented on code in PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#discussion_r202663


##
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 = '999';
 
-logical_plan TableScan: t projection=[a], full_filters=[CAST(t.a AS Utf8) = 
Utf8("999")]
+

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 = '999';
 
-logical_plan TableScan: t projection=[a], full_filters=[CAST(t.a AS Utf8) = 
Utf8("999")]
+
 
 # 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 = '999';
 

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 = '999';
 

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]

Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-03-29 Thread via GitHub 


alan910127 commented on code in PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#discussion_r2019692309


##
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 = '999';
 
-logical_plan TableScan: t projection=[a], full_filters=[CAST(t.a AS Utf8) = 
Utf8("999")]
+

Review Comment:
   I thought it's because it returnd a `plan_err!()` when the cast fails, but 
I'm not quite sure about that.



-- 
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]

Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-03-28 Thread via GitHub 


alan910127 commented on code in PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#discussion_r2019692309


##
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 = '999';
 
-logical_plan TableScan: t projection=[a], full_filters=[CAST(t.a AS Utf8) = 
Utf8("999")]
+

Review Comment:
   I thought it's because it returns a `plan_err!()` when the cast fails, but 
I'm not quite sure about that.



-- 
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]

Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-03-28 Thread via GitHub 


jayzhan211 commented on code in PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#discussion_r2019668440


##
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 = '999';
 
-logical_plan TableScan: t projection=[a], full_filters=[CAST(t.a AS Utf8) = 
Utf8("999")]
+

Review Comment:
   why is there no plan?



-- 
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]

Re: [PR] fix!: incorrect coercion when comparing with string literals [datafusion]

2025-03-28 Thread via GitHub 


alan910127 commented on code in PR #15482:
URL: https://github.com/apache/datafusion/pull/15482#discussion_r2019310584


##
datafusion/core/tests/expr_api/mod.rs:
##
@@ -330,12 +330,12 @@ async fn test_create_physical_expr_coercion() {
 create_expr_test(lit(1i32).eq(col("id")), "CAST(1 AS Utf8) = id@0");
 // compare int col to string literal `i = '202410'`
 // Note this casts the column (not the field)
-create_expr_test(col("i").eq(lit("202410")), "CAST(i@1 AS Utf8) = 202410");
-create_expr_test(lit("202410").eq(col("i")), "202410 = CAST(i@1 AS Utf8)");
+create_expr_test(col("i").eq(lit("202410")), "i@1 = 202410");
+create_expr_test(lit("202410").eq(col("i")), "202410 = i@1");
 // however, when simplified the casts on i should removed
 // https://github.com/apache/datafusion/issues/14944
-create_simplified_expr_test(col("i").eq(lit("202410")), "CAST(i@1 AS Utf8) 
= 202410");
-create_simplified_expr_test(lit("202410").eq(col("i")), "CAST(i@1 AS Utf8) 
= 202410");
+create_simplified_expr_test(col("i").eq(lit("202410")), "i@1 = 202410");
+create_simplified_expr_test(lit("202410").eq(col("i")), "i@1 = 202410");

Review Comment:
   not sure if this test is still needed since the literal casting behavior is 
not considered an "optimization"



-- 
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]