Lucene - Core
  1. Lucene - Core
  2. LUCENE-1754

Get rid of NonMatchingScorer from BooleanScorer2

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      Over in LUCENE-1614 Mike has made a comment about removing NonMatchinScorer from BS2, and return null in BooleanWeight.scorer(). I've checked and this can be easily done, so I'm going to post a patch shortly. For reference: https://issues.apache.org/jira/browse/LUCENE-1614?focusedCommentId=12715064&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12715064.

      I've marked the issue as 2.9 just because it's small, and kind of related to all the search enhancements done for 2.9.

      1. LUCENE-1754.patch
        25 kB
        Michael McCandless
      2. LUCENE-1754.patch
        26 kB
        Shai Erera
      3. LUCENE-1754.patch
        26 kB
        Shai Erera
      4. LUCENE-1754.patch
        12 kB
        Shai Erera
      5. LUCENE-1754-2.patch
        7 kB
        Shai Erera
      6. LUCENE-1754-2.patch
        7 kB
        Shai Erera

        Activity

        Hide
        Shai Erera added a comment -

        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?

        Show
        Shai Erera added a comment - 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?
        Hide
        Michael McCandless added a comment -

        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?

        Show
        Michael McCandless added a comment - 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?
        Hide
        Shai Erera added a comment -

        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.

        Show
        Shai Erera added a comment - 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.
        Hide
        Yonik Seeley added a comment -

        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.

        Show
        Yonik Seeley added a comment - 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.
        Hide
        Shai Erera added a comment -

        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?

        Show
        Shai Erera added a comment - 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?
        Hide
        Michael McCandless added a comment -

        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().

        Show
        Michael McCandless added a comment - 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().
        Hide
        Yonik Seeley added a comment -

        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.

        Show
        Yonik Seeley added a comment - 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.
        Hide
        Shai Erera added a comment -

        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.

        Show
        Shai Erera added a comment - 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.
        Hide
        Tim Smith added a comment -

        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)

        Show
        Tim Smith added a comment - 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)
        Hide
        Shai Erera added a comment -

        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?

        Show
        Shai Erera added a comment - 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?
        Hide
        Yonik Seeley added a comment -

        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".

        Show
        Yonik Seeley added a comment - 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".
        Hide
        Tim Smith added a comment -

        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

        Show
        Tim Smith added a comment - 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
        Hide
        Shai Erera added a comment -

        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)?

        Show
        Shai Erera added a comment - 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)?
        Hide
        Shai Erera added a comment -

        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.

        Show
        Shai Erera added a comment - 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.
        Hide
        Shai Erera added a comment -

        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.

        Show
        Shai Erera added a comment - 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.
        Hide
        Tim Smith added a comment -

        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

        Show
        Tim Smith added a comment - 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
        Hide
        Michael McCandless added a comment -

        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.

        Show
        Michael McCandless added a comment - 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.
        Hide
        Shai Erera added a comment -

        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.

        Show
        Shai Erera added a comment - 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.
        Hide
        Shai Erera added a comment -
        • 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.

        Show
        Shai Erera added a comment - 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.
        Hide
        Michael McCandless added a comment -

        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 @@
        
        Show
        Michael McCandless added a comment - 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 @@
        Hide
        Shai Erera added a comment -

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

        Show
        Shai Erera added a comment - My fault. After I created it, I manually edited the CHANGES section, which messed up the lines count.
        Hide
        Michael McCandless added a comment -

        OK patch looks good, thanks Shai!

        I plan to commit in a day or two.

        Show
        Michael McCandless added a comment - OK patch looks good, thanks Shai! I plan to commit in a day or two.
        Hide
        Michael McCandless added a comment -

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

        Show
        Michael McCandless added a comment - New patch attached – sync'd to trunk, and, fixed places to also catch when disi.iterator() returns null.
        Hide
        Michael McCandless added a comment -

        Thanks Shai!

        Show
        Michael McCandless added a comment - Thanks Shai!
        Hide
        Shai Erera added a comment -

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

        Show
        Shai Erera added a comment - EmptyDocIdSetIterator is not necessary as DocIdSet already defines an EMPTY_DOCIDSET member. We should use it.
        Hide
        Shai Erera added a comment -

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

        Show
        Shai Erera added a comment - Removed EmptyDocIdSetIterator and changed DocIdSet.EMPTY_DOCIDSET to impl iterator() to always return the same, empty, instance.
        Hide
        Michael McCandless added a comment -

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

        Show
        Michael McCandless added a comment - New patch looks good Shai, thanks! I'll commit shortly.
        Hide
        Uwe Schindler added a comment -

        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!

        Show
        Uwe Schindler added a comment - 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!
        Hide
        Shai Erera added a comment -

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

        Show
        Shai Erera added a comment - Wait ! DocIdSet.EMPTY should not do "new SortedVIntList", just "new DocIdSet()".
        Hide
        Shai Erera added a comment -

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

        We probably posted at the same time

        Show
        Shai Erera added a comment - There is no need to make this empty DIS a subvclass of SortedVIntList We probably posted at the same time
        Hide
        Uwe Schindler added a comment -

        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)).

        Show
        Uwe Schindler added a comment - 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)).
        Hide
        Michael McCandless added a comment -

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

        OK, committed.

        Show
        Michael McCandless added a comment - DocIdSet.EMPTY should not do "new SortedVIntList", just "new DocIdSet()". OK, committed.
        Hide
        Uwe Schindler added a comment -

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

        Show
        Uwe Schindler added a comment - I also removed the note about SortedVInitList in the JavaDocs, Mike, should I commit?
        Hide
        Shai Erera added a comment -

        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.

        Show
        Shai Erera added a comment - 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.
        Hide
        Uwe Schindler added a comment -

        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).

        Show
        Uwe Schindler added a comment - 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).
        Hide
        Shai Erera added a comment -

        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").

        Show
        Shai Erera added a comment - 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").
        Hide
        Uwe Schindler added a comment -

        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.

        Show
        Uwe Schindler added a comment - 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.
        Hide
        Shai Erera added a comment -

        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?

        Show
        Shai Erera added a comment - 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?
        Hide
        Uwe Schindler added a comment -

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

        Show
        Uwe Schindler added a comment - DONE! I hope this is the last commit for this issue
        Hide
        Michael McCandless added a comment -

        Phew, thanks guys!

        Show
        Michael McCandless added a comment - Phew, thanks guys!

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Shai Erera
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development