[fix](function) return empty string for split empty input - #65199
Open
Mryange wants to merge 3 commits into
Open
[fix](function) return empty string for split empty input#65199Mryange wants to merge 3 commits into
Mryange wants to merge 3 commits into
Conversation
Mryange
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
July 3, 2026 08:16
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morrySnow
reviewed
Jul 3, 2026
| @@ -23,6 +23,7 @@ suite("test_split_by_string") { | |||
| qt_sql "select split_by_string('','');" | |||
| qt_sql "select split_by_string('',',');" | |||
| qt_sql "select split_by_string('','a');" | |||
| qt_issue_64803 "select split_by_string('', ','), split_by_string('', 'abc'), split_by_string('', '');" | |||
Contributor
There was a problem hiding this comment.
有一个function的回归，会检查fe be 结果一致性。把这个测试case移动到那里边吧：query_p0/sql_functions/string_functions/test_string_all.groovy
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review completed for PR 65199. I did not find a new substantiated issue that needs an inline comment.
Checkpoint conclusions:
- Goal and tests: the PR changes
split_by_string('', non_empty_delimiter)to return[""]while keepingsplit_by_string('', '')as[]. The BE runtime path, FE executable constant-folding path, regression expected outputs, and BE unit coverage are all updated for that behavior. - Scope: the code change is focused on
FunctionSplitByStringandStringArithmetic.splitByString; the rest is test coverage and expected-output updates. - Concurrency, lifecycle, config, persistence, transactions, FE-BE protocol: not applicable to this function-behavior fix.
- Compatibility and parallel paths: no function symbol or protocol change. BE execution and FE constant folding now agree for the issue case;
splitstill maps to the canonicalSplitByString, and generator wrappers use the scalar split path. - Conditions and nullability: empty delimiter and empty source are handled distinctly; NULL propagation still happens before FE executable evaluation, and BE null handling remains delegated to the default nullable implementation.
- Test coverage/results:
test_string_all.groovynow includes both regular execution andtestFoldConstfor the issue case, andtest_split_by_stringcontinues to cover direct and alias runtime cases. The.outchanges match the updated semantics. I did not run tests in this runner becausethirdparty/installedand executablethirdparty/installed/bin/protocare absent. - Performance and observability: the change is a constant-time branch adjustment in the existing loop; no new observability requirement found.
Existing review context: the prior inline thread asking to move the issue regression case into test_string_all.groovy is addressed at current HEAD, so I did not duplicate it.
User focus: no additional user-provided focus points were supplied.
Subagent conclusions: optimizer-rewrite found no new valuable findings; tests-session-config found no new valuable findings. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same ledger and empty final inline comment set.
Contributor
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Possible file(s) that should be tracked in LFS detected: 🚨The following file(s) exceeds the file size limit:
Consider using |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29781 ms |
Contributor
TPC-DS: Total hot run time: 180808 ms |
Contributor
ClickBench: Total hot run time: 25.19 s |
Contributor
Author
|
run buildall |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29485 ms |
Contributor
TPC-DS: Total hot run time: 178717 ms |
Contributor
ClickBench: Total hot run time: 24.94 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Summary:
I found one blocking issue in generated expected output: Array1.out now contains NUL bytes in two unrelated sql_array_sum_decimal256 expected rows. The split_by_string BE runtime change, FE constant-folding change, alias path, and new regression/unit coverage otherwise look aligned under static review.
Checkpoint conclusions:
- Goal/test coverage: the PR addresses
split_by_string('', non_empty_delimiter)and adds BE unit coverage plustest_string_all.groovyruntime/folded coverage; the accepted issue is corrupted expected output. - Scope: the implementation change is focused.
- Concurrency/lifecycle/config/persistence/FE-BE transport: not involved.
- Parallel paths: BE runtime, FE constant folding, and the
splitalias were reviewed; no extra issue found. - Test output correctness: not OK because the unrelated Array1 decimal expected rows contain NUL bytes.
- Existing review focus/thread: no extra user focus was provided; the prior request to move the case into
test_string_all.groovyis addressed.
Validation was static only. The review prompt forbids builds/tests in this runner, thirdparty/installed and thirdparty/installed/bin/protoc are absent, and git diff --check against the PR base could not run because the shallow checkout lacks the base object.
| \N | ||
| 0E-56 | ||
| 329042925341109538642.54940061644684653763274585062761879968415992086870360064 | ||
| 329042925341109538642.5494006164468465376327458506276187996841599208687036006 |
Contributor
There was a problem hiding this comment.
These sql_array_sum_decimal256 expected rows were corrupted while refreshing this output: the previous value ended with ...70360064, but this patch replaces the final 4 with a NUL byte (^@). The same unrelated corruption appears again at line 47112, and this file is now detected as binary data rather than text. Since this PR only changes split_by_string behavior and does not change the Array1 suite query or decimal array-sum logic, please restore/regenerate these decimal expected rows so the .out only contains the legitimate split_by_string updates.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #64803
split_by_string('', non_empty_delimiter)returned an empty array, while empty substrings are preserved for leading, trailing, and consecutive delimiters. Root cause: both BE runtime execution and FE constant folding special-cased empty source strings as zero output elements before considering whether the delimiter was empty. This PR keepssplit_by_string('', '') -> [], and changes non-empty delimiters to return one empty substring, for examplesplit_by_string('', ',') -> [""].Release note
Fix
split_by_string('', non_empty_delimiter)to return[""]instead of[].Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)