Refactors TermsQueryBuilder and Parser by alexksikes · Pull Request #12042 · elastic/elasticsearch

alexksikes · 2015-07-06T07:34:44Z

Refactors TermsQueryBuilder and Parser for #10217. Also removes deprecated
TermsLookupQuery class and unused or deprecated options execution,
min_should_match, lookup_cache.

This PR is against the query-refactoring branch.

javanna · 2015-08-07T15:35:33Z

core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java

not a big fan of these two new methods to be honest. Maybe since they are needed in one query only we can loop there instead?

not a big fan either, but note we do use convertToStringListIfBytesRefList in two places. So I moved them to the builder instead.

so the alternative is looping in two places instead in TermsQueryBuilder? I think I would go for that.

javanna · 2015-08-07T16:07:31Z

I did a first review, can you please rebase and address comments? Also, I would like to know from @s1monw and @jpountz what they think about having a Client within IndexQueryParseService, like we are adding here, so that we can retrieve it through QueryShardContext and execute the needed get calls (still on the data node). Wonder if there's any other/better way, I couldn't come up with any.

alexksikes · 2015-08-12T13:33:53Z

@javanna Thanks for the review. Just a couple of notes here.

execution and lookup_cache although available in the builder, are never used in the parser.
min_should_match is used in the parser but marked as deprecated, and can never be set in the builder.
TermsLookupQueryBuilder is a pure delegate to TermsQueryBuilder, and Queries.termsLookupQuery directly creates a new TermsQueryBuilder.

I think we should only document the change in migrate doc for 2) and 3)

alexksikes · 2015-08-12T15:04:39Z

@javanna rebased and addressed comments. We should still figure out how best to mock a handleTermsLookup or a Client in our test. This will also be useful for queries such as MLT. Thanks for the review.

javanna · 2015-08-13T11:07:41Z

core/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java

I think this is ok used this way, @cbuescher do you agree ?

I think this is ok, the isFilter() flag should be set when parent queries that are already using toQuery() produce some inner query in a filter context.

cool then remove the NOCOMMIT here?

cbuescher · 2015-08-25T09:35:35Z

core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java

Same here as above, since both methods are related and read similar.

javanna · 2015-08-25T09:57:59Z

This is much closer now I did another round of review and left some comments

javanna · 2015-08-25T12:29:27Z

core/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTest.java

can we add a test that verifies that if you specify both terms_lookup and values on the REST layer we get back a validation error? This part in the parser seems a bit tricky, I want to make sure we don't lose this.

alexksikes · 2015-08-25T16:13:37Z

@javanna I addressed all the comments. Hopefully it should be good to go. Thank you for the review.

javanna · 2015-08-26T07:19:25Z

core/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java

Iterable<Object>

javanna · 2015-08-26T07:37:14Z

I did a last review round, left some minor comments. LGTM otherwise

javanna · 2015-08-26T10:43:40Z

core/src/main/java/org/elasticsearch/indices/cache/query/terms/TermsLookup.java

last super small comment, can we remove the get prefix from these setters from now, for consistency reasons?

I'd like to but it would break bwc. Mark them as deprecated?

it doesn't break anything given that this class was only used internally, it was not part of the java api. Only this change makes it part of the java api.

javanna · 2015-08-26T10:44:00Z

left one tiny comment, LGTM though

Refactors TermsQueryBuilder and Parser for elastic#10217. This PR is against the query-refactoring branch. Closes elastic#12042

alexksikes added review labels Jul 6, 2015

clintongormley mentioned this pull request Jul 6, 2015

Refactor parsing of queries/filters, aggs, suggester APIs #10217

Closed

javanna reviewed Aug 7, 2015
View reviewed changes

alexksikes force-pushed the feature/query-refactoring-terms-query branch from 0815151 to 671e7c7 Compare August 12, 2015 15:03

alexksikes force-pushed the feature/query-refactoring-terms-query branch from 671e7c7 to 3651c99 Compare August 13, 2015 10:31

javanna reviewed Aug 13, 2015
View reviewed changes

cbuescher reviewed Aug 25, 2015
View reviewed changes

javanna reviewed Aug 25, 2015
View reviewed changes

alexksikes force-pushed the feature/query-refactoring-terms-query branch from b92d0c3 to 9b8eef6 Compare August 25, 2015 16:10

javanna reviewed Aug 26, 2015
View reviewed changes

core/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java Outdated

Copy link
Copy Markdown

Contributor

javanna Aug 26, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iterable<Object>

javanna reviewed Aug 26, 2015
View reviewed changes

Refactors TermsQueryBuilder and Parser

1af0a39

Refactors TermsQueryBuilder and Parser for elastic#10217. This PR is against the query-refactoring branch. Closes elastic#12042

alexksikes force-pushed the feature/query-refactoring-terms-query branch from 7f3d530 to 1af0a39 Compare August 26, 2015 11:59

alexksikes merged commit 1af0a39 into elastic:feature/query-refactoring Aug 26, 2015

alexksikes deleted the feature/query-refactoring-terms-query branch August 26, 2015 12:02

javanna removed the review label Aug 26, 2015

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query Refactoring labels Feb 14, 2018

Conversation

alexksikes commented Jul 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Aug 7, 2015

Uh oh!

alexksikes commented Aug 12, 2015

Uh oh!

alexksikes commented Aug 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Aug 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexksikes commented Aug 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Aug 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Aug 26, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants