Query refactoring: QueryFilterBuilder and Parser #11729
Conversation
| if (queryName == null) { | ||
| return new QueryFilterBuilder(innerQueryBuilder); | ||
| } else { | ||
| return new FQueryFilterBuilder(innerQueryBuilder).queryName(queryName); |
There was a problem hiding this comment.
I wish we could avoid having this if, it's a PITA really, I would consider breaking java api bw comp here and move the queryName to FQueryFilterBuilder so the code is more human. Thoughts?
There was a problem hiding this comment.
What if we accept a little bit of code duplication and separate FQueryFilterBuilder and QueryFilterBuilder in so far as they both extend QueryBuilder. Then FQueryBuilder would be the one that has a queryName. The only thing that remains slightly odd then is that when rendering out, FQuery still uses the QueryFilterBuilder name ("query") because it essentially wraps it. But thats the state of the DSL at the moment.
|
left one comment, looks good! |
cbuescher
force-pushed
the
feature/query-refactoring-fquery
branch
2 times, most recently
from
a5ab7d5 to
8f5cca5
Compare
|
With last commit I moved the queryName member which was just used for FQuery from the QueryFilterBuilder to the new FQueryFilterBuilder that we introduced on the feature branch to separate the two variants of this builder. This creates some code duplication but avoids the former confusion between the two cases. Only problem here might be that for the Java-API we slightly break the existing QueryFilterBuilder because now it doesn't support queryName() any longer, but that part conceptually belonged to FQuery anyway. |
| /** | ||
| * test wrapping an inner filter that returns null also returns <tt>null</null> to pass on upwards | ||
| */ | ||
| public void testInnerQueryReturnsNull() throws IOException { |
There was a problem hiding this comment.
we should @Test to forbidden API
|
LGTM besides the few comments I left. Looking forward to seeing these two filters removed to be honest, with the merge of filters and queries they make very little sense (only bw comp). I think we need to create a migrate_query_refactoring.asciidoc page and add our first breaking change (only for the java api). |
|
@javanna added asciidoc to track breaking changes on the feature branch, mind to have a look if you think this is a good location and this is verbose enough to keep track? Also, this is inteded for internal use mostly, should be merged with other migration doc when we merge with master, right? |
|
looks great. Yes it will eventually be merged but given that we have one file per version and we don't know yet when we will merge the branch, let's just keep a separate file and decide later on what to do with it. |
cbuescher
force-pushed
the
feature/query-refactoring-fquery
branch
from
feb983f to
f12c4e2
Compare
Moving the query building functionality from the parser to the builders new toQuery() method analogous to other recent query refactorings. In this case this also includes FQueryFilterParser, since both queries are closely related. Relates to elastic#10217 Closes elastic#11729
cbuescher
force-pushed
the
feature/query-refactoring-fquery
branch
from
f12c4e2 to
b6cdc46
Compare
…uery Query refactoring: QueryFilterBuilder and Parser
|
This change is breaking for the java api as the setter |
Moving the query building functionality from the parser to the builders new toQuery() method analogous to other recent query refactorings. In this case this also includes FQueryFilterParser, since both queries are closely related. Relates to elastic#10217 Closes elastic#11729
…ring-fquery Query refactoring: QueryFilterBuilder 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. In
this case this also includes FQueryFilterParser, since both queries are
closely related.
Relates to #10217
Closes #11729