[SPARK-58441][SQL] Fix wrong results in instr and substring_index for ICU collations caused by inconsistent StringSearch iteration - #57647
Conversation
9eeaa12 to
c5f813e
Compare
… ICU collations caused by inconsistent StringSearch iteration
…ess the stretch past the anchor dominates
…ight reference and state the window boundary invariant accurately
…TF8_BINARY, cover the windowed paths under CI and AI, and document the ring buffer's allocation
…rlapping search gets stuck, skip the out-of-range match at a snapped start index, and bound the repeated walk past the anchor
…h the AI cases to UNICODE_CI_AI
HyukjinKwon
left a comment
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 0 nits.
Exceptionally well-analyzed collation correctness fix; design is sound, with one honestly-documented worst-case perf shape.
Verification
Confirmed the forward-enumeration approach (previous() is genuinely unreliable here, matching the existing trimRight precedent), the sentinel/pre-start/anchor corrections against UTF8String.indexOf, and that UTF8_BINARY/UTF8_LCASE paths are untouched. The one shape that regresses (a large negative start with no match before the anchor becomes a full forward scan) is documented and left intentionally because bounding the scan would change the match set -- a reasonable, clearly-stated tradeoff worth a maintainer's eye before this leaves draft.
Scanner note: contract_claim_verifier could not complete (an infra hang), so its automated behavioral/type-contract pass was skipped; link_verifier, text_quality and local_efficiency ran clean, and the contract/semantics claims were verified manually here.
What changes were proposed in this pull request?
Fix four defects in
CollationAwareUTF8String's use of ICUStringSearch, affectinginstr(3/4-arg) andsubstring_indexunder ICU collations:previous(), which does not visit the same match set asnext(): it skips overlapping matches, and skips or misaligns matches when a character maps to multiple collation elements. (trimRightalready documents this and avoidsprevious().) Both backward helpers now enumerate forward and select the requested occurrence from the end.UTF8String.indexOf.UTF8String.indexOfreports none. The 0-based anchor is nownumCodePoints + start.Enumerating forward means the search cannot start at the answer, so the backward search runs over a trailing window that grows until it holds the requested match or reaches the start of the target. See the
findIndexFromEndJavadoc.Why are the changes needed?
instrandsubstring_indexsilently return wrong results under ICU collations:UTF8_BINARY and UTF8_LCASE are unaffected; they do not use these code paths.
Performance
Measured with an ad-hoc in-process harness over
CollationSupport(CollationBenchmarkhas noinstr/substring_indexcoverage today). On short targets, parity to about 2.4x slower — the worst case issubstring_index('/usr/local/share/spark/conf', '/', -1)at 831ns -> 2,025ns. On long targets, parity to 1.4x slower while the answer is near the anchor, and 1.3x to 1.5x faster once it is far away or absent (200KB target, delimiter absent: 8.27ms -> 6.30ms), whereprevious()had to walk the whole target anyway.One shape is much slower:
instrwith a large-magnitude negative start and no match before the anchor puts the anchor at index 0, so the search becomes a single forward scan of the whole target, whereprevious()from index 0 returned immediately. On a 100K target, 116us -> 2.21ms. Bounding that scan would mean truncating the target, which changes the match set, so I have left it.Does this PR introduce any user-facing change?
Yes, bug fix:
instrandsubstring_indexreturn correct results under ICU collations in the cases above.How was this patch tested?
Test cases added to
CollationSupportSuitecovering each defect, the trailing window's growth steps, the ring buffer's resize and wrap, and window boundaries falling inside a surrogate pair, a combining sequence and a contraction. The new backward-search expectations are pinned againstUTF8_BINARY, which does not use the windowed search, so they are anchored to an independent reference rather than to the new implementation.Separately, the windowed search was checked against a single full forward pass from index 0 using the identical iteration protocol, over targets built from combining sequences, mixed precomposed/decomposed forms, sharp s, the fi ligature, surrogate pairs and contractions, against UNICODE, UNICODE_CI, UNICODE_CI_AI, cs, da and sr_Cyrl_SRB. No differences.
Was this patch authored or co-authored using generative AI tooling?
Yes