Repository navigation
Conversation
The ASCII (u8) and Unicode (char) branches of levenshtein_distance_with_threshold contained the same banded DP algorithm twice, differing only in element type and variable names. Extracted the shared logic into a generic banded_levenshtein<T: PartialEq + Copy> helper so band bounds, out-of-band handling and threshold capping only need to be maintained in one place. The is_ascii fast path is preserved: the ASCII case still calls the helper with raw bytes, avoiding a char-vector allocation. Fixes apache#6980.
sunchao
left a comment
There was a problem hiding this comment.
No introduced P1/P2 issues found within this review.
Reviewed the full base-relative diff from da2ff08134ba6ceeccade058aea09e7c21d9472b to aa298df7cbc36e3a68c3d5c92260c1410b545ea0: one commit changing native/spark-expr/src/string_funcs/levenshtein.rs. The PR remains non-draft. Read AGENTS.md and applied review-comet-pr and review-comet-expression-pr. The supplied snapshot and refreshed GitHub discussions contained no reviews, issue comments, inline comments, or review threads.
Summary
- Prior state and problem: Thresholded Levenshtein duplicated its banded dynamic-programming algorithm for ASCII bytes and Unicode characters, requiring fixes to be maintained twice.
- Design approach: Extracts
banded_levenshtein<T: PartialEq + Copy>while keeping string decoding and ASCII detection inlevenshtein_distance_with_threshold. - Correctness: Length checks, shorter-input selection, band bounds, sentinel handling, threshold capping, and final result filtering are preserved. Checked Spark’s
Levenshtein,UTF8String.levenshteinDistance, and relevant tests. Local validation covered empty strings, negative and maximum thresholds, Unicode, argument reversal, and scratch-buffer boundaries without finding a regression. - Compatibility analysis: Checked Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0 sources. Spark 3.4 exposes only the unchanged two-argument form. The threshold implementation is identical across the four inspected newer versions. Comet’s explicit integer return type, null handling, and non-default-collation fallback remain unchanged.
- Key design decisions: Retains borrowed byte slices for ASCII, collects
Vec<char>only for non-ASCII inputs, and preserves the existing bounded scratch-buffer reuse. - Implementation sketch: The wrapper rejects negative thresholds, selects bytes or characters, and invokes the shared helper. The helper performs the existing checks and dynamic-programming recurrence.
- Performance: The extraction preserves allocation choices, band width, loop work, and scratch-buffer sizing. Generic specialization introduces no dynamic dispatch. No unnecessary new allocation or algorithmic overhead was identified. Runtime performance was not benchmarked.
- Design: One shared implementation makes the two paths easier to maintain without changing expression routing or expanding the refactor into the unthresholded algorithm.
- Abstraction & complexity: A private slice-based helper is an appropriate abstraction for the two element types.
PartialEq + Copydirectly supports the existing comparison loop without additional machinery. - Behavioral changes worth calling out: No new result, error, configuration, or fallback behavior was identified. Compared with
branch-1.1at89c729a9697467e8f8d89e3eadf1797a75e2328c, threshold results matched the validation corpus. ASCII acceleration and scratch-buffer reuse are earlier changes already present in this PR’s base. - Suggested improvements: No additional change meeting the P1/P2 reporting bar was identified.
Validation: cargo test --locked --offline -p datafusion-comet-spark-expr string_funcs::levenshtein::tests --lib passed all 10 tests. A disposable Rust harness using the actual base, head, and release kernel bodies passed 1,162,810 exhaustive input/threshold combinations and 1,858 additional Unicode and scratch-buffer cases against an independent full-matrix reference, with overflow checks enabled. git diff --check passed. Project source remained unchanged.
Exact-head CI: Label pull requests succeeded. Comet CI, CodeQL, and Check PR Title report action_required, so GitHub provides no build/test verdict for this head. Full JVM/Spark SQL suites and end-to-end benchmarks were not run locally. No GitHub state was changed.
Summary
levenshtein_distance_with_thresholdhad the same banded dynamic-programming algorithm duplicated for the ASCII (u8) fast path and the Unicode (char) path, differing only in element type and variable names.banded_levenshtein<T: PartialEq + Copy>(s: &[T], t: &[T], threshold: i32) -> i32helper, so band bounds, out-of-band handling and threshold capping only need to be maintained once.is_asciifast path is preserved: ASCII input still goes through the helper vias.as_bytes()/t.as_bytes(), so nochar-vector allocation is introduced for that case. Non-ASCII input still collects intoVec<char>before calling the helper, as before.T.Fixes #6980.
Test plan
s_bytes/s_chars→s,t_bytes/t_chars→t,t_byte/t_char→t_elem).cargo testin this environment — relying on CI to run the existing suite (test_levenshtein_distance_with_threshold,test_levenshtein_distance_with_threshold_max_int,test_levenshtein_distance_with_threshold_unicode,test_longer_inputs_correctness, etc. inlevenshtein.rs).