Query Refactoring: DisMaxQueryBuilder and Parser#11703
Conversation
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 |
e3ecde7 to
8fa73da
Compare
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.
8fa73da to
9f6d3dc
Compare
|
left a few minor comments, LGTM besides those! |
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
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
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.