Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add regexp_position and regexp_count functions #2136

Merged
merged 1 commit into from Dec 30, 2019
Merged

Add regexp_position and regexp_count functions #2136

merged 1 commit into from Dec 30, 2019

Conversation

ssquan
Copy link
Contributor

@ssquan ssquan commented Nov 28, 2019 •
edited by ebyhr

Fixes #2133

function list:
three regexp_instr methods
one regexp_count method

@cla-bot
Copy link

cla-bot bot commented Nov 28, 2019

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: unknown.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@ebyhr
Copy link
Member

ebyhr commented Nov 29, 2019

Thanks for creating this PR.

Could you confirm a commit author and squash commits into one? Also, I would recommend updating AbstractTestRegexpFunctions.java and regexp.rst.

If you need help, you can join our community Slack. https://prestosql.io/slack.html

@cla-bot
Copy link

cla-bot bot commented Nov 29, 2019

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: unknown.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot cla-bot bot added the cla-signed label Nov 29, 2019
@ssquan ssquan force-pushed the regexp_functions branch 2 times, most recently from c0d17a0 to acd0ee5 Compare November 30, 2019 08:01
@ssquan
Copy link
Contributor Author

ssquan commented Dec 6, 2019

Can anyone help me review this PR?

@findepi findepi requested a review from electrum December 6, 2019 13:45
@ssquan ssquan force-pushed the regexp_functions branch 2 times, most recently from 5ca4783 to 6e235b5 Compare December 7, 2019 07:38
@ebyhr ebyhr changed the title Regexp functions: regexp_count and regexp_instr Add regexp_position and regexp_count functions Dec 10, 2019
sopel39
sopel39 reviewed Dec 11, 2019
View reviewed changes
presto-docs/src/main/sphinx/functions/regexp.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/functions/regexp.rst Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/Re2JRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/Re2JRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/AbstractTestRegexpFunctions.java Show resolved Hide resolved
@ssquan ssquan force-pushed the regexp_functions branch 3 times, most recently from 6ea75df to cbaf23c Compare December 12, 2019 15:20
@sopel39
Copy link
Member

sopel39 commented Dec 13, 2019

@ssquan please click re-review button when you are ready with changes

sopel39
sopel39 reviewed Dec 17, 2019
View reviewed changes
presto-docs/src/main/sphinx/functions/regexp.rst Outdated
.. function:: regexp_position(string, pattern) -> integer

Returns the index of the first occurrence of ``pattern`` in ``string``.
Returns -1 if not found, otherwise returns index counting from 1.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martint could you check this function contract?

presto-docs/src/main/sphinx/functions/regexp.rst Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
@ssquan ssquan requested a review from sopel39 December 17, 2019 16:48
@ssquan ssquan force-pushed the regexp_functions branch 2 times, most recently from 3526f64 to a34a895 Compare December 19, 2019 02:33
sopel39
sopel39 reviewed Dec 19, 2019
View reviewed changes
presto-docs/src/main/sphinx/functions/regexp.rst Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/Re2JRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/Re2JRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/AbstractTestRegexpFunctions.java Outdated Show resolved Hide resolved
@ssquan ssquan force-pushed the regexp_functions branch 5 times, most recently from 1bd7dce to 7ffeb23 Compare December 20, 2019 08:02
@ssquan ssquan requested a review from sopel39 December 20, 2019 10:11
@ssquan ssquan force-pushed the regexp_functions branch 2 times, most recently from 6cb8dd2 to d0fc396 Compare December 21, 2019 06:08
@ssquan ssquan requested a review from electrum December 21, 2019 12:36
@sopel39
Copy link
Member

sopel39 commented Dec 23, 2019

Don't use merge commit, just rebase

sopel39
sopel39 reviewed Dec 23, 2019
View reviewed changes
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments

presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/type/JoniRegexp.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/Re2JRegexpFunctions.java Show resolved Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/AbstractTestRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/AbstractTestRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/AbstractTestRegexpFunctions.java Show resolved Hide resolved
@ssquan ssquan force-pushed the regexp_functions branch 2 times, most recently from 00eb236 to 24777e2 Compare December 23, 2019 12:34
@ssquan ssquan requested a review from sopel39 December 23, 2019 13:49
sopel39
sopel39 approved these changes Dec 27, 2019
View reviewed changes
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some minor comments

presto-main/src/main/java/io/prestosql/operator/scalar/JoniRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/AbstractTestRegexpFunctions.java Outdated Show resolved Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/AbstractTestRegexpFunctions.java Show resolved Hide resolved
@sopel39 sopel39 merged commit 2718bf7 into trinodb:master Dec 30, 2019
@sopel39
Copy link
Member

sopel39 commented Dec 30, 2019

Merged, thanks!

@sopel39 sopel39 mentioned this pull request Dec 30, 2019
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sopel39 sopel39 sopel39 approved these changes

@ebyhr ebyhr Awaiting requested review from ebyhr

@electrum electrum Awaiting requested review from electrum

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add regexp_count and regexp_count functions
4 participants
@ssquan @ebyhr @sopel39 @electrum