-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix bug "synonym_graph filter fails with word_delimiter_graph when using whitespace or classic tokenizer in synonym_analyzer" #19248
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
base: main
Are you sure you want to change the base?
Conversation
…whitespace or classic tokenizer in synonym_analyzer" bug opensearch-project#18037 add 'analyzersBuiltSoFar' to getChainAwareTokenFilterFactory to build custom analyzers depending on other (already built) analyzers The analyzers are built following the order of precedence specified in the settings Signed-off-by: Lamine Idjeraoui <[email protected]>
|
❌ Gradle check result for 5c8fbe6: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Only fail in CI, cannot reproduce locally with same seeds |
|
❌ Gradle check result for 267f48e: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
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.
The DCO check is failed, please amend your commit with '-s' to include the sign off info, and change log is needed.
| List<TokenFilterFactory> previousTokenFilters, | ||
| Function<String, TokenFilterFactory> allFilters | ||
| Function<String, TokenFilterFactory> allFilters, | ||
| Function<String, Analyzer> analyzersBuiltSoFar |
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.
This change is breaking because the method is public, do we have another solution?
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.
Hi @gaobinlong
Do you have time to finish the review?
…rgs) Signed-off-by: Lamine Idjeraoui <[email protected]>
|
Thanks @laminelam looks like similar issue from past #16263 (comment) ? |
|
❌ Gradle check result for 48cc847: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Yes I think they are related. This fix should handle both situations |
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.
@prudhvigodithi Can you review this PR since you've worked on a similar issue in the past?
server/src/main/java/org/opensearch/index/analysis/AnalysisRegistry.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Lamine Idjeraoui <[email protected]>
|
❌ Gradle check result for 49bb330: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
This PR fixes the "synonym_graph filter fails with word_delimiter_graph when using whitespace or classic tokenizer in synonym_analyzer" bug
Investigated the issue and looks like there are 2 causes:
analysisRegistry.getAnalyzer(synonymAnalyzerName);This is not enough because it only looks into the built in and pre built in analyzers. The one from settings are not there.
The solution is two-fold:
Fail safe instead of fail fast when building the analyzers
Right now, if an analyzer fails for some reason the whole building process fails with an exception.
Build the depending analyzers first:
Synonym custom analyzers may depend on another analyzer that has to be built first.
The PR adds a logic to:
Resolves #18037
Check List
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.