alamb commented on code in PR #5820:
URL: https://github.com/apache/arrow-datafusion/pull/5820#discussion_r1155092312
##########
datafusion/optimizer/src/type_coercion.rs:
##########
@@ -330,8 +330,35 @@ impl TreeNodeRewriter for TypeCoercionRewriter {
}
}
Expr::Case(case) => {
- // all the result of then and else should be convert to a
common data type,
- // if they can be coercible to a common data type, return
error.
+ // Given expressions like:
+ //
+ // CASE a1
+ // WHEN a2 THEN b1
+ // WHEN a3 THEN b2
+ // ELSE b3
+ // END
+ //
+ // or:
+ //
+ // CASE
+ // WHEN x1 THEN b1
+ // WHEN x2 THEN b2
+ // ELSE b3
+ // END
+ //
+ // Then all aN (a1, a2, a3) must be converted to a common data
type in the first example
Review Comment:
❤️
##########
datafusion/core/tests/sqllogictests/test_files/select.slt:
##########
@@ -214,3 +214,9 @@ select * from (select 1 a union all select 2) b order by a
limit null;
query I
select * from (select 1 a union all select 2) b order by a limit 0;
----
+
+# select case when type coercion
+query I
+select CASE 10.5 WHEN 0 THEN 1 ELSE 10 END as col;
+----
+10
Review Comment:
Could you add some additional tests?
1. Alternate Syntax:
```
select CASE
WHEN 10 = 5 THEN 1
WHEN 'true' THEN 1
ELSE 10
END as col;
```
Also negative cases like
```sql
select CASE 10.5 WHEN 'not a number' THEN 1 ELSE 10 END as col;
```
```sql
select CASE
WHEN 10 = 5 THEN 1
WHEN 'not boolean' THEN 1
ELSE 10
END as col;
```
```sql
select CASE
WHEN 10 = 5 THEN 1
WHEN 5 THEN 'not a number'
ELSE 10
END as col;
```
```sql
select CASE
WHEN 10 = 5 THEN 1
WHEN 5 THEN 0
ELSE 'goo'
END as col;
```
##########
datafusion/optimizer/src/type_coercion.rs:
##########
@@ -330,8 +330,35 @@ impl TreeNodeRewriter for TypeCoercionRewriter {
}
}
Expr::Case(case) => {
- // all the result of then and else should be convert to a
common data type,
- // if they can be coercible to a common data type, return
error.
+ // Given expressions like:
+ //
+ // CASE a1
+ // WHEN a2 THEN b1
+ // WHEN a3 THEN b2
+ // ELSE b3
+ // END
+ //
+ // or:
+ //
+ // CASE
+ // WHEN x1 THEN b1
+ // WHEN x2 THEN b2
+ // ELSE b3
+ // END
+ //
+ // Then all aN (a1, a2, a3) must be converted to a common data
type in the first example
+ // (case-when expression coercion)
+ //
+ // And all bN (b1, b2, b3) must be converted to a common data
type in both examples
+ // (then-else expression coercion)
+ //
+ // If either fail to find a common data type, will return error
+
+ // prepare types
+ let case_type = match &case.expr {
+ None => Ok(None),
+ Some(expr) => expr.get_type(&self.schema).map(Some),
+ }?;
Review Comment:
You can write this more concisely / functionally using `transpose` like this
if you want
```suggestion
// prepare types
let case_type = case.expr.as_ref()
.map(|expr| expr.get_type(&self.schema))
.transpose()?;
```
The same basic transformation applies to the other `match` statements below
-- I don't think it is a big deal I just figured I would point it out to you
--
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]
