Repository navigation
Stable 25.8 Backport of #93663 - Fix trash in TokenExtractor - #2608
VighneshPath wants to merge 3 commits into
Conversation
`NgramTokenExtractor::nextInString` adds `UTF8::seqLength` of the lead byte without clamping to the string length. A string that ends with a truncated multibyte UTF-8 character makes the token extend past the end of the string, so `ngrambf_v1` hashes bytes beyond it. For the last row of a block those bytes are uninitialized column padding, so the index content becomes nondeterministic. Replicas then build different `skp_idx_*.idx` files for the same merge, which shows up as `CHECKSUM_DOESNT_MATCH` / `DataAfterMergeDiffersFromReplica` and a re-fetch of the merged part. Upstream cherry-pick to 25.8 (ClickHouse#93766) was closed on conflicts. This applies the same bounds checks to the 25.8 code: `NgramTokenExtractor::nextInString`, `NgramTokenExtractor::nextInStringLike` and `DefaultTokenExtractor::nextInStringLike`. `SparseGramsTokenExtractor` does not exist in this file in 25.8. Original PR: ClickHouse#93663 Closes upstream issue: ClickHouse#92576 (cherry picked from commit f9fc3b2, conflicts resolved manually) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…8 tail `05317_ngrambf_truncated_utf8_overrun` uses one row per granule, so the bytes after the first string are the second row. Without the fix the n-gram `ab\xE4xx` lands in the first granule's filter and `LIKE '%ab\xE4xx%'` reads `Granules: 1/2`; with the fix both granules are skipped (`0/2`). Verified on this branch: fails without the `NgramTokenExtractor::nextInString` change, passes with it. `03784_msan_token_iterator` from upstream relies on `create_table_empty_primary_key_by_default`, which the 25.8 test infrastructure does not set, so add `ENGINE = MergeTree ORDER BY ()`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CI triageVerdict: 2 failures, both infra flakes — neither is caused by this PR. All 6 1. Build (arm_release) — infra (artifact upload stalled)The build itself compiled cleanly — ninja reached This is a GitHub artifact/storage network hiccup during upload, unrelated to the 2. Stateless tests (amd_asan, distributed plan, parallel, 2/2) — infra (docker pull timeout)Job lasted only ~4 min and never started any test — it failed pulling the test image: A Docker Hub TLS handshake timeout before any query ran. The sibling shard PR healthThe backport diff is small and well-contained (three bounds clamps in 🤖 Triage by @blau-ai · evidence: run 39218448627 |
mkmkme
changed the title
TokenExtractor
|
@VighneshPath please, reopen this PR from Altinity's fork. seems like our community workflow is not yet fully functioning |
|
closed in favor of #2672 |
Backport of ClickHouse#93663.
The automatic cherry-pick to 25.8 (ClickHouse#93766) was closed because of conflicts, so I applied the same bounds checks by hand in
NgramTokenExtractor::nextInString,NgramTokenExtractor::nextInStringLikeandDefaultTokenExtractor::nextInStringLike.If a string ends in a cut-off multibyte UTF-8 char,
ngrambf_v1reads a few bytes past the end of it. For the last row in a block that's uninitialized padding, so two replicas can build differentskp_idx_*.idxfiles from the same parts. We hit this on a25.8.16cluster as a constant stream ofCHECKSUM_DOESNT_MATCH/DataAfterMergeDiffersFromReplicaon tables withngrambf_v1over truncated comments. The data files were identical; only the ngram.idxdiffered.Repro: https://fiddle.clickhouse.com/5891f09a-5316-4504-9a42-1ff64b32c8f7 (25.8, different checksums), https://fiddle.clickhouse.com/04608712-7ad0-4b87-a8bc-5ed4663f5b71 (26.3, same).
Added test
05317_ngrambf_truncated_utf8_overrun, which fails without the fix.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
The usage of
ngram_bfindex on a non-UTF-8 data led to an uninitialized memory read, with values that could reside in the resulting index structure. (ClickHouse#93663 by @alexey-milovidov)CI/CD Options
Exclude tests:
Regression jobs to run: