Fix nested all-negative boolean clauses matching zero documents - #3025
Fix nested all-negative boolean clauses matching zero documents#3025stumpylog wants to merge 4 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73dff4e9b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !is_empty && all_negative(&ast) { | ||
| make_non_negative(&mut ast); |
There was a problem hiding this comment.
When parse_query_lenient turns every leaf in a nested clause into an empty clause after recoverable errors, this new nested all_negative check treats those empty children as all-negative and appends *. For example, title:b AND (missing:a OR missing:c) now trims the two invalid leaves away and leaves the injected AllQuery, so the invalid OR group becomes match-all instead of match-nothing and the query matches every title:b document despite both nested alternatives being unusable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93aa565 -- added is_effectively_empty/all_negative_ignoring_invalid so a nested clause is only recognized as all-negative once its literally-invalid children (produced by lenient parse failures) are excluded from consideration, rather than being treated as MustNot content. Covered by test_invalid_nested_leaves_do_not_become_match_all.
| if !is_empty && all_negative(&ast) { | ||
| make_non_negative(&mut ast); |
There was a problem hiding this comment.
Because the nested children are padded before the outer all_negative check runs, a strict top-level query made only of negative OR operands no longer raises AllButQueryForbidden: -title:a OR -title:b parses as two Should children that each contain a synthetic *, so parse_query now succeeds and matches almost every document instead of rejecting a query with only excluding terms.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93aa565 -- pad_nested_all_negative_clauses now threads an is_root flag so the root itself is never padded by the nested pass; the pre-existing root-level all_negative/AllButQueryForbidden check still runs on the un-padded root first. Covered by test_top_level_negative_or_still_rejected.
| if !is_empty && all_negative(&ast) { | ||
| make_non_negative(&mut ast); |
There was a problem hiding this comment.
For valid nested negation used under scoring, such as tag:invoice AND NOT title:draft with TopDocs, padding the inner NOT with a normal Should AllQuery makes the positive child contribute AllQuery's 1.0 score for every non-draft hit. That means the NOT form ranks and explains differently from the flat tag:invoice AND -title:draft form even though the synthetic clause is only there to provide candidates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93aa565 -- nested padding now uses make_non_negative_zero_scored, which injects a 0.0-boosted AllQuery instead of the default-scored one used at the root, so the synthetic clause supplies candidates without contributing to the score. Covered by test_nested_not_scores_identically_to_dash.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93aa5656cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| && children.iter().all(|(ref occur, child)| { | ||
| !is_effectively_empty(child) | ||
| && (*occur == Occur::MustNot || all_negative_ignoring_invalid(child)) |
There was a problem hiding this comment.
When lenient parsing leaves an explicit AND group with both an invalid child and a direct MustNot child, this all() returns false as soon as it sees the empty child, so the group is not padded before trim_ast removes that invalid sibling. For example, title:invoice AND (missing:a AND -title:draft) trims the nested group down to a BooleanQuery containing only MustNot title:draft, which has no positive candidate source and therefore makes the whole query match zero invoice docs instead of applying the draft exclusion; the empties need to be ignored rather than making the clause ineligible for padding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b834683 -- all_negative_ignoring_invalid now filters out effectively-empty children before checking the rest, instead of letting one invalid sibling disqualify the whole clause via .all(). Added test_and_not_with_invalid_sibling_still_excludes covering exactly this query; verified it fails pre-fix (0 hits) and passes post-fix (1 hit).
| /// delegates to what it wraps. | ||
| fn is_effectively_empty(ast: &LogicalAst) -> bool { | ||
| match ast { | ||
| LogicalAst::Leaf(_) => false, |
There was a problem hiding this comment.
Fresh evidence is that lenient IN parsing still returns LogicalLiteral::Set { elements: [] } with errors when every element fails conversion, and this new leaf-is-never-empty rule makes that unusable set look like real negative content. In a query such as title:final OR NOT (unsigned: IN [foo bar]), the padding pass adds *^0 to the invalid NOT branch, so it matches every document instead of dropping the unusable branch and leaving only the valid title:final alternative.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b834683 -- is_effectively_empty now recognizes a Leaf(Set { elements }) with no elements as empty, not just empty Clauses. Added test_not_on_all_invalid_set_elements_does_not_become_match_all covering exactly this query; verified it fails pre-fix (2 hits) and passes post-fix (1 hit).
| for (_, child) in children.iter_mut() { | ||
| pad_nested_all_negative_clauses(child, false); |
There was a problem hiding this comment.
When the all-negative group is itself negated, e.g. title:final -(-title:draft -title:approved), padding it in place makes the matching set equivalent to final AND (draft OR approved), but the draft/approved terms stay inside an excluded subquery, and scoring ignores MustNot children. As a result all matching docs score only on final, so a doc matching both draft and approved cannot outrank one matching only draft, unlike the positive equivalent; the pass needs to consider the parent occur or rewrite the double negation instead of discarding the occurrence here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is an inherent limitation of BooleanQuery scoring, not something this padding fix introduces: Occur::MustNot subtrees never contribute to the parent's score regardless of what they contain, so the flat form title:final -title:draft -title:approved has the exact same "only scores on final" behavior as the double-negation form -- there's no flat equivalent that scores differently here. Fixing this would mean rewriting double negation via De Morgan's laws into a positive Should form rather than padding the negated subgroup in place, which is a bigger change than this PR's scope (giving nested all-negative clauses a candidate source). Leaving as-is for now; happy to file a follow-up issue if that's useful.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8346839ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn is_effectively_empty(ast: &LogicalAst) -> bool { | ||
| match ast { | ||
| LogicalAst::Leaf(leaf) => { | ||
| matches!(leaf.as_ref(), LogicalLiteral::Set { elements } if elements.is_empty()) |
There was a problem hiding this comment.
When every element of a lenient IN list fails inside a group that also has a real exclusion, e.g. title:invoice AND (unsigned: IN [foo] AND -title:draft), this treats the empty Set as ignorable for the padding decision, so the group is padded as if only -title:draft remains. But trim_ast only removes empty Clauses, not LogicalLiteral::Set { elements: [] }, so conversion still keeps a Must TermSetQuery(empty) and the nested clause matches zero documents instead of applying the draft exclusion. The empty set needs to be removed/converted to an empty clause before it is ignored here.
Useful? React with 👍 / 👎.
| pad_nested_all_negative_clauses(child, false); | ||
| } | ||
| if !is_root && !is_effectively_empty(ast) && all_negative_ignoring_invalid(ast) { | ||
| make_non_negative_zero_scored(ast); |
There was a problem hiding this comment.
When the padded all-negative clause is a SHOULD/OR alternative, e.g. title:foo OR NOT title:bar, the synthetic AllQuery is the positive side of that alternative rather than just a filter candidate source. Using the zero-scored padding here makes documents that satisfy only the NOT title:bar branch match with score 0, unlike the existing top-level negative rewrite or an explicit title:foo OR (* AND -title:bar) branch, which contribute the match-all score; the padding needs to depend on the parent occurrence instead of always using the filter-style zero boost.
Useful? React with 👍 / 👎.
AST -> tantivy.Query for Term (TEXT/KEYWORD/U64/BOOLEAN_EXISTS), And, Or, Not, AndNot, AndMaybe, Require, Boosted, Nothing, Every. Central _boolean_query() choke point applies the quickwit-oss/tantivy#3025 all-MustNot padding workaround uniformly; Not is emitted as a self-contained padded boolean query so nested negative groups (the paperless bug shape) resolve correctly without special-casing. Phrase/Prefix/Wildcard/ranges are deferred to Task 13.
|
@PSeitz can you have a look? |
b834683 to
df97ff4
Compare
The query parser already normalizes an all-negative query at the top level (all_negative/make_non_negative, used to reject/rewrite a bare "-x" or "NOT x" query), but never applied that normalization to nested clauses produced while walking down the AST. As a result, an explicit "a AND NOT b" parses NOT b into its own BooleanQuery containing only a MustNot clause. A boolean query with no Must/Should clause can't produce any candidates on its own, so that nested clause always matches zero documents, and since it's wrapped in a Must at the parent level, the whole query returns nothing -- even though the semantically equivalent "a AND -b" works, because "-b" stays a flat MustNot clause instead of getting nested. This also affected double-negation, e.g. "b -(-a -c)" (intended as "b AND (a OR c)"): the inner "(-a -c)" clause was itself all-negative and matched nothing, silently turning the outer "-(...)" into a no-op filter. Thread a top_level flag through compute_logical_ast_with_occur_lenient so the existing all_negative/make_non_negative fixup also applies to every nested Clause, not just the outermost one. Top-level behavior (reporting QueryParserError::AllButQueryForbidden for a bare negative query) is unchanged. Related: quickwit-oss#1433 (fixed the same nesting problem, but only for the implicit "a NOT b" form -- explicit "AND"/"OR" goes through a different path that this didn't cover), quickwit-oss#1980 (isolated top-level NOT queries, a narrower related case).
- Nested match-all padding now uses boost 0.0 instead of 1.0, so a padded NOT clause no longer scores +1.0 higher than the equivalent "-x" form when the excluded term exists in the index. - all_negative_ignoring_invalid now checks MustNot targets for invalidity too (recursively, via is_effectively_empty), so "NOT missing_field" and "NOT (missing:a OR missing:b)" get dropped by trim_ast instead of padded into an unconditional match. Covered by new regression tests.
- Ignore empty siblings in all_negative_ignoring_invalid instead of disqualifying the whole clause, so trim_ast dropping an invalid sibling doesn't leave a bare MustNot unpadded. - Treat an empty Set leaf (all IN [...] elements invalid) as effectively empty so it isn't padded into a match-all.
df97ff4 to
b125a7c
Compare
2 participants
What
The query parser already normalizes an all-negative query at the top level (
all_negative/make_non_negative, used to reject/rewrite a bare-xorNOT xquery), but never applied that normalization to nested clauses produced while walking down the AST.As a result, an explicit
a AND NOT bparsesNOT binto its ownBooleanQuerycontaining only aMustNotclause. A boolean query with noMust/Shouldclause can't produce any candidates on its own, so that nested clause always matches zero documents, and since it's wrapped in aMustat the parent level, the whole query returns nothing -- even though the semantically equivalenta AND -bworks, because-bstays a flatMustNotclause instead of getting nested.This also affected double-negation, e.g.
b -(-a -c)(intended asb AND (a OR c)): the inner(-a -c)clause was itself all-negative and matched nothing, silently turning the outer-(...)into a no-op filter.Fix
Thread a
top_levelflag throughcompute_logical_ast_with_occur_lenientso the existingall_negative/make_non_negativefixup also applies to every nestedClause, not just the outermost one. Top-level behavior (reportingQueryParserError::AllButQueryForbiddenfor a bare negative query, pertest_single_negative_term) is unchanged.Related
NOTand-operators not always consistent #1433 fixed the same nesting problem, but only for the implicita NOT bform. ExplicitAND/ORgoes through a different grammar path that fix didn't cover.NOTqueries, a narrower related case.Testing
Two new matcher-level tests (real in-memory index + query execution, not just AST-shape assertions):
test_nested_and_not_matches_like_dash_negationtest_negation_semantics_of_nested_not_group(De Morgan double-negation semantics)One pre-existing AST-shape test (
test_parse_query_negative) had its expected string updated to include the newShould(AllQuery)padding, the same kind of change #1609 made for the top-level case.cargo test --libpasses (1163 tests).