|
Yonik,
I remember that we talked briefly about a QueryFactory in Atlanta and This looks useful – Michael or Yonik, what do you think?
What's wrong with just subclassing QueryParser and overriding the desired methods? Either way someone wanting to provide custom Query implementations will be writing effectively the same code, just with more indirection with this method.
I think subclassing would work fine too, as long as we fix QueryParser so that instead of doing things like: TermQuery q = new TermQuery(...);
it would do: q = newTermQuery(...); and then define a newTermQuery(...) method which a subclass could then override. Is that what you have in mind Erik? John does this sound OK? > What's wrong with just subclassing QueryParser and overriding the desired methods?
For what it's worth, I find the idea of a separate class appealing from an API documentation design standpoint. This usage of QueryParser is relatively arcane. Hiding all these methods away in a separate class means less clutter and less effort for most people scanning through the API docs of an important, commonly used class. I think the idea of sub-classing makes more sense when you're talking about a QueryParser. The parser includes, mainly, two logics - parsing a query into query "segments" or "nodes" and then creating the appropriate Lucene Query instance for each sengment/node.
By modifying QueryParser so that it uses protected callback methods for creating the appropriate Query instance for different node types, you get a very clean and elegant query parsing solution. All sub-classes can share the same parsing rules (which generally don't change), and just instantiating their own TermQuery or PrefixQuery instances. Wouldn't you need to do that anyway with the Query factory? I mean, what if I want to create all the query types like the default implementation, but change the PrefixQuery instance to something I wrote? Wouldn't I need to extend the default query factory? Sounds exactly like extending QueryParser to me. I recently implemented a query parser for a different query syntax than Lucene's, but I still wanted to create Lucene Query objects. The parser includes different protected callback methods like getTermQuery, getPhraseQuery, getPrefixQuery etc, providing a default implementation to each of course. It parses the query into "nodes" and then invoke the appropriate callback method per node (i.e., for a phrase node, it invokes getPhraseQuery). In this specific instance I think subclassing of QueryParser is the right way to go, and avoids introducing another class. People are going to want more customizations than just changing the types of created subclauses, and will need to subclass for these other types of changes anyway.
OK then let's take the subclassing approach; I'll rework the patch.
OK reworked patch to subclass. I plan to commit in a day or two.
Michael - you are a machine!
+1 to the subclassing approach and your general patch. What might be even more interesting is to make the newXXX methods return Query instead of a specific type. I'm not sure if that would work in all cases (surely not for BooleanQuery), but might for most of 'em. For example, what if newTermQuery(Term term) returned a Query instead of a TermQuery? That'd add a fair bit more flexibility, as long as none of the calling code needed a specific type of Query. The hoops we jump through because we're in Java.... sheesh.
Alas, yes... maybe we should all switch to Erlang!
OK I was able to do this with many of them – attached new patch. Only PhraseQuery, MultiPhraseQuery, BooleanQuery/Clause required strong typing. All others now return Query. Committed revision 690535. Thanks John!
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Major
Adding a factory to QueryParser to instantiate query instances
And the class QueryParser is modified to use use the factory to build the final query.
This is backward compatible.