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: Character conversion still goes through ICU even though the supported sets are ASCII, Latin1, UTF-8, and UTF-16. Use simdutf for those transcodes. Keep strict errors, Java/Hive UTF-16 BOM behavior, and the constant character-set contract. ### Release note None ### Check List (For Author) - Test: Unit Test - Existing function_character_encoding_test covers the charset, BOM, null, and invalid-input cases. Not rerun in this commit because the local tree does not have the simdutf third-party build. - Behavior changed: No. Same supported character sets and error conditions. The error suffix now uses simdutf's error name instead of an ICU status name. - Does this need documentation: No
…and decode ### What problem does this PR solve? Issue Number: apache#48203 Related PR: apache#68131 Problem Summary: The character set argument must be a string literal, not a general constant expression. Reject columns and expressions such as upper('utf-8') during analysis, and reject literals outside US-ASCII, ISO-8859-1, UTF-8, UTF-16BE, UTF-16LE, and UTF-16. A NULL literal is still allowed. Run the same check again after rewrite. ### Release note encode and decode now require the character set to be a supported string literal or NULL. ### Check List (For Author) - Test: Unit Test / Regression test - EncodeDecodeTest covers literal, NULL, unsupported charset, and non-literal arguments. - regression-test/suites/query_p0/sql_functions/binary_functions/test_encode_decode.groovy updated for the literal contract. - Behavior changed: Yes. upper('utf-8') and other non-literal character set expressions are rejected. - 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 23, 2026
The charset argument is a supported string literal or NULL, not a general constant expression. Related PR: apache/doris#68131
Contributor
Author
|
run buildall |
Contributor
Author
|
run buildall |
### What problem does this PR solve? Issue Number: close apache#48203 Related PR: apache#68131 Problem Summary: The strict Linux BE builds treated shadowed name parameters and an unused UTF-16 output offset as errors. Rename the character-set parameters and remove the unused local without changing conversion behavior. ### Release note None ### Check List (For Author) - Test: Strict Clang syntax compilation, clang-format, and build hygiene checks. The local targeted BE UT was blocked during macOS OpenBLAS/OpenMP configuration before Doris compilation. - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27856 ms |
Contributor
TPC-DS: Total hot run time: 152402 ms |
Contributor
ClickBench: Total hot run time: 23.84 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27833 ms |
Contributor
TPC-DS: Total hot run time: 152057 ms |
Contributor
ClickBench: Total hot run time: 23.96 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 27666 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-DS: Total hot run time: 151961 ms |
Contributor
ClickBench: Total hot run time: 24 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
encode(string, charset)anddecode(binary, charset)scalar functions with Hive-compatible character conversion semantics, which makes Hive workload migration harder.This PR:
encodeanddecode;NULL; columns and other expressions are rejected during analysis so the vectorized BE can select the conversion path once per query;US-ASCII,ISO-8859-1,UTF-8,UTF-16BE,UTF-16LE, andUTF-16, matched case-insensitively;NULL;The conversion semantics are Hive-compatible for the supported character sets. Doris intentionally restricts the charset argument to a literal (or
NULL) rather than supporting a per-row charset column.Release note
Add
encodeanddecodescalar functions with Hive-compatible conversion semantics forUS-ASCII,ISO-8859-1,UTF-8,UTF-16BE,UTF-16LE, andUTF-16. The charset argument must be a supported string literal orNULL.Check List (For Author)
Test
test_encode_decodepassed in generated-output and comparison modes on a native Linux Doris cluster.EncodeDecodePlannerTest: 2 tests passed, covering planner folding and charset-validation precedence over NULL folding.EncodeDecodeTest: 4 tests passed.function_character_encoding_test.*: 3 tests passed before the simdutf refactor; the same suite covers the simdutf implementation in CI.Behavior changed:
encodeanddecode, including UTF-16 BOM semantics, strict conversion errors, FE constant folding, and the literal charset contract.Does this need documentation?
Check List (For Reviewer who merge this PR)