Refactors MultiMatchQueryBuilder and Parser #13405
Conversation
alexksikes
force-pushed
the
feature/query-refactoring-multi-match-query
branch
from
ae46164 to
93f6831
Compare
|
@cbuescher can you review this once you are done with the match query please? |
| } | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think Object readGenericValue() works for Float and we don't need this.
|
@alexksikes did a first round of reviews. Since this query relies much in MatchQueryBuilder which is currently WIP in #13402, there is a bit of movement here. Also, in working on the match query I first went with having many fields as optional objects, but after discussion today changed many to using primitive types plus defaults. Maybe this applies in other places for this PR as well. |
Relates to elastic#10217 This PR is against the query-refactoring branch. Closes elastic#13405
alexksikes
force-pushed
the
feature/query-refactoring-multi-match-query
branch
from
93f6831 to
8496c27
Compare
|
@cbuescher I rebased and addressed all the comments. Thanks for the review. |
| @Override | ||
| protected void setFinalBoost(Query query) { | ||
| query.setBoost(boost * query.getBoost()); | ||
| } |
There was a problem hiding this comment.
Took me some time to get this overwrite, maybe some javadocs would help here.
There was a problem hiding this comment.
I think the base method in AbstractQueryBuilder has already extensive javadocs, maybe we can add a simple comment here explaining that the query might get a boost coming out of field boosts parsing, which we need to combine with the boost assigned to the query as part of the query dsl.
That said, I don't see this same behaviour in master, it feels like it is the right behaviour especially looking at what query_string does, but on master we actually replace the parsed boost with the boost assigned to the query in the query DSL.
Maybe open a separate issue and mark it fixed together with this PR so we at least keep track of this small change?
|
@alexksikes left a few comments, very minor and mostly open questions on my side. Not sure if someone else should take a final look, otherwise looks good to me. |
|
@cbuescher I addressed all the comments. In some specific cases MatchQueryBuilder (and so does MultiMatch) fails on date fields, which is untested yet on MatchQueryBuilder. I'll open a PR for this. Thanks for the review. |
|
Had a look at the last changes, LGTM |
…ession results in no fields. The query will now return 0 hits (null query) instead of throwing an exception. This matches the behavior if a nonexistent field is specified. These changes were backported from latest master (mostly from elastic#13405).
clintongormley
added
:Search/Search
Relates to #10217
This PR is against the query-refactoring branch.