alamb commented on code in PR #8331:
URL: https://github.com/apache/arrow-datafusion/pull/8331#discussion_r1406858141
##########
datafusion/expr/src/type_coercion/binary.rs:
##########
@@ -353,6 +354,20 @@ fn string_temporal_coercion(
}
}
+/// Coerce `Boolean` to other larger types, like Numeric as `1` or String as
"1"
+fn bool_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType>
{
+ match (lhs_type, rhs_type) {
Review Comment:
I think this also needs to check when `rhs_type` is a `DataType::Boolean` as
well.
I would expect both of the following queries to work and return the same
thing
```
sDataFusion CLI v33.0.0
❯ select 1 = true;
Error during planning: Cannot infer common argument type for comparison
operation Int64 = Boolean
❯ select true = 1;
+--------------------------+
| Boolean(true) = Int64(1) |
+--------------------------+
| true |
+--------------------------+
1 row in set. Query took 0.006 seconds.
```
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -557,10 +557,16 @@ select column1[0:5], column2[0:3], column3[0:9] from
arrays;
## make_array (aliases: `make_list`)
# make_array scalar function #1
-query ???
-select make_array(1, 2, 3), make_array(1.0, 2.0, 3.0), make_array('h', 'e',
'l', 'l', 'o');
-----
-[1, 2, 3] [1.0, 2.0, 3.0] [h, e, l, l, o]
+query ??????
Review Comment:
Can you also add some tests for other uses of this logic (not just in
make_array) such as comparisons
I notice that postgres doesn't handle boolean coercion
```
postgres=# select true = 1;
ERROR: operator does not exist: boolean = integer
LINE 1: select true = 1;
^
HINT: No operator matches the given name and argument types. You might need
to add explicit type casts.
```
However, after this PR datafusion does:
```sql
DataFusion CLI v33.0.0
❯ select true = 1;
+--------------------------+
| Boolean(true) = Int64(1) |
+--------------------------+
| true |
+--------------------------+
1 row in set. Query took 0.006 seconds.
```
##########
datafusion/expr/src/type_coercion/binary.rs:
##########
@@ -353,6 +354,20 @@ fn string_temporal_coercion(
}
}
+/// Coerce `Boolean` to other larger types, like Numeric as `1` or String as
"1"
+fn bool_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType>
{
+ match (lhs_type, rhs_type) {
Review Comment:
I think this also needs to check when `rhs_type` is a `DataType::Boolean` as
well.
I would expect both of the following queries to work and return the same
thing
```
sDataFusion CLI v33.0.0
❯ select 1 = true;
Error during planning: Cannot infer common argument type for comparison
operation Int64 = Boolean
❯ select true = 1;
+--------------------------+
| Boolean(true) = Int64(1) |
+--------------------------+
| true |
+--------------------------+
1 row in set. Query took 0.006 seconds.
```
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -557,10 +557,16 @@ select column1[0:5], column2[0:3], column3[0:9] from
arrays;
## make_array (aliases: `make_list`)
# make_array scalar function #1
-query ???
-select make_array(1, 2, 3), make_array(1.0, 2.0, 3.0), make_array('h', 'e',
'l', 'l', 'o');
-----
-[1, 2, 3] [1.0, 2.0, 3.0] [h, e, l, l, o]
+query ??????
Review Comment:
Can you also add some tests for other uses of this logic (not just in
make_array) such as comparisons
I notice that postgres doesn't handle boolean coercion
```
postgres=# select true = 1;
ERROR: operator does not exist: boolean = integer
LINE 1: select true = 1;
^
HINT: No operator matches the given name and argument types. You might need
to add explicit type casts.
```
However, after this PR datafusion does:
```sql
DataFusion CLI v33.0.0
❯ select true = 1;
+--------------------------+
| Boolean(true) = Int64(1) |
+--------------------------+
| true |
+--------------------------+
1 row in set. Query took 0.006 seconds.
```
--
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]
