Refactors WrapperQueryBuilder and Parser by alexksikes · Pull Request #12037 · elastic/elasticsearch

alexksikes · 2015-07-04T21:26:22Z

Relates to #10217

This PR is against the query-refactoring branch.

javanna · 2015-07-06T08:41:41Z

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

why was this changed?

javanna · 2015-07-06T08:51:44Z

left some comments

alexksikes · 2015-07-13T15:20:10Z

@javanna Thanks for the review. I updated the PR.

MaineC · 2015-07-14T08:20:30Z

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

See the comment I added to the original discussion on the outdated diff.

MaineC · 2015-07-14T08:21:57Z

Added some comments.

alexksikes · 2015-07-15T09:47:31Z

@MaineC Thanks I have addressed your comment.

javanna · 2015-07-15T15:56:58Z

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

we can just pass in null here?

alexksikes · 2015-07-24T14:59:15Z

@javanna I addressed the comments and rebased. You can take a look. Thank you.

javanna · 2015-07-24T15:04:25Z

docs/reference/migration/migrate_2_0.asciidoc

the method names start with a lowercase w. Shouldn't we have both changes here? QueryBuilders and WrapperQueryBuilder constructor? those are both public.

sure I'll make that change

javanna · 2015-07-24T15:08:02Z

I did another round, left some comments, also we need some discussion on when we effectively parse the binary query here...

alexksikes · 2015-07-30T13:06:11Z

@javanna I rebased it to incorporate the latest changes. Something I had to do is move supportsBoostAndQueryName() method to AbstractQueryBuilder. Please see comment in code. Thank you.

javanna · 2015-08-04T09:24:33Z

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

//norelease is used for code that can be removed later on in the refactoring. I don't think it should be used here, or at least I don't see how we can remove this method if we introduce it here. The idea for supportsBoostAndQueryName was to be only a test method to "mark" few exceptional queries, it wasn't meant to be moved to prod code ever.

The reason why you did this is because the wrapped query has its own boost that gets overwritten by the default 1.0f boost of the outer query. I tend to think that this is more a test problem, and the doAssertLuceneQuery in WrapperQueryBuilderTest should be adjusted. Maybe there are other ways or ideas, but we should not move this supportsBoostAndQueryName to prod code I think.

AbstractQueryBuilder#toQuery does set a boost unless it is disabled. If I remember correctly I think that was the idea of being to disable boost as part of the actual query builder, not just in the test.

I am on the fence on this. We have one query with boost: 2.0 which holds an inner query with boost: 3.0. Let's say that the final lucene query will be the lucene representation of the inner query. What about the final boost, should it be 2.0 or 3.0? Right now the boost set on the main query overrides the inner boost. I am not sure if this is the right behaviour, which would simply require to adjust the doAssertLuceneQuery here to take this behaviour into account. But supportsBoostAnQueryName should not be moved to prod code. I would go back to having it in test and I would adjust the doAssertLuceneQuery here to work properly without needing to disable anything. Then we can open an issue and discuss what the final boost should be.

I see that you moved back to supportsBoostAndQueryName in BaseQUeryTestCase. The way you do it now makes perfect sense.

javanna · 2015-08-04T09:27:59Z

I had a look, left a comment, thinking more about it this query will give us headache, especially in relation to #12527. This is the only query that needs a QueryParseContext in toQuery, along with the QueryShardContext. We will need to discuss this with @cbuescher and see what we can do about it.

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

@alexksikes could you please rebase and address comments?

alexksikes · 2015-08-11T15:45:59Z

@javanna I rebased and addressed the comments. Thanks for the review.

javanna · 2015-08-13T11:33:24Z

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

why was this removed?

javanna · 2015-08-13T15:17:11Z

did another round, left a few comments

alexksikes · 2015-08-15T12:29:38Z

@javanna comments addressed. Thanks for the review.

javanna · 2015-08-17T10:04:15Z

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

can you replace with expected.setBoost(AbstractQueryBuilder.DEFAULT_BOOST) for clarity? I will also open up an issue for discussion on this boost matter.

javanna · 2015-08-17T10:04:48Z

