Query Refactoring: DisMaxQueryBuilder and Parser #11703
Conversation
| * @return an immutable list copy of the current sub-queries of this disjunction | ||
| */ | ||
| public List<QueryBuilder> queries() { | ||
| return ImmutableList.copyOf(this.queries); |
There was a problem hiding this comment.
curious about making a copy here. Did we do this in other queries too? You went this path to prevent users from making changes to the list that we might return instead?
There was a problem hiding this comment.
Yes, that was my reasoning. I think I haven't seen it in other queries, what do you think, should we use this also elsewhere or is this too restrictive?
There was a problem hiding this comment.
I dunno, I think we should do the same in all queries though, whatever we decide to do. we have a public method to add queries, so this list can be modified from the outside, why can't we just return the same list through getter? What is the advantage of making a copy here?
There was a problem hiding this comment.
Will do as in BoolQueryBuilder then.
|
left a couple of comments |
cbuescher
force-pushed
the
feature/query-refactoring-dismax
branch
from
e3ecde7 to
8fa73da
Compare
| parseContext.addNamedQuery(queryName, query); | ||
| } | ||
| return query; | ||
| disMaxQuery.validate(); |
There was a problem hiding this comment.
shall we drop the validate call?
There was a problem hiding this comment.
sure, missed that one. I also deleted it from BoolQueryParser I think. Will do so when I spot it in other places except tests.
cbuescher
force-pushed
the
feature/query-refactoring-dismax
branch
from
8fa73da to
9f6d3dc
Compare
| * @throws IOException | ||
| * @throws QueryParsingException | ||
| */ | ||
| public void testNoInnerQueries() throws QueryParsingException, IOException { |
There was a problem hiding this comment.
missing @test just for consistency?
|
left a few minor comments, LGTM besides those! |
cbuescher
force-pushed
the
feature/query-refactoring-dismax
branch
from
9f6d3dc to
5c78cf9
Compare
|
Addressed last comment. I wouldn't force all query builders to overwrite validation method at this point. As far as I can see, only RangeQueryBuilder and BaseTermQueryBuilder really overwrite this in a meaningful way so far. What about even removing all the empty implementations in the spirit of "less code is good" and only add it if needed later? |
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#11703
cbuescher
force-pushed
the
feature/query-refactoring-dismax
branch
from
5c78cf9 to
3f4b493
Compare
|
Rebased, added validate() to builder and introduced default tie breaker as a constant. |
|
LGTM |
…smax Query Refactoring: DisMaxQueryBuilder 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#11703
…ring-dismax Query Refactoring: DisMaxQueryBuilder 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
PR goes agains query-refactoring feature branch.