Drop name from all analysis component factories #24976
Conversation
A continuation of elastic#24869, this drops the `name()` method from all remaining analysis component factories, moving name members to `CustomAnalayzer` in service of the `_analyze` action. Like elastic#24869 I've kept a `String` in the method signature of the ctor of some abstract components so that back these analysis component factories. It is *much* more convenient to remove all the names at once in a followup. That change will be large but purely mechanical. This breaks the API for plugins adding analyzers because they no longer must implement fairly redundant feeline method.
nik9000
added
:Core/Infra/Plugins
| @@ -462,96 +466,106 @@ private void analyze(TokenStream stream, Analyzer analyzer, String field, Set<St | |||
| return extendedAttributes; | |||
| } | |||
|
|
|||
| private static CharFilterFactory[] getCharFilterFactories(AnalyzeRequest request, IndexSettings indexSettings, AnalysisRegistry analysisRegistry, | |||
| Environment environment, CharFilterFactory[] charFilterFactories) throws IOException { | |||
| if (request.charFilters() != null && request.charFilters().size() > 0) { | |||
There was a problem hiding this comment.
I've switched this around to return early rather than indent a bunch so the change look bigger than it is.
There was a problem hiding this comment.
The functionality change here is to return a Tuple with the names as well.
| @@ -63,10 +69,24 @@ public TokenizerFactory tokenizerFactory() { | |||
| return tokenizerFactory; | |||
| } | |||
|
|
|||
| /** | |||
| * Names of the token filters. | |||
There was a problem hiding this comment.
These are needed by the analysis API but not really anywhere else. They aren't a huge burden to maintain though, not like the name() member was.
|
|
||
| private interface MultiTermAwareTokenFilterFactory extends TokenFilterFactory, MultiTermAwareComponent {} | ||
|
|
||
| public synchronized TokenFilterFactory getTokenFilterFactory(final Version version) { |
There was a problem hiding this comment.
This is just not needed any more.
|
@nik9000 this PR is approved, I think it just needs to have master merged and then be merged right? |
I'd literally forgotten about it. I'll have a look. |
|
Thanks! |
|
@martijnvg I just merged master into this. It is fairly similar to how it used to be but I think it is worth another look just in case. Could you look again please? |
|
@martijnvg can you give this another look? The merge changed it a lot after your last review. |
|
@nik9000 Is this something you would still like to get in? |
Yes! Oh boy, all out of date again.... I'll take a look soon. |
| @@ -29,14 +29,10 @@ public PreBuiltAnalyzerProvider(String name, AnalyzerScope scope, Analyzer analy | |||
| // we create the named analyzer here so the resources associated with it will be shared | |||
| // and we won't wrap a shared analyzer with named analyzer each time causing the resources | |||
| // to not be shared... | |||
| // TODO why not pass a NamedAnalyzer into the method? | |||
There was a problem hiding this comment.
What is this comment for? It is unclear what it refers to.
| @@ -24,8 +24,6 @@ | |||
| import org.elasticsearch.search.fetch.subphase.highlight.FastVectorHighlighter; | |||
|
|
|||
| public interface TokenFilterFactory { | |||
There was a problem hiding this comment.
Can this be made a FunctionalInterface now?
|
Update: I do still plan to merge this but it currently doesn't build cleanly. I've got to get into the right head space to get it properly working again because it is all around code I haven't read in a while. I'll get it though! |
|
@nik9000 still wanna merge this? |
|
At this point I'm not sure I could get it merge-able. It has been too long. |
A continuation of #24869, this drops the
name()method fromall remaining analysis component factories, moving name members
to
CustomAnalayzerin service of the_analyzeaction.Like #24869 I've kept a
Stringin the method signature of thector of some abstract components so that back these analysis
component factories. It is much more convenient to remove all
the names at once in a followup. That change will be large but
purely mechanical.