limit tag pruning to where it works - #6788
trinity-1686a 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
https://github.com/quickwit-oss/quickwit/blob/2edd659026ab6b39285921c86c2b6c8f17f2e631/quickwit/quickwit-docball-mapper/src/tag_pruning.rs#L258-L260
When an accepted empty TermSet appears beneath must_not (for example, {"bool":{"must_not":[{"terms":{"field":[]}}]}}), extraction creates Or([]) and negate_ast converts it to And([]), which this branch classifies as NoMatch. An empty term set matches no documents, so its negation matches every document; both search callers now translate NoMatch into an immediate empty response, silently dropping all results. Preserve the constant through negation or classify this empty conjunction as match-all/MaybeMatch.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
f1e1535 to
e23362b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e23362b8d7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // an empty And before MaybeMatch filtering matches nothing | ||
| if simplified_conditions.is_empty() { | ||
| return MaybeAst::NoMatch; |
There was a problem hiding this comment.
When an empty term set is negated (for example, an Elasticsearch bool.must_not containing terms: {"owner": []}), negate_ast turns its empty Or into an empty And. An empty conjunction represents true here, but this branch returns NoMatch, so root search prunes every split and returns zero hits instead of all documents; delete-by-query similarly skips the deletion. Return MaybeMatch (or model an explicit always-match state) for this case.
Useful? React with 👍 / 👎.
| }, | ||
| QueryAst::MatchAll | QueryAst::MatchNone => UnsimplifiedTagFilterAst::Uninformative, | ||
| QueryAst::MatchAll => UnsimplifiedTagFilterAst::Uninformative, | ||
| QueryAst::MatchNone => UnsimplifiedTagFilterAst::NoMatch, |
There was a problem hiding this comment.
When MatchNone is a should child of a bool query with minimum_should_match: 0, that clause is optional and the query can still match (for example, a bool whose sole child is match_none simplifies to match-all). The tag extractor ignores minimum_should_match, propagates this new NoMatch, and the root call sites consequently return an empty response without searching any split. Account for the bool's minimum before treating a should-side MatchNone as conclusive.
Useful? React with 👍 / 👎.
e23362b to
647d4ab
Compare
This branch has not been deployed
1 participant
service:quickwit*searches for the literalquickwit*in the metastore, which it ain't gonna find, so no split matches and this query searches through zero split, despite some splits possibly containingquickwit-searcher, or evenquickwit