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 interval source #1917

Merged
merged 2 commits into from
Feb 7, 2022
Merged

Add regexp interval source #1917

merged 2 commits into from
Feb 7, 2022

Conversation

mattweber
Copy link
Contributor

Description

Add a regexp interval source provider so people can use regular
expressions inside of intervals queries.

Issues Resolved

#1858

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@mattweber mattweber requested a review from a team as a code owner January 16, 2022 21:22
@opensearch-ci-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 8142d8419d894f444ae68fa235e14b4934bad4ca
Log 1946

Reports 1946

@mattweber
Copy link
Contributor Author

A comment on #1916 regarding bwc makes me wonder if I need to handle it here. In this case, a regexp source is not even possible on old versions. Do I even need to worry about it?

@reta
Copy link
Collaborator

reta commented Jan 17, 2022

A comment on #1916 regarding bwc makes me wonder if I need to handle it here. In this case, a regexp source is not even possible on old versions. Do I even need to worry about it?

I think it won't be an issue, the regexp type will be only supported by 2.x, so should never come from 1.x

@mattweber
Copy link
Contributor Author

@nknize can you please take a look at this when you have a chance?

@dblock dblock requested a review from andrross February 1, 2022 20:48
dblock
dblock requested changes Feb 1, 2022
View reviewed changes
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I see a license block with Elasticsearch, which is probably a copy-paste from another file. Please remove it.

Please also confirm that this code is not copied from Elasticsearch.

@mattweber
Copy link
Contributor Author

@dblock yes, just copy paste issue from WildcardIntervalsSourceProviderTests.java. There is no code copied from elasticsearch in this PR. I will get this updated.

@mattweber mattweber requested a review from dblock February 2, 2022 17:37
@mattweber
Add regexp interval source
e92ffb0 
Add a regexp interval source provider so people can use regular
expressions inside of intervals queries.

Signed-off-by: Matt Weber <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success de05cd3568ee08f6b72a010d44017fa6fee0bf24
Log 2177

Reports 2177

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure e92ffb0
Log 2180

Reports 2180

@mattweber
Copy link
Contributor Author

Unrelated failure, please rerun check.

@andrross andrross mentioned this pull request Feb 2, 2022
Closed
@andrross
Copy link
Member

andrross commented Feb 2, 2022

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success e92ffb0
Log 2185

Reports 2185

@mattweber
Fixes
a8a8417 
- register regexp interval in SearchModule
- use fully-qualified name for lucene RegExp
- get rid of unnecessary variable

Signed-off-by: Matt Weber <[email protected]>
@mattweber mattweber requested a review from andrross February 2, 2022 23:41
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure a8a8417
Log 2202

Reports 2202

@mattweber
Copy link
Contributor Author

another unrelated failure, please run check again

@mattweber
Copy link
Contributor Author

@andrross can you please take another look of this?

@dblock dblock mentioned this pull request Feb 7, 2022
Closed
@dblock
Copy link
Member

dblock commented Feb 7, 2022

For integ test failures that are intermittent, look for an existing issue on that, then if that doesn't exist, open one.

org.opensearch.repositories.s3.RepositoryS3ClientYamlTestSuiteIT > test {yaml=repository_s3/20_repository_permanent_credentials/Register a repository with a non existing client} FAILED
    java.lang.RuntimeException: Failure at [repository_s3/20_repository_permanent_credentials:7]: 60,000 milliseconds timeout on connection http-outgoing-3 [ACTIVE]
        at __randomizedtesting.SeedInfo.seed([25A4942AE204EF9D:ADF0ABF04CF88265]:0)

I opened #2064

@dblock
Copy link
Member

dblock commented Feb 7, 2022

start gradle check

@dblock
Copy link
Member

dblock commented Feb 7, 2022

Make sure to open an issue to document this feature in https://github.com/opensearch-project/documentation-website when this is merged, please.

@mattweber
Copy link
Contributor Author

Thanks @dblock, will do docs for this once everything is merged and hopefully backported.

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success a8a8417
Log 2274

Reports 2274

dblock
dblock approved these changes Feb 7, 2022
View reviewed changes
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Leaving for @andrross to take a final look.

@andrross andrross merged commit b9420d8 into opensearch-project:main Feb 7, 2022
@mattweber
Copy link
Contributor Author

Thank you @dblock and @andrross! Can I backport?

@dblock
Copy link
Member

dblock commented Feb 8, 2022

Yes, let's see if automation does it (labeled backport 1.x).

github-actions bot pushed a commit that referenced this pull request Feb 8, 2022
@mattweber @github-actions
Add regexp interval source (#1917)
82d8cfa 
* Add regexp interval source

Add a regexp interval source provider so people can use regular
expressions inside of intervals queries.

Signed-off-by: Matt Weber <[email protected]>

* Fixes

- register regexp interval in SearchModule
- use fully-qualified name for lucene RegExp
- get rid of unnecessary variable

Signed-off-by: Matt Weber <[email protected]>
(cherry picked from commit b9420d8)
@github-actions github-actions bot mentioned this pull request Feb 8, 2022
Merged
@mattweber
Copy link
Contributor Author

@dblock will need to introduce some serialization version checks I believe

dblock pushed a commit that referenced this pull request Feb 16, 2022
@github-actions @mattweber
Add regexp interval source (#1917) (#2069)
bfe128c 
* Add regexp interval source

Add a regexp interval source provider so people can use regular
expressions inside of intervals queries.

Signed-off-by: Matt Weber <[email protected]>

* Fixes

- register regexp interval in SearchModule
- use fully-qualified name for lucene RegExp
- get rid of unnecessary variable

Signed-off-by: Matt Weber <[email protected]>
(cherry picked from commit b9420d8)

Co-authored-by: Matt Weber <[email protected]>
@mattweber mattweber mentioned this pull request Feb 19, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrross andrross andrross approved these changes

@dblock dblock dblock approved these changes

Assignees
No one assigned
Labels
backport 1.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mattweber @opensearch-ci-bot @reta @andrross @dblock