Query Refactoring: Merging Parser and Builder classes

[cbuescher]

A first intermediate step in the query parsing refactoring proposed in #10217. It merges the current parser code from *QueryParser into the corresponding Builder & deleting the former.

In some cases the current implementation of the Parsers require a no-arg constructor for the guice injection. In cases where the original Builder class has final fields these are set to either null or a default value where possible.

[dakrone]

Part of this has me wondering if, since this is a large change, we should go ahead and do the rename similar to what Simon did in #9901 and go ahead and rename all the FooQueryBuilders to FooQuery.

Some potential side effects:

• There can be name conflicts between Lucene queries and ES queries (like TermQuery)
• Potential confusion if we remove the Builder pattern completely

On the other hand, this does allow us to move away from the Builder pattern (if that is something we want to do) and FooQuery is a clearer and more succinct way of naming the queries.

I think the name conflicts issue is not particularly pressing, since end users are not expected to ever have to deal with Lucene queries, so I think that's fine to deal with.

@javanna @cbuescher what do you guys think? I'd like to get the big name-change PRs out of the way first so we can start working on individual queries.

[cbuescher]

We could also rename FooQueryBuilder to FooQuery and add a deprecated
"FooQueryBuilder extends FooQuery" class with all old style constructors
delegating to the renamed class. Introduces lots of new classes though,
maybe not necessary if breaking the client API is not an issue here.

On Mon, Mar 30, 2015 at 10:27 PM, Lee Hinman [email protected]
wrote:

Part of this has me wondering if, since this is a large change, we should
go ahead and do the rename similar to what Simon did in #9901
#9901 and go ahead and
rename all the FooQueryBuilders to FooQuery.

Some potential side effects:

• There can be name conflicts between Lucene queries and ES queries
  (like TermQuery)
• Potential confusion if we remove the Builder pattern completely

On the other hand, this does allow us to move away from the Builder
pattern (if that is something we want to do) and FooQuery is a clearer
and more succinct way of naming the queries.

I think the name conflicts issue is not particularly pressing, since end
users are not expected to ever have to deal with Lucene queries, so I think
that's fine to deal with.

@javanna https://github.com/javanna @cbuescher
https://github.com/cbuescher what do you guys think? I'd like to get
the big name-change PRs out of the way first so we can start working on
individual queries.

—
Reply to this email directly or view it on GitHub
#10324 (comment)
.

Christoph Büscher

[dakrone]

I think I'm +1 on renaming to FooQuery and adding a deprecated FooQueryBuilder that can be removed at a later point. I think it might help ease the transition for the tests also.

[javanna]

I think the main concern we have at this stage is backwards compatibility, others are secondary aspects. These classes are not going to be pure builders anyways... I am either for renaming or keeping names as they are, wouldn't rename and keep builder classes only for bw comp. We can definitely postpone this change/decision.

[dakrone]

+1 to postpone a rename

[dakrone]

LGTM

[cbuescher]

Open question: There are several places where I had to move no-arg constructor from FooParser to Builder where I had to instantiate a 'final' member to null. At other places I chose to remove the 'final' from the member. Not sure whats better here, maybe this changes in further refactoring anyway?

[dakrone]

Yeah, I think in further refactoring this will end up not being a final method and we won't have to set it for this.