Refactoring multiterm analysis [LUCENE-5470]

[asfimport]

There are currently three methods to analyze multiterms in Lucene and Solr:

1. QueryParserBase
2. AnalyzingQueryParser
3. TextField (Solr)

The code in QueryParserBase and in TextField do not consume the tokenstream if more than one token is generated by the analyzer. (Admittedly, thanks to the magic of MultitermAwareComponents in Solr, this type of exception probably never happens and the unconsumed stream problem is probably non-existent in Solr.)

I propose consolidating the multiterm analysis code into one place: QueryBuilder in Lucene core.

This is part of a refactoring that will also help reduce duplication of code with #6269.

---

Migrated from LUCENE-5470 by Tim Allison (@tballison), 1 vote, updated Apr 09 2014
Attachments: LUCENE-5470_QBuilder.patch, LUCENE-5470_QPBase.patch, LUCENE-5470.patch

[asfimport]

Tim Allison (@tballison) (migrated from JIRA)

First version of patch.

I used the code from AnalyzingQueryParser as the template.

If we want to do this consolidation, some questions:

1. should analyzeMultitermTerm be static?
2. is the unchecked IllegalArgumentException the way to go if zero or more than one token is generated by the analyzer?
3. should we try to coalesce around one name: analyzeMultiTerm vs AnalyzeMultitermTerm?

Thank you.

[asfimport]

Ahmet Arslan (@iorixxx) (migrated from JIRA)

Hey Tim,

Since you are into multi term analysis, and familiar with query parsers, can you look at SOLR-4382 ? May be you can tell where we should look into?

Thanks,

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

Can we just analyze multiterm queries without trying to parse around wildcards or what not? This is basically what solr is doing today. I think trying to interpret the syntax is a bit too funky and error-prone, and its better if someone wants "magic" to have that in their QP itself.

I think its assumed you will use a different analyzer for this stuff (e.g. with keywordtokenizer) than you normally would, so this syntax is no problem really. This is why for example, QueryParserBase is specifically setup for you to supply that simpler, different analyzer:

  protected BytesRef analyzeMultitermTerm(String field, String part, Analyzer analyzerIn) {

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

btw, i do think we want to consolidate this. But I'm not sure QueryBuilder is the appropriate place (in general thats a very very simple subclass and doesnt handle these types of queries).

Personally i was htinking more along the lines of making the default implementation in QueryParserBase (analyzeMultitermTerm) a simple static method available somewhere that parsers can use? e.g. it just takes an Analyzer, a field name, and some text and ensures there is only one token and returns the term.

[asfimport]

Tim Allison (@tballison) (migrated from JIRA)

Can we just analyze multiterm queries without trying to parse around wildcards or what not? This is basically what solr is doing today. I think trying to interpret the syntax is a bit too funky and error-prone, and its better if someone wants "magic" to have that in their QP itself.

I'm of two minds on this. From the Solr perspective, absolutely, getMultiterm is sufficient. From the Lucene perspective, users may want to use the off-the-shelf analyzers like StandardAnalyzer and be puzzled that they don't work for multiterms...AnalyzingQueryParser fits this need for wildcard queries (not for regex, though). Some thoughts on this:

1. Do the least harm option: consolidate getMultitermTerm as a public static method in QueryParserBase and let AnalyzingQueryParser do its wildcard stuff as is.

2. Do the above, but also add an AnalyzingQueryParserBase layer that does the wildcard trickery (and maybe add something for regex)? Classic QueryParser and others could then subclass AnalyzingQP. The benefit of this is that we could get rid of AnalyzingQP and add multiterm analysis to other parsers that currently subclass QPBase. This would only benefit people working at the Lucene level.

3. A more drastic step would be to move the Solr MultitermAware processing in FieldTypePluginLoader down into the Lucene layer...but this wouldn't solve the problem of Lucene users misusing off the shelf Analyzers.

[asfimport]

Tim Allison (@tballison) (migrated from JIRA)

On second thought...please disregard above. We shouldn't be encouraging misuse of analyzers via hacks for wildcard analysis now that the problem has been fixed at the Solr level.

I feel fairly strongly that analyzeMultitermTerm should be fairly high up the stack...not quite at Object, perhaps. :)

The case for putting analyzeMultitermTerm as a static helper function ? in QueryBuilder or at least something higher up than QueryParserBase is mostly aesthetic IMHO. It feels awkward to me to create SolrQueryParserBase as a sister to QueryParserBase and then have it effectively calling its sister's static function (if we put analyzeMultitermTerm in QueryParserBase). It feels slightly less awkward to have it calling its parent's helper function. By "effectively," of course, I don't mean that SQPB is actually calling it itself, but it does effectively do it through TextField.

So, my proposal for this patch is to leave analyzeMultitermTerm in QueryBuilder and put analyzeWildcard back into AnalyzingQueryParser where it belongs.

I'm happy to defer to the community, though. I've come late to the party and this has probably been thoroughly discussed.

[asfimport]

Tim Allison (@tballison) (migrated from JIRA)

Two patches. One consolidates analysis in QueryBuilder and one consolidates it in QueryParserBase.

I've added a check for position increment. I don't think this will create false exceptions, but let me know if anyone thinks otherwise.

If we go with QueryBuilder, I'm not held to static.