Deprecate and remove camel-case nGram and edgeNGram tokenizers #50862
Conversation
We already deprecated and removed the camel-case versions of the nGram and edgeNGram filters a while ago and we should do the same with the nGram and edgeNGram tokenizers. This PR deprecates the use of these names in favour of ngram and edge_ngram in 7 and disallows usage in new indices starting with 8. The deprecation part will be backported to 7.6. Closes elastic#50561
|
Pinging @elastic/es-search (:Search/Analysis) |
|
@cbuescher Is the intent of this PR to forbid to create an index with If it is, the it doesn't seem to work. With your patch, I can successfully create a index with curl -X PUT my_index
{
"settings":{
"analysis":{
"analyzer":{
"my_analyzer":{
"type":"custom",
"tokenizer":"nGram"
}
}
}
},
"mappings":{
"properties":{
"title": {
"type":"text",
"analyzer":"my_analyzer"
}
}
}
}{
"acknowledged" : true,
"shards_acknowledged" : true,
"index" : "my_index"
} |
Yes, but the way we lazy-load analyzers and contained tokenizers doesn't allow us to catch this merely on definition of the analysis settings. The warnings/errors should be triggered however as soon as trying to use the defined analyzer through either the "_analysis" API or indexing something into the field. |
|
@cbuescher Thanks for the clarification, it makes sense now. |
| { | ||
| try (IndexAnalyzers indexAnalyzers = buildAnalyzers(Version.CURRENT, "edgeNGram")) { | ||
| try (IndexAnalyzers indexAnalyzers = buildAnalyzers( | ||
| VersionUtils.randomVersionBetween(random(), Version.V_7_3_0, VersionUtils.getPreviousVersion(Version.CURRENT)), |
There was a problem hiding this comment.
instead of Version.CURRENT should we put a specific version to make sure the test doesn't fail after v 8.0?
|
@mayya-sharipova thanks for reviewing, I'm going to wait for #50908 to be merged since that might change this PR and tests slightly and rework it from there. If the changes are big I might ping you again to take another look after that. |
|
@elasticmachine update branch |
|
merge conflict between base and head |
|
@mayya-sharipova with #50908 your previous example now also warns/throws right after the custom analyzer definition. |
| try (CommonAnalysisPlugin commonAnalysisPlugin = new CommonAnalysisPlugin()) { | ||
| Map<String, TokenizerFactory> tokenizers = createTestAnalysis( | ||
| IndexSettingsModule.newIndexSettings("index", settings), settings, commonAnalysisPlugin).tokenizer; | ||
| TokenizerFactory tokenizerFactory = tokenizers.get(deprecatedName); |
There was a problem hiding this comment.
You shouldn't need to pull a factory and call create to get warnings anymore, they will be emitted when createTestAnalysis is called.
There was a problem hiding this comment.
This tests prebuild tokenizer directly though, e.g. when they are used via “_analyze” endpoint directly they wouldn’t appear in analyzers. createTestAnalysis doesn’t trigger the warning in that case if nothing refers to them from analysis settings.
…ic#50862) We already deprecated and removed the camel-case versions of the nGram and edgeNGram filters a while ago and we should do the same with the nGram and edgeNGram tokenizers. This PR deprecates the use of these names in favour of ngram and edge_ngram in 7. Usage will be disallowed on new indices starting with 8 then. Closes elastic#50561
… (#50991) We deprecated and removed the camel-case versions of the nGram and edgeNGram filters a while ago and we should do the same with the nGram and edgeNGram tokenizers. This PR deprecates the use of these names in favour of ngram and edge_ngram in 7. Usage will be disallowed on new indices starting with 8 then.
…ic#50862) We already deprecated and removed the camel-case versions of the nGram and edgeNGram filters a while ago and we should do the same with the nGram and edgeNGram tokenizers. This PR deprecates the use of these names in favour of ngram and edge_ngram in 7 and disallows usage in new indices starting with 8. Closes elastic#50561
We already deprecated and removed the camel-case versions of the nGram and edgeNGram filters
a while ago and we should do the same with the nGram and edgeNGram tokenizers.
This PR deprecates the use of these names in favour of ngram and edge_ngram in 7
and disallows usage in new indices starting with 8. The deprecation part will be
backported to 7.6.
Closes #50561