Ticket #857: 0001-Fix-AndNotPostList-skip_to-before-it-has-been-positi.patch

xapian-core/matcher/andnotpostlist.cc

@@ -66 +66 @@
 PostList*
 AndNotPostList::skip_to(Xapian::docid did, double w_min)
 {
-    if (did > pl->get_docid()) {
-        PostList* result = pl->skip_to(did, w_min);
+    // We can't skip the work here when did <= pl->get_docid(): before we've
+    // been positioned, get_docid() is unspecified, and a leaf postlist may
+    // already return its first docid, which r hasn't yet been checked
+    // against.  pl->skip_to() does nothing if pl is already at or past did.
+    PostList* result = pl->skip_to(did, w_min);
+    if (result) {
+        delete pl;
+        pl = result;
+    }
+    if (pl->at_end()) {
+        result = pl;
+        pl = nullptr;
+        return result;
+    }
+    Xapian::docid l_did = pl->get_docid();
+    if (l_did > r_did) {
+        bool r_valid;
+        result = r->check(l_did, 0, r_valid);
         if (result) {
-            delete pl;
-            pl = result;
+            delete r;
+            r = result;
         }
-        if (pl->at_end()) {
+        if (!r_valid)
+            return nullptr;
+        if (r->at_end()) {
             result = pl;
-            pl = NULL;
+            pl = nullptr;
             return result;
         }
-        Xapian::docid l_did = pl->get_docid();
-        if (l_did > r_did) {
-            bool r_valid;
-            result = r->check(l_did, 0, r_valid);
-            if (result) {
-                delete r;
-                r = result;
-            }
-            if (!r_valid)
-                return NULL;
-            if (r->at_end()) {
-                result = pl;
-                pl = NULL;
-                return result;
-            }
-            r_did = r->get_docid();
-        }
-        if (l_did == r_did) {
-            // Advance to the next match.
-            return AndNotPostList::next(w_min);
-        }
+        r_did = r->get_docid();
     }
-    return NULL;
+    if (l_did == r_did) {
+        // Advance to the next match.
+        return AndNotPostList::next(w_min);
+    }
+    return nullptr;
 }
 
 PostList*

xapian-core/tests/api_query.cc

@@ -1495 +1495 @@
     } catch (const Exception_estimateop2&) {
     }
 }
+
+/// Regression test for bug in AndNotPostList::skip_to() fixed after 2.1.0.
+DEFINE_TESTCASE(boolandnotbug1, backend) {
+    Xapian::Database db = get_database("boolandnotbug1",
+                                       [](Xapian::WritableDatabase& wdb,
+                                          const string&)
+                                       {
+                                           Xapian::Document doc;
+                                           doc.add_term("A");
+                                           doc.add_term("B");
+                                           doc.add_term("D");
+                                           wdb.add_document(doc);
+                                           Xapian::Document doc2;
+                                           doc2.add_term("C");
+                                           wdb.add_document(doc2);
+                                           Xapian::Document doc3;
+                                           doc3.add_term("A");
+                                           doc3.add_term("D");
+                                           wdb.add_document(doc3);
+                                           // More docs matching just C, so
+                                           // that D has the lower termfreq,
+                                           // the AND advances it first and
+                                           // calls skip_to() on the OR.
+                                           wdb.add_document(doc2);
+                                           wdb.add_document(doc2);
+                                       });
+    Xapian::Query q = (Xapian::Query("A") &~ Xapian::Query("B")) |
+                      Xapian::Query("C");
+    q &= Xapian::Query("D");
+    Xapian::Enquire enq(db);
+    enq.set_query(q);
+    mset_expect_order(enq.get_mset(0, 10), 3);
+    // With BoolWeight the OR is a BoolOrPostList, which calls skip_to() on
+    // the AND_NOT before it has been positioned, and the AND_NOT used to
+    // return docid 1 without checking it against the right side.
+    enq.set_weighting_scheme(Xapian::BoolWeight());
+    mset_expect_order(enq.get_mset(0, 10), 3);
+}