Query refactoring: SpanMultiTermQueryBuilder and Parser #12182
Conversation
| protected Query doToQuery(QueryParseContext parseContext) throws IOException { | ||
| Query subQuery = multiTermQueryBuilder.toQuery(parseContext); | ||
| if (subQuery instanceof DateFieldMapper.DateFieldType.LateParsingQuery) { | ||
| subQuery = ((DateFieldMapper.DateFieldType.LateParsingQuery) subQuery).rewrite(null); |
There was a problem hiding this comment.
add a simple comment, just so we know why we do this and why we live with that although it sucks? :)
There was a problem hiding this comment.
I was going to say in #12123 that this workaround means SpanMultiTermQuery with this particular type of inner query will not work with now then, but I added it here to make our tests work for now. We can track that particular issue in #12123. Wonder if we should add this early rewrite on master as well as I already brought it up there? Need to ask Martjin about it, I guess.
There was a problem hiding this comment.
not sure what we should do in master, seems like things are broken either way. This is fine in our branch cause that's how we move forward for now, till we have found a better solution.
|
left a minor comment, LGTM though |
|
Thanks, I still wonder if it makes sense at all to allow Date/Numeric range queries inside a SpanMultiTerm query or if we should simply not allow this case (see #12123 (comment)). In this case I would rather change the test slightly here than have this rewrite. |
|
@javanna removed the check for the LateParsingQuery case, Date Ranges will (and should) not work inside this type of query, so cast will not work in this case. Only problem now is we don't detect this until we generate the lucene query, since we need to check the field mapping if it is a Range query (possibly also for other nested MultiTermQueries like Fuzzy/Regex/... where field doesn't have positions). I don't know if we can do this already in the parser (coordinating node) later, for now I changed the test setup so it only uses String ranges for the nested queries. |
cbuescher
force-pushed
the
feature/query-refactoring-spanmultiterm
branch
2 times, most recently
from
43f30a5 to
6c62d18
Compare
|
Added type check for inner query and test that we throw UnsupportedOperationException instead of ClassCastException when trying to use nested query that doesn't resolve to MultiTermQuery. |
cbuescher
force-pushed
the
feature/query-refactoring-spanmultiterm
branch
from
6c62d18 to
bfcba0e
Compare
|
Adressed the last comment regarding error message and rebased on master. |
| @@ -42,6 +58,42 @@ protected void doXContent(XContentBuilder builder, Params params) | |||
| } | |||
|
|
|||
| @Override | |||
| protected Query doToQuery(QueryParseContext parseContext) throws IOException { | |||
| Query subQuery = multiTermQueryBuilder.toQuery(parseContext); | |||
| if (!(subQuery instanceof MultiTermQuery)) { | |||
There was a problem hiding this comment.
Simon would say replace with == false ;)
|
Added changes considering last round of comments. |
|
Removed calling RangeQueryBuilder from RandomQueryBuilder utility class, instead creating simple string field range query there now. |
| } | ||
| if (randomBoolean()) { | ||
| query.format("yyyy-MM-dd'T'HH:mm:ss.SSSZZ"); | ||
| ; |
cbuescher
force-pushed
the
feature/query-refactoring-spanmultiterm
branch
from
c15e8a6 to
24d1603
Compare
| RangeQueryBuilder query = new RangeQueryBuilder(BaseQueryTestCase.STRING_FIELD_NAME); | ||
| query.from("a"+RandomStrings.randomAsciiOfLengthBetween(r, 1, 10)); | ||
| query.to("z"+RandomStrings.randomAsciiOfLengthBetween(r, 1, 10)); | ||
| return query; |
There was a problem hiding this comment.
maybe we should call this method createRangeStringQuery instead, and when we add a broader method for multi term queries we call it? so we can also call it where we duplicate its code currently?
There was a problem hiding this comment.
Okay, can do, but I was trying to avoid the RangeQueryBuilderTest calling RandomQueryBuilder or did I missunderstand this previously? So the individual test should be able to call the utility class but not the other way round? Cause we already do so in RandomQueryBuilder#createQuery()
There was a problem hiding this comment.
yea I know we already do that, and I don't like it very much... I would try not to have the utility class calling test classes if possible.
There was a problem hiding this comment.
I tried this but having the RangeQueryBuilderTest setup calling the utility class just for one of the three cases covered there looks odd. Too much indirection just to save three lines, if I'm honest. Would like to leave it like this for now, so only non-leaf queries need to use RandomQueryBuilder for nested queries, and try to extend this method with other refactored multi term queries later.
|
Last minor cleanup, rebased once again on current |
|
did another round, left a few comments |
|
Sorry, didn't mean to close this. |
|
Added some clarifying comment around edge test case. |
|
LGTM |
Moving the query building functionality from the parser to the builders new toQuery() method analogous to other recent query refactorings. Relates to elastic#10217 Closes elastic#12182
cbuescher
force-pushed
the
feature/query-refactoring-spanmultiterm
branch
from
969644a to
966c02b
Compare
…anmultiterm Query refactoring: SpanMultiTermQueryBuilder and Parser
Moving the query building functionality from the parser to the builders new toQuery() method analogous to other recent query refactorings. Relates to elastic#10217 Closes elastic#12182
…ring-spanmultiterm Query refactoring: SpanMultiTermQueryBuilder and Parser
clintongormley
added
:Search/Search
Moving the query building functionality from the parser to the builders
new toQuery() method analogous to other recent query refactorings.
Relates to #10217