Adding strrpos Presto function #2903
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
This pull request was exported from Phabricator. Differential Revision: D40527007 |
| const std::string_view string, | ||
| const std::string_view subString, | ||
| const size_t instance = 1) { | ||
| assert(instance > 0); |
There was a problem hiding this comment.
please use VELOX_CHECK_GT() instead of assert
| * must be a positive number. Positions start with 1. If not found, 0 is | ||
| * returned. | ||
| **/ | ||
| class StringPositionFromEnd : public exec::VectorFunction { |
There was a problem hiding this comment.
Why do we need a vector function? This can likely be written using the simple function API with the same level of performance.
There was a problem hiding this comment.
@pedroerp , this is a good question. Actually initially I implemented using simple API but then discovered strpos implemented in this way so I decided to mimick the behavior. I think if the agreement is that vector function does provide any added-value we can have a follow-up PR with switching all implementations that rely on it.
There was a problem hiding this comment.
I suspect that the reason is that the strpos function was written before the simple function API was in place. To avoid us from iterating and reviewing the vector code (which is more complicated and might end up not being needed), could we either converge into the simple function version, or write a small micro-benchmark to justify if the vectorized version actually has perf benefits?
There was a problem hiding this comment.
If we see that the simple function version is just as fast, we could also refactor the strpos function to simplify it.
There was a problem hiding this comment.
@pedroerp It seems like all functions in that file are implemented as VectorFunctions actually(at first glance hard to spot obvious perf gain). I'd prefer adding strrpos following the same pattern to have a consistent file and have a separate activity of assessing/moving all those to simple function API. As far as the review is concerned the actual implementation in StringCore is going to stay the same as well as the the test cases. We'll have to review only declaration part in the follow-up pr which should be pretty straightforward bulk activity.
|
This pull request was exported from Phabricator. Differential Revision: D40527007 |
gosharz
force-pushed
the
export-D40527007
branch
from
55ed4da
to
427acc1
Compare
…acebookincubator#2903) Summary: Pull Request resolved: facebookincubator#2903 Based on the usage and benchmark analysis: 1. Move strpos Presto function implementation to use Simple Function API 2. Add strrpos Presto function based on Simple Function API 3. Add test coverage Reviewed By: mbasmanova Differential Revision: D40527007 fbshipit-source-id: f4c897be39f51b951bd32e29e6be34b61f3a088d
…acebookincubator#2903) Summary: Pull Request resolved: facebookincubator#2903 Based on the usage and benchmark analysis: 1. Move strpos Presto function implementation to use Simple Function API 2. Add strrpos Presto function based on Simple Function API 3. Add test coverage Reviewed By: mbasmanova Differential Revision: D40527007 fbshipit-source-id: 70ad282fa8d18683e1e36db004acf38c1207fee2
gosharz
force-pushed
the
export-D40527007
branch
from
427acc1
to
e624204
Compare
|
This pull request was exported from Phabricator. Differential Revision: D40527007 |
Summary: Adding strrpos Presto function
Differential Revision: D40527007