[fix](function) Preserve trailing zero bytes in string distances - #66236
[fix](function) Preserve trailing zero bytes in string distances#66236Mryange wants to merge 1 commit into
Conversation
### What problem does this PR solve? Issue Number: close #xxx Related PR: apache#63291 Problem Summary: String distance functions trimmed trailing zero bytes from every input during execution. CHAR padding is already removed at the storage read boundary, so this work was redundant for CHAR values and incorrectly discarded legitimate trailing zero bytes from STRING and VARCHAR values. Use the complete ColumnString values for Hamming, Levenshtein, and Damerau-Levenshtein distance calculations. ### Release note Fix string distance functions to preserve trailing zero bytes. ### Check List (For Author) - Test: Not run; awaiting author review before compilation and test validation - Behavior changed: Yes; trailing zero bytes now participate in string distance calculations - Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review: request changes
Two blocking issues are in the inline comments: a NULL row can now abort Damerau-Levenshtein before its null map is reapplied, and no regression reaches the BE paths changed by this patch. Please also update the PR metadata: trailing-NUL distance results deliberately change, so Release note: None, the unchecked Behavior changed item, and the empty test checklist do not accurately describe the PR.
Critical checkpoints:
- Goal and proof: non-null inputs consistently retain real trailing NUL bytes across Hamming, Levenshtein, and Damerau-Levenshtein, but mixed nullable blocks can fail before NULL is reapplied and no base-distinguishing test proves the intended behavior.
- Scope and clarity: the production change is small and focused on the two affected BE implementations.
- Concurrency and lifecycle: no shared mutable state, locks, threads, static initialization dependency, or special resource lifetime is introduced.
- Error, nullability, and parallel paths: Hamming's custom nullable paths skip NULL rows, but the shared Levenshtein/Damerau wrapper evaluates arbitrary nested NULL-row payloads and can surface a matrix-cap error; const/vector routes, aliases, and FE folding otherwise use complete values consistently.
- Configuration and compatibility: no configuration, protocol, serialized format, or FE-BE variable changes are introduced. Production CHAR storage read paths remove padding before expression evaluation, while STRING/VARCHAR lengths remain intact.
- Tests and results: blocking gap; existing tests cover ordinary branch shapes but never the changed trailing-NUL behavior or the mixed nullable error path, and this PR changes no generated result file.
- Performance and memory: removing repeated tail scans is favorable for non-null values and adds no allocation site, but arbitrary NULL-row payloads must not trigger avoidable distance-matrix work.
- Observability, persistence, transactions, and data writes: not applicable to this expression-only change.
- User focus: no additional focus was provided; the whole PR was reviewed.
Review was static as required by the action prompt; no build or test command was run.
| const auto previous_offset = i == 0 ? 0 : offsets[i - 1]; | ||
| return StringRef(data.data() + previous_offset, offsets[i] - previous_offset) | ||
| .trim_tail_padding_zero(); | ||
| return StringRef(data.data() + previous_offset, offsets[i] - previous_offset); |
There was a problem hiding this comment.
[P1] Do not evaluate retained payloads for NULL rows
This function family uses the default nullable wrapper with need_replace_null_data_to_default() == false, so mixed nullable blocks are evaluated on arbitrary nested values before the null map is reapplied. NULLIF(s, k) retains s under its NULL row; if one row has s = k = 4096 NUL bytes and another row is non-NULL, the base trims that nested row to empty, but this line keeps all 4096 bytes. damerau_levenshtein_distance then rejects (4096 + 2)^2 = 16,793,604 cells above the 16,777,216 cap and aborts the query even though that row's result must be NULL. Please replace or skip null-row nested data before running the distance implementation and add mixed null/non-null coverage.
| RETURN_IF_ERROR(scalar_vector(left_str_col->get_data_at(0).trim_tail_padding_zero(), | ||
| *right_str_col, res_data)); | ||
| RETURN_IF_ERROR( | ||
| scalar_vector(left_str_col->get_data_at(0), *right_str_col, res_data)); |
There was a problem hiding this comment.
[P1] Add a regression that reaches the patched BE paths
These removals are the entire behavior change, but the changed-file list contains no test/output file and the existing distance suite never constructs a trailing NUL. Literal-only cases are insufficient because Nereids StringArithmetic already counts U+0000, so they can pass on the base revision without executing these BE branches. Please add generated regression coverage that fails on the base and passes here, using table-backed binary values to exercise runtime vector/nullable and const-vector paths for Hamming, Levenshtein, and Damerau-Levenshtein (for example a\0 vs a and a\0 vs aX), plus a CHAR case proving storage padding stays invisible.
|
/review 当前这个pr只是去掉了trim_tail_padding_zero，如果你发现了一些问题，可能是之前的遗留问题，当前pr不考虑这个 |
There was a problem hiding this comment.
Automated follow-up review: changes still requested
Round 1 converged with two normal full-review subagents and a separate risk-focused subagent. All three returned NO_NEW_VALUABLE_FINDINGS: no new inline issue survives validation or duplicate suppression. The two existing P1 threads are still unresolved, non-outdated, and anchored to the current head:
- A mixed nullable block can evaluate a retained trailing-NUL nested payload before the null map is reapplied, so a semantically NULL row can abort Damerau-Levenshtein at its matrix cap.
- No regression currently reaches the changed BE paths with trailing-NUL data or proves that CHAR storage padding stays invisible.
Critical checkpoints:
- Goal and proof: the non-NULL execution paths consistently preserve legitimate trailing NUL bytes for Hamming, Levenshtein, and Damerau-Levenshtein, but the nullable failure and missing base-distinguishing regression mean the goal is not safely proven end to end.
- Scope and clarity: the production diff is small, focused, and limited to removing tail trimming in the two affected BE implementations.
- Concurrency: no threads, shared mutable state, locks, atomics, or lock ordering are involved.
- Lifecycle and initialization: no new static initialization dependency, ownership cycle, cache handle, or special resource lifetime is introduced.
- Error handling, nullability, and parallel paths: Hamming skips NULL rows before evaluation and propagates failures; the shared Levenshtein/Damerau default nullable wrapper does not. Vector/vector, both constant orientations, ASCII/UTF-8 branches, aliases, and FE folding otherwise use complete values consistently.
- Configuration and compatibility: no configuration, FE-BE variable, protocol, function symbol, or persisted/storage format changes are introduced. No rolling-upgrade compatibility branch is required for this scalar semantic correction. Current CHAR direct, dictionary, fallback, and statistics read paths strip physical padding before expression evaluation, while VARCHAR/STRING preserve trailing NUL.
- Conditional logic: no new special condition is added; the existing Hamming null checks have concrete nullable-row paths and remain consistent across branches.
- Tests and results: blocking gap. Existing tests cover ordinary dispatch shapes but contain no table-backed trailing NUL, no CHAR comparison for this behavior, and no mixed nullable retained-payload case; this PR changes no generated result file. Literal folding cannot prove the BE fix because Nereids already counts U+0000.
- Performance and memory: removing repeated tail scans is beneficial on non-NULL inputs and adds no allocation site, but the nullable path can now perform avoidable quadratic work or fail the existing matrix limit on a result that must be NULL.
- Observability: no new operational path needs logs or metrics.
- Persistence, transactions, data writes, and crash recovery: not applicable to this expression-only change.
- Other review state: the existing review-body request to align
Release note: None, the unchecked behavior-change item, and the empty test checklist with the deliberate user-visible change remains applicable. - User focus: I treated unrelated legacy concerns as out of scope, as requested. The two retained blockers are directly tied to this trim removal: one is newly exposed by preserving the payload, and the other is the missing proof for the behavior this PR changes. No additional legacy issue is being raised.
This was a static review as required by the action prompt; no build or test command was run in this review environment.
2 participants
What problem does this PR solve?
Problem Summary: String distance functions trimmed trailing zero bytes from every input during execution. CHAR padding is already removed at the storage read boundary, so this work was redundant for CHAR values and incorrectly discarded legitimate trailing zero bytes from STRING and VARCHAR values. Use the complete ColumnString values for Hamming, Levenshtein, and Damerau-Levenshtein distance calculations.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)