Set analyzer to regex query string search #3967

yyyogev · 2022-07-21T10:13:36Z

Description

Set default analyzer to query string search with regex

Issues Resolved

Resolves #3578

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.

Signed-off-by: yyyogev <[email protected]>

github-actions · 2022-07-21T10:37:17Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/866/
CommitID: d2e6e08

dblock

See below.

I am not familiar with this part of the code. Why does this work?

server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java

dblock · 2022-07-21T17:04:30Z

server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java

-                return super.getWildcardQuery(currentFieldType.name(), termStr);
+            if (currentFieldType != null) {
+                // return newUnmappedFieldQuery(field);
+                setAnalyzer(forceAnalyzer == null ? queryBuilder.context.getSearchAnalyzer(currentFieldType) : forceAnalyzer);


Since we do this in multiple places maybe add a private getSearchAnalyzer(field) method?

if/else would probably look cleaner

Signed-off-by: yogev mets <[email protected]>

yyyogev · 2022-07-24T07:58:07Z

Refer to the issue associated with this PR, we discussed it all there

github-actions · 2022-07-24T08:13:10Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/940/
CommitID: 036029a

Signed-off-by: yogev mets <[email protected]>

… default-analyzer-regex-search

Signed-off-by: yogev mets <[email protected]>

github-actions · 2022-07-24T09:02:22Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/941/
CommitID: 64e6098

github-actions · 2022-07-24T09:08:16Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/942/
CommitID: f5351fa

github-actions · 2022-07-24T09:14:32Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/943/
CommitID: 04fb97f

Signed-off-by: yogev mets <[email protected]>

github-actions · 2022-07-24T10:11:49Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/944/
CommitID: b3a4cec

reta · 2022-07-24T16:31:41Z

server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java

-            if (forceAnalyzer != null && (analyzeWildcard || currentFieldType.getTextSearchInfo().isTokenized())) {
-                setAnalyzer(forceAnalyzer);
-                return super.getWildcardQuery(currentFieldType.name(), termStr);
+            if (currentFieldType != null) {


Hm ... did we lost if (currentFieldType == null) { condition?

I figured it will be cleaner to check if (currentFieldType != null) now that we always set the analyzer

But this is unmapped field case, we probably should return newUnmappedFieldQuery ?

Signed-off-by: yogev mets <[email protected]>

yyyogev · 2022-07-25T13:33:31Z

See below.

I am not familiar with this part of the code. Why does this work?

@dblock currently, when using regexp query, we only set the analyzer if it's forced (explicitly specify an analyzer in the search request). the reason is because of the wildcard queries ES introduced, but since wildcard queries are not being supported in OS, it's redundant now and only harm the regexp queries while searching for uppercase letters.

The change I made removes the condition to check if the force analyzer is specified and set the analyzer regardless - either the forced or by the currentFieldType

github-actions · 2022-07-25T13:45:12Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/953/
CommitID: 105f68d

Signed-off-by: yogev mets <[email protected]>

github-actions · 2022-07-26T19:58:38Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/1008/
CommitID: f28e335

github-actions · 2022-07-26T22:06:56Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/1013/
CommitID: f28e335

… default-analyzer-regex-search

github-actions · 2022-07-27T10:12:48Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/1036/
CommitID: 930cbd8

yyyogev · 2022-07-27T10:19:49Z

@reta @dblock I'm not sure why the gradle check is failing now.. can you take a look?

reta · 2022-07-27T11:44:53Z

@reta @dblock I'm not sure why the gradle check is failing now.. can you take a look?

@yyyogev seems to be issue(s) with build pipeline:

/var/jenkins/workspace/gradle-check@tmp/durable-ea4df87d/script.sh: 41: docker-compose: not found

@peterzhuamazon any hints?

peterzhuamazon · 2022-07-27T13:47:03Z

@reta @dblock I'm not sure why the gradle check is failing now.. can you take a look?

@yyyogev seems to be issue(s) with build pipeline:
/var/jenkins/workspace/gradle-check@tmp/durable-ea4df87d/script.sh: 41: docker-compose: not found
@peterzhuamazon any hints?

Hi sorry for the late reply guys.
We had a new deployment in Jenkins and missed some lines.

And we have a fix now to be reviewed.

Please be patient on this as it requires a new deployment.

Sorry and thanks.
Peter

peterzhuamazon · 2022-07-27T20:38:01Z

@reta @dblock I'm not sure why the gradle check is failing now.. can you take a look?

@yyyogev seems to be issue(s) with build pipeline:
/var/jenkins/workspace/gradle-check@tmp/durable-ea4df87d/script.sh: 41: docker-compose: not found
@peterzhuamazon any hints?
Hi sorry for the late reply guys. We had a new deployment in Jenkins and missed some lines.

And we have a fix now to be reviewed.

Please be patient on this as it requires a new deployment.

Sorry and thanks. Peter

fixed now.

github-actions · 2022-07-27T20:56:22Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/1050/
CommitID: 930cbd8

AmiStrn · 2022-08-01T06:37:37Z

@yyyogev Looks like this passed the tests 🎸
@reta @dblock can you review the changes made?

AmiStrn · 2022-08-03T11:40:56Z

@dblock - just pinging as this is 2 days since the first approval. can you please check this as well?

AmiStrn · 2022-08-08T04:05:50Z

@reta is there anyone else who can review this one?

reta · 2022-08-08T13:52:37Z

@reta is there anyone else who can review this one?

@nknize @andrross could you please take a look? the change seems to be reasonable

AmiStrn · 2022-08-14T17:09:17Z

Weekly ping :/

andrross · 2022-08-15T16:53:35Z

Weekly ping :/

So sorry for the delay. @nknize any concerns here?

nknize

Nice! Clean fix. No concerns at all. Sorry for the delay here and thanks for fixing this! 🎉

Changes were addressed...

AmiStrn · 2022-08-15T21:59:20Z

Nice! Clean fix. No concerns at all. Sorry for the delay here and thanks for fixing this! 🎉

Great news:) Thanks @nknize and @reta !
We would like to backport this since it was in all versions as a bug since 1.0

How would we get this in version 1.x as a bug fix? (We could create the pr's, just asking for the versions required to backport to)

@yyyogev - FYI

reta · 2022-08-15T22:21:38Z

@AmiStrn a bit late for backport label, but basically we need to cherry-pick the commit to 2.x / 2.2 / 2.1 / 2.0 / 1.x / 1.3 manually (I think).

Sets analyzer to regex query string search Signed-off-by: yyyogev <[email protected]> (cherry picked from commit ea4cfcc)

andrross · 2022-08-15T23:42:35Z

@reta The labels work even after the PR has been merged :)

reta · 2022-08-16T00:11:09Z

@reta The labels work even after the PR has been merged :)

Thanks @andrross !

andrross · 2022-08-16T00:16:59Z

I think 1.3 is the end of the line for 1.x releases, so backporting there will ensure the bug fix gets in to any future 1.3 release if one happens. 2.x branch will ensure the next minor release gets the fix. I think that should be sufficient? Please let me know if I'm wrong here.

nknize · 2022-08-16T13:33:29Z

FYI: I opened #4226 until the feature is back ported since the test is consistently failing bwc CI.

Sets analyzer to regex query string search Signed-off-by: yyyogev <[email protected]> (cherry picked from commit ea4cfcc) Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>

Sets analyzer to regex query string search Signed-off-by: yyyogev <[email protected]> (cherry picked from commit ea4cfcc) Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]> Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]> Co-authored-by: Yogev Mets <[email protected]>

