I thought it's going to be easy too soon .

After I've made the changes, some tests in TestBooleanMinShouldMatch fail w/ NPE. That's because previously we always returned BS2, which used NonMatchingScorer internally, and now we return null. Therefore this looks like a back-compat issue, but not sure how serious it is. The reason is that BooleanWeight.scorer() already returns null today in some situations, therefore any code that calls BooleanWeight.scorer() directly should already check that it didn't get null. So I'm not sure if it's indeed back-compat problem or not.

What do you think?

It seems like Weight.scorer() in general is allowed to return null to indicate "no docs will match" (since some scorers do so, though others do not)? Though that "right" wasn't spelled out anywhere in the javadocs as best I can tell. So... I don't think this is really a back-compat break?

I think we should allow this, and document it clearly, going forward (in QueryWeight.scorer)?

I suppose we could fix the emulation of Weight on top of QueryWeight (QueryWeightWrapper) to return NonMatchingScorer if it gets null back from QueryWeight.scorer?

Patch which removes NonMatchingScorer, fixes tests and tag.

I also agree this is not a back-compat change, since BooleanQuery returns null today already. I just fixed the test to check whether the scorer returned is not null.

I don't think we should return NonMatchingScorer from QueryWeightWrapper, since it will hide the already returned null Scorer. I.e., I think that will cause some apps to break, since if previously they relied on null, now they will get an instance which if they'll call its score() or doc() methods will get UOE.

It seems like Weight.scorer() in general is allowed to return null to indicate "no docs will match"

Hmmm, that's a bit of a surprise... I had thought one would always get a scorer instance back . There's probably a lot of lurking bugs.
See ConstantScoreQuery.explain() for instance.

See ConstantScoreQuery.explain() for instance.

CSQ calls its own scorer(), therefore it knows it's going to get an instance back.

In general, BooleanQuery already returns null today, so all I was saying is that if someone called BQ.weight.scorer(), he had to check for null anyway. If he didn't then his app would be exposed?

TermWeight.scorer returns null if the termDocs is null (though it looks like DirectoryIndexReader will never return null termDocs).

PhraseQuery and MultiPhraseQuery return null scorer if they have 0 terms.

A number of scorers check for null returns from their sub-scorers and then return null to their caller. IndexSearcher also handles a null scorer.

It looks like null is already in general an accepted/possible return from Weight.scorer().

Still surprising that scorer() can return null... I guess it's been a blind spot.

Another question - from QueryWrapperFilter:

  public DocIdSet getDocIdSet(final IndexReader reader) throws IOException {
    final QueryWeight weight = query.queryWeight(new IndexSearcher(reader));
    return new DocIdSet() {
      public DocIdSetIterator iterator() throws IOException {
        return weight.scorer(reader, true, false);
      }
    };
  }

So is it the case that DocIdSet.iterator() can also return null? That's what this code does.

That's a good point. IndexSearcher does not protect against this case (it only asserts the filter is not null, but not the DISI returned from it).

I guess it's been a blind spot.

I think that you're right. And more than that, we don't have unit tests that cover these cases (I believe there are some that cover a null Scorer, but not a null DISI). And I wonder how this hasn't come up until now. I mean, the examples Mike gave above do not sound way too extreme to me.

So does this mean we should document that scorer cannot be null? And if so, we make NonMatchingScorer public and recommend to return it if the query does not expect to return any documents? We can also make NMS not instantiable (is it a word?), with a static instance() method, but that's a minor thing.

Or ... we leave everything as-is, and add some test cases which test this exactly, and change QueryWrapperFilter to return NMS in case the scorer returned by Weight is null.

I think I prefer the first approach.

If making NonMatchingScorer public, i suggest potentially implementing a NonMatchingQuery as well

this way, if a query has to rewrite() to null, it could return a NonMatchingQuery instead (which would protect against null pointer exceptions)

So if we agree that scorer() cannot return null, I think that most of the patch can be reverted, and I'll change the scorer() impls which return null to return NMS instead?

About NonMatchingQuery, I don't mind adding it, though why should a Query rewrite to null?

So if we agree that scorer() cannot return null, I think that most of the patch can be reverted, and I'll change the scorer() impls which return null to return NMS instead?

I haven't looked deeply into it, but at first blush that could simplify client code.
Need to watch out if there are any implementations of Weight.explain() that cast to a Specific type of scorer - hence one would get a cast exception if "return null" were blindly replaced with "return NMS".

Here's a couple of use cases where a Query should rewrite to null/NonMatchingQuery (in my opinion):

• Wildcard query that maps to 0 terms in the index
• range query that again maps to 0 terms in the index
• boolean query that has 0 clauses
• filter based constant score query that has 0 docs in it
• (and thats just in the in the lucene tree, nothing to do with custom query implementations)

Most of these situations would actually preempt needing to return null from a scorer, since the query would already have been rewritten to a NonMatchingQuery/null prior to asking for a scorer

BooleanQuery could then also do instanceof checks on return from clause rewrite. If a NonMatchingQuery/null is returned for a required clause, the BooleanQuery itself can rewrite to NonMatchingQuery, also, if a prohibited clause rewrites to NonMatchingQuery/null, that clause can be removed entirely, if an optional clause rewrites to NonMatchingQUery/null, that clause can also be removed safely

hence one would get a cast exception if "return null" were blindly replaced with "return NMS".

