Refactors CommonTermsQuery #11345
Refactors CommonTermsQuery #11345
Conversation
There was a problem hiding this comment.
we can leave final fields (at least the mandatory ones) now that we implement Writeable instead of Streamable, then we can (almost) drop default constructors (see #11344 for more details)
|
I did a first round of review, left some comments |
alexksikes
force-pushed
the
feature/query-refactoring-common-terms
branch
from
e86c0c5 to
b85a191
Compare
There was a problem hiding this comment.
oh boy this never worked through java api it seems, because disable_coords should have been disable_coord? if this is the case this same fix should be ported to master and probably 1.x too.
There was a problem hiding this comment.
Yes the query parser did not check on disable_coords unfortunately. I'll submit a fix to this.
There was a problem hiding this comment.
thanks that would be great, one less bug to fix here.
|
thanks @alexksikes I left some comments |
There was a problem hiding this comment.
I think I'd leave this out of the enum, also would prefer a switch so that we have the OR case, the AND case, and we throw error otherwise (rather than doing AND and assume everything else is OR)
There was a problem hiding this comment.
I think having a method somehere to do the mapping to Occur is good, we might need this in other queries as well. Personally don't mind this beeing part of the enum type. Also covering the error case here may not be necessary, after all there are only two cases in the enum.
There was a problem hiding this comment.
The mapping to occur is needed in the SimpleQueryStringBuilder as well - so might make sense to leave it here.
If implicitly we default to OR here (that's the default for the default operator* in SimpleQueryStringBuilder as well) we should do so in the serialisation methods as well, no?
.* too many defaults in one line...
There was a problem hiding this comment.
what happens if we set this when the Operator is AND? maybe we should test that scenario too?
|
I did a last round of review, you can rebase, it would be good to have #11730 fixed upstream though before merging this I think. |
alexksikes
force-pushed
the
feature/query-refactoring-common-terms
branch
from
8618300 to
537f24a
Compare
There was a problem hiding this comment.
if you have rebased, this one should appear like a new method right?
|
left a couple of minor comments, this is very close |
Refactors CommonTermsQuery analogous to TermQueryBuilder. Still left to do are the tests to compare between builder and actual Lucene query. Relates to elastic#10217 This PR is against the query-refactoring branch.
alexksikes
force-pushed
the
feature/query-refactoring-common-terms
branch
from
537f24a to
494a468
Compare
|
LGTM besides the few cosmetic comments I left |
|
LGTM |
clintongormley
added
:Search/Search
Refactors CommonTermsQuery analogous to TermQueryBuilder. Still left to do are
the tests to compare between builder and actual Lucene query.
Relates to #10217
This PR is against the query-refactoring branch.