Sets analyzer to regex query string search Signed-off-by: yogev mets <[email protected]> (cherry picked from commit ea4cfcc)

set analyzer to regex query string search

d2e6e08

Signed-off-by: yyyogev <[email protected]>

yyyogev requested review from a team and reta as code owners July 21, 2022 10:13

dblock previously requested changes Jul 21, 2022

View reviewed changes

pr changes

036029a

Signed-off-by: yogev mets <[email protected]>

yyyogev added 3 commits July 24, 2022 11:45

remove comments

64e6098

Signed-off-by: yogev mets <[email protected]>

Merge branch 'main' of github.com:/opensearch-project/OpenSearch into…

f5351fa

… default-analyzer-regex-search

small fix

04fb97f

Signed-off-by: yogev mets <[email protected]>

lint

b3a4cec

Signed-off-by: yogev mets <[email protected]>

reta reviewed Jul 24, 2022

View reviewed changes

small change

105f68d

Signed-off-by: yogev mets <[email protected]>

undo change in wildcard queries to preserve pipeline

f28e335

Signed-off-by: yogev mets <[email protected]>

Merge branch 'main' of github.com:/opensearch-project/OpenSearch into…

930cbd8

… default-analyzer-regex-search

reta approved these changes Aug 1, 2022

View reviewed changes

yyyogev requested a review from dblock August 1, 2022 16:59

andrross approved these changes Aug 15, 2022

View reviewed changes

nknize approved these changes Aug 15, 2022

View reviewed changes

nknize merged commit ea4cfcc into opensearch-project:main Aug 15, 2022

andrross added the backport 2.x Backport to 2.x branch label Aug 15, 2022

opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 15, 2022

Set analyzer to regex query string search (#3967)

d7b8abf

Sets analyzer to regex query string search Signed-off-by: yyyogev <[email protected]> (cherry picked from commit ea4cfcc)

opensearch-trigger-bot bot mentioned this pull request Aug 15, 2022

[Backport 2.x] Set analyzer to regex query string search #4219

Merged

andrross added the backport 1.3 Backport to 1.3 branch label Aug 15, 2022

opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 15, 2022

Set analyzer to regex query string search (#3967)

3970b43

Sets analyzer to regex query string search Signed-off-by: yyyogev <[email protected]> (cherry picked from commit ea4cfcc)

opensearch-trigger-bot bot mentioned this pull request Aug 15, 2022

[Backport 1.3] Set analyzer to regex query string search #4220

Merged

nknize mentioned this pull request Aug 16, 2022

[MUTE] search with uppercase regex in bwc testing #4226

Merged

nknize pushed a commit that referenced this pull request Oct 12, 2022

Set analyzer to regex query string search (#3967)

d675e6b

Sets analyzer to regex query string search Signed-off-by: yogev mets <[email protected]> (cherry picked from commit ea4cfcc)

nknize mentioned this pull request Jul 17, 2023

[BUG] WIldcard query behavior for text fields changed in an backward-incompatible way #8711

Closed

Set analyzer to regex query string search #3967

Set analyzer to regex query string search #3967

Conversation

yyyogev commented Jul 21, 2022

Description

Issues Resolved

Check List

github-actions bot commented Jul 21, 2022

Gradle Check (Jenkins) Run Completed with:

dblock left a comment

Choose a reason for hiding this comment

dblock Jul 21, 2022

Choose a reason for hiding this comment

yyyogev Jul 24, 2022

Choose a reason for hiding this comment

yyyogev commented Jul 24, 2022

github-actions bot commented Jul 24, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 24, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 24, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 24, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 24, 2022

Gradle Check (Jenkins) Run Completed with:

reta Jul 24, 2022

Choose a reason for hiding this comment

yyyogev Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

reta Jul 25, 2022

Choose a reason for hiding this comment

yyyogev commented Jul 25, 2022 • edited Loading

github-actions bot commented Jul 25, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 26, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 26, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 27, 2022

Gradle Check (Jenkins) Run Completed with:

yyyogev commented Jul 27, 2022

reta commented Jul 27, 2022

peterzhuamazon commented Jul 27, 2022

peterzhuamazon commented Jul 27, 2022

github-actions bot commented Jul 27, 2022

Gradle Check (Jenkins) Run Completed with:

AmiStrn commented Aug 1, 2022 • edited Loading

AmiStrn commented Aug 3, 2022

AmiStrn commented Aug 8, 2022

reta commented Aug 8, 2022 • edited Loading

AmiStrn commented Aug 14, 2022

andrross commented Aug 15, 2022

nknize left a comment

Choose a reason for hiding this comment

AmiStrn commented Aug 15, 2022

reta commented Aug 15, 2022

andrross commented Aug 15, 2022

reta commented Aug 16, 2022

andrross commented Aug 16, 2022

nknize commented Aug 16, 2022 • edited Loading

yyyogev Jul 25, 2022 •

edited

Loading

yyyogev commented Jul 25, 2022 •

edited

Loading

AmiStrn commented Aug 1, 2022 •

edited

Loading

reta commented Aug 8, 2022 •

edited

Loading

nknize commented Aug 16, 2022 •

edited

Loading