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


##########
datafusion/substrait/src/logical_plan/consumer/expr/scalar_function.rs:
##########
@@ -41,12 +41,24 @@ pub async fn from_scalar_function(
             f.function_reference
         );
     };
+
     let fn_name = substrait_fun_name(fn_signature);
     let args = from_substrait_func_args(consumer, &f.arguments, 
input_schema).await?;
 
+    let udf_func = match consumer.get_function_registry().udf(fn_name) {
+        Ok(f) => Ok(f),
+        Err(e) => {
+            if let Some(alt_name) = substrait_to_df_name(fn_name) {
+                consumer.get_function_registry().udf(alt_name).or(Err(e))
+            } else {
+                Err(e)
+            }
+        }
+    };

Review Comment:
   You can write this kind of thing a little more "rust idomatically" (aka more 
functionally) with `and_then`, something like this
   
   ```suggestion
       let udf_func = consumer.get_function_registry().udf(fn_name).or_else(|e| 
{
           if let Some(alt_name) = substrait_to_df_name(fn_name) {
               consumer.get_function_registry().udf(alt_name).or(Err(e))
           } else {
               Err(e)
           }
       });
   ```
   
   
   However, I think this structure is also perfectly clear so no changes are 
needed



##########
datafusion/substrait/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -426,6 +426,34 @@ async fn simple_scalar_function_substr() -> Result<()> {
     roundtrip("SELECT SUBSTR(f, 1, 3) FROM data").await
 }
 
+#[tokio::test]
+// Test that DataFusion ISNAN function gets correctly mapped to Substrait 
"is_nan" in Substrait producer, and checks roundtrip comparison
+// Follows the same structure as existing roundtrip tests, but more explicitly 
tests for name mappings
+async fn scalar_function_with_diff_substrait_df_names() -> Result<()> {
+    let ctx = create_context().await?;
+    let df = ctx.sql("SELECT ISNAN(a) FROM data").await?;

Review Comment:
   Should this also be testing `signum` and `log` given they are also added?



-- 
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-unsubscr...@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to