Make Multiplexer inherit filter chains analysis mode #50662
Conversation
cbuescher
added
>enhancement
WIP
:Search Relevance/Analysis
|
Pinging @elastic/es-search (:Search/Analysis) |
|
@elasticmachine update branch |
There was a problem hiding this comment.
Thanks @cbuescher, looks good; there's one more test that I think is worth adding.
| @@ -20,6 +20,7 @@ | |||
| package org.elasticsearch.index.mapper; | |||
|
|
|||
| import com.carrotsearch.hppc.ObjectHashSet; | |||
|
|
|||
There was a problem hiding this comment.
nit: I think this is a leftover?
| String synonymsFileName = "synonyms.txt"; | ||
| Path configDir = node().getEnvironment().configFile(); | ||
| if (Files.exists(configDir) == false) { | ||
| Files.createDirectory(configDir); | ||
| } | ||
| Path synonymsFile = configDir.resolve(synonymsFileName); | ||
| if (Files.exists(synonymsFile) == false) { | ||
| Files.createFile(synonymsFile); | ||
| } | ||
| try (PrintWriter out = new PrintWriter( | ||
| new OutputStreamWriter(Files.newOutputStream(synonymsFile, StandardOpenOption.WRITE), StandardCharsets.UTF_8))) { | ||
| out.println("foo, baz"); | ||
| } |
There was a problem hiding this comment.
Given that this is reused in three different tests now, I wonder if it's worth extracting it as a separate method?
| "Failed to parse mapping: analyzer [my_synonym_analyzer] " | ||
| + "contains filters [synonym_filter] that are not allowed to run in all mode.", | ||
| ex.getMessage()); | ||
| MapperException ex = expectThrows(MapperException.class, |
There was a problem hiding this comment.
This compression of multiple settings onto single lines seems to me to make test more difficult to follow, can we keep the indentation as it was before?
There was a problem hiding this comment.
Good point, this was not intended, must be the new Eclipse installation doing some autoformatting.
| response = client().prepareSearch(indexName).setQuery(QueryBuilders.matchQuery("field", "buzz")).get(); | ||
| assertHitCount(response, 1L); | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe also add a test asserting that a multiplexer containing updateable synonyms is rejected as an index-time analyzer as well?
|
@romseygeek thanks for the review, your comments should be adressed now |
Currently, if an updateable synonym filter is included in a multiplexer filter, it is not reloaded via the _reload_search_analyzers because the multiplexer itself doesn't pass on the analysis mode of the filters it contains, so its not recognized as "updateable" in itself. Instead we can check and merge the AnalysisMode settings of all filters in the multiplexer and use the resulting mode (e.g. search-time only) for the multiplexer itself, thus making any synonym filters contained in it reloadable. This, of course, will also make the analyzers using the multiplexer be usable at search-time only. Closes #50554
Currently, if an updateable synonym filter is included in a multiplexer filter, it is not reloaded via the _reload_search_analyzers because the multiplexer itself doesn't pass on the analysis mode of the filters it contains, so its not recognized as "updateable" in itself. Instead we can check and merge the AnalysisMode settings of all filters in the multiplexer and use the resulting mode (e.g. search-time only) for the multiplexer itself, thus making any synonym filters contained in it reloadable. This, of course, will also make the analyzers using the multiplexer be usable at search-time only. Closes elastic#50554
Currently, if an updateable synonym filter is included in a multiplexer filter, it is not reloaded
via the
_reload_search_analyzersbecause the multiplexer itself doesn't pass on the analysis modeof the filters it contains, so its not recognized as "updateable" in itself. Instead we can check and merge
the
AnalysisModesettings of all filters in the multiplexer and use the resulting mode (e.g. search-time only)for the multiplexer itself, thus making any synonym filters contained in it reloadable. This, of course, will also
make the analyzers using the multiplexer be usable at search-time only.
Marking this as WIP for now to get some test runs and for discussion.
Closes #50554