yjshen commented on code in PR #5911:
URL: https://github.com/apache/arrow-datafusion/pull/5911#discussion_r1168344133
##########
datafusion/physical-expr/src/functions.rs:
##########
@@ -64,6 +65,22 @@ pub fn create_physical_expr(
let data_type = function::return_type(fun, &input_expr_types)?;
+ // if function will be converted to case expression
+ if let BuiltinScalarFunction::If = fun {
+ if input_phy_exprs.len() != 3 {
+ return Err(DataFusionError::Internal(format!(
+ "{:?} args were supplied but IF function takes exactly 3 args",
+ input_phy_exprs.len(),
+ )));
+ }
+ let (when_expr, then_expr, else_expr) = (
+ Arc::clone(&input_phy_exprs[0]),
+ Arc::clone(&input_phy_exprs[1]),
+ Arc::clone(&input_phy_exprs[2]),
+ );
+ return case(None, vec![(when_expr, then_expr)], Some(else_expr));
Review Comment:
I would like to transfer the conversion code from its current location
(where the physical expression was created) to a new analyzer rule. The new
rule should happen before the `TypeCoercion` rule so that `expr[1]`, `expr[2]`
can be correctly type coerced; by doing this, your test against Int32 and Int64
could be cast into common types. Cc @jackwener for expert opinion.
For example:
```rust
impl Analyzer {
/// Create a new analyzer using the recommended list of rules
pub fn new() -> Self {
let rules: Vec<Arc<dyn AnalyzerRule + Send + Sync>> = vec![
Arc::new(InlineTableScan::new()),
Arc::new(RewriteIfExpr::new()), // rule that rewrite if expr
Arc::new(TypeCoercion::new()),
Arc::new(CountWildcardRule::new()),
];
Self::with_rules(rules)
}
```
##########
datafusion/core/tests/sql/functions.rs:
##########
@@ -565,3 +565,119 @@ async fn test_power() -> Result<()> {
Ok(())
}
+
+#[tokio::test]
+async fn if_static_value() -> Result<()> {
+ let ctx = SessionContext::new();
+ let sql = "SELECT IF(true, 1, 2)";
+ let actual = execute_to_batches(&ctx, sql).await;
+ let expected = vec![
+ "+-------------------------------------+",
+ "| if(Boolean(true),Int64(1),Int64(2)) |",
+ "+-------------------------------------+",
+ "| 1 |",
+ "+-------------------------------------+",
+ ];
+ assert_batches_eq!(expected, &actual);
+
+ let sql = "SELECT IF(false, 1, 2)";
+ let actual = execute_to_batches(&ctx, sql).await;
+ let expected = vec![
+ "+--------------------------------------+",
+ "| if(Boolean(false),Int64(1),Int64(2)) |",
+ "+--------------------------------------+",
+ "| 2 |",
+ "+--------------------------------------+",
+ ];
+ assert_batches_eq!(expected, &actual);
+
+ let sql = "SELECT IF(1 = 2, 1 + 5, 2 + 4)";
Review Comment:
Shall we make it `1+4` and `2+4`?
--
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]
