alamb commented on code in PR #8988:
URL: https://github.com/apache/arrow-datafusion/pull/8988#discussion_r1466825788


##########
datafusion/physical-expr/src/string_expressions.rs:
##########
@@ -570,6 +570,48 @@ pub fn uuid(args: &[ColumnarValue]) -> 
Result<ColumnarValue> {
     let array = GenericStringArray::<i32>::from_iter_values(values);
     Ok(ColumnarValue::Array(Arc::new(array)))
 }
+/// position function, similar logic as instr

Review Comment:
   Is there any difference to `instr`? If not maybe we can reuse the instr 
function (see my comment below)



##########
datafusion/sql/src/expr/mod.rs:
##########
@@ -514,7 +514,9 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
             SQLExpr::Struct { values, fields } => {
                 self.parse_struct(values, fields, schema, planner_context)
             }
-
+            SQLExpr::Position { expr, r#in } => {

Review Comment:
   👍 



##########
datafusion/sql/src/expr/mod.rs:
##########
@@ -704,7 +706,20 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
         };
         Ok(Expr::ScalarFunction(ScalarFunction::new(fun, args)))
     }
-
+    fn sql_position_to_expr(
+        &self,
+        substr_expr: SQLExpr,
+        str_expr: SQLExpr,
+        schema: &DFSchema,
+        planner_context: &mut PlannerContext,
+    ) -> Result<Expr> {
+        let fun = BuiltinScalarFunction::Position;

Review Comment:
   I wonder would it be possible here to reuse the existing 
`BuiltinScalarFunction::Instr` rather than creating a new function for 
`position`?



-- 
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]

Reply via email to