feat:implement postgres style 'overlay' string function#8117
feat:implement postgres style 'overlay' string function#8117Ted-Jiang merged 13 commits intoapache:mainfrom
Conversation
github-actions
Bot
added
logical-expr
|
Thank you for your contribution! Could you move tests to sqllogictest, and add SQL reference? |
alamb
changed the title
I agree -- thank you @Syleechan |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you for this contribution @Syleechan - I agree with @2010YOUY01 that all this needs are some end to end sql tests (in sqllogictest): https://github.com/apache/arrow-datafusion/tree/main/datafusion/sqllogictest
# Conflicts: # datafusion/expr/src/built_in_function.rs # datafusion/physical-expr/src/functions.rs # datafusion/proto/proto/datafusion.proto # datafusion/proto/src/generated/pbjson.rs # datafusion/proto/src/generated/prost.rs # datafusion/proto/src/logical_plan/from_proto.rs
|
Thanks @2010YOUY01, @alamb . I have added the sqllogic test and sql reference. |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you for this contribution @Syleechan -- I think this is looking very nice
I had a few small suggestions, but they can also be done as follow on PRs if you prefer.
Thanks again!
| ('123', 'abc', 4, 5), | ||
| ('abcdefg', 'qwertyasdfg', 1, 7), | ||
| ('xyz', 'ijk', 1, 2), | ||
| ('Txxxxas', 'hom', 2, 4) |
There was a problem hiding this comment.
Can you please also test NULL handling here? I think your code handles it properly, but it would be good to have this in the SQL tests. We can do this as a follow on PR if you prefer.
Specifically
| ('Txxxxas', 'hom', 2, 4) | |
| ('Txxxxas', 'hom', 2, 4), | |
| (NULL, 'hom', 2, 4), -- expect NULL output | |
| ('Txxxxas', 'hom', NULL, 4), -- expect NULL output | |
| ('Txxxxas', 'hom', 2, NULL) -- expect NULL output |
There was a problem hiding this comment.
Have added these cases, thanks.
| - **str**: String expression to operate on. | ||
| - **substr**: the string to replace part of str. | ||
| - **pos**: the start position to replace of str. | ||
| - **count**: the count of characters to be replaced from start position of str.If not specified, will use substr length instead. |
There was a problem hiding this comment.
| - **count**: the count of characters to be replaced from start position of str.If not specified, will use substr length instead. | |
| - **count**: the count of characters to be replaced from start position of str. If not specified, will use substr length instead. |
There was a problem hiding this comment.
resolved, thanks.
| ``` | ||
| overlay(str PLACING substr FROM pos [FOR count]) | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Having the example made it much easier for me to see what was going on in postgres. Can we add an example here as well?
| For example, `overlay('Txxxxas' placing 'hom' from 2 for 4) → Thomas` |
There was a problem hiding this comment.
To resolve the conflict, I have added this, thanks
# Conflicts: # datafusion/proto/proto/datafusion.proto # datafusion/proto/src/generated/pbjson.rs # datafusion/proto/src/generated/prost.rs
Ted-Jiang
left a comment
Ted-Jiang
left a comment
There was a problem hiding this comment.
@Syleechan lgtm, thx, will merge this, some pr is depend on this,
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
This looks great -- thank you @Syleechan and @Ted-Jiang
4 participants
Which issue does this PR close?
https://www.postgresql.org/docs/current/functions-string.html#:~:text=overlay%20(%20string,4)%20%E2%86%92%20Thomas
Closes #.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?