Expose OffsetsEnum directly from UnifiedHighlighter [LUCENE-8158]

[asfimport]

Now that OffestsEnum is unitary, we can expose it directly from the UnifiedHighlighter, allowing clients to use them in place of snippets.

---

Migrated from LUCENE-8158 by Alan Woodward (@romseygeek), 1 vote
Attachments: LUCENE-8158.patch, LUCENE-8158-alt.patch

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

This patch adds an OffsetProducer abstraction which handles offsets for an individual Query.  Clients can call UnifiedHighlighter.getOffsetProducer(Query) to get one, and then call .getFieldOffsets() to get the offsets for a particular document and field.

I've tried to keep the API change as minimal as possible, which means keeping a reference to the UnifiedHighlighter and calling back to overrideable methods to create Automata lists and PhraseHelpers, but ideally we could pull all this into the OffsetProducer as well, and have the overrideable methods there.

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

Here's an alternative, considerably more invasive patch, which moves all Offsets-related code into OffsetProducer.

[asfimport]

David Smiley (@dsmiley) (migrated from JIRA)

This looks interesting Alan.  I looked at both patches.  Lets consider the more invasive one. I like that UnifiedHighlighter is shorter as it was getting quite long, and this looks like a reasonable way to break it up.

• These methods were on UnifiedHighlighter (and not other internal things) deliberately: requiresRewrite, preSpanQueryRewrite, preMultiTermQueryRewrite, shouldHandleMultiTermQuery, shouldHighlightPhrasesStrictly, shouldPreferPassageRelevancyOverSpeed. By having them on the UH, we can get a method reference and use them internally where needed without exposing the internals to the user. PhraseHelper is an internal thing, and I believe the OffsetProducer abstraction you are introducing here is too. That said, we could have fewer such methods:
  • I'd prefer that we do away with those "should" methods that merely examine defaultFieldFlags because by creating a second method to customize the settings (separate from getFlags(field) it's both extra/unnecessary plus if you override one of those should methods it may in fact do nothing if you have overridden getFlags as well. So it's error-prone. I'm okay with these disappearing in 7.x; users merely need to switch to overriding getFlags(String field).
  • I think preSpanQueryRewrite and preMultiTermQueryRewrite could be combined in purpose to simply be customRewrite or perhaps worded as customizeQuery avoiding the overloaded word "rewrite". Since such hooks are advanced, I'm okay with the API changing in 7.x.
• OffsetProducer.getFieldOffsets(field,text) is unused and has no javadocs; do you want to add it to the visibility test? Also, it's implementation appears to duplicate that of getFieldOffsetStrategy

BTW I've been thinking of a refactoring similar to your proposal here. It would be nice if there was a class that held the Query and the things we extract from it: position-insensitive terms (or all terms), MTQ automata, position-sensitive queries. It's job would be to extract all this information from the Query, and that's it. Today it has these aforementioned things but it would be a nice option to extract the term->boost as well so that we can customize the relevancy based on boosts in the query (like the original Highlighter & FVH can). Eventually it would be nice to "visit" the query tree once directly in this proposed class instead of several times (weight.extractTerms, MultiTermHighlighting, PhraseHelper). That would be the extent of the purpose of this thingamajig – perhaps called QueryExtractor. An instance of this class would go on each FieldOffsetStrategy and would obviate the need to pass so many arguments to these classes and for them to have as many fields as they do (not that there's that much but still). OffsetProducer here slightly overlaps with this idea but the ideas could be combined. OP could accept a QueryExtractor and would then have some less stuff to deal with internally since QueryExtractor extracts terms, automata, PhraseHelper, etc. not OP.