Add IntervalQuery and IntervalsSource to expose minimum interval semantics across term fields [LUCENE-8196]

[asfimport]

This ticket proposes an alternative implementation of the SpanQuery family that uses minimum-interval semantics from http://vigna.di.unimi.it/ftp/papers/EfficientAlgorithmsMinimalIntervalSemantics.pdf to implement positional queries across term-based fields.  Rather than using TermQueries to construct the interval operators, as in #3952 or the current Spans implementation, we instead use a new IntervalsSource object, which will produce IntervalIterators over a particular segment and field.  These are constructed using various static helper methods, and can then be passed to a new IntervalQuery which will return documents that contain one or more intervals so defined.

---

Migrated from LUCENE-8196 by Alan Woodward (@romseygeek), resolved Apr 03 2018
Attachments: LUCENE-8196.patch (versions: 5), LUCENE-8196-debug.patch
Linked issues:

• Add interval iterators to Scorer [LUCENE-6226] #7288

Pull requests: apache/lucene-solr#334

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

Thanks Alan. I agree that growing a separate hierarchy of objects might help land this feature. We might even want to put first iterations of this work in sandbox to give time for the API to stabilize before we move it to core or misc.

I have some questions/comments:

• Do we need IntervalIterator.score()? It seems to be the same value on all implementations.
• Do we need advanceTo? It seems to me that things would be simpler and as efficient if you documented that nextPosition() may only be called when the approximation is positioned and then advanceTo would be equivalent to checking the return value of nextInterval?
• Let's make the IntervalFunction API an implementation detail?
• The documentation of cost() says it is the cost of finding the next interval but given how you use it in the query it looks like it is actually more about the average cost of iterating over all intervals.
• In terms of testing I would like some form of AssertingIntervalsSource to make sure that intervals are always consumed in legal ways and behave correctly.
• More docs would help read the code. For instance IntervalsSource.intervals has no docs. By the way we might want to mention there that the same instance might be reused across calls.
• TermIntervalsSource should check whether positions were indexed.
• I was a bit annoyed to see the field masking hack but actually those intervals source do not need term statistics which makes the hack less horrible. Could you still document it to make sure users are aware it is a hack and explain it which circumstances it might be ok?

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

Do we need IntervalIterator\.score()?

Yes, terms and phrases return 1 instead than taking the overall width into account.  This is so that they score the same as TermQuery and PhraseQuery

Do we need advanceTo?

Unfortunately yes, or at least we need a way of resetting the iterator on each new document. It might be possible to avoid passing the doc down and having a return value though, I'll see what I can do.

I would like some form of AssertingIntervalsSource

This is a bit trickier, as it's not obvious where the wrapping would happen.

+1 to everything else, I'll work on a follow-up.
 

[asfimport]

Jim Ferenczi (@jimczi) (migrated from JIRA)

I was a bit annoyed to see the field masking hack but actually those intervals source do not need term statistics which makes the hack less horrible. Could you still document it to make sure users are aware it is a hack and explain it which circumstances it might be ok?

 
I think that the proposed API should be more restrictive regarding the targeted field. Could we restrict the IntervalsSource to work on a single field ? Something like:

public abstract class IntervalsSource {
 protected final String field;

 public IntervalsSource(String field) {
   this.field = field;
 }

