Workflow [PR], commit [1deafd2]

AI Review

Summary

This PR adds a postprocessor option for text indexes and wires it into index build, index condition construction, direct-read rewrites, tests, and documentation. The main flow is close, but there are still unresolved result-correctness issues where hasToken can evaluate a different token set on the row-scan/direct-read fallback than the materialized text index path, plus several user-facing documentation mismatches around the same feature. Verdict: block until the hasToken equivalence problems are fixed or explicitly rejected for unsupported cases.

Findings

❌ Blockers

• [src/Processors/QueryPlan/Optimizations/optimizeDirectReadFromTextIndex.cpp:645] hasToken fallback joins postprocessed haystack tokens with a space and then lets hasToken split that string again. If a postprocessor intentionally emits a token containing separators, the materialized index stores that value as one token but the fallback turns it into multiple tokens. For example, with postprocessor = if(val = 'foo', 'bar baz', val), a row containing foo can match hasToken(val, 'baz') on the fallback path even though the built index only contains token bar baz. Existing thread: Text index postprocessor #98939 (comment). Suggested fix: keep the fallback on Array(String) with exact element semantics, or reject/disable the optimized hasToken path when final postprocessed tokens may contain separators.
• [src/Processors/QueryPlan/Optimizations/optimizeDirectReadFromTextIndex.cpp:656] [dismissed by author -- https://github.com/ClickHouse/ClickHouse/pull/98939#discussion_r3407943138] The fallback postprocesses the raw hasToken needle as one token, while the materialized index path tokenizes first and postprocesses every generated token. This still makes non-splitByNonAlpha tokenizers such as ngrams(3) disagree between materialized-index and fallback/direct-read paths; a query like hasToken(val, 'hello') with postprocessor = substring(val, 1, 2) can compare only he on the fallback path instead of the indexed token set he, el, ll. The linked follow-up is not in this PR, and current code still marks hasToken as exact for all tokenizers. Suggested fix: tokenize before postprocessing and express the fallback as an all-token check, or disable hasToken text-index optimization outside tokenizers whose single-token contract is actually valid.

⚠️ Majors

• [docs/en/engines/table-engines/mergetree-family/textindexes.md:327] The docs say hasToken, hasAllTokens, and hasAnyTokens work with any postprocessor and apply it to both haystack and search needle, but they omit the current hasToken restriction that a postprocessed needle containing separators throws BAD_ARGUMENTS. Existing thread: Text index postprocessor #98939 (comment). Suggested fix: document the restriction precisely, or change the implementation so the documented contract is true.
• [src/Functions/hasAnyAllTokens.cpp:455,598] The function documentation says an index-defined postprocessor is applied to hasAnyTokens and hasAllTokens needle/input tokens in general. The SQL function only receives the tokenizer argument; index-specific postprocessing is injected by the text-index rewrite/default-expression path, so use_skip_indexes = 0 or a query without a usable index cannot apply that postprocessor. Existing thread: Text index postprocessor #98939 (comment). Suggested fix: scope this wording to the text-index path and spell out the same limitation as the preprocessor note.
• [src/Functions/hasTokenCaseInsensitive.cpp:33] The source docs only say hasTokenCaseInsensitive has pitfalls with non-default tokenizers and transforms, but do not state the operational rule users need: this function is not text-index-aware for the new preprocessor/postprocessor rewrite path. Existing thread: Text index postprocessor #98939 (comment). Suggested fix: explicitly direct users to hasAnyTokens or hasAllTokens when they expect index-defined transforms to be applied.
• [docs/en/engines/table-engines/mergetree-family/textindexes.md:435] The docs say postprocessor-mapped empty search tokens are ignored, but the current behavior for an all-empty search is no match, not a vacuous match. Existing thread: Text index postprocessor #98939 (comment). Suggested fix: document the all-empty case and the observable result.
• [docs/en/engines/table-engines/mergetree-family/textindexes.md:426] The stemming example says a query token stemmed to run matches rows containing ran; the English stem behavior does not conflate ran with run. Existing thread: Text index postprocessor #98939 (comment). Suggested fix: remove ran from the example or choose examples that the documented stemmer actually normalizes together.

💡 Nits

• [src/Processors/QueryPlan/Optimizations/optimizeDirectReadFromTextIndex.cpp:650-655] The comment still says separator-containing postprocessed tokens keep the original needle and mentions hasToken*, but the code now throws BAD_ARGUMENTS and the optimizer no longer rewrites hasTokenOrNull with a postprocessor. Existing thread: Text index postprocessor #98939 (comment). Suggested fix: update the comment to match the current branch.

Tests

• ⚠️ Please add a focused regression for hasToken with a non-splitByNonAlpha tokenizer, for example ngrams(3) plus postprocessor = substring(val, 1, 2), proving that hasToken(val, 'hello') does not match an unrelated value that only shares the first postprocessed token.
• ⚠️ Please add a focused regression for a separator-emitting postprocessor, for example postprocessor = if(val = 'foo', 'bar baz', val), proving that hasToken(val, 'baz') behaves the same with materialized-index, unmaterialized-part, query_plan_direct_read_from_text_index = 1, and query_plan_direct_read_from_text_index = 0 paths.

Final Verdict

Status: ❌ Block

Minimum required actions: make the hasToken fallback/direct-read behavior equivalent to the materialized text index path, or restrict unsupported tokenizer/postprocessor combinations, then update the source and user-facing docs so the documented contract matches the implementation.

The MSan stress test failure (MemorySanitizer: use-of-uninitialized-value, STID 4179-5154 or 4148-3044) is a known pre-existing issue unrelated to this PR. Fix: #102158

The last 3 CI runs are failing in tsan builds due to #108393 introduced in #82414

This is totally orthogonal to the changes and seems to be also present in master. Everything else ran correctly.

There is already a fix for the root cause of the failure: #108391

And we don't want to delay the merge into master anymore.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 495/511 (96.87%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report