-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Conditional stemming for 'persian' analyzer #113421
Conditional stemming for 'persian' analyzer #113421
Conversation
Documentation preview: |
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
c3a3e26
to
8486254
Compare
The 'persian' analyzer for Lucene 10 comes with PersianStemFilter as the last token filter by default. In order to maintain compatibility for old indices, we only use the new analyzer for newly created indices but configure a legacy analyzer with the old behaviour for older index versions.
8486254
to
89ead07
Compare
@@ -901,6 +901,16 @@ | |||
- length: { tokens: 1 } | |||
- match: { tokens.0.token: خورد } | |||
|
|||
- do: |
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.
Is this to test stemming?
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.
Yes
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.
Then this test will need to be guarded with skip version or something. ES 9 will have mixed cluster tests with 8.last & 9.current and the 8.last won't have the stemming automatically correct?
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.
I would prefer a new test that is guarded, that way the original test isn't always skipped.
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.
All that would have made sense to me a few months ago. Now we live in a world where we merge Lucene 10 to "main" at some point which not necessarily is the point in which it becomes 9.0. So this is becoming a bit of a head-scratcher for me. I need to figure out if we can skip based on IndexVersion or not (since that is what we condition the behavioural change on), if we need some new sort of capability voodoo for that etc...
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.
fwiw I'm afraid I might have to introduce a "cluster_feature" for this. Maybe it makes sense to have one for "Cluster runs with Lucene 10".
# integration tests for persian analyzer changes from Lucene 9 to Lucene 10 | ||
setup: |
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.
If you want to test with old data, then upgrade, then verify the query results don't change, a rolling upgrade test, or one of the full restart tests.
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.
Yes I was afraid I'd need that full-blown infra and was somehow hoping I could leverage some yaml test. This at least shows that stemming works in the new version of the analyzer, i.e. both search terms are matching both documents which means they are analyzed to the same root form.
Settings.EMPTY | ||
); | ||
Analyzer analyzer = persianAnalyzerProvider.get(); | ||
assertAnalyzesTo(analyzer, " من کتاب های زیادی خوانده ام.", new String[] { "كتاب", "زياد", "خوانده" }); |
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.
"كتاب", "زياد", "خوان"
would make the most sense to me. That is, compared to the one on L74, [1] is correct here, and in both [2] is the same and correct, and what is mentioned as stem in both of them in [0], seems not to be correct to me.
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.
Okay, thanks. Token 0 is the output in both cases though and might be a general issues with the "persian" analyzer in both versions though. The goal here is not to test the quality of the analyzer per-se but the changes between the output in Lucene 9 and Lucene 10. I was mostly interested in the input sentence and if that makes sense.
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.
yes, it makes sense, and is the same in both: I have read a lot of books. The dot is at the wrong end, I'm assuming right-to-left issues.
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.
I'll probably remove the dots, already had trouble copy/pasting this since I'm not used to right-to-left languages unfortunately.
Reopened against the new Lucene 10 integration branch (lucene_snapshot) over here #113482 |
The 'persian' analyzer for Lucene 10 comes with PersianStemFilter as the last token filter by default. In order to maintain compatibility for old indices, we only use the new analyzer for newly created indices but configure a legacy analyzer with the old behaviour for older index versions.