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.

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

Expose Lucenes Ukrainian analyzer #21176

Merged
merged 2 commits into from Oct 31, 2016

Conversation

Projects
None yet
4 participants
@cbuescher
Copy link
Member

commented Oct 28, 2016

Since Lucene 6.2. the UkrainianMorfologikAnalyzer is available through the lucene-analyzers-morfologik jar. This change exposes the analyzer to be used as an elasticsearch plugin.

Closes #19433

...nalysis-ukrainian/src/test/java/org/elasticsearch/index/analysis/UkrainianAnalysisTests.java Outdated
MatcherAssert.assertThat(analyzer, instanceOf(UkrainianMorfologikAnalyzer.class));
}

// public void testDefaultsUkrainianAnalysis() throws IOException {

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 28, 2016

Contributor

leftover?

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 28, 2016

Author Member

yes, I will delete that

@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2016

Since I'm new to how the licenses directory should look like: I just mapped the dependency licenses for the morfologik-* jars to the Lucene NOTICE and LICENSE file since they seem to include the sections for the morfologik project. Hopre this is okay. After reading the original issue in https://issues.apache.org/jira/browse/LUCENE-7287 I also wasn't sure if the dictionary included in lucene-analyzers-morfologik has it's own license that we need to copy over or if the Lucene license and notice files cover that.

@cbuescher cbuescher force-pushed the cbuescher:analysis-ukrainian branch 4 times, most recently Oct 28, 2016

...nian/src/main/java/org/elasticsearch/index/analysis/ukrainian/UkrainianAnalyzerProvider.java Outdated
* under the License.
*/

package org.elasticsearch.index.analysis.ukrainian;

This comment has been minimized.

Copy link
@johtani

johtani Oct 31, 2016

Member

Do we need ukrainian directory?
In core and icu, and kuromoji, there are no directory under analysis.
But phonetic and stempel have own directory.
I don't think it needs.

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 31, 2016

Author Member

Thanks @johtani. I don't mind putting all code into the o.e.index.analysis package, I think I followed some other analysis plugin layout. Does anybody else have an opinion on this? Maybe there was a reason for grouping analysis plugin code in sub-packages? I somehow doubt it and will move all the code of this PR into o.e.index.analysis if nobody objects.

This comment has been minimized.

Copy link
@cbuescher

cbuescher Oct 31, 2016

Author Member

Kuromoji has the *AnalyzerProvider under o.e.index.analysis but the Plugin class under o.e.plugin.analysis.kuromoji. Is there a reason for this? Same for Stempel.

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

I don't think it makes a big difference which package the stuff is in but for a plugin of this size I'd just put it all in one package. And I'd try not to clash with a package in core. Though I don't think it is big deal either way.

@johtani

This comment has been minimized.

Copy link
Member

commented Oct 31, 2016

I agree with @nik9000 . my comment is just out of curiosity.

Expose Lucenes Ukrainian analyzer
Since Lucene 6.2. the UkrainianMorfologikAnalyzer is available through the
lucene-analyzers-morfologik jar. This change exposes it to be used as an
elasticsearch plugin.

@cbuescher cbuescher force-pushed the cbuescher:analysis-ukrainian branch Oct 31, 2016

@cbuescher cbuescher force-pushed the cbuescher:analysis-ukrainian branch to 42ffaf6 Oct 31, 2016

@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2016

@jpountz this can go on master and 5.x as well, right?

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

Correct.

@cbuescher cbuescher merged commit 1f5adaa into elastic:master Oct 31, 2016

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details
@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Nov 2, 2016

Merged to 5.x with f0eade6 and 42d4b60

@cbuescher cbuescher removed the review label Nov 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.