nseekhao commented on code in PR #6840:
URL: https://github.com/apache/arrow-datafusion/pull/6840#discussion_r1254887499
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -67,8 +67,12 @@ use crate::variation_const::{
enum ScalarFunctionType {
Builtin(BuiltinScalarFunction),
Op(Operator),
- // logical negation
+ /// [Expr::Not]
Not,
+ /// [Expr::Like]
+ Like,
+ /// [Expr::ILike]
+ ILike,
Review Comment:
Might be more informative to add the something like:
```suggestion
/// [Expr::Like] Used for filtering rows based on the given wildcard
pattern. Case sensitive
Like,
/// [Expr::ILike] Case insensitive operator counterpart of `Like`
ILike,
```
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -104,7 +108,7 @@ pub fn name_to_op(name: &str) -> Result<Operator> {
}
}
-fn name_to_op_or_scalar_function(name: &str) -> Result<ScalarFunctionType> {
+fn scalar_function_or_expr(name: &str) -> Result<ScalarFunctionType> {
Review Comment:
Would a name like `scalar_function_type_from_str()` describe this function
better?
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -903,6 +904,36 @@ pub fn to_substrait_rex(
bounds,
))
}
+ Expr::Like(Like {
+ negated,
+ expr,
+ pattern,
+ escape_char,
+ }) => make_substrait_like_expr(
+ false,
+ *negated,
+ expr,
+ pattern,
+ *escape_char,
+ schema,
+ col_ref_offset,
+ extension_info,
+ ),
+ Expr::ILike(Like {
+ negated,
+ expr,
+ pattern,
+ escape_char,
+ }) => make_substrait_like_expr(
+ true,
+ *negated,
+ expr,
+ pattern,
+ *escape_char,
+ schema,
+ col_ref_offset,
Review Comment:
[For future improvement. **Not completely related to this PR**]
I think having to carry around `col_ref_offset` for any expression-related
functions unnecessarily overcrowds the code. Once we have `SubqueryAlias`
support implemented, this should not be necessary anymore. I'll refactor the
code when that happens.
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -1329,3 +1272,66 @@ 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>> {
+ if f.arguments.len() != 3 {
+ return Err(DataFusionError::NotImplemented(
+ "Expect three arguments for `LIKE` expr".to_string(),
+ ));
+ }
Review Comment:
```suggestion
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")
));
}
```
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -1329,3 +1272,66 @@ 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>> {
+ if f.arguments.len() != 3 {
+ return Err(DataFusionError::NotImplemented(
+ "Expect three arguments for `LIKE` expr".to_string(),
+ ));
+ }
+
+ let Some(ArgType::Value(expr_substrait)) = &f.arguments[0].arg_type else {
+ return Err(DataFusionError::NotImplemented(
+ "Invalid arguments type for `Like` expr".to_string()
Review Comment:
```suggestion
format!("Invalid arguments type for `{}` expr", fn_name)
```
--
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]
