-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add support for regexpReplace scalar function #9123
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
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. |
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.
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 =
featureThis PR adds a scalar function to perform REGEXP REPLACE in Pinot. The semantics of using this scalar function is as follows:
occurenceparameter is not -1.flagparameterParameters:
We can support more flags in the future.