Conversation
### What problem does this PR solve? Issue Number: apache#48203 Related PR: None Problem Summary: Doris lacks Hive-compatible encode(string, charset) and decode(binary, charset) scalar functions. Add FE signatures and constant folding, BE vectorized ICU conversion with strict malformed and unmappable input handling, case-insensitive support for the six Hive-documented character sets, Java-compatible UTF-16 BOM behavior, null propagation, and focused tests. ### Release note Add Hive-compatible encode and decode scalar functions for US-ASCII, ISO-8859-1, UTF-8, UTF-16BE, UTF-16LE, and UTF-16. ### Check List (For Author) - Test: Unit and regression tests - BE function_character_encoding_test.*: 3 tests passed. - FE StringArithmeticTest: 10 tests passed; Maven reactor succeeded. - Native Linux FE/BE build succeeded. - test_encode_decode regression suite passed in generated-output and comparison modes. - Behavior changed: Yes. Add encode and decode with Hive-compatible types, supported character sets, UTF-16 BOM semantics, null propagation, and strict conversion errors. - Does this need documentation: Yes. A follow-up doris-website PR is required.
### What problem does this PR solve? Add positive regression coverage for Hive-compatible encode and decode across supported character sets, BOM handling, empty strings, and null propagation. ### How does this PR solve the problem? Exercise table-driven encode/decode queries and record runner-generated expected output while retaining the existing invalid-conversion checks. ### Check List - [x] Regression test - [x] FE unit test - [x] BE unit test
hacklu-tu
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
September 17, 2026 08:44
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
…lict ### What problem does this PR solve? Related PR: apache#68131 Problem Summary: CI stopped before compilation because master URL tests and encode/decode helpers conflicted in StringArithmeticTest. Merge master and preserve both test groups. ### Release note None ### Check List (For Author) - Test: run-fe-ut.sh --run org.apache.doris.nereids.trees.expressions.functions.executable.StringArithmeticTest (12 tests passed) - Behavior changed: No - Does this need documentation: No
10 tasks
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27961 ms |
Contributor
TPC-DS: Total hot run time: 153039 ms |
Contributor
ClickBench: Total hot run time: 24.12 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: apache#48203 Related PR: apache#68131 Problem Summary: The FE incremental coverage gate reported 75.44% because the encode and decode scalar expression contracts were not exercised. Add focused unit tests for signatures, child rewriting, argument validation, and scalar visitor delegation. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.nereids.trees.expressions.functions.scalar.EncodeDecodeTest (3 tests passed) - ./run-fe-ut.sh --coverage --run org.apache.doris.nereids.trees.expressions.functions.scalar.EncodeDecodeTest (3 tests passed; affected lines fully covered) - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27620 ms |
Contributor
TPC-DS: Total hot run time: 153063 ms |
Contributor
ClickBench: Total hot run time: 23.9 s |
Contributor
Author
|
run cloud_p0 |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run check_coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
1 similar comment
Collaborator
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static-only review of exact head 7a9f82b620e68885d50a0ba7410ee2a53a4643fb against base e85575e3ad60389259dfe267a90124e6299a230a. I found two correctness inconsistencies; the exact failure paths and requested coverage are in the inline comments.
Checkpoint conclusions:
- Goal, approach, and tests: the six advertised charsets, strict conversion errors, UTF-16 BOM behavior, and NULL propagation are implemented and broadly tested, but the two retained invariance cases are uncovered.
- Scope control: all 12 authoritative changed files plus registration, constant execution, literal serialization, column ownership, source discovery, and test discovery call chains were covered.
- Concurrency: converter and scratch state are execution-local; no shared mutable state or lock-order issue is introduced.
- Lifecycle and static initialization: ICU resources are RAII-owned and reset safely; no static-initialization hazard was found.
- Configuration: no configuration surface is added or changed.
- Compatibility: no persisted or wire format changes are introduced; old BEs cannot execute the new function names until upgraded, which is the ordinary limitation for a new scalar function.
- Parallel paths: FE folding and BE runtime types/names agree, except for the retained charset-name normalization mismatch.
- Null, const, and type safety: result ownership and checked shapes are sound, except for the retained constant-charset validation ordering.
- Test coverage and result correctness: BE, FE, and ordered regression oracles are sound for covered cases; tests do not compare NULL plus an invalid constant charset or Unicode-folded literal names with runtime column names.
- Observability: no new long-lived service or state transition needs metrics/logging.
- Persistence and replay: not applicable; no durable state is changed.
- Data writes: only scalar result columns are produced, and their storage owns the copied bytes.
- FE/BE contract: signatures, nullability, physical return types, visitor dispatch, and serialization agree apart from the inline normalization issue.
- Performance and memory: the implementation is linear and task-thread allocations are accounted; repeated converter setup and the no-op varbinary reserve were reviewed but lacked evidence of a material regression.
- Other issues: none survived adjudication or the second convergence round.
Validation status: no local build or test was run, as required by the review task. Author and CI reports were treated as external claims, not independent validation. At submission time GitHub reports successful compile, BE UT, FE UT, P0, cloud, and performance checks, while check_coverage_fe is failing and the automated code-review check remains pending. No additional review focus was supplied.
| ConverterCache converters; | ||
| std::string converted; | ||
| CharacterSet constant_character_set = CharacterSet::UTF_8; | ||
| if (character_set_is_const && input_rows_count != 0 && |
Contributor
There was a problem hiding this comment.
[P2] Preserve NULL propagation before validating a constant charset
This parse runs before the row loop, so a nullable source column containing NULL plus a constant unsupported charset (for example, GBK) fails here. The same logical row with the charset supplied by a nonconstant column reaches the NULL branch first and returns NULL, making results depend on the physical const/vector shape. Please validate and cache the constant charset only when the first non-NULL row needs it, and cover this case for both functions.
|
|
||
| private static Charset supportedCharacterSet(String name) { | ||
| String canonicalName; | ||
| switch (name.toUpperCase(Locale.ROOT)) { |
Contributor
There was a problem hiding this comment.
[P2] Use identical charset-name folding in FE and BE
Locale.ROOT uppercasing applies Unicode mappings: Uſ-ASCII (U+017F long s) becomes US-ASCII, so a literal can fold successfully here. BE only uppercases ASCII bytes and rejects the identical value when it comes from a column, so constant folding changes query behavior. Please use the same ASCII-only validation on both sides (or otherwise share exactly the same normalization) and add a literal-versus-runtime regression case.
### What problem does this PR solve? Issue Number: close apache#48203 Related PR: apache#68131 Problem Summary: FE constant folding normalized character set names with Unicode uppercasing, while BE only performs ASCII case-insensitive matching. This allowed names such as Uſ-ASCII to fold successfully in FE but fail when supplied through a runtime column. Match supported names with ASCII-only comparison and add literal-versus-runtime regression coverage. ### Release note Fix inconsistent character set validation between FE constant folding and BE runtime execution for encode and decode. ### Check List (For Author) - Test: Unit Test / Regression test - ./run-fe-ut.sh --run org.apache.doris.nereids.trees.expressions.functions.executable.StringArithmeticTest (13 tests passed) - Added regression coverage for literal and runtime character set arguments; not run locally because it requires a Linux Doris cluster - Behavior changed: Yes. Non-ASCII names that only become supported through Unicode case conversion are now rejected consistently. - Does this need documentation: No
Contributor
Author
|
run buildall |
### What problem does this PR solve? Issue Number: apache#48203 Related PR: apache#68131 Problem Summary: The clean TeamCity compile build rejects the U+017F test literal because its Unicode escape lacks the trailing code point explanation required by FE Checkstyle. Define the charset once with a trailing U+017F comment and reuse it for encode and decode. ### Release note None ### Check List (For Author) - Test: Unit Test - mvn -pl fe-core -DskipTests -Dcheckstyle.cache.file=/tmp/doris-checkstyle-green-cache-68131 checkstyle:check (passed) - ./run-fe-ut.sh --run org.apache.doris.nereids.trees.expressions.functions.executable.StringArithmeticTest (13 tests passed) - Behavior changed: No - Does this need documentation: No
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 27588 ms |
Contributor
TPC-DS: Total hot run time: 153699 ms |
Contributor
ClickBench: Total hot run time: 23.94 s |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27642 ms |
Contributor
TPC-DS: Total hot run time: 153391 ms |
Contributor
ClickBench: Total hot run time: 23.96 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
linrrzqqq
reviewed
Sep 20, 2026
| return Status::OK(); | ||
| } | ||
|
|
||
| Status convert(StringRef input, std::string_view character_set_name, std::string& output) { |
Collaborator
There was a problem hiding this comment.
P1：每行做了四次 ICU 扫描，实测慢约 2 倍
每个非空输入依次执行：
ucnv_toUChars预计算 UTF-16 大小ucnv_toUChars真正转换ucnv_fromUChars预计算结果大小ucnv_fromUChars真正转换
这会完整扫描输入/中间结果四次，并生成 _utf16 中间缓冲区，最后还要把 std::string converted 再复制进结果列。
我用仓库自带 ICU、复用 converter，做了隔离转换内核的本地 microbenchmark，UTF8 → UTF16BE 的结果为：
| 输入长度 | 当前实现 | ucnv_convertEx 直接转换 |
差距 |
|---|---|---|---|
| 15 B | 200 ns/row | 75 ns/row | 2.67× |
| 63 B | 383 ns/row | 157 ns/row | 2.43× |
| 1023 B | 3978 ns/row | 1991 ns/row | 2.00× |
| 65535 B | 272 μs/row | 124 μs/row | 2.19× |
吞吐测试：
- Encode UTF8 → UTF16BE：230 MiB/s → 499 MiB/s
- Decode Latin1 → UTF8：659 MiB/s → 1336 MiB/s
这不是完整 Doris 端到端 benchmark，但已经能证明转换内核有约 2 倍的优化空间。建议使用 ucnv_convertEx 的 pivot buffer 做直接转换，并使用可增长的目标 buffer。
此外：
_utf16是普通std::vector<UChar>- 输出 scratch 是普通
std::string - 一个 block 最多缓存 7 个 converter，每个 converter 都可能保留自己的最大
_utf16容量
大字符串和混合 charset block 下，这些显著 scratch allocation 没有使用 Doris allocator。直接转换既能减少内存峰值，也能避免这部分中间缓冲。
### What problem does this PR solve? Issue Number: close apache#48203 Related PR: apache#68131 Problem Summary: The BE character conversion path preflighted and converted every value twice through a full UTF-16 intermediate buffer. This repeatedly scanned input and pivot data and copied decode output once more into the result column. Stream conversion through ICU ucnv_convertEx with a bounded pivot, reuse block buffers, append decode output directly to ColumnString, and preserve strict conversion errors and Hive-compatible UTF-16 BOM behavior. On the same Linux Release build, the final implementation improved all 40 column execution cases by 1.96x to 3.92x. Extended reruns for every case whose initial CV exceeded 5% reduced CV to 1.0% to 2.8% and measured 2.17x to 3.93x. ### Release note Improve the query performance of encode and decode without changing their SQL behavior. ### Check List (For Author) - Test: Unit Test / Manual performance test - Linux Release build with ./build.sh --benchmark (passed) - ./run-be-ut.sh --run --filter='function_character_encoding_test.*' (5 tests passed) - 40-case Release benchmark correctness smoke test (40/40 passed) - Fixed-CPU ABBA performance test with 10 samples per case (1.96x to 3.92x) - Extended ABBA rerun for every case with initial CV above 5% (2.17x to 3.93x, CV 1.0% to 2.8%) - build-support/check-format.sh (passed) - build-support/check-build-hygiene.sh (passed) - clang-tidy attempted but blocked by the pre-existing unmatched NOLINTEND in be/src/core/types.h:576 - Behavior changed: No - Does this need documentation: Yes. apache/doris-website#4151
### What problem does this PR solve? Issue Number: apache#48203 Related PR: apache#68131 Problem Summary: encode and decode previously accepted a per-row character set column. Require the second argument to be a constant expression, including folded expressions such as upper('utf-8'). Reject table columns in FE. Do not fold away an invalid character set when the first argument is a null literal, so FE constant nulls and BE column nulls return the same error. Rewrite the regression CASE queries that evaluated every charset against every row. ### Release note encode and decode now require a constant character set expression. An unsupported character set is rejected even when the input is NULL. ### Check List (For Author) - Test: Unit Test / Regression test - ./run-fe-ut.sh --run org.apache.doris.nereids.trees.expressions.functions.scalar.EncodeDecodeTest (4 tests passed) - ./run-be-ut.sh --run --filter='function_character_encoding_test.*' (7 tests passed) - ./run-regression-test.sh --run -d query_p0/sql_functions/binary_functions -s test_encode_decode (1 suite passed) - Behavior changed: Yes. The character set argument must be constant. Invalid character sets error even when the first argument is NULL. - Does this need documentation: Yes. apache/doris-website#4151
hacklu-tu
added a commit
to hacklu-tu/doris-website
that referenced
this pull request
Sep 21, 2026
Document that encode and decode take a constant character set expression, reject table columns, and still error on an unsupported charset when the input is NULL. Related PR: apache/doris#68131
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28306 ms |
Contributor
TPC-DS: Total hot run time: 153038 ms |
Contributor
ClickBench: Total hot run time: 23.65 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
Collaborator
|
run external |
Collaborator
|
run feut |
Collaborator
|
/review |
Contributor
There was a problem hiding this comment.
Static-only review of exact head 632fa2a98c7770f581d2b87611830acafb806baf against base e85575e3ad60389259dfe267a90124e6299a230a. I am requesting changes for two P2 issues: the new FE implementations are bypassed by the production folding rule, and the constant-only charset restriction rejects a query shape supported by Hive.
Checkpoint conclusions:
- Goal and scope: registration, FE/BE types, strict conversion, UTF-16 BOM handling, and the focused factory integration are present, but the advertised behavior is incomplete because of the two inline findings.
- Production paths and tests: direct evaluator tests bypass the planner gate; value-only regression tests can pass through BE; and the nonconstant regression cases codify the Hive-incompatible rejection. No build, test, benchmark, or runtime reproduction was run under this review task, so author/CI claims were not treated as independent validation.
- Correctness and compatibility: FE/BE nullability and valid constant conversion behavior otherwise align; no persistence, wire-format, upgrade, configuration, transaction, or data-write change is introduced. The per-row charset contract is the material compatibility gap.
- Concurrency, lifecycle, memory, and errors: converter state and result storage are execution-local, ICU handles are RAII-owned, errors are propagated, and static inspection found no separate thread-safety, ownership, offset, or overflow defect.
- Integration and alternatives: builtin/visitor/factory registration is complete and there is no live legacy-planner implementation that restores the missing FE route. Optional BE folding is disabled by default and does not make the new FE evaluator production-reachable.
- Observability and security: no new distributed/persistent operation needs additional telemetry, and no security-sensitive boundary is changed.
- User focus: no additional review focus was supplied.
Coverage and convergence: all 14 changed paths were reviewed; normal BE, normal FE/integration, and risk-focused reviews converged after two rounds. The live head and all review threads were refreshed immediately before submission. The three existing threads were treated as hard duplicate fences and are not repeated here.
| // calls with any null child to NULL and skips backend evaluation. | ||
| @Override | ||
| public boolean foldable() { | ||
| return false; |
Contributor
There was a problem hiding this comment.
[P2] Keep valid literal calls reachable by planner folding
FoldConstantRuleOnFE.visitBoundFunction() calls preProcess() first, and that method returns this expression immediately when foldable() is false. Thus a real OneRowRelation -> Project(decode(X'E4B8AD', 'UTF-8')) plan never reaches the new StringArithmetic.decode implementation; the project remains a function instead of becoming Project('中'), so PhysicalOneRowRelation.computeResultInFe() cannot return the literal result without a BE. The new tests call ExpressionEvaluator directly, whose separate isConstant() gate bypasses this production path, so they do not prove planner folding. Please preserve invalid-charset-before-NULL behavior with targeted validation/folding instead of disabling folding for every input, and add a planner-level test; Encode has the same reachability problem.
|
|
||
| @Override | ||
| public void checkLegalityBeforeTypeCoercion() { | ||
| if (!getArgument(1).isConstant()) { |
Contributor
There was a problem hiding this comment.
[P2] Preserve Hive's per-row charset contract
This rejects encode(payload, charset_name) (and the equivalent decode call), but Hive treats a constant charset only as a cache optimization: GenericUDFEncode and GenericUDFDecode read argument 2 per row when it is not constant. Since this PR is advertised as Hive-compatible, migrated queries using a charset column now fail analysis instead of running. Please retain per-row charsets (the BE already has a bounded converter cache) and add mixed-charset/NULL regression coverage; otherwise narrow the API and release claim explicitly to constant-only charsets.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
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
3 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: #48203
Related PR: apache/doris-website#4151
Problem Summary:
Doris does not provide Hive-compatible
encode(string, charset)anddecode(binary, charset)scalar functions, which makes Hive workload migration harder.This PR:
encodeanddecode;Release note
Add Hive-compatible
encodeanddecodescalar functions for US-ASCII, ISO-8859-1, UTF-8, UTF-16BE, UTF-16LE, and UTF-16.Check List (For Author)
Test
test_encode_decodepassed in generated-output and comparison modes on a native Linux Doris cluster.function_character_encoding_test.*: 3 tests passed.StringArithmeticTest: 10 tests passed; Maven reactor succeeded.Behavior changed:
encodeanddecodefunctions, including UTF-16 BOM semantics, null propagation, and strict conversion errors.Does this need documentation?
Check List (For Reviewer who merge this PR)