morningman opened a new pull request, #69143:
URL: https://github.com/apache/doris/pull/69143
### 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 byte `FF`, and `SELECT unhex('FF')` returned `EF 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`,
`FoldConstantRuleOnBE` sends 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 into
`PValues.string_value`, a proto2 `string` field that does not require UTF-8,
and FE read them with `getStringValue(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 for `unhex`, 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):
| Query | BE folding off, or `debug_skip_fold_constant=true` | master,
`enable_fold_constant_by_be=true` | with this PR |
|---|---|---|---|
| `SELECT k FROM t WHERE s = unhex('FF')` | 2 | **3** | 2 |
| `... WHERE s = concat('a', unhex('FF'))` | 4 | **no row** | 4 |
| `... WHERE array_contains(array(unhex('FF')), s)`, and the same with a MAP
key or a STRUCT field | 2 | **3** | 2 |
| `SELECT hex(concat(s, unhex('FF'))) FROM t WHERE k = 1` | `61FF` |
**`61EFBFBD`** | `61FF` |
| `SELECT unhex('FF')`, bytes sent to the client | `FF` | **`EF BF BD`** |
`FF` |
| `EXPLAIN ... 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, `getResultExpression` reads the
bytes (`getStringValueBytes(i)`) and builds the literal only when
`ByteString.isValidUtf8()` holds; otherwise it returns no literal, which
`evalOnBE` already 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.**
- **FE UT.** `LiteralTest.testGetResultExpressionLeavesInvalidUtf8Unfolded`:
`FF`, `61FF`, `C3`, `C080` and `EDA080` are not folded as STRING, VARCHAR or
CHAR, nor as an ARRAY element, a MAP key or value, or a STRUCT field; the same
results with `C3A9` fold. `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 in `LiteralTest` pass.
- **Regression.** The new `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 two `EXPLAIN` checks show that
`unhex('C3A9')` is still folded on BE and `unhex('FF')` is not. Its `.out` was
generated on a local cluster with this change; with the FE before this change
the suite fails at `filter_fold_by_be` (row 3 instead of 2).
- **End to end.** The table above, on the same local cluster before and
after this change.
### 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 <!-- At least one of them must be included. -->
- [x] Regression test
- `fold_constant_by_be_invalid_utf8`
(query_p0/expression/fold_constant): filters and projections on `unhex('FF')`
and the strings, arrays, maps and structs built from it, in three folding modes.
- [x] Unit Test
- `LiteralTest.testGetResultExpressionLeavesInvalidUtf8Unfolded` and
`LiteralTest.testGetResultExpressionStringKeepsItsBytes`.
- [x] Manual test (add detailed scripts or steps below)
- The queries above on a local cluster, before and after this change.
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
- With `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
existing `Be constant folding convert ... failed` warning.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
