-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for regexpReplace scalar function #9123
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9123 +/- ##
============================================
+ Coverage 61.23% 63.51% +2.28%
+ Complexity 4842 4735 -107
============================================
Files 1836 1799 -37
Lines 97524 95732 -1792
Branches 14703 14497 -206
============================================
+ Hits 59716 60806 +1090
+ Misses 33342 30542 -2800
+ Partials 4466 4384 -82
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
1db45f7
to
0f6e104
Compare
0f6e104
to
8aa670d
Compare
@siddharthteotia @Jackie-Jiang please take a look and provide feedback. Thanks! |
8aa670d
to
87a5086
Compare
@vvivekiyer - can you rebase this once and also look at test failures ? |
8760195
to
4b44076
Compare
@siddharthteotia Rebased and reran the tests. Please review. |
* @return replaced input string | ||
*/ | ||
@ScalarFunction | ||
public static String regexpReplace(String inputStr, String matchStr, String replaceStr, int startPos, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) startPos -> matchStartPos ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
patternFlag = Pattern.MULTILINE; | ||
break; | ||
case "x": | ||
patternFlag = Pattern.COMMENTS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow the purpose / use of this flag. Can you please elaborate ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest not handling m and x now. We can add them later and keep it simple for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed m and x.
0aab909
to
1fb6ffe
Compare
1fb6ffe
to
ece856e
Compare
@vvivekiyer - can you please rebase on top of #9114 |
ece856e
to
655b1b4
Compare
@siddharthteotia rebased and all tests are passing now. |
@vvivekiyer - let's please add user docs to gitbook |
Not specific to this PR, but suggest using draft to mark the WIP PR instead of putting it as the title. |
@Jackie-Jiang Sure, makes sense. |
Fixes issue = #9079
Label =
feature
This PR adds a scalar function to perform REGEXP REPLACE in Pinot. The semantics of using this scalar function is as follows:
occurence
parameter is not -1.flag
parameterParameters:
We can support more flags in the future.