Query refactoring: MatchAllQueryBuilder

[cbuescher]

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]

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

[cbuescher]

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.

[javanna]

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?

[javanna]

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.

[javanna]

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]

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

[javanna]

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.

[cbuescher]

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?

[cbuescher]

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?

[javanna]

yes sounds good

[javanna]

thanks @cbuescher left a few comments

[cbuescher]

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]

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

[javanna]

good point

[cbuescher]

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]

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

[cbuescher]

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.

[javanna]

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

[cbuescher]

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]

@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]

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

[javanna]

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]

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]

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

[cbuescher]

Obviously. They are gone now.

[javanna]

LGTM

[javanna]

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

[cbuescher]

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