Skip to content

fix(query): preserve phrase slop above 255 - #3248

Open
0xamlab wants to merge 5 commits into
quickwit-oss:mainfrom
0xamlab:fix/phrase-slop-overflow-3246
Open

0xamlab wants to merge 5 commits into
quickwit-oss:mainfrom
0xamlab:fix/phrase-slop-overflow-3246

Conversation

@0xamlab

@0xamlab 0xamlab commented Nov 6, 2026 •

Copy link
Copy Markdown

Fixes #3246.

PhraseQuery::set_slop accepts u32, but PhraseScorer carried its accumulated slop in u8 and cast each new value back to u8. At 256 the carried budget wrapped to zero. Carried values are now u32, and distance sums use u64 before the max-slop check.

Slopped match existence is computed exactly with a k-way scan over the position lists. This avoids materializing position-pair or span Cartesian products and uses linear memory in the total position count. Scoring uses the existing approximate frequency counter in deterministic per-document position-list length order, with phrase indices breaking ties. This keeps scoring independent of document-frequency posting reordering and avoids merging long lists before rarer terms. The approximate count is only used after the exact scan confirms a match and is clamped to at least one. Expanded count candidates are bounded before acceptance, avoiding the prior debug assertion.

Regression coverage includes:

  • Positions 0, 257, and 514: slops 255, 256, 300, and 511 return no match, while 512 returns one with both Count and scored search.
  • a x b x c plus a separate b document: the document-frequency-dependent merge order returns one match in both paths.
  • a x b x c x d x e: no debug panic; slop 3 returns zero and slop 4 returns one in both paths.
  • a a b c a d c with a b c d at slop 1 returns one match in both paths.
  • A 30-term phrase at slop 29: at most 153 count candidates and one match in both paths.
  • a b c repeated 10,000 times at slop 30,000: one match in both paths; direct runs peaked at 19.14 MB unscored and 19.51 MB scored RSS after the prior version crossed a 256 MiB watchdog.
  • 10,000 as followed by 10,000 bs and one c, plus a separate a b document: scored search returns one hit at 19.68 MB peak RSS after the prior version crossed a 256 MiB watchdog.
  • a a b c c: phrase_count == 1 in both phrase and document-frequency posting orders.
  • Exhaustive three-term checks: 163,840 comparisons against brute-force span matching, plus the u32::MAX boundary.

Validation:

  • cargo test -p tantivy --lib query::phrase_query — 39 passed, 1 ignored
  • cargo test -p tantivy --lib — 1353 passed, 8 ignored
  • cargo clippy -p tantivy --tests -- -D warnings
  • cargo +nightly fmt --all -- --check
  • cargo +nightly bench --no-run --profile=dev --all-features

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-06T18:21:04.372557Z a530e4f New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8df7e0f84c

ℹ️ 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".

Comment thread src/query/phrase_query/phrase_scorer.rs Resolved
Comment thread src/query/phrase_query/phrase_scorer.rs Resolved
@0xamlab

0xamlab commented Nov 6, 2026

Copy link
Copy Markdown
Author

bb64bbd carries the matched span start and u32 width, so interior terms do not consume slop twice. Each expanded candidate is now bounded before acceptance.

Both reproductions pass in scored and unscored paths: a x b x c + b returns 1 at slop 2; the five-term query does not panic and returns 0 at slop 3 and 1 at slop 4. Large-slop tests cover 255 through 512 and u32::MAX.

Phrase-query tests: 32 passed, 1 ignored. Full lib: 1346 passed, 8 ignored. Clippy -D warnings, nightly fmt check, and all-features bench compilation pass.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bb64bbdbac

ℹ️ 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".

Comment thread src/query/phrase_query/phrase_scorer.rs Resolved
Comment thread src/query/phrase_query/phrase_scorer.rs Resolved
@0xamlab

0xamlab commented Nov 6, 2026

Copy link
Copy Markdown
Author

899efc2 coalesces exact duplicate spans before each merge and uses interval matching instead of greedily consuming anchors. All distinct spans remain available.

The tighter-span case returns 1 in both paths. The 30-term case has at most 1 candidate per merge and returns 1 in both paths; direct peak RSS was 18.94 MB unscored and 18.86 MB scored, after the prior version crossed a 256 MiB watchdog. Previous boundary regressions and 163,840 brute-force comparisons pass.

Phrase-query: 36 passed, 1 ignored. Full lib: 1350 passed, 8 ignored. Clippy -D warnings, nightly fmt, and all-features bench compilation pass.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 899efc23eb

ℹ️ 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".

Comment thread src/query/phrase_query/phrase_scorer.rs Resolved
Comment thread src/query/phrase_query/phrase_scorer.rs Resolved
@0xamlab

0xamlab commented Nov 6, 2026

Copy link
Copy Markdown
Author

c43f9a7 uses a linear-memory k-way scan for exact slop matching and the widened parent frequency counter in canonical phrase order. It does not materialize position-pair or span products.

The 10,000-repetition case returns 1 in both paths with 19.14 MB unscored and 19.51 MB scored peak RSS, after the prior version crossed a 256 MiB watchdog. Phrase count is 1 in both posting orders. The 30-term case stays within 153 candidates; all prior regressions and 163,840 brute-force comparisons pass.

Phrase-query: 38 passed, 1 ignored. Full lib: 1352 passed, 8 ignored. Clippy -D warnings, nightly fmt, and all-features bench compilation pass.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c43f9a7b6e

ℹ️ 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".

Comment thread src/query/phrase_query/phrase_scorer.rs Resolved
@0xamlab

0xamlab commented Nov 6, 2026

Copy link
Copy Markdown
Author

a530e4f orders the approximate count pass by per-document position-list length, with phrase-index tie-breaking. Exact matching still uses the linear k-way scan.

The blocked 10,000-a/10,000-b/one-c scored case returns 1 at 19.68 MB peak RSS, after the prior version crossed a 256 MiB watchdog. Count remains 1 in both posting orders, and all earlier memory, interior, large-slop, and boundary regressions pass.

Phrase-query: 39 passed, 1 ignored. Full lib: 1353 passed, 8 ignored. Clippy -D warnings, nightly fmt, and all-features bench compilation pass.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PhraseQuery with a slop above 255 matches terms further apart (u8 carried slop)

1 participant