So this means that that someone is prepared to get a null Scorer right? So I'm confused - are you for returning null or not? Of course, something I just realized, returning null from scorer() does not break back-compat, while returning NMS does (or at least may).

How about if we fix QueryWrapperFilter to return NMS then? Or we document that Filter.getDocIdSet may return null, and fix IndexSearcher accordingly (this again is not a back-compat thing, while returning NMS is)?

BooleanQuery could then also do instanceof checks on return from clause rewrite

That is a good point you raise. Actually today, BQ is quite efficient in that it compares to null. If we change scorer() to return NMS, we'd need to check instanceof which is way more expensive (we'd still need to check for null though, because we cannot enforce NMS).

So I think I'm still in favor of returning null ... or should I say - keep the currently not documented, but in effect, behavior.

Any thoughts on that?

If we keep null, then I'll fix IndexSearcher to check whether filter.getDocIdSet did not return null. If it did, don't execute the query.

I'd like to move on with this, if we have some sort of consensus.

keeping null should be fine, as long as this is documented and all core query implementations handle this behavior, and all searcher code handles the null return properly
at this point, NonMatchingScorer could be removed and null returned in its place (being package private, no one writing applications can make any assumptions on a NonMatchingScorer being returned)

however, this should also be documented for the rewrite() method (currently this looks to always expect a non-null return value), also the searcher will throw null pointers if a null query is passed to it

I think we continue to allow scorer() and getDocIdSet to return null to mean "no matches", though they are not required too (ie, one cannot assume that a non-null return means there are matches).

And we should make this clear in the javadocs.

And remove NonMatchingScorer.

ok then I'll add a test case to the patch which uses QWF w/ a query that it's scorer returns null, and then fix IndexSearcher accordingly. And update the javadocs as needed.

• Added a test case to TestDocIdSet which returns a null DocIdSet and indeed IndexSearcher failed.
• Fixed IndexSearcher and all other places in the code which called scorer() or getDocIdSet() and could potentially hit NPE.
• Added EmptyDocIdSetIterator for use by classes (such as ChainFilter) that need a DISI, but got a null DocIdSet.
• Updated CHANGES.

All search tests pass.

For some reason I can't apply the patch – I get this:

$ patch -p0 < /x/tmp/LUCENE-1754.patch.txt 
patching file CHANGES.txt
patch: **** malformed patch at line 20: @@ -629,6 +638,11 @@

My fault. After I created it, I manually edited the CHANGES section, which messed up the lines count.

OK patch looks good, thanks Shai!

I plan to commit in a day or two.

New patch attached – sync'd to trunk, and, fixed places to also catch when disi.iterator() returns null.

Thanks Shai!

EmptyDocIdSetIterator is not necessary as DocIdSet already defines an EMPTY_DOCIDSET member. We should use it.

Removed EmptyDocIdSetIterator and changed DocIdSet.EMPTY_DOCIDSET to impl iterator() to always return the same, empty, instance.

New patch looks good Shai, thanks! I'll commit shortly.

There is no need to make this empty DIS a subvclass of SortedVIntList

-public static final DocIdSet EMPTY_DOCIDSET = new SortedVIntList(new int[0]) {
+-public static final DocIdSet EMPTY_DOCIDSET = new DocIdSet() {

Else is OK!

Wait ! DocIdSet.EMPTY should not do "new SortedVIntList", just "new DocIdSet()".

There is no need to make this empty DIS a subvclass of SortedVIntList

We probably posted at the same time

Hihi! Sometimes it is not the best to be too fast

I am now satisfied with the latest patch, this new DocdSet impl is better than the SortedVInitList (which was OK, too, in my opinion. There was a very lengthly discussion in another JIRA issue about that DIS(I)).

DocIdSet.EMPTY should not do "new SortedVIntList", just "new DocIdSet()".

OK, committed.

I also removed the note about SortedVInitList in the JavaDocs, Mike, should I commit?

I also removed the note about SortedVInitList in the JavaDocs

Argh ... I guess I tried to create the patch too fast (before Mike commits) .
When you do this, the import of SVIL can also be removed.

I am a little bit confused about the JavaDocs of iterator():

/** Provides a {@link DocIdSetIterator} to access the set.
 * This may (but is not required to) return null if there
 * are no docs that match. */

Is it really allowed to return null instead of an empty iterator? If not, we should silently remove this part of the docs (and hopefully do not break some filters written by someone).

Is it really allowed to return null instead of an empty iterator?

Some iterators already return null as an iterator, perhaps unknowingly. QueryWrapperFilter will do that if the Query produced a null Scorer, which is legal, or at least the de facto impl of some Queries.

I think that the decisions on this issue were made because we observed that returning null "already exists", and moving to not return null, or saying that null should not be returned, may break back-compat (e.g. if someone relies on null to mean "there are no docs to match").

OK, so maybe we change the JavaDoc and state, that optimally a Filter should return the empty DocIdSet instance. This is e.g. how MultiTermQueryWrapperFilter works.

Even though I don't know if it's "optimal", I think this makes sense to document, i.e. change the javadoc to something like "if there are no docs that match, the implementation can return null or DocIdSet.EMPTY_DOCIDSET.iterator()"?

Can you do this while changing the javadoc of EMPTY_DOCIDSET as well as the unnecessary import?

DONE! I hope this is the last commit for this issue

Phew, thanks guys!