[GitHub] [arrow-datafusion] yjshen commented on a diff in pull request #5911: Add if function support

["yjshen (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org