I did a final round, one old comment on parsing wasn't addressed yet, plus left a minor comment on testing.

alexksikes · 2015-08-17T17:19:23Z

@javanna addressed the comments and rebased. Thanks for the review.

javanna · 2015-08-17T17:29:12Z

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

I think the field name should be query in the error message

javanna · 2015-08-17T17:31:11Z

LGTM besides the tiny comment I left

Relates to elastic#10217 Closes elastic#12037 This PR is against the query-refactoring branch.

javanna · 2015-08-28T10:20:38Z

This change is breaking for the java API as it removes the WrapperQueryBuilder(byte[] source, int offset, int length) constructor, use WrapperQueryBuilder(byte[] source) instead.

alexksikes added review and removed :Query DSL labels Jul 4, 2015

javanna reviewed Jul 6, 2015
View reviewed changes

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

Copy link
Copy Markdown

Contributor

javanna Jul 6, 2015

Choose a reason for hiding this comment

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

why was this changed?

clintongormley mentioned this pull request Jul 6, 2015

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

Closed

MaineC reviewed Jul 14, 2015
View reviewed changes

MaineC assigned alexksikes Jul 14, 2015

javanna reviewed Jul 15, 2015
View reviewed changes

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

Copy link
Copy Markdown

Contributor

javanna Jul 15, 2015

Choose a reason for hiding this comment

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

we can just pass in null here?

alexksikes force-pushed the feature/query-refactoring-wrapper-query branch from 04080fe to fb61ce8 Compare July 24, 2015 14:58

javanna reviewed Jul 24, 2015
View reviewed changes

alexksikes force-pushed the feature/query-refactoring-wrapper-query branch from fb61ce8 to d6687d2 Compare July 30, 2015 13:04

javanna reviewed Aug 4, 2015
View reviewed changes

alexksikes force-pushed the feature/query-refactoring-wrapper-query branch from d6687d2 to 328f7b4 Compare August 11, 2015 15:45

javanna mentioned this pull request Aug 13, 2015

Refactoring of Indices Query #12031

Merged

javanna reviewed Aug 13, 2015
View reviewed changes

core/src/test/java/org/elasticsearch/index/query/QueryFilterBuilderTest.java Outdated

Copy link
Copy Markdown

Contributor

javanna Aug 13, 2015

Choose a reason for hiding this comment

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

why was this removed?

javanna reviewed Aug 17, 2015
View reviewed changes

alexksikes force-pushed the feature/query-refactoring-wrapper-query branch from 49e193e to c1cc5f4 Compare August 17, 2015 17:18

javanna reviewed Aug 17, 2015
View reviewed changes

Refactors WrapperQueryBuilder and Parser

afcbd29

Relates to elastic#10217 Closes elastic#12037 This PR is against the query-refactoring branch.

alexksikes force-pushed the feature/query-refactoring-wrapper-query branch from c1cc5f4 to afcbd29 Compare August 17, 2015 17:54

alexksikes merged commit afcbd29 into elastic:feature/query-refactoring Aug 17, 2015

alexksikes deleted the feature/query-refactoring-wrapper-query branch August 17, 2015 17:54

alexksikes restored the feature/query-refactoring-wrapper-query branch August 17, 2015 19:26

alexksikes added a commit to alexksikes/elasticsearch that referenced this pull request Aug 17, 2015

Refactors WrapperQueryBuilder and Parser

3ebf7a4

Relates to elastic#10217 Closes elastic#12037 This PR is against the query-refactoring branch.

javanna added >breaking and removed review labels Aug 28, 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 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Jul 6, 2015

Uh oh!

alexksikes commented Jul 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaineC commented Jul 14, 2015

Uh oh!

alexksikes commented Jul 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexksikes commented Jul 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Jul 24, 2015

Uh oh!

alexksikes commented Jul 30, 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 4, 2015

Uh oh!

javanna commented Aug 7, 2015

Uh oh!

alexksikes commented Aug 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Aug 13, 2015

Uh oh!

alexksikes commented Aug 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Aug 17, 2015

Uh oh!

alexksikes commented Aug 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Aug 17, 2015

Uh oh!

javanna commented Aug 28, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants