Repository navigation
[fix](nereids) Do not turn invalid UTF-8 from BE constant folding into U+FFFD - #69143
Merged
morningman merged 1 commit intoOct 17, 2026
Merged
Conversation
…o U+FFFD ### What problem does this PR solve? Issue Number: None Related PR: apache#69025 Problem Summary: With enable_fold_constant_by_be=true, FoldConstantRuleOnBE built string literals from the values BE returned with PValues.getStringValue(i), which decodes the bytes as UTF-8 with replacement. BE writes the bytes of a string as they are, so bytes that are not valid UTF-8 became U+FFFD: WHERE s = unhex('FF') matched rows holding U+FFFD (EF BF BD) instead of the byte FF, s = concat('a', unhex('FF')) matched nothing, an array, map or struct holding such a string did the same, and SELECT unhex('FF') returned EF BF BD. With the variable off, or with debug_skip_fold_constant=true, the results are right. getResultExpression now reads the bytes with getStringValueBytes(i) and builds the literal only when they are valid UTF-8; otherwise it returns no literal, so the expression stays for BE to evaluate, as for a NaN float. An array, map or struct holding such a string is not folded either. Valid strings fold as before. ### Release note With enable_fold_constant_by_be=true, a constant string whose bytes are not valid UTF-8, such as unhex('FF'), keeps its bytes instead of turning into U+FFFD, so filters and results match those with the variable off. ### Check List (For Author) - Test: Regression test / Unit Test / Manual test - Regression test: fold_constant_by_be_invalid_utf8 runs filters and projections on unhex('FF') with FE folding, BE folding and folding skipped, and checks that the results match - Unit Test: LiteralTest, getResultExpression on invalid and valid UTF-8, also inside arrays, maps and structs - Manual test: the queries above on a local cluster before and after - Behavior changed: Yes. With enable_fold_constant_by_be=true such constants are no longer folded; BE evaluates them, and the results match those with the variable off. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morningman
requested review from
924060929,
englefly,
morrySnow,
starocean999 and
yujun777
as code owners
October 15, 2026 19:46
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed all four changed files at 2c4082e8fd9df6aeaa160540add9d80e489c9357. No new actionable findings or existing blocking inline comments were found.
The raw-byte UTF-8 check preserves invalid string results by retaining the original BE expression. Array, map and struct conversion propagates that decision, while valid UTF-8 continues to fold. All three reviewers returned NO_NEW_VALUABLE_FINDINGS after their final rechecks.
Critical checkpoint conclusions:
- Goal and proof: source tracing supports byte preservation; added unit and regression cases exercise the original corruption. Tests were inspected, not executed.
- Scope and reuse: one focused production branch reuses ByteString validation and the existing unsupported-result fallback.
- Concurrency and locks: new values are local/immutable; no shared-state or locking changes.
- Lifecycle and initialization: no new ownership, resource, thread, callback or static-initialization dependency.
- Configuration: no new settings; existing folding switches retain their gates.
- Compatibility: no symbol, storage-format or FE/BE protocol requirement changes.
- Thrift/protobuf IDs: no definition or field-ID changes.
- Parallel paths: STRING/CHAR/VARCHAR and nested children share the guard. JSON eligibility/parser and legacy protocol paths were checked; no distinct bypass was established.
- Conditions and errors: the comment explains the concrete invalid-byte case. Rejection retains the original subtree without partial literal replacement; equality checks stop unchanged rewrites.
- Test coverage: malformed encodings, valid NUL/supplementary/U+FFFD bytes, character types and complex values are covered. Regression predicates/projections compare three modes and check valid/invalid EXPLAIN behavior.
- Expected results: all seven golden blocks match the inserted bytes, filter matches, concatenation order and null propagation under static inspection. Ordered tags, session switches, cache aliases and Java 17 API compatibility were checked.
- Observability: existing fallback logging includes the expression and query ID; no additional metrics requirement identified.
- Transactions and persistence: no EditLog, failover, visible-version or delete-bitmap behavior changes.
- Writes and atomicity: production changes only optimizer result conversion; no transactional write-path changes.
- FE/BE variables: no new transmitted variable or request option.
- Performance: validation/decoding remain linear, without an added raw byte-array copy or a demonstrated rewrite-loop regression.
- Other optimizer invariants: rule registration/stage, slots, ExprIds, joins, distribution and plan wrappers remain unchanged; no further issue found.
No additional user focus was provided. Both initial risk focuses are resolved with source evidence, and no unresolved candidate or uncovered changed-file scope remains.
Static review only: no builds, tests, cluster queries or source changes were performed. Runtime behavior and the author's reported test execution/golden-generation provenance were not independently verified.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 29608 ms |
Contributor
TPC-DS: Total hot run time: 152937 ms |
Contributor
ClickBench: Total hot run time: 24.64 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
924060929
approved these changes
Oct 16, 2026
yiguolei
pushed a commit
that referenced
this pull request
Nov 3, 2026
… folding into U+FFFD #69143 (#70024) picked from master #69143 Co-authored-by: Mingyu Chen (Rayner) <[email protected]>
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
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: None
Related PR: #69025
Problem Summary:
In short. With
enable_fold_constant_by_be=true, FE turned the bytes of a string that BE computed for a constant into a literal by decoding them as UTF-8 with replacement, so bytes that are not valid UTF-8 became U+FFFD (EF BF BD).WHERE s = unhex('FF')then matched the rows holding U+FFFD instead of the rows holding the byteFF, andSELECT unhex('FF')returnedEF BF BD. This PR leaves such a constant unfolded, so BE evaluates it as it does with the variable off. Strings that are valid UTF-8 fold as before. This was found while testing #69025, which keeps its VARBINARY casts away from BE folding partly for this reason.Background. With
enable_fold_constant_by_be=true,FoldConstantRuleOnBEsends the largest constant subtrees of an expression to a BE and replaces them with literals built from the values BE returns (getResultExpression). BE writes the bytes of a string value as they are intoPValues.string_value, a proto2stringfield that does not require UTF-8, and FE read them withgetStringValue(i), which decodes with replacement. A FE string literal is a Java string, so it holds only text that is valid UTF-8. FE has no rule of its own forunhex, so with the variable off the call stays in the plan and BE evaluates it on the real bytes. The variable is off by default; regression pipelines turn it on at random.The problem, and what it cost. A table
t(k, s)holds'a'(1),FF(2), U+FFFD,EFBFBD(3),61FF(4), NULL (5) and'é',C3A9(6):debug_skip_fold_constant=trueenable_fold_constant_by_be=trueSELECT k FROM t WHERE s = unhex('FF')... WHERE s = concat('a', unhex('FF'))... WHERE array_contains(array(unhex('FF')), s), and the same with a MAP key or a STRUCT fieldSELECT hex(concat(s, unhex('FF'))) FROM t WHERE k = 161FF61EFBFBD61FFSELECT unhex('FF'), bytes sent to the clientFFEF BF BDFFEXPLAIN ... WHERE s = unhex('FF')s = unhex('FF')s = '�'s = unhex('FF')EXPLAIN ... WHERE s = unhex('C3A9')s = unhex('C3A9')s = 'é's = 'é'The query succeeds either way, so the wrong rows and bytes go unnoticed: a session variable changes which rows a filter on binary data matches.
How this PR fixes it. For a string type,
getResultExpressionreads the bytes (getStringValueBytes(i)) and builds the literal only whenByteString.isValidUtf8()holds; otherwise it returns no literal, whichevalOnBEalready treats as "keep the expression" (as for a NaN float). That check rejects exactly the bytes a Java string cannot carry: stray bytes, truncated sequences, overlong forms, surrogates and code points beyond U+10FFFF. An array, map or struct holding such a string is not folded either, since those branches already stop when an element is not folded. A valid string decodes to the same text as before, NUL bytes and supplementary characters included.The JSON branch is unchanged: BE never folds a JSON expression (
shouldSkipFold), and a JSON value inside a folded array, map or struct can only come from a CAST that parses text, and BE's parser rejects text that is not valid UTF-8 (CAST(array(unhex('22FF22')) AS ARRAY<JSON>)is[null]with and without BE folding).Results.
LiteralTest.testGetResultExpressionLeavesInvalidUtf8Unfolded:FF,61FF,C3,C080andEDA080are not folded as STRING, VARCHAR or CHAR, nor as an ARRAY element, a MAP key or value, or a STRUCT field; the same results withC3A9fold.testGetResultExpressionStringKeepsItsBytes: valid UTF-8 with a NUL byte, U+1F600 or U+FFFD itself folds to the same bytes. Without the fix the first test fails. All tests inLiteralTestpass.fold_constant_by_be_invalid_utf8(query_p0/expression/fold_constant) runs the filters and projections above with FE folding, with BE folding and with folding skipped; the results of each block must match in the three modes, and twoEXPLAINchecks show thatunhex('C3A9')is still folded on BE andunhex('FF')is not. Its.outwas generated on a local cluster with this change; with the FE before this change the suite fails atfilter_fold_by_be(row 3 instead of 2).Release note
With
enable_fold_constant_by_be=true, a constant string whose bytes are not valid UTF-8, such asunhex('FF'), keeps its bytes instead of turning into U+FFFD, so filters and results match those with the variable off.Check List (For Author)
Test
fold_constant_by_be_invalid_utf8(query_p0/expression/fold_constant): filters and projections onunhex('FF')and the strings, arrays, maps and structs built from it, in three folding modes.LiteralTest.testGetResultExpressionLeavesInvalidUtf8UnfoldedandLiteralTest.testGetResultExpressionStringKeepsItsBytes.Behavior changed:
enable_fold_constant_by_be=true, a constant whose string result is not valid UTF-8 is no longer folded; BE evaluates it, and FE logs the existingBe constant folding convert ... failedwarning.Does this need documentation?
Check List (For Reviewer who merge this PR)