From 368a5e2d2a62be9ff59c8bdbe5961c33a98ac203 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Fri, 30 Oct 2026 15:44:26 +0200 Subject: [PATCH] Fix AndNotPostList::skip_to() before it has been positioned skip_to() did nothing if the target docid wasn't after the left side's current docid, but before the first next() or skip_to() that current docid is unspecified, and a leaf postlist may already return its first docid then. So the first skip_to() could stop on that docid without checking it against the right side, and return a document which the AND_NOT should exclude. It took BoolWeight to hit this in practice: the OR is then a BoolOrPostList, which passes skip_to() straight to its subqueries, so in ((A AND_NOT B) OR C) AND D, where D has the lower termfreq, the AND_NOT is first skip_to()'d to D's first docid. If that's the first docid of A and it also has B, it was wrongly matched. notmuch uses BoolWeight for every search and count, so its searches which OR "x and not y" with another query and AND the result could return mail that "and not y" excludes. Always forward skip_to() to the left side, which already does nothing if it is at or past the target, and check the result against the right side. 1.4 tracked the left side's position itself and wasn't affected; this regressed when AndNotPostList was reimplemented using WrapperPostList. Co-Authored-By: Claude Opus 5.5 --- xapian-core/matcher/andnotpostlist.cc | 58 ++++++++++++++------------- xapian-core/tests/api_query.cc | 38 ++++++++++++++++++ 2 files changed, 68 insertions(+), 28 deletions(-) diff --git a/xapian-core/matcher/andnotpostlist.cc b/xapian-core/matcher/andnotpostlist.cc index cfc94af4834c..7c35e897f2e8 100644 --- a/xapian-core/matcher/andnotpostlist.cc +++ b/xapian-core/matcher/andnotpostlist.cc @@ -66,40 +66,42 @@ AndNotPostList::next(double w_min) 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* diff --git a/xapian-core/tests/api_query.cc b/xapian-core/tests/api_query.cc index 4780a354d9cf..0fbb9a6fc6cb 100644 --- a/xapian-core/tests/api_query.cc +++ b/xapian-core/tests/api_query.cc @@ -1495,3 +1495,41 @@ DEFINE_TESTCASE(estimateopbug2, backend && !remote) { } 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); +} -- 2.56.0