Refactors FilteredQueryBuilder #11885
Conversation
There was a problem hiding this comment.
We changed the boost default to 1.0f in the other builders, should we do this here to for consistency? Can't add this comment on the top where the default is set.
|
I went through it and it looks like there only a few minor changes to address. Left a few comments. |
|
Thanks for your comments. Looking into the changes needed. |
MaineC
force-pushed
the
feature/filter-query-builder-refactoring
branch
from
12f119f to
6dfeccc
Compare
|
@cbuescher I believe I addressed your comments. Added call to AbstractQueryBuilder#doXContentInnerBuilder in doXContent - happy to remove if #11915 comes in before this one. |
There was a problem hiding this comment.
Can't find validate() here. I think we went for always having empty implementation in builder class, even if there's nothing to validate.
There was a problem hiding this comment.
it wouldn't be empty here at this point, it needs to validate the inner query and filter, see #11889
There was a problem hiding this comment.
this will need to become protected doToQuery as soon as you rebase
There was a problem hiding this comment.
I don't see why the suppress warnings are needed here and in the previous method. Can you clarify that?
There was a problem hiding this comment.
NamedWritable is a parameterised type so the compiler will complain when used w/o a parameter.
There's a few ways out: Specify the exact type of NamedWriteable that will be written in this location - clearly not an option at compile time here. Specify <?> instead of leaving the raw type - this would trickle down to any classes using this method, including e.g. the list of must clauses in BoolQueryBuilder and imply that all entries in that list need to be of the same type - not a good idea neither. The only way to get rid of the warning I saw was to disable with SuppressWarnings. (javac -X gave the hint on which warnings the JDK I use supports as part of the static analysis)
Checked the codebase for other uses of the annotation found it only in org/elasticsearch/search/aggregations/metrics/ScriptedMetricTests.java and org/elasticsearch/search/aggregations/pipeline/SiblingPipelineAggregator.java so I suppose we don't care too much about it. Happy to remove the annotations I added.
There was a problem hiding this comment.
alright, I do get unchecked warnings when it comes to generics, but not these ones, it is just the level of the warnings in the IDE. I agree we don't usually suppress these, I think we can remove.
MaineC
force-pushed
the
feature/filter-query-builder-refactoring
branch
from
6dfeccc to
ef95d8c
Compare
|
Left some explanations and updated the PR. |
|
left a few comments, @cbuescher please have a look too :) |
|
Also went through the changes once again, would prefer null-checks in constructors as well, left some more remarks. |
MaineC
force-pushed
the
feature/filter-query-builder-refactoring
branch
from
da4fa26 to
fd12784
Compare
|
Went through your comments and updated the PR (noticed only after pushing that one of the questions I had left was answered by both of you in the mean time - you are too fast answering questions). Updated code is ready for another look. |
|
@MaineC left a few comments, sorry I didn't realize I should not to that on the individual commits but rather on the overall change. Will do so next time. |
|
I did another round of review, left some comments. I'd need @cbuescher to have another look too though :) |
There was a problem hiding this comment.
Boost will be set in BaseQueryTestCase. No harm here, but doppelt gemoppelt (I want to try to gradually introduce this expression into the english vocabulary like Kindergarten and Schadenfreude) ;-)
|
Also did another round of reviews and left some comments, mostly minor stuff. Agree that the query null-check in the parser doesn't need to be there, happy to clear this up more if there are questions remaining. |
MaineC
force-pushed
the
feature/filter-query-builder-refactoring
branch
from
fd12784 to
68c806e
Compare
|
Ready for another look. |
There was a problem hiding this comment.
minor thing but how about getting rid of the result variable and just return the query from the if?
|
LGTM besides the two minor comments I left |
…treaming, hashCode and equals as well as unit tests. Relates to elastic#10217
MaineC
force-pushed
the
feature/filter-query-builder-refactoring
branch
from
68c806e to
0202c99
Compare
…factoring Refactors FilteredQueryBuilder. Relates to #10217
…lder-refactoring Refactors FilteredQueryBuilder. Relates to elastic#10217
clintongormley
added
:Search/Search
Separates JSON parsing from Lucene query creation, adds support for streaming, hashCode and equals as well as unit tests.
Relates to #10217
@cbuescher assigning to you for a first round of reviews. Not particularly happy with the createExpectedQuery implementation at the moment.