nseekhao commented on code in PR #6840:
URL: https://github.com/apache/arrow-datafusion/pull/6840#discussion_r1258478050
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -819,116 +840,38 @@ pub async fn from_substrait_rex(
),
})))
}
- Ok(ScalarFunctionType::Builtin(fun)) => {
-
Ok(Arc::new(Expr::ScalarFunction(expr::ScalarFunction {
- fun,
- args: vec![
- from_substrait_rex(l, input_schema,
extensions)
- .await?
- .as_ref()
- .clone(),
- from_substrait_rex(r, input_schema,
extensions)
- .await?
- .as_ref()
- .clone(),
- ],
- })))
- }
- Ok(ScalarFunctionType::Not) => {
- Err(DataFusionError::NotImplemented(
- "Not expected function type: Not".to_string(),
- ))
- }
- Err(e) => Err(e),
- }
- }
- (l, r) => Err(DataFusionError::NotImplemented(format!(
- "Invalid arguments for binary expression: {l:?} and {r:?}"
- ))),
- },
- // ScalarFunction or Expr::Not
- 1 => {
- let fun = match extensions.get(&f.function_reference) {
- Some(fname) => scalar_function_or_not(fname),
- None => Err(DataFusionError::NotImplemented(format!(
- "Function not found: function reference = {:?}",
- f.function_reference
- ))),
- };
-
- match fun {
- Ok(ScalarFunctionType::Op(_)) => {
- Err(DataFusionError::NotImplemented(
- "Not expected function type: Op".to_string(),
- ))
- }
- Ok(scalar_function_type) => {
- match &f.arguments.first().unwrap().arg_type {
- Some(ArgType::Value(e)) => {
- let expr =
- from_substrait_rex(e, input_schema,
extensions)
- .await?
- .as_ref()
- .clone();
- match scalar_function_type {
- ScalarFunctionType::Builtin(fun) =>
Ok(Arc::new(
-
Expr::ScalarFunction(expr::ScalarFunction {
- fun,
- args: vec![expr],
- }),
- )),
- ScalarFunctionType::Not => {
- Ok(Arc::new(Expr::Not(Box::new(expr))))
- }
- _ => Err(DataFusionError::NotImplemented(
- "Invalid arguments for Not expression"
- .to_string(),
- )),
- }
- }
- _ => Err(DataFusionError::NotImplemented(
- "Invalid arguments for Not
expression".to_string(),
- )),
- }
+ (l, r) => Err(DataFusionError::NotImplemented(format!(
+ "Invalid arguments for binary expression: {l:?}
and {r:?}"
+ ))),
}
- Err(e) => Err(e),
}
- }
- // ScalarFunction
- _ => {
- let fun = match extensions.get(&f.function_reference) {
- Some(fname) => BuiltinScalarFunction::from_str(fname),
- None => Err(DataFusionError::NotImplemented(format!(
- "Aggregated function not found: function reference =
{:?}",
- f.function_reference
- ))),
- };
-
- let mut args: Vec<Expr> = vec![];
- for arg in f.arguments.iter() {
+ ScalarFunctionType::Not => {
+ let arg = f.arguments.first().ok_or_else(|| {
Review Comment:
Should we check that `f.arguments.len() == 1` is true, since
`first().ok_or_else()` would only give us an error if the vector is empty? Or
do we not care if there's more than one argument?
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -1342,3 +1285,67 @@ fn from_substrait_null(null_type: &Type) ->
Result<ScalarValue> {
))
}
}
+
+async fn make_datafusion_like(
+ case_insensitive: bool,
+ f: &ScalarFunction,
+ input_schema: &DFSchema,
+ extensions: &HashMap<u32, &String>,
+) -> Result<Arc<Expr>> {
+ let fn_name = if case_insensitive { "ILIKE" } else { "LIKE" };
+ if f.arguments.len() != 3 {
+ return Err(DataFusionError::NotImplemented(format!(
+ "Expect three arguments for `{fn_name}` expr"
+ )));
+ }
+
+ let Some(ArgType::Value(expr_substrait)) = &f.arguments[0].arg_type else {
+ return Err(DataFusionError::NotImplemented(
+ format!("Invalid arguments type for `{}` expr", fn_name)
+ ))
+ };
Review Comment:
I'm not sure if
```Rust
format!("Invalid arguments type for `{}` expr", fn_name)
```
or
```Rust
format!("Invalid arguments type for `{fn_name}` expr")
```
is preferred in Rust, but maybe we should choose one to be consistent here?
It may be confusing to the reader if different syntaxes are used to implement
the same semantic.
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -1342,3 +1285,67 @@ fn from_substrait_null(null_type: &Type) ->
Result<ScalarValue> {
))
}
}
+
+async fn make_datafusion_like(
+ case_insensitive: bool,
+ f: &ScalarFunction,
+ input_schema: &DFSchema,
+ extensions: &HashMap<u32, &String>,
+) -> Result<Arc<Expr>> {
+ let fn_name = if case_insensitive { "ILIKE" } else { "LIKE" };
+ if f.arguments.len() != 3 {
+ return Err(DataFusionError::NotImplemented(format!(
+ "Expect three arguments for `{fn_name}` expr"
+ )));
+ }
+
+ let Some(ArgType::Value(expr_substrait)) = &f.arguments[0].arg_type else {
+ return Err(DataFusionError::NotImplemented(
+ format!("Invalid arguments type for `{}` expr", fn_name)
+ ))
+ };
+ let expr = from_substrait_rex(expr_substrait, input_schema, extensions)
+ .await?
+ .as_ref()
+ .clone();
+ let Some(ArgType::Value(pattern_substrait)) = &f.arguments[1].arg_type
else {
+ return Err(DataFusionError::NotImplemented(
+ "Invalid arguments type for `Like` expr".to_string()
+ ))
+ };
+ let pattern = from_substrait_rex(pattern_substrait, input_schema,
extensions)
+ .await?
+ .as_ref()
+ .clone();
+ let Some(ArgType::Value(escape_char_substrait)) = &f.arguments[2].arg_type
else {
+ return Err(DataFusionError::NotImplemented(
+ "Invalid arguments type for `Like` expr".to_string()
Review Comment:
Same comment as above.
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -1342,3 +1285,67 @@ fn from_substrait_null(null_type: &Type) ->
Result<ScalarValue> {
))
}
}
+
+async fn make_datafusion_like(
+ case_insensitive: bool,
+ f: &ScalarFunction,
+ input_schema: &DFSchema,
+ extensions: &HashMap<u32, &String>,
+) -> Result<Arc<Expr>> {
+ let fn_name = if case_insensitive { "ILIKE" } else { "LIKE" };
+ if f.arguments.len() != 3 {
+ return Err(DataFusionError::NotImplemented(format!(
+ "Expect three arguments for `{fn_name}` expr"
+ )));
+ }
+
+ let Some(ArgType::Value(expr_substrait)) = &f.arguments[0].arg_type else {
+ return Err(DataFusionError::NotImplemented(
+ format!("Invalid arguments type for `{}` expr", fn_name)
+ ))
+ };
+ let expr = from_substrait_rex(expr_substrait, input_schema, extensions)
+ .await?
+ .as_ref()
+ .clone();
+ let Some(ArgType::Value(pattern_substrait)) = &f.arguments[1].arg_type
else {
+ return Err(DataFusionError::NotImplemented(
+ "Invalid arguments type for `Like` expr".to_string()
Review Comment:
Format with `fn_name` here in place of `Like` as well?
--
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]
