Refactors PrefixQuery by alexksikes · Pull Request #12032 · elastic/elasticsearch

alexksikes · 2015-07-04T15:45:04Z

Relates to #10217

This PR is against the query-refactoring branch.

javanna · 2015-07-06T08:00:43Z

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

out of curiosity, why do we have a newRandomQueryBuilder method here?

I was actually thinking that this could later be moved as a helper method in the BaseQueryTestCase class, something that returns a random field name together with its value.

ah ok I see

javanna · 2015-07-06T08:03:09Z

left a couple of small comments

cbuescher · 2015-07-06T10:00:09Z

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

Should this be rewrite field instead of fieldName (mentioned twice)

Thanks good catch!

alexksikes · 2015-07-07T20:56:23Z

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

javanna · 2015-07-08T08:14:00Z

looks good to me, @cbuescher can you have a final look plese?

cbuescher · 2015-07-08T08:45:05Z

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

This can't happen, and if for some reason after the switch fielname/value are unassigned the returned query will throw exceptions at some point (validation, doXContent, toQuery), so given this is a test method I'd remove it.

cbuescher · 2015-07-08T08:48:35Z

Did another round of reviews, left just two question/comments regarding setup of test query, otherwise looks good.

The parser takes an Object value, so should the builder. Relates to elastic#12032

alexksikes · 2015-07-09T09:05:13Z

@javanna @cbuescher Thanks for the review. I rebased the PR and updated it.

cbuescher · 2015-07-09T16:05:09Z

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

Could also reuse the BaseTermQueryBuilder#validate here? I realize we agreed on always overwriting this for the AbstractQueryBuilder, but since this just repeats the super types implementation, maybe we should call it?

definitely! +1

javanna · 2015-07-09T16:19:33Z

left a couple of minor comments, I would wait for #12124 (or corresponding adapted PR) before getting this in though.

cbuescher · 2015-07-09T16:21:22Z

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

I don't know if it's possible to also extend the generic BaseTermQueryTestCase here, since a lot of the setup looks similar. That one is also testing Boolean as a value type while this test adds Date there. Maybe the creation of the expected query gets a bit too complicated, but the two existing Test classes (SpanTermQueryBuilderTest, TermQueryBuilderTest) are quiet small that way. Might be worth looking into merging, unless you already tried and found it to hard to read afterwards.

Patially revoking my last comment, I would only try this and add the inheritance level in the test if in the end you decide to have PrefixQueryBuilder extend BaseTermQueryBuilder. Thinking about it a bit more, I'm not sure if this is maybe growing to complex at this point and a bit of code duplication should be traded for ease of understanding.

+1 for using BaseTermQueryTestCase

if PrefixQueryBuilder won't extend BaseTermQueryBuilder anymore I think we can't extend BaseTermQueryTestCase either here

It might be possible, but I would try to avoid it in this case. I would go for either using both BaseTerm classes or none.

OK then we go for none? I am ok either way.

I would leave it as-is, it needs to extend BaseQueryTestCase

cbuescher · 2015-07-09T16:34:54Z

Also did another round and left a couple of comments.

Relates elastic#12032

alexksikes · 2015-07-13T12:48:56Z

@javanna Thanks for the review. I rebased and updated the PR accordingly.

javanna · 2015-07-14T06:17:01Z

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

we can revert these two changes no given that we dont' extend from this class anymore?

I think we should allow these to be overridden, but I'm happy to make the change.

well we don't need to extend this anywhere, plus these changes don't have anything to do with this PR at this point, would prefer to leave them out.

sure I'll revert this change.

alexksikes · 2015-07-15T10:47:30Z

@javanna Thanks for the review. We need to address #11865 (comment) before it can be pushed though, and waiting on #12204 as well.

Relates #12032 Closes #12204

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

alexksikes · 2015-07-24T09:49:11Z

@javanna it's rebased, you can take a look. Thank you.

javanna · 2015-07-24T09:52:59Z

LGTM

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

alexksikes added review labels Jul 4, 2015

javanna reviewed Jul 6, 2015
View reviewed changes

clintongormley mentioned this pull request Jul 6, 2015

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

Closed

cbuescher reviewed Jul 6, 2015
View reviewed changes

cbuescher reviewed Jul 8, 2015
View reviewed changes

alexksikes added a commit to alexksikes/elasticsearch that referenced this pull request Jul 8, 2015

Fix PrefixQueryBuilder to support an Object value

d69e2af

The parser takes an Object value, so should the builder. Relates to elastic#12032

alexksikes mentioned this pull request Jul 8, 2015

Fix PrefixQueryBuilder to support an Object value #12124

Closed

alexksikes force-pushed the feature/query-refactoring-prefix branch 2 times, most recently from b882a2d to 0aaac85 Compare July 9, 2015 09:04

cbuescher reviewed Jul 9, 2015
View reviewed changes

alexksikes force-pushed the feature/query-refactoring-prefix branch from 0aaac85 to 4997b9e Compare July 13, 2015 12:40

alexksikes added a commit to alexksikes/elasticsearch that referenced this pull request Jul 13, 2015

PrefixQueryParser takes a String as value like its Builder

3f43080

Relates elastic#12032

alexksikes mentioned this pull request Jul 13, 2015

PrefixQueryParser takes a String as value like its Builder #12204

Closed

javanna reviewed Jul 14, 2015
View reviewed changes

MaineC assigned alexksikes Jul 14, 2015

alexksikes added a commit that referenced this pull request Jul 16, 2015

PrefixQueryParser takes a String as value like its Builder

165ee4a

Relates #12032 Closes #12204

Refactors PrefixQuery

37c6347

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

alexksikes force-pushed the feature/query-refactoring-prefix branch from 8a75083 to 37c6347 Compare July 24, 2015 09:48

alexksikes merged commit 37c6347 into elastic:feature/query-refactoring Jul 24, 2015

alexksikes deleted the feature/query-refactoring-prefix branch July 24, 2015 10:06

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

Refactors PrefixQuery

b315e48

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

clintongormley removed the review label Aug 7, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Jul 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexksikes commented Jul 7, 2015

Uh oh!

javanna commented Jul 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Jul 8, 2015

Uh oh!

alexksikes commented Jul 9, 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 Jul 9, 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!

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!

cbuescher commented Jul 9, 2015

Uh oh!

alexksikes commented Jul 13, 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!

alexksikes commented Jul 15, 2015

Uh oh!

alexksikes commented Jul 24, 2015

Uh oh!

javanna commented Jul 24, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants