Repository navigation
[fix](nereids) Fold a VARBINARY to string cast to its bytes like BE does - #69025
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: the ordinary ASCII/BMP correction is focused, but three new constant-folding regressions can change predicate results. All are explained inline: [P1] decoded NUL comparison, [P2] bounded VARCHAR supplementary-character corruption, and [P2] trim_in splitting decoded characters. There were no existing inline threads or applicable existing blocking comment IDs.
Critical checkpoint conclusions:
- Goal and proof: FE binary-to-string casts should retain BE bytes. The new unit and paired regression tests cover ordinary valid UTF-8 and invalid-byte fallback, but the three findings disprove complete equivalence.
- Scope and clarity: one literal implementation and its unit/regression coverage; the malformed-input fallback is documented clearly.
- Concurrency and locks: each conversion uses a local decoder and ByteBuffer; no shared decoder, new thread, lock, heavy locked operation or lock-order change.
- Lifecycle/static initialization: temporary Java objects only; no persistent resources, cycles, release duties or new static-order dependency.
- Configuration: none added. Tests use the existing session folding switch; suite connections are owned and closed by their context.
- Compatibility: no wire, storage-format or function-symbol change. Existing STRING_LITERAL/VARBINARY_LITERAL translation is reused; the intended semantic correction has the three regressions above.
- Thrift/protobuf: no definition changes, so release-branch field-ID checks do not apply.
- Parallel paths: STRING/CHAR/VARCHAR, CAST/TRY_CAST, strict/non-strict and nested casts, direct literal callers, legacy translation and BE execution were traced. Ordinary malformed UTF-8 remains a BE cast rather than becoming NULL; direct FE-only conversions cannot represent arbitrary malformed bytes.
- Conditional/error handling: REPORT rejects concrete malformed UTF-8 without replacement; a general AnalysisException deliberately reaches the visitor's defer-to-BE catch rather than its CastException-to-NULL branch. No additional error-handling defect was verified.
- Test coverage: JUnit 5 value/fallback checks and ordered fold-on/off regression queries are present. Embedded/trailing NUL comparisons, bounded supplementary characters and trim_in with distinct supplementary characters need coverage for the reported bugs. No tests were executed.
- New expected results: all supplied hex/base64/cast/length rows are statically consistent with their SQL, including FF and empty bytes. Neither successful execution nor output-generation provenance was independently verified.
- Observability: this local folding change introduces no distributed operation requiring new metrics/logs; no additional observability issue found.
- Transactions/persistence: no transaction, visible-version, delete-bitmap, EditLog, replay or failover changes.
- Writes/atomicity/crash behavior: no write, commit or recovery mechanism added; no new atomicity, leak or deadlock path verified.
- FE-BE variables: none added; existing literal transmission carries actual Java string values and binary buffers.
- Performance/memory: linear UTF-8 decoding replaces linear hex encoding. ByteBuffer wraps the existing bytes and local Java allocations have bounded lifetimes; no substantiated performance or BE memory-accounting regression.
- Other checks: reviewed caller coercion, null/const/nested cases, Unicode boundaries, comparison rewrites, wire byte lengths, source-based view SQL and test-session isolation. Quoting/view and session-leak suspicions were dismissed with concrete code paths; duplicate NUL reports were merged.
User focus: no additional focus was supplied. All four authoritative changed paths, relevant upstream/downstream code, tests and goldens were reviewed. This was static-only: no builds, tests or source modifications; author-reported runs are not review execution evidence. There are no unresolved candidates or uncovered static scope. Convergence is assessed separately from correctness: the three verified bugs remain even when the review converges.
Review converged after 3 round(s).
| /** The bytes decoded as UTF-8, or empty when they are not valid UTF-8 and only BE can cast them. */ | ||
| public Optional<String> toUtf8String() { | ||
| try { | ||
| return Optional.of(StandardCharsets.UTF_8.newDecoder() |
There was a problem hiding this comment.
[P1] Preserve BE comparison semantics for decoded NUL bytes
SELECT CAST(X'610062' AS STRING) = 'a' now folds to TRUE. visitCast creates the string containing a, NUL, b, then visitEqualTo calls StringLikeLiteral.compareTo, which returns equality when the first unmatched byte is zero, even with significant bytes after it. BE preserves all three bytes and compares their explicit lengths, returning FALSE; the previous hex-text fold also returned FALSE.
A reduced triggering plan is Filter(EqualTo(Cast(X'610062', STRING), 'a')) -> Scan(t). visitCast followed by visitEqualTo rewrites its predicate to TRUE, admitting rows BE would reject. Defer NUL-bearing casts until FE comparisons match BE, or correct that comparator alongside this change, and add folded/unfolded comparison coverage.
There was a problem hiding this comment.
Fixed in 0c522c3 by leaving such casts to BE, not by changing how FE evaluates strings.
The FE behavior is not specific to binary input. With a plain string literal, SELECT 'a\0b' = 'a' also folds to true, because StringLikeLiteral.compareTo takes the bytes after a NUL for padding, while BE compares all three bytes. Before this PR the binary form folded to the hex text 610062, which happened to give BE's answer. VarBinaryLiteral.castValueToString() now throws for decoded text with a NUL byte or a supplementary character, as it already does for invalid UTF-8, so FoldConstantRuleOnFE.visitCast leaves the cast to BE. Ordinary text still folds.
FoldConstantTest.testCastVarBinaryToStringFold asserts that the cast of X'610062' stays unfolded. test_varbinary_cast_fold has edge_fold / edge_no_fold blocks with this comparison and the two expressions from the other threads; both give false, F09F9880, F09F9880. With folding on, the same expressions gave true, 3F, 3F before this commit.
There was a problem hiding this comment.
Both forms in the latest review, checked on a local cluster with this PR:
- With
enable_fold_constant_by_be=true,SELECT CAST(X'610062' AS STRING) = 'a'returns false.FoldConstantRuleOnBEmatches only the root expression, so BE evaluates the whole comparison; it does not fold the cast alone and leave the comparison to FE. Where BE did fold the cast alone, inside a predicate that is not constant, the cast is now left to BE as well (see the U+FFFD thread). SELECT CAST(X'61' AS STRING) = 'a\0b'does fold to true while BE returns false. The NUL is in the other operand, a plain string literal, andSELECT 'a' = 'a\0b'folds to true on master as well, becauseStringLikeLiteral.compareTotreats the bytes after a NUL as padding. Before this PR the hex text61happened to give BE's answer. This PR does not change how FE compares string literals.
| @Override | ||
| protected String castValueToString() { | ||
| return toString(); | ||
| return toUtf8String().orElseThrow(() -> new AnalysisException( |
There was a problem hiding this comment.
[P2] Keep supplementary characters intact in bounded VARCHAR folds
SELECT CAST(X'F09F9880' AS VARCHAR(1)) = '?' now folds to TRUE. The decoder returns the intact emoji, but Literal.uncheckedCastToString constructs VarcharLiteral(value, 1), whose UTF-16 substring(0, 1) retains only its high surrogate. The parser produces Project(EqualTo(substr(Cast(X'F09F9880', VARCHAR(1)), 1, 1), '?')); the outer character-count substr cannot restore the discarded half, and StringLikeLiteral.compareTo encodes the lone surrogate as ? (3F). BE retains the whole character before substring and returns FALSE, as did the old hex-text predicate ('F' = '?').
Defer this bounded cast or construct its literal without splitting code points, and cover a supplementary character at the length boundary with folding enabled and disabled.
There was a problem hiding this comment.
Fixed in 0c522c3, together with the NUL comparison (see that thread): a cast whose decoded text has a supplementary character is now left to BE. The truncation is not specific to binary input either: CAST('😀' AS VARCHAR(1)) with a plain literal folds to the lone high surrogate, which encodes as 3F, while BE keeps F09F9880. FoldConstantTest asserts that CAST(X'F09F9880' AS VARCHAR(1)) stays unfolded, and test_varbinary_cast_fold checks hex(CAST(X'F09F9880' AS VARCHAR(1))) = F09F9880 with folding on and off.
There was a problem hiding this comment.
With enable_fold_constant_by_be=true, a constant expression is still evaluated as a whole by BE, so hex(CAST(CAST(X'F09F9880' AS STRING) AS VARCHAR(1))) stays F09F9880. Inside a filter, WHERE s = CAST(CAST(X'F09F9880' AS STRING) AS VARCHAR(1)) was rewritten to s = '?', because BE folded the inner part and FE cut the result to one UTF-16 unit. This is fixed in 6b6e19d by not folding casts from VARBINARY on BE, and the filter block of test_varbinary_cast_fold covers it in all three modes.
| return Optional.of(StandardCharsets.UTF_8.newDecoder() | ||
| .onMalformedInput(CodingErrorAction.REPORT) | ||
| .onUnmappableCharacter(CodingErrorAction.REPORT) | ||
| .decode(ByteBuffer.wrap(byteValues)).toString()); |
There was a problem hiding this comment.
[P2] Keep decoded characters whole during trim_in folding
SELECT trim_in(CAST(X'F09F9880' AS STRING), '😁') = '?' newly folds to TRUE. The decoded 😀 and trim-set 😁 share a UTF-16 high surrogate. StringArithmetic.trimInImpl checks membership with charAt/indexOf, removes that matching half and leaves an unmatched low surrogate; UTF-8 comparison turns it into ?. BE TrimInUtil compares whole UTF-8 characters, leaves 😀 intact and returns FALSE, as did the old hex-text predicate.
The reduced Project(EqualTo(trim_in(Cast(binary, STRING), '😁'), '?')) becomes Project(TRUE). This uses an unbounded STRING cast, so guarding bounded VARCHAR targets alone does not fix it. Use code-point membership in the trim-set implementation or defer affected folding, and add folded/unfolded coverage for trim_in and its shared ltrim_in/rtrim_in paths.
There was a problem hiding this comment.
Fixed in 0c522c3: the cast of X'F09F9880' is now left to BE, so trim_in over it is no longer folded either. StringArithmetic.trimInImpl does split surrogate pairs, and a plain literal reaches it too: trim_in('😀', '😁') folds to 3F while BE returns F09F9880. That trim-set code is shared with ltrim_in / rtrim_in and is unchanged here. test_varbinary_cast_fold checks hex(trim_in(CAST(X'F09F9880' AS STRING), CAST(X'F09F9881' AS STRING))) = F09F9880 with folding on and off.
There was a problem hiding this comment.
With enable_fold_constant_by_be=true, BE evaluates the whole constant trim_in(...), so the edge block of test_varbinary_cast_fold gives F09F9880 for hex(trim_in(CAST(X'F09F9880' AS STRING), CAST(X'F09F9881' AS STRING))) in all three modes. A cast from VARBINARY inside a larger expression is no longer folded on BE (6b6e19d), so FE never sees the decoded string there either.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes. Three new P2 findings cover invalid UTF-8 losing its byte identity during optional BE constant folding, a legal nullable IN subquery rejected after deferral, and incorrect nested bounded-array conversion. The existing P1 NUL-comparison finding also remains applicable; it is retained without a duplicate inline comment. All four changed files were reviewed at head 0c522c34565490355e0ec132e9161114d3f667c9.
The original direct FE reproducers are guarded, but the complete folding pipeline still has these known residuals:
- NUL comparison, 4258348115: with
enable_fold_constant_by_be=true, the originalCAST(X'610062' AS STRING) = 'a'again folds true after backend materialization bypasses the guard. With the default FE-only mode,CAST(X'61' AS STRING) = 'a\0b'also folds true because the opposite operand contains NUL. BE returns false for both. This existing P1 is carried inexisting_blocking_comment_ids. - Bounded supplementary truncation, 4258348122: the original single bounded binary cast is repaired. With optional BE folding,
CAST(CAST(X'F09F9880' AS STRING) AS VARCHAR(1)) = '?'reaches the same known UTF-16 split through a subsequent cast. No duplicate inline is added. - Supplementary trim-set folding, 4258348131: the original predicate remains wrong with optional BE folding, which recreates the supplementary string outside the guard before
trim_infolds. Sharedltrim_in/rtrim_inuse the same mechanism. No duplicate inline is added.
The required review checkpoint conclusions are:
- Goal and proof: safe UTF-8 decoding fixes the scalar hex-text conversion, but parent evaluation, optional backend materialization, nested bounded casts, and mark-join checks still prevent complete correctness. Added tests support the direct paths; no tests were executed here.
- Scope and clarity: one focused literal change with relevant FE unit and regression coverage; comments explain the special guards.
- Concurrency and locks: no new shared mutable state, threads, locks, or lock-order change. Decoder and buffers are per-call locals.
- Lifecycle and initialization: synchronous local resources; no new ownership transfer, static initialization dependency, or release path.
- Configuration: none added. Existing strict, TRY_CAST, skip-fold, and optional BE-fold modes were traced; the enabled BE-fold mode exposes the new finding.
- Symbols, formats, and rolling compatibility: no symbol, stored format, or protocol definition changes. Existing backend result decoding has the newly reachable limitation reported inline.
- Thrift/protobuf IDs: no changed definitions; a cross-branch ID audit is inapplicable.
- Parallel paths: explicit/implicit casts, strict/TRY_CAST, scalar and recursive complex literal casts, nested normalization, direct caller domains, legacy/raw serialization, downstream consumers, hash/mark joins, and backend constant replacement were covered.
- Special conditions and errors: invalid UTF-8/NUL/supplementary deferral has concrete justification and avoids NULL folding; deferral alone does not protect the complete pipeline.
- Test coverage: ASCII, BMP, empty, invalid UTF-8, NUL, and supplementary values have scalar unit/regression sources. A deterministic backend-fold on/off matrix, nullable constant-IN subqueries, nested bounded-array casts, and known residuals need coverage.
- Expected outputs: all eight blocks match inspected semantics with backend folding disabled; cache aliases and normalization make the paired paths meaningful. Fuzzy P0 can enable backend folding, so the suite does not establish correctness across that mode.
- Observability: no new distributed operation needing additional logging or metrics; existing fold deferral/error handling remains available.
- Transactions and persistence: no transaction, EditLog, failover, or persistence path changed.
- Data writes and crash atomicity: no data-writing or storage lifecycle change; version visibility and MoW bitmap invariants are unaffected.
- FE/BE variables: none added; admitted strings and deferred binary inputs use existing send paths.
- Performance and memory: linear decoding/validation and local allocations; no substantiated additional performance or ownership defect.
- Other issues: no additional distinct verified defect after the complete static scope and duplicate review.
No additional user focus was supplied. Validation was static only: no builds, unit/regression tests, or style commands ran, and generated-golden provenance is not independently proven by the checkout. Runtime results described above are inferred from inspected control flow and the pinned protobuf implementation.
Review converged after three rounds: all three final reviewers returned NO_NEW_VALUABLE_FINDINGS, full changed-file and related-path coverage is complete, and every candidate/risk was verified, accepted, deduplicated, or dismissed with concrete evidence. No unresolved static review question remains; the execution and test-source coverage limits above remain disclosed.
Review converged after 3 round(s).
Existing P0/P1 findings confirmed for this head: #69025 (comment)
| @Override | ||
| protected String castValueToString() { | ||
| return toString(); | ||
| String value = toUtf8String().orElseThrow(() -> new AnalysisException( |
There was a problem hiding this comment.
[P2] Preserve invalid bytes through optional BE constant folding
With enable_fold_constant_by_be=true, SELECT CAST(X'FF' AS STRING) = '�' is newly folded to TRUE; full BE evaluation returns FALSE. The right operand is U+FFFD (EFBFBD), distinct from the input byte FF.
A reduced triggering plan is:
Filter(EqualTo(Cast(X'FF', STRING), '�'))
Scan(t)
visitCast catches this AnalysisException and retains the cast, but the bottom-up FoldConstantRule then applies FoldConstantRuleOnBE to that child. BE returns raw FF through PValues.string_value; getResultExpression creates StringLiteral(getStringValue(i)), and the protobuf Java getter substitutes U+FFFD for invalid UTF-8. Parent visitEqualTo consequently produces Filter(TRUE), admitting rows BE would reject. The previous hex-text fold compared 'FF' with U+FFFD and returned FALSE.
Keep unrepresentable casts out of BE constant replacement as well, or validate the raw returned bytes before constructing an FE string literal. Add predicate and byte-visible coverage with enable_fold_constant_by_be both on and off; the new FE-only unit test does not exercise this result transport.
There was a problem hiding this comment.
Confirmed where the cast is not the whole constant expression, and fixed in 6b6e19d. With enable_fold_constant_by_be=true, WHERE s = CAST(X'FF' AS STRING) was rewritten to s = '�': the cast is the largest constant part of the predicate, BE folded it, and getResultExpression decoded the returned byte 0xFF as U+FFFD, so the filter matched a U+FFFD row instead of the 0xFF row. FoldConstantRuleOnBE.shouldSkipFold now skips a cast from a type that contains VARBINARY, as it already skips from_base64 for this reason, so BE evaluates the cast when it runs the query.
The constant form in this comment, SELECT CAST(X'FF' AS STRING) = '�', returns false on a local cluster: the BE rule matches only the root expression (root(Expression.class)), so BE evaluated the whole comparison. test_varbinary_cast_fold now runs every block with FE folding, with FE and BE folding, and without folding; its filter and in blocks cover this case.
| return toString(); | ||
| String value = toUtf8String().orElseThrow(() -> new AnalysisException( | ||
| "Cannot cast VARBINARY " + computeToSql() + " to a string type on FE: it is not valid UTF-8")); | ||
| if (value.indexOf('\0') >= 0 || value.codePoints().anyMatch(Character::isSupplementaryCodePoint)) { |
There was a problem hiding this comment.
[P2] Allow deferred string constants in nullable IN subqueries
With the default enable_fold_constant_by_be=false, SELECT CAST(X'610062' AS STRING) IN (SELECT s FROM t2) now fails with varbinary type could not in join equal conditions when t2.s is nullable STRING. The comparison operands are both STRING.
The reduced rewritten plan is:
Project(mark)
MarkJoin(mark: EqualTo(Cast(X'610062', STRING), t2.s))
OneRowRelation(dummy)
Scan(t2: s nullable STRING)
This new guard retains the constant cast. SubqueryToApply keeps it as the compare expression, and InApplyToJoin preserves the nullable equality as a mark conjunct. JoinUtils.isHashJoinCondition rejects the slot-free constant operand, so PushDownExpressionsInHashCondition leaves this non-hash mark conjunct unchanged; the nested-loop projection rule also excludes mark joins. CheckAfterRewrite then rejects the binary literal descendant inside the STRING cast. Previously, folding to hex text removed that descendant and the query passed this check.
Keep supported deferred STRING operands valid at this planner boundary, by checking the actual compared types or projecting the constant expression to a STRING slot, and cover a constant cast in a nullable IN subquery's SELECT result.
There was a problem hiding this comment.
Confirmed, and fixed in 6b6e19d. CheckAfterRewrite rejected a hash or mark join condition that had any VARBINARY sub-expression, so CAST(X'610062' AS STRING) IN (SELECT s FROM t) failed although the condition compares two strings. It now rejects a condition only when a comparison in it has a VARBINARY operand, which is what BE cannot hash or compare. VarBinarySqlSupportTest.testJoinConditionComparingStringsCastFromBinary plans IN and NOT IN subqueries over such a cast and checks that comparing two VARBINARY slots is still rejected in hash and mark join conditions. The in block of test_varbinary_cast_fold returns the same results in all three folding modes.
| throw new AnalysisException("Cannot cast VARBINARY " + computeToSql() | ||
| + " to a string type on FE: it has a NUL byte or a supplementary character"); | ||
| } | ||
| return value; |
There was a problem hiding this comment.
[P2] Match BE semantics for bounded strings inside array casts
SELECT CAST([X'6162'] AS ARRAY<VARCHAR(1)>)[1] = 'a' now returns TRUE with FE folding, whereas skipping folding returns FALSE. This uses the two ASCII bytes ab, so the NUL/supplementary guards do not apply.
In the reduced Project(ElementAt(Cast([X'6162'], ARRAY<VARCHAR(1)>), 1) = 'a'), ArrayLiteral.uncheckedCastTo calls the element's checked cast. This method returns ab, and Literal.uncheckedCastToString builds VarcharLiteral("ab", 1), replacing the array cast with ['a']. On BE, create_array_wrapper recursively invokes the string cast wrapper, which preserves both bytes and does not enforce the nested VARCHAR length. The parser adds substr only when the whole cast target is string-like, so no nested substring narrows ['ab']; extracting its element and comparing with a returns false. The previous hex-text fold truncated 6162 to 6, so this predicate previously also returned false.
Defer these bounded recursive conversions or make FE element folding agree with BE, and add folded/unfolded bounded-array coverage. This is a nested length-semantics mismatch, distinct from the existing supplementary-character splitting report.
There was a problem hiding this comment.
Not changed here. The difference is not specific to binary input. With a plain string array, SELECT CAST(['ab'] AS ARRAY<VARCHAR(1)>)[1] = 'a' also folds to true on FE, because the array literal truncates each element to the target length, while BE returns false. This happens on master and with this PR. Before this PR, CAST([X'6162'] AS ARRAY<VARCHAR(1)>) folded to ['6'], the truncated hex text, so the comparison with 'a' gave BE's answer only because the folded value was wrong. Making FE fold nested VARCHAR(n) casts like BE is a separate change to how array literals are cast.
TPC-H: Total hot run time: 28995 ms |
TPC-DS: Total hot run time: 151603 ms |
ClickBench: Total hot run time: 24.53 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of all 12 changed files at 6b6e19d020cca68c4156d25445bdef71b7ca3aa6: no additional independent inline findings. All three assigned reviewers returned NO_NEW_VALUABLE_FINDINGS; all candidates and initial risks were verified, dismissed with evidence, or deduplicated. The review converged in one round.
The inspected chain is binary literal -> FE checked string cast -> UTF-8 literal or deferred Cast -> optional BE constant collection -> join validation/expression translation -> BE byte-preserving evaluation. A retained binary literal propagates digest zero through its expression and scan, disabling condition-cache lookup/insertion.
Existing review context: the original P1 NUL-bearing cast in 4258348115 is fixed by FE deferral and the deep BE-folding skip; no existing original P0/P1 finding remains applicable. The invalid-byte BE-folding, supplementary-character, trim_in and nullable-IN reports are addressed by the inspected guards/validator. The bounded ARRAY<VARCHAR(n)> mismatch in 4260479286 still applies and was not reposted. The opposite-operand plain-string NUL case discussed in the P1 thread's reply also remains known context.
Individual critical-checkpoint conclusions:
- Goal and evidence: representable binary-to-text constants now carry their UTF-8 bytes instead of hex text; unsafe casts remain for BE. Added unit/regression test sources cover the primary reproductions, with the existing bounded-array limitation above.
- Focus and reuse: changes remain focused on folding, its join/cache consequences and tests; recursive FE type detection reuses the existing helper.
- Concurrency and locks: no new production shared mutation, threads or locks; decoder state is local and BE type traversal is read-only. Existing lock order is unaffected.
- Lifecycle and static initialization: no new owners, callbacks, cycles or cross-TU static dependencies; existing literal column ownership is retained.
- Configuration: no new option; tests exercise existing FE/BE folding and cache settings on suite-owned connections.
- Compatibility: no wire, storage-format or function-symbol layout change. Existing Cast execution and cache-disabled sentinel remain in use; no new libc API is introduced.
- Thrift/protobuf IDs: no
gensrcdefinitions changed. - Parallel paths: scalar string types, TryCast, array/map/struct recursion, outer wrappers, optional BE folding, hash/mark joins and cache consumers were traced. Whole-subtree collection checks prevent unsafe casts being folded through wrappers. Complex join keys use implemented serialization/comparison; standalone marks broadcast and avoid partitioned spill.
- Special conditions and errors: concrete UTF-8/NUL/supplementary comments explain deferral. FE representation failures retain casts rather than create NULL; direct VARBINARY comparison restrictions remain enforced.
- Tests: source coverage includes ASCII/BMP text, invalid UTF-8, empty bytes, NUL, supplementary text, scan filters, ARRAY and nullable IN/NOT IN in three folding modes. Planner tests retain negative direct-binary hash/mark checks. Shared map/struct, CHAR and external-scan paths were inspected without added dedicated runtime cases.
- Changed results: all new golden blocks and the changed old
sql_25match the SQL's byte values and mode settings under static inspection. Ordering and table setup follow regression conventions. Generation provenance was not verified. - Observability: existing analysis errors, folding diagnostics and cache counters remain sufficient for these paths; no new distributed operation needs metrics.
- Transactions/persistence: no EditLog, visible-version, delete-bitmap or transaction lifecycle changes.
- Writes/atomicity: no production write or crash-cleanup mechanism is modified; existing insert evaluation retains the byte semantics.
- FE/BE variables: no new transmitted variable or type; typed Cast/literal translation uses the existing protocol.
- Performance/memory: decoding and type/tree traversal are linear and short-circuit where applicable; BE adds no per-row copying or new owned buffer. Cache bypass is the intended correctness fallback.
- Other issues: the final missed-path sweep found no additional substantiated issue; no changed file or correctness question remains unreviewed.
User focus: no additional focus was supplied. Validation limits: this invocation performed static inspection only. No builds, tests, formatting/hygiene scripts or ELF compatibility checks were run; inspected tests, goldens and author reports are not executed test evidence.
Review converged after 1 round(s).
TPC-H: Total hot run time: 29109 ms |
TPC-DS: Total hot run time: 152470 ms |
ClickBench: Total hot run time: 24.6 s |
TPC-H: Total hot run time: 29529 ms |
TPC-DS: Total hot run time: 152795 ms |
ClickBench: Total hot run time: 24.62 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#68968 Problem Summary: BE casts a VARBINARY value to a string type by keeping its bytes: CAST(X'616263' AS STRING) is 'abc'. FE constant folding turned the same cast into the hex text of the bytes ('616263'), because VarBinaryLiteral.castValueToString() returned toString(). Functions that only have string signatures, such as hex() and to_base64(), take a VARBINARY argument through that cast, so their folded results were wrong: SELECT hex(cast('abc' AS varbinary(16))) -- 363136323633, BE: 616263 SELECT to_base64(cast('abc' AS varbinary(16))) -- NjE2MjYz, BE: YWJj SELECT hex(X'FF') -- 4646, BE: FF Fold the cast to the bytes decoded as UTF-8. Bytes that are not valid UTF-8 cannot be carried by an FE string literal; castValueToString() throws an AnalysisException for them, and FoldConstantRuleOnFE.visitCast, which turns only a CastException into NULL, leaves such a cast for BE to evaluate. ### Release note Fix wrong results of constant expressions that cast a VARBINARY value to a string, including hex() and to_base64() of a VARBINARY constant. ### Check List (For Author) - Test: Unit Test, Regression test - FE UT: FoldConstantTest.testCastVarBinaryToStringFold - Regression: query_p0/sql_functions/binary_functions/test_varbinary_cast_fold (the same queries with constant folding on and off) - Behavior changed: Yes. Folded casts of VARBINARY constants to string types now return the bytes instead of their hex text, as BE does. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…ters to BE Folding CAST(VARBINARY AS <string type>) to the decoded bytes is right for ordinary text, but FE does not evaluate two kinds of text like BE: - StringLikeLiteral.compareTo takes the bytes after a NUL for padding, so CAST(X'610062' AS STRING) = 'a' folded to TRUE; BE compares all three bytes and returns FALSE. - VARCHAR(n) truncation and trim_in work on UTF-16 units, so CAST(X'F09F9880' AS VARCHAR(1)) and trim_in(CAST(X'F09F9880' AS STRING), '<U+1F601>') kept half of U+1F600, which encodes as '?'; BE keeps the whole character. Plain string literals such as 'a\0b' or a literal emoji reach the same FE code. Before the decoded fold these binary inputs folded to their hex text, which happened to give BE's answer here. castValueToString() now throws for text with a NUL or a supplementary character, as it does for invalid UTF-8, and FoldConstantRuleOnFE.visitCast leaves such a cast for BE. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…ies and BE folding ### What problem does this PR solve? Issue Number: None Related PR: apache#68968, apache#68930 Problem Summary: FE leaves a cast of VARBINARY bytes to a string type for BE when no FE string literal can carry its result: bytes that are not valid UTF-8, or text with a NUL byte or a supplementary character. Master always folded such a cast to the hex text of the bytes, so three places had never met it in a plan. Two are in FE and are fixed here: - FE: CheckAfterRewrite rejected a hash or mark join condition with any VARBINARY sub-expression, so CAST(X'610062' AS STRING) IN (SELECT s FROM t) on a nullable s failed with "varbinary type could not in join equal conditions". It now rejects a condition only when a comparison in it has a VARBINARY operand. - FE: with enable_fold_constant_by_be=true, BE folded such a cast when it was the largest constant part of a predicate, and FoldConstantRuleOnBE.getResultExpression decoded the result as UTF-8 with U+FFFD for invalid bytes, so WHERE s = CAST(X'FF' AS STRING) became s = '�'. shouldSkipFold now skips a cast from a type that contains VARBINARY, as it already skips from_base64 for the same reason. The third is in BE: ScanOperator digests every conjunct for the condition cache, which is on by default, and VLiteral::get_digest hashed the literal's column, which ColumnVarbinary does not support, so WHERE s = CAST(X'FF' AS STRING) failed with "update_xxHash_with_value is not supported for ColumnVarbinary". apache#68930 has since made VLiteral::get_digest return 0 for a VARBINARY column, which keeps such a conjunct out of the condition cache, so no BE change is needed here. The .out of test_binary_hex_function recorded the folded hex text for sql_25 (61 for X'61'), while its sql_24 already expected the bytes for the same values read from a column; it is regenerated. ### Release note None ### Check List (For Author) - Test: Regression test (test_varbinary_cast_fold runs every query with FE folding, with FE and BE folding, and without folding, with the condition cache on; test_binary_hex_function regenerated) and Unit Test (FE: FoldConstantRuleOnBETest.testSkipCastFromVarBinary, VarBinarySqlSupportTest.testJoinConditionComparingStringsCastFromBinary) - Behavior changed: Yes. A scan conjunct that keeps such a cast does not use the condition cache (apache#68930), and a cast from VARBINARY is not folded on BE. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
6b6e19d to
344590f
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: the narrowed VARBINARY join check still admits a nested binary hash key that fails in the shuffle exchange. The earlier scalar literal-folding findings are fixed at this head and are not reposted.
[P1] Reject nested VARBINARY keys before shuffle hashing: comparesVarBinary checks only each comparison operand's immediate type, so ARRAY<VARBINARY> equality passes the post-rewrite check and is extracted as a hash conjunct. In a partitioned join, the exchange invokes Crc32HashPartitioner::_do_hash/Crc32CHashPartitioner::_do_hash; ColumnArray recursively calls update_crcs_with_value/update_crc32c_batch on its nested ColumnVarbinary, whose hash methods intentionally throw NOT_IMPLEMENTED_ERROR (the unsupported behavior is covered by ColumnVarbinaryTest.HashingIsNotSupported). The serialized in-node hash path runs only after this exchange, so a forced-shuffle join on two array-of-binary keys fails before the join executes. Check the operand type recursively, for example with TypeCoercionUtils.hasVarBinaryType(operand.getDataType()), or add compatible nested-binary exchange hashing, and add a forced-shuffle ARRAY<VARBINARY> regression. This is distinct from the existing scalar CAST(VARBINARY AS STRING) join thread.
Critical checkpoint conclusions:
- Goal and proof: the PR changes VARBINARY-to-string FE folding from hex text to byte-preserving UTF-8 literals and defers values FE cannot represent. The added unit/regression paths cover ordinary text, malformed bytes, NUL/supplementary guards, optional BE folding, filters, and mark-join planning; the new P1 shows nested binary hash exchange handling is incomplete.
- Scope and clarity: the literal/folding changes are focused; the join helper is the narrow change that introduced the nested-key gap.
- Concurrency and locks: no new threads, shared mutable state, locks, or lock-order changes; the failing path is synchronous exchange hashing.
- Lifecycle/static initialization: only local decoder/buffer state is added; no new ownership, callbacks, cycles, or static-order dependency.
- Configuration: no configuration is added; existing FE folding, BE folding, condition-cache, strict-cast, and shuffle settings were traced.
- Compatibility: no thrift/protobuf definitions, storage formats, symbols, or FE-BE variables change. Existing BE cast and exchange interfaces are reused.
- Parallel paths: scalar and nested casts, STRING/CHAR/VARCHAR, strict/TRY_CAST, FE and optional BE folding, hash/mark joins, and serialized versus shuffle hashing were inspected. The nested ARRAY exchange path is the accepted finding; MAP/STRUCT comparison and mark-IN paths remain rejected by existing legality checks.
- Error handling: malformed UTF-8 is deferred instead of fabricated as NULL; the accepted exchange path reaches existing throwing unsupported hash APIs because the new FE check does not recurse.
- Data correctness: direct decoded strings now preserve BE bytes under the guarded paths, but an accepted
ARRAY<VARBINARY>shuffle key can fail at runtime. No transaction, visible-version, delete-bitmap, or persistence path is changed. - Test coverage: tests and goldens were inspected for all changed files and mode matrices; no builds or tests were run under the static-only review contract. A nullable IN/NOT IN unmatched-NULL assertion is a future coverage improvement, not a demonstrated defect.
- Performance/memory/observability: UTF-8 decoding and recursive type checks are linear with local lifetimes; no separate memory, metric, or logging issue was substantiated.
- Existing review context: the prior P1 NUL-cast finding is fixed by the current FE deferral and BE-fold skip, and all existing P2 threads are either fixed or pre-existing/duplicated; no existing P0/P1 ID is carried.
User focus: no additional focus was supplied. The complete changed-file scope, required AGENTS.md guides, existing threads, tests/goldens, and related FE/BE mechanisms were reviewed. This was static-only; author-reported runs and checked-in goldens were not treated as executed test evidence.
Review converged after 2 rounds: all three round-2 reviewers returned NO_NEW_VALUABLE_FINDINGS, the sole accepted M1 finding was independently confirmed, and every other candidate was deduplicated or dismissed with evidence.
Review converged after 2 round(s).
| // BE cannot hash or compare VARBINARY values, but a join condition may compare strings cast from VARBINARY, | ||
| // such as CAST(X'FF' AS STRING) = s, so only the compared operands are checked. | ||
| private static boolean comparesVarBinary(Expression conjunct) { | ||
| return conjunct.anyMatch(e -> e instanceof ComparisonPredicate && ((ComparisonPredicate) e).children() |
There was a problem hiding this comment.
[P1] Reject nested VARBINARY keys before shuffle hashing: comparesVarBinary checks only each comparison operand's immediate type, so ARRAY<VARBINARY> equality passes the post-rewrite check and is extracted as a hash conjunct. In a partitioned join, the exchange invokes Crc32HashPartitioner::_do_hash/Crc32CHashPartitioner::_do_hash; ColumnArray recursively calls update_crcs_with_value/update_crc32c_batch on its nested ColumnVarbinary, whose hash methods intentionally throw NOT_IMPLEMENTED_ERROR. The serialized in-node hash path runs only after this exchange, so a forced-shuffle join on two array-of-binary keys fails before the join executes. Check the operand type recursively (for example with TypeCoercionUtils.hasVarBinaryType(operand.getDataType())) or add compatible nested-binary exchange hashing, and add a forced-shuffle ARRAY<VARBINARY> regression.
There was a problem hiding this comment.
Not changing this here: it does not come from this PR, and master behaves the same. Master's check, conjunct.anyMatch(e -> e.getDataType().isVarBinaryType()), tests each expression node's own type, so an ARRAY<VARBINARY> slot never matched it either; this PR only narrows which operands are checked. On a local cluster with ARRAY<VARBINARY> keys built from numbers(), JOIN [broadcast] returns the expected 24 rows and JOIN [shuffle] fails with [NOT_IMPLEMENTED_ERROR]Method update_crc32c_single is not supported for ColumnVarbinary, with or without this PR. Rejecting nested VARBINARY keys in this check would turn the broadcast joins that work today into analysis errors. What is missing is the exchange hash in the BE: #69142 gives ColumnVarbinary the CRC32 / CRC32C hashes (the same bytes hash as a STRING does), which the Iceberg binary partition routing needs as well, and adds a broadcast-versus-shuffle ARRAY<VARBINARY> join regression.
TPC-H: Total hot run time: 29242 ms |
TPC-DS: Total hot run time: 152234 ms |
ClickBench: Total hot run time: 24.98 s |
FE Regression Coverage ReportIncrement line coverage |
924060929
left a comment
924060929
left a comment
There was a problem hiding this comment.
Reviewed the FE literal conversion, deferred evaluation for unrepresentable strings, optional BE constant-folding guard, hash/mark join validation, and regression coverage. LGTM for the scoped fix; the previously discussed general nested VARCHAR folding limitation remains outside this change.
Static source review only; I did not independently run builds or tests.
…o U+FFFD (#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: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]>
### What problem does this PR solve? Related PR: apache#69025 Problem Summary: VARBINARY 转字符串 FE 折叠成十六进制文本，与 BE 保留原始字节不一致 Backport adaptation: Keep branch legacy-VARIANT join restrictions, timestamp/date compatibility helpers and VarBinary equality handling. Include the required BE condition-cache fix apache#68930 with its unit/regression tests: unfolder VARBINARY/complex object literals must disable unsupported digest hashing rather than throw. Both FE and BE validation are required for this backport. The branch Literal base has no master castValueToString hook: override VarBinaryLiteral.deprecatingUncheckedCastTo for string-like targets and delegate decoded values to the existing StringLiteral conversion, preserving other literal conversions. Add CHAR, VARCHAR truncation and identity-cast assertions. Route both direct and nested-container fallback conversions through the same decoding hook; retain the invalid-UTF8 array rejection test and add a valid array conversion assertion. Use const accessors for Const/Nullable/Array/Map/Struct digest recursion because the branch mutable traversal moves offsets and updates variant state; keep existing scalar/variant hashing behavior. CI follow-up: Regenerate send_long_data with the standard regression runner. Exactly five binary HEX cells change from the legacy double encoding 3030464638303431 to the correct raw-byte encoding 00FF8041: three streamed executions and two force-forward-mode executions. MD5, empty streams, inline strings, scalar parameters and connection reuse results are unchanged. Full ASAN build and all four required regression suites passed on the isolated cluster; production and unit-test sources are unchanged. ### Release note VARBINARY 转字符串 FE 折叠成十六进制文本，与 BE 保留原始字节不一致 ### Check List (For Author) - Test: Unit Test — org.apache.doris.nereids.rules.expression.FoldConstantRuleOnBETest,org.apache.doris.nereids.rules.expression.FoldConstantTest,org.apache.doris.nereids.types.VarBinarySqlSupportTest; FE Checkstyle passed. BE ASAN build, formatting, targeted unit tests and clang-tidy passed; see local validation record. Static analysis used a task-local VFS overlay to remove an orphan NOLINTEND comment from the unchanged baseline types.h; production files and enabled checks were unchanged. Header hygiene gate unavailable: branch-4.2 does not contain build-support/check-build-hygiene.sh or its gates. Cluster regression tests passed: prepared_stmt_p0/send_long_data, query_p0/condition_cache/test_condition_cache_complex_literal, query_p0/sql_functions/binary_functions/test_binary_hex_function, query_p0/sql_functions/binary_functions/test_varbinary_cast_fold. - Behavior changed: Yes, fixes the behavior described above. - Does this need documentation: No (cherry picked from commit a8b95fa)
What problem does this PR solve?
Issue Number: None
Related PR: #68968, #68930
Problem Summary:
In short. FE constant folding cast a VARBINARY constant to a string type as the hex text of its bytes, while BE keeps the bytes. Every folded expression that takes a VARBINARY constant through such a cast, including
hex()andto_base64()of a VARBINARY constant, returned a different result from the same expression evaluated by BE. This PR folds the cast to the bytes, leaves the cast for BE when no FE string literal can carry its result, and makes the two FE places that could not handle such a cast accept it; the third, in BE, is fixed by #68930. This came up while testing #68968.Background.
hex()andto_base64()only have string signatures, so a VARBINARY argument reaches them through an implicitCAST(... AS VARCHAR). BE implementsCAST(VARBINARY AS VARCHAR/STRING)by keeping the bytes as they are. When the argument is a constant,FoldConstantRuleOnFEfolds that cast withLiteral.checkedCastTo, which builds the string fromVarBinaryLiteral.castValueToString(); that method returnedtoString(), the base16 text of the bytes.The problem, and what it cost.
debug_skip_fold_constant=true) / with this PRSELECT hex(cast('abc' AS varbinary(16)))363136323633616263SELECT to_base64(cast('abc' AS varbinary(16)))NjE2MjYzYWJjSELECT length(cast(cast('abc' AS varbinary(16)) AS string))SELECT hex(X'FF')4646FFSELECT k FROM t WHERE s = cast(X'61' AS string)'61')'a'The plan shows the difference: master sends
hex('616263')to BE, while with folding disabled it sendshex(CAST(X'616263' AS varchar(65533))). The same wrong value appears for a column read from a constant subquery or a view.How this PR fixes it.
VarBinaryLiteral.castValueToString()decodes the bytes as UTF-8 (CodingErrorAction.REPORT), so the folded string carries exactly the bytes BE keeps; valid UTF-8 round-trips unchanged when the literal is sent to BE.castValueToString()throws anAnalysisException, andFoldConstantRuleOnFE.visitCast, which turns only aCastExceptioninto NULL, then leaves the cast for BE to evaluate:StringLikeLiteral.compareTotakes the bytes after a NUL as padding, soCAST(X'610062' AS STRING) = 'a'would fold to TRUE, andVARCHAR(n)truncation andtrim_inwork on UTF-16 units, so they would split U+1F600.ScanOperatordigests every conjunct for the condition cache (on by default), andVLiteral::get_digesthashed the literal's column, whichColumnVarbinarydoes not support.WHERE s = CAST(X'FF' AS STRING)failed withupdate_xxHash_with_value is not supported for ColumnVarbinary. [fix](be) Skip condition cache for complex object literals #68930 has since madeVLiteral::get_digestreturn 0 for a VARBINARY column, which keeps such a conjunct out of the condition cache. This PR made the same change before it was rebased onto [fix](be) Skip condition cache for complex object literals #68930, and now leaves BE unchanged.CheckAfterRewriterejected a hash or mark join condition with any VARBINARY sub-expression, soCAST(X'610062' AS STRING) IN (SELECT s FROM t)on a nullablesfailed withvarbinary type could not in join equal conditions. It now rejects a condition only when a comparison in it has a VARBINARY operand.enable_fold_constant_by_be=true, BE folded such a cast when it was the largest constant part of a predicate, andFoldConstantRuleOnBE.getResultExpressiondecoded the result as UTF-8 with U+FFFD for invalid bytes, soWHERE s = CAST(X'FF' AS STRING)becames = '�'.shouldSkipFoldnow skips a cast from a type that contains VARBINARY, as it already skipsfrom_base64for the same reason.Results. On a local cluster, with a table
tholding'a','a'NUL'b', the byte 0xFF, U+FFFD, NULL and U+1F600:WHERE s = CAST(X'FF' AS STRING)WHERE s = CAST(X'610062' AS STRING)'a'NUL'b'rowWHERE s = CAST(CAST(X'F09F9880' AS STRING) AS VARCHAR(1))SELECT CAST(X'FF' AS STRING) IN (SELECT s FROM t WHERE k <> 4)The three modes are FE folding (the default), FE and BE folding (
enable_fold_constant_by_be=true), and no folding (debug_skip_fold_constant=true); P0 runs turnenable_fold_constant_by_beon at random.test_varbinary_cast_foldruns every query in the three modes with the condition cache on, and each block returns the same rows in all three. The FE UT fails whencastValueToString()returns the hex text again (expected: <'abc'> but was: <'616263'>), and each of the two FE fixes above has a test that fails without it.The existing
.outofquery_p0/sql_functions/binary_functions/test_binary_hex_functionrecorded the folded hex text forsql_25(select cast(X'61' as string), ...gave61), while itssql_24already expectedafor the same bytes read from a column. It is regenerated by the script. The.outofexternal_table_p0/dialect_compatible/sql/presto/scalar/TestVarbinaryFunctionsalready expects the BE values (YWJjforto_base64(CAST('abc' AS VARBINARY))).Release note
Fix wrong results of constant expressions that cast a VARBINARY value to a string, including
hex()andto_base64()of a VARBINARY constant.Check List (For Author)
Test
query_p0/sql_functions/binary_functions/test_varbinary_cast_fold:hex(),to_base64(),length()and casts of VARBINARY constants (valid UTF-8, multi-byte,X'FF', empty, NUL, U+1F600), the same casts in scan filters and in nullable IN / NOT IN subqueries, each run with FE folding, with FE and BE folding, and without folding, with the condition cache on; the results of each block match..outgenerated by the script.query_p0/sql_functions/binary_functions/test_binary_hex_function:.outregenerated by the script (sql_25).FoldConstantTest.testCastVarBinaryToStringFold(folds to the bytes for ASCII and multi-byte UTF-8, leavesX'FF', NUL and U+1F600 unfolded),FoldConstantRuleOnBETest.testSkipCastFromVarBinary,VarBinarySqlSupportTest.testJoinConditionComparingStringsCastFromBinary.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)