 public abstract IntervalIterator intervals(LeafReaderContext ctx) throws IOException;
...

... and then we can check in each implementation that the sources are all targeting the same field.
I understand that it might be powerful to mix multiple fields in an interval query but with the current API that seems to be the norm rather than an exception. We can add the field masking hack afterward but for the first iteration I think it's better to focus on the main use case for this new query which is to provide a way to find the minimum intervals in a single field.

Regarding the score of the intervals, it seems that the patch uses the inverse length of the interval rather than the slop within the interval like the sloppy phrase scorer. Could we compute the total slop of the current interval (as the sum of the slop of each interval source that composed this interval) and use its inverse to score each ? This would make different interval query more comparable in terms of score since an interval with few terms and a slop>0 would score less that one with more terms but no slop.

I'll look deeper at the implementation of the different queries but I like the simplicity of the patch and the fact that there is a paper with a proof for each of them.

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

I opened a pull request at apache/lucene-solr#334 to make this easier to review.  @jpountz I think I've addressed most of your feedback?

@jimczi I'd rather keep the API as it is, with the field being passed to IntervalQuery and then recursing down the IntervalSource tree.  Otherwise you end up having to declare the field on all the created sources, which seems redundant.  I've removed the cross-field hack entirely for the moment.

I'll see if I can improve the scoring next.

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

I discussed scoring with @jimczi and @jpountz offline, and we decided to just use the inverse length of intervals as a sloppy frequency for now, as described in the Vigna paper linked above.  This means that we can't compare scores directly with existing phrase queries, but the query mechanism is quite different (particularly for SloppyPhraseScorer) so it makes sense that scores won't be the same either.

[asfimport]

Jim Ferenczi (@jimczi) (migrated from JIRA)

I'd rather keep the API as it is, with the field being passed to IntervalQuery and then recursing down the IntervalSource tree. Otherwise you end up having to declare the field on all the created sources, which seems redundant. I've removed the cross-field hack entirely for the moment.

+1 to remove the cross-field hack, thanks. Regarding the API it's ok since IntervalQuery limits all sources to one field so I am fine with that (I misunderstood how the IntervalQuery can be used).

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

This patch makes IntervalIterator extend DocIdSetIterator, and makes the per-document reset() function protected and called automatically on nextDoc() and advance(target).

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

This patch moves everything into the sandbox for now, and adds a package-info explaining how to use things.  I had to duplicate DisiWrapper, DisiPriorityQueue and DisjunctionDISIApproximation, but I don't think that's too much of a problem.  Having things in sandbox should reduce confusion with Span queries, and give us time to try and switch things over.

I think this is ready to be committed.

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

Some comments:

• ConjunctionIntervalIterator does so little, I suspect things would be easier to read if we removed it.
• I'd like it better if we kept the API definition to a minimum on IntervalIterator and removed the constructor that takes an approximation and the reset() method. To me these should be implementation details?
• Let's make the utility classes that you copied pkg-private?
• Maybe let's put this in a sub package of search, ie. org.apache.lucene.search.intervals?

Otherwise +1

[asfimport]

David Smiley (@dsmiley) (migrated from JIRA)

• Nice package javadocs!
• Maybe you will some day add a means of extracting offsets (e.g. for highlighting) or payloads?
• Just curious, how did you arrive at the conclusion that you needed to specialize the PriorityQueue?
• what if extractTerms took a Consumer<Term> instead of a Set<Term>? It's easy to invoke with a myset::add for the common case when you have a Set, and I've seen cases where you might want to provide a filter before storing it wherever.

 

[asfimport]

Jim Ferenczi (@jimczi) (migrated from JIRA)

+1 too, there are some places where you could initialize the current interval with[−∞ . . −∞] in order to avoid the nullity check.

Most of the operators algorithm seem good, though I don't understand why you change the order of the disjunction ? If you don't start with the smallest right interval from the queue you could miss a lot of minimum intervals that could be needed if the disjunction is used inside another operator ?

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

I moved the approximation and reset() from IntervalIterator, and it turned out that ConjunctionIntervalIterator was a convenient place to put them, so I kept that in.  I also cleaned up the utility classes (no need to check for TwoPhaseIterator or BitSetIterator when we know that the leaves are always PostingsEnum) and moved to org.apache.lucene.search.intervals.

Payload matches can be added trivially at a later point.  Payload scoring will be a more interesting one, but I'm not entirely happy with the way scoring works at the moment anyway, I need to read around a bit more on good ways of scoring proximity queries in general.  Offsets may not be trivial to add, as we have the same problem we originally had with Spans here, in that some of the algorithms advance leaf intervals before returning.  Something to consider later on, definitely.

The PQ specialization is just lifted directly from DisjunctionDISIApproximation in the existing core search package, so it wasn't my conclusion :)

re extractTerms() - I don't like this method being here in the first place really, see my comment about scoring above.  I think let's keep it simple for now, you can always filter afterwards.

@jimczi - where can I remove null checks?  I think I'm constrained by having to return [-1..-1] when iterator has moved to a new doc but hasn't been advanced over intervals yet.

The disjunction order is to address #8451.  If a disjunction is appearing within a block, then sorting for minimum intervals can miss valid matches.  The Vigna paper doesn't seem to discuss this case anywhere.  One possible solution that would work in all cases would be to add a boolean to IntervalsSource.getIntervals() that indicates whether or not the source is in a final position - if it is, then it should return minimal intervals, otherwise it should return wider ones.  This would only be applicable to disjunctions.

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

I talked over disjunction ordering with @jimczi, and we agreed to revert back to the ordering specified in the original paper.  In future it might be worth contacting the authors and seeing if they've covered the case of prefix disjunctions elsewhere.

I think this is ready?

[asfimport]

Jim Ferenczi (@jimczi) (migrated from JIRA)

+1

[asfimport]

ASF subversion and git services (migrated from JIRA)

Commit 974c03a in lucene-solr's branch refs/heads/branch_7x from @romseygeek
https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=974c03a

LUCENE-8196: Add IntervalQuery and IntervalsSource to the sandbox