Query refactoring: IdsQuery #10670
Query refactoring: IdsQuery #10670
Conversation
|
|
||
| private String queryName; | ||
| String queryName; |
There was a problem hiding this comment.
comment about visibility too, why not private with getters?
cbuescher
force-pushed
the
feature/query-refactoring-idsquery
branch
2 times, most recently
from
7eacb55 to
6551fe7
Compare
|
I rebased this PR on current state of feature branch, added the changes to BaseQuery and the QueryBuilder interface that we already discussed in #10669. |
| @@ -108,4 +149,84 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep | |||
| protected String parserName() { | |||
| return IdsQueryParser.NAME; | |||
| } | |||
|
|
|||
| public Query toQuery(QueryParseContext parseContext) throws IOException, QueryParsingException { | |||
| ArrayList<BytesRef> ids = new ArrayList<>(); | |||
There was a problem hiding this comment.
can we init the arraylist providing its size as input since we know it? also maybe use List on the left side?
|
I quickly went through it and it LGTM besides the few minor comments I left. @dakrone can you have another look too please? |
cbuescher
force-pushed
the
feature/query-refactoring-idsquery
branch
from
6551fe7 to
a00966b
Compare
|
Added changes adressing the comments and rebased on current feature branch. Will wait for another look from @dakrone. |
cbuescher
force-pushed
the
feature/query-refactoring-idsquery
branch
from
a00966b to
81e1ca9
Compare
|
|
||
| private String queryName; | ||
|
|
||
| public IdsQueryBuilder(String... types) { | ||
| this.types = types == null ? null : Arrays.asList(types); | ||
| this.types = (types == null || types.length == 0) ? new ArrayList<String>() : Arrays.asList(types); |
There was a problem hiding this comment.
you dont need the type here new ArrayList<>() right? your IDE should give you a warning for this type of things.
There was a problem hiding this comment.
Actually I get a compile error when I don't use the type here. Maybe because of the ternary operator here.
|
I just had another look, left a few comments |
cbuescher
force-pushed
the
feature/query-refactoring-idsquery
branch
2 times, most recently
from
b749dbc to
475c083
Compare
|
Went over the comments and added serialization of string lists to the StreamInput/Output classes, also added test for checking exception on missing value field. |
cbuescher
force-pushed
the
feature/query-refactoring-idsquery
branch
from
475c083 to
73044ac
Compare
| * @throws IOException | ||
| */ | ||
| @Test(expected=QueryParsingException.class) | ||
| public void idsNotProvided() throws IOException { |
There was a problem hiding this comment.
can you prepend the name of the method with test, just to make sure?
cbuescher
force-pushed
the
feature/query-refactoring-idsquery
branch
from
297dab3 to
f4df5a0
Compare
|
Just rebased this on current state of feature branch which now includes the addition if streaming string lists & adressed the last two comments. |
|
LGTM |
Split the parse(QueryParseContext ctx) method into a parsing and a query building part, adding Streamable for serialization and hashCode(), equals() for better testing. Add basic unit test for Builder and Parser. Closes #10670
Split the parse(QueryParseContext ctx) method into a parsing and a query building part, adding Streamable for serialization and hashCode(), equals() for better testing. Add basic unit test for Builder and Parser. Closes elastic#10670
clintongormley
added
:Search/Search
Split the parse(QueryParseContext ctx) method into a parsing and a query building part, adding Streamable for serialization and hashCode(), equals() for better testing.
Add basic unit test for Builder and Parser.
PR goes agains query-refacoring feature branch.