Patches BooleanQuery only. Tests pass, javadocs build.

Committed. Thanks, Paul!

Hmm, I was just trying to compile Luke source against the latest Lucene and am getting compiler errors on this being missing. Doesn't this violate our back-compatibility clause or did I miss something? Don't we have to wait until 3.0-dev to remove these?

You are right, Grant.

I will revert this for 2.3. Thanks for catching this!!

Some test fail with this patch when a MatchAllDocQuery is used, e.g. TestSimpleExplanations.testMA1. skipTo() tries to get the delete bit for a negative document, which throws an Exception of course. This happens for a searcher that contains a MultiReader created by QueryUtils.wrapUnderlyingReader().

I assume this is related to the change I made in QueryUtils. I need to spend some more time to figure out what exactly leads to this exception. Maybe I'm doing something silly here - I'm still catching up on some changes, such as the out-of-order scoring changes.

I can't work on this till Monday, so anyone familiar with these tests feel free to take a look!

Hoss added some multireader test stuff that counts on where the deletes are if I remember right.

so

-    if (BooleanQuery.getAllowDocsOutOfOrder()) return;  // in this case order of skipTo() might differ from that of next().
+    if (q.weight(s).scoresDocsOutOfOrder()) return;  // in this case order of skipTo() might differ from that of next().

must not be exactly equiv and where it wasn't scoring out of order for this test, now it is, or vice versa (I havn't looked closely).

Yeah I figured it has to do with that, but it seems weird to me that a call to scorer.advance() (in QueryUtils, line 348) leads to a ArrayIndexOutOfBoundsException?

So this was always there - but in the past, because BoolQuery.getAllowOutOfOrder was always returning true for these tests, it didn't get into the code thats now failing.

With the replacement, because it only returns true or false based on if its a booleanquery, it gets through there for other queries, and we are seeing the results.

but it seems weird to me that a call to scorer.advance() (in QueryUtils, line 348) leads to a ArrayIndexOutOfBoundsException?

We had similar issues when we were working on these tests before. Hoss knows how the MultiReader/delete stuff is better than I do though - he wrote it, I just addressed issues/bugs.

When we change this, don't forget to remove the 2.9 BW tests that may fail because of this change - whenever we remove deprecated methods, the new 2.9 BW branch must be updated.

Part of it or all of it is that the tests that are failing are using external ids, which doesn't work with this multireader stuff (since they are tech illegal, though they worked before per segment). We fixed a bunch of these, but missed these ones, since they didn't hit these tests. They need to be fixed in the same manner.

A second part is that there looks to be some insane fieldcache usage that needs to be fixed in these tests - again, because the code wasn't hit before, we didn't see it before ...

I just keep randomly taking peeks

So this was always there - but in the past, because BoolQuery.getAllowOutOfOrder was always returning true for these tests, it didn't get into the code thats now failing.

Exactly.

Now I just hope all bugs are in the tests...
Sorry, have to leave now. Thanks for your help, Mark!

Now I just hope all bugs are in the tests...

I'm pretty sure they are (though obviously I cant gaurantee). Its all stuff Hoss and I saw before and fixed from what I can tell - we just fixed what was causing failures though, so getting past that line is exposing stuff we didn't handle.

Hey Mark, any interest in taking this, since you seem to have experience with fixing those tests?

Otherwise I'll continue here later tonight or tomorrow... you'd probably be faster though

All core & contrib tests pass now.

The problem was QueryUtils#checkSkipTo(). It still performed the checks using a global scorer, which resulted in failures in the FieldCacheSanityChecker. (the same values were cached twice: once for the global reader plus for the per-segment readers).

QueryUtils#checkFirstSkipTo() was already implemented correctly so that it performs the checks with per-segment scorers.

I think this is ready to commit now. Mark, do you want to take a look at my checkSkipTo() changes before I commit?

Tiny change in QueryUtils#checkSkipTo() to keep it more consistent to how it worked before.

Also attaching the back-compat patch. Note that I have to make the change to checkSkipTo() there too, because it was not changed before to do the search per-segment. Now more tests actually run this check, exposing this problem.

All tests pass now.

Committed revision 824870.