Query refactoring: MatchAllQueryBuilder by cbuescher · Pull Request #10668 · elastic/elasticsearch

cbuescher · 2015-04-20T08:14:02Z

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.

javanna · 2015-04-20T09:54:38Z

src/main/java/org/elasticsearch/index/query/MatchAllQueryBuilder.java

why package private? shall we add a getter for it instead?

I think we only need to access the fields from toQuery(), equals() or from the corresponding test class. I talked to Lee about getters vs. package private, had the feeling we could go with pp here. In this class it would be only one getter, in some of the classes with more fields it's a lot more and just bloats the builders interface I'd say, but I'm also good with getters.

the getter doesn't seem to be needed either. I am leaning towards not adding getters in this round unless they are needed, which they don't seem to. Thoughts?

we were commenting at the same time :) lemme reply to your last comment. here you are using the package private field to set it, that doesn't make sense as we have a setter for it. There is no reason here to hide anything, this is public api exposed through java API. Lee's concerns were more around toQuery visibility, which is another story I think.
I agree on not adding getters as part of these changes simply because we would have to follow setter with no set prefix convention etc., it's out of the scope of this change. But users might want to retrieve what they set at some point...in general it makes no sense to be able to set stuff that you cant retrieve.

mmm ok looking again... I think I am changing my mind. It feels bad that we use package visibility to retrieve stuff from a test, just because we don't want to add getters that would be useful for users too. Thoughts?

cbuescher · 2015-04-20T14:20:38Z

Added getter, pulled out test base class and removed usage of jdk 1.8 api.

javanna · 2015-04-20T14:36:52Z

src/main/java/org/elasticsearch/index/query/MatchAllQueryBuilder.java

I still wonder if for this simple case we should just to return boost != other.boost , doesn't make sense to call Objects.equals to me.

Agreed and changed. Should we do so for all primitive types where null checks are not necessary, or what do you suggest for other queries?

Thinking about it, using "==" is probably not the best way for float primitives. The internal Float.equals() uses floatToIntBits() just like the hashCode(), maybe we should better use that for consitency?

yes sounds good

javanna · 2015-04-20T14:41:16Z

thanks @cbuescher left a few comments

… to base test class

cbuescher · 2015-04-20T16:20:19Z

Pulled some more of the test setup in the BaseQueryTestCase, I was able to do a generic testFromXContent method but since not all queries will be Streamable from the start and to the readFrom/writeTo won't be in the interface until all classes implemen this I wasn't able to pull that up. Also, tests for toQuery() will probably do different assertions on the produced lucene queries, so I left it in the individual test.

MaineC · 2015-04-21T06:50:04Z

src/main/java/org/elasticsearch/index/query/MatchAllQueryBuilder.java

Calling this.equals(null) will yield a NullPointerException - shouldn't this return false?

cbuescher · 2015-04-21T09:40:35Z

Small change to the equal() method to prevent possible npe. Other than that I went with reverting the base test so that injector is not static again because I'm not sure if that would work when multiple tests are running. Maybe we should revisit that question once we have more tests extending this base test.

javanna · 2015-04-21T11:59:13Z

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

shall we rename T to QB ? Also can it simply be <QB extends QueryBuilder & Streamable> ? I think this might help generifying more code.

Renaming works for me, but having the type for the testQuery field is good because only then we can use the type dependend getters for assertions in the actual test class. Otherwise we need cast there.

having the type is great, my suggestion was different but it wasn't shown properly... it should be clear now

cbuescher · 2015-04-21T14:20:07Z

Refactored the base test class even more, but I'm not sure if the test code is now spread too much between the base test and the abstract methods. The serialization test can be generalized even more once we put Streamable in the QueryBuilder interface. Not sure if the "testToQuery()" test is going to be very useful for more complex queries because it seems tricky to do check internals of the constructed lucene queries.

cbuescher · 2015-04-21T16:44:16Z

@javanna Was able to follow your suggestions on pulling the serialization test up to base test class, this works great now, was also able to use this base class with the other queries I currently have open PRs for (IdsQuery, TermQuery). Let me know what you think.

javanna · 2015-04-22T07:26:45Z

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

nitpick: can you move the afterClass up close to the beforeClass?

javanna · 2015-04-22T07:47:40Z

looks great, left a bunch of minor comments, @dakrone can you have a look too please so we can get it in and move on with other queries?

javanna · 2015-04-22T08:48:57Z

src/test/java/org/elasticsearch/index/query/MatchAllQueryBuilderTest.java

Change looks good but I would also do: public void assertLuceneQuery(MatchAllQueryBuilder queryBuilder, Query query) throws IOException { and call toQuery in the parent class.just provide the source query as argument and the result of toQuery, then the assert prefix makes more sense too for this method

javanna · 2015-04-22T12:58:12Z

src/main/java/org/elasticsearch/index/query/MatchAllQueryBuilder.java

did you copy paste from my broken code? :) I think we can remove the outer parentheses :)

Obviously. They are gone now.

javanna · 2015-04-22T13:00:57Z

LGTM

javanna · 2015-04-22T22:49:56Z

this got merged right @cbuescher ? if so can you close it?

cbuescher · 2015-04-23T07:35:13Z

Yes, merged on feature branch with eea7ee5, sorry I didn't close yet.

Query refactoring: MatchAllQueryBuilder

6071c77

kevinkluge added the in progress label Apr 20, 2015

cbuescher added :Query Refactoring labels Apr 20, 2015

javanna self-assigned this Apr 20, 2015

javanna reviewed Apr 20, 2015
View reviewed changes

javanna mentioned this pull request Apr 20, 2015

Refactor MatchAllQueryBuilder, TermQueryBuilder, IdsQueryBuilder #10454

Closed

Add getters, extract BaseQueryTest class

ff6743d

cbuescher force-pushed the feature/query-refactoring-matchall branch from 2904248 to ff6743d Compare April 20, 2015 14:22

javanna reviewed Apr 20, 2015
View reviewed changes

Change equals check for float primitive, extracting common test setup…

7438190

… to base test class

Revert test setup in @BeforeClass

c1b369a

MaineC reviewed Apr 21, 2015
View reviewed changes

Change equals() to prevent possibility of npe

e587336

javanna reviewed Apr 21, 2015
View reviewed changes

Pulled out all of serialization test to base class, added javadoc

a2e665b

javanna reviewed Apr 22, 2015
View reviewed changes

Christoph Büscher added 2 commits April 22, 2015 10:37

Renamed methods, directly testing toQuery() on the initial testQuery

0b5ac31

Renamed test query creation method

2cfe30c

javanna reviewed Apr 22, 2015
View reviewed changes

Change abstract method visibility and removing common setup

84e3d23

javanna reviewed Apr 22, 2015
View reviewed changes

Deleted superflous brackets

6d41e1f

cbuescher closed this Apr 23, 2015

kevinkluge removed in progress labels Apr 23, 2015

clintongormley mentioned this pull request Sep 10, 2015

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

Closed

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

cbuescher deleted the feature/query-refactoring-matchall branch March 20, 2024 20:15

Conversation

cbuescher commented Apr 20, 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!

cbuescher commented Apr 20, 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 Apr 20, 2015

Uh oh!

cbuescher commented Apr 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Apr 21, 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!

cbuescher commented Apr 21, 2015

Uh oh!

cbuescher commented Apr 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Apr 22, 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 Apr 22, 2015

Uh oh!

javanna commented Apr 22, 2015

Uh oh!

cbuescher commented Apr 23, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants