feat: issue #8969 adding position function #8988
Conversation
github-actions
bot
added
sql
Lordworms
force-pushed
the
issue_8969
branch
5 times, most recently
from
e63a87d to
2ef9087
Compare
There was a problem hiding this comment.
Thank you @Lordworms -- this is looking good and close. I wonder if we need a whole new function
| @@ -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 | |||
There was a problem hiding this comment.
Is there any difference to instr? If not maybe we can reuse the instr function (see my comment below)
| @@ -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 } => { | |||
datafusion/sql/src/expr/mod.rs
Outdated
| schema: &DFSchema, | ||
| planner_context: &mut PlannerContext, | ||
| ) -> Result<Expr> { | ||
| let fun = BuiltinScalarFunction::Position; |
There was a problem hiding this comment.
I wonder would it be possible here to reuse the existing BuiltinScalarFunction::Instr rather than creating a new function for position?
There was a problem hiding this comment.
I think there is no difference between the two functions, but I just browsed the previous implementation, each scalarbilutin function would have a corresponding implementation in string_expression, I don't know whether it is good to maintain the consistency or not. I can definitely reuse instr though.
There was a problem hiding this comment.
I think we should reuse Instr unless there is a compelling reason to have a new function
There was a problem hiding this comment.
OK, I will fix it soon
There was a problem hiding this comment.
I think if we changed this code to use BuiltinScalarFunction::Instr we could remove BuiltinScalarFunction::Position entirely which would make this PR much simpler and address @Weijun-H and my concerns about duplication
What do you think @Lordworms
There was a problem hiding this comment.
YES, I would do this right now, sorry I have been working on another issue
There was a problem hiding this comment.
YES, I would do this right now, sorry I have been working on another issue
No rush -- sorry I want to make it clear there is no pressure here. I just wanted to make sure your PR wasn't waiting on feedback (as I get anxious when there are so many PRs waiting on review)
Thank you for all your help
Lordworms
force-pushed
the
issue_8969
branch
2 times, most recently
from
911c4be to
48244df
Compare
| @@ -1498,6 +1505,7 @@ impl BuiltinScalarFunction { | |||
| BuiltinScalarFunction::Reverse => &["reverse"], | |||
| BuiltinScalarFunction::Right => &["right"], | |||
| BuiltinScalarFunction::Rpad => &["rpad"], | |||
| BuiltinScalarFunction::Position => &["position"], | |||
There was a problem hiding this comment.
To reduce duplication, please add position as an alias in the Instr field.
|
I think this PR is just waiting on implementing @Weijun-H 's suggestion to use a alias rather than a new function #8988 (comment) Marking as a draft to signal it isn't waiting on feedback. Please mark as ready for review when it is ready for another look |
| @@ -1487,7 +1487,7 @@ impl BuiltinScalarFunction { | |||
| BuiltinScalarFunction::Chr => &["chr"], | |||
| BuiltinScalarFunction::EndsWith => &["ends_with"], | |||
| BuiltinScalarFunction::InitCap => &["initcap"], | |||
| BuiltinScalarFunction::InStr => &["instr"], | |||
| BuiltinScalarFunction::InStr => &["instr", "position"], | |||
There was a problem hiding this comment.
Is it necessary to create a new file for in test 🤔 ?
There was a problem hiding this comment.
I don't think it is necessary, but I also don't think it hurts either.
There was a problem hiding this comment.
Thank you @Lordworms and @Weijun-H
I agree with @Weijun-H that it might make sense to consolidate the test with the other string functions https://github.com/apache/arrow-datafusion/pull/8988/files#r1475910565, perhaps one of you could make a follow on PR to do so.
But thank you for sticking with this @Lordworms 🏆
There was a problem hiding this comment.
I don't think it is necessary, but I also don't think it hurts either.
Which issue does this PR close?
#8969
Closes #.
Rationale for this change
What changes are included in this PR?
adding a new function position(str in fullstr)
Are these changes tested?
yes, but not fully tested
Are there any user-facing changes?