morningman commented on code in PR #69025:
URL: https://github.com/apache/doris/pull/69025#discussion_r4263726870
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/VarBinaryLiteral.java:
##########
@@ -73,8 +79,39 @@ public Object getValue() {
return byteValues;
}
+ /**
+ * BE casts VARBINARY to a string type by keeping the bytes as they are,
so CAST(X'616263' AS STRING) is
+ * 'abc', not the hex text '616263'. A Java string carries those bytes
only when they are valid UTF-8; for
+ * other bytes, such as X'FF', this throws, and
FoldConstantRuleOnFE.visitCast then leaves the cast for BE
+ * to evaluate (it turns only a CastException into NULL).
+ *
+ * <p>Valid UTF-8 with a NUL byte or a supplementary character is left for
BE too, because FE does not
+ * evaluate such string literals like BE does: StringLikeLiteral.compareTo
takes the bytes after a NUL for
+ * padding, so CAST(X'610062' AS STRING) = 'a' would fold to TRUE while BE
compares all three bytes, and
+ * VARCHAR(n) truncation and trim_in work on UTF-16 units, so
CAST(X'F09F9880' AS VARCHAR(1)) would keep
+ * half of U+1F600 where BE keeps the whole character.</p>
+ */
+ @Override
protected String castValueToString() {
- return toString();
+ String value = toUtf8String().orElseThrow(() -> new AnalysisException(
Review Comment:
Confirmed where the cast is not the whole constant expression, and fixed in
6b6e19d020c. 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/VarBinaryLiteral.java:
##########
@@ -73,8 +79,39 @@ public Object getValue() {
return byteValues;
}
+ /**
+ * BE casts VARBINARY to a string type by keeping the bytes as they are,
so CAST(X'616263' AS STRING) is
+ * 'abc', not the hex text '616263'. A Java string carries those bytes
only when they are valid UTF-8; for
+ * other bytes, such as X'FF', this throws, and
FoldConstantRuleOnFE.visitCast then leaves the cast for BE
+ * to evaluate (it turns only a CastException into NULL).
+ *
+ * <p>Valid UTF-8 with a NUL byte or a supplementary character is left for
BE too, because FE does not
+ * evaluate such string literals like BE does: StringLikeLiteral.compareTo
takes the bytes after a NUL for
+ * padding, so CAST(X'610062' AS STRING) = 'a' would fold to TRUE while BE
compares all three bytes, and
+ * VARCHAR(n) truncation and trim_in work on UTF-16 units, so
CAST(X'F09F9880' AS VARCHAR(1)) would keep
+ * half of U+1F600 where BE keeps the whole character.</p>
+ */
+ @Override
protected String castValueToString() {
- 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)) {
Review Comment:
Confirmed, and fixed in 6b6e19d020c. `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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/VarBinaryLiteral.java:
##########
@@ -73,8 +79,39 @@ public Object getValue() {
return byteValues;
}
+ /**
+ * BE casts VARBINARY to a string type by keeping the bytes as they are,
so CAST(X'616263' AS STRING) is
+ * 'abc', not the hex text '616263'. A Java string carries those bytes
only when they are valid UTF-8; for
+ * other bytes, such as X'FF', this throws, and
FoldConstantRuleOnFE.visitCast then leaves the cast for BE
+ * to evaluate (it turns only a CastException into NULL).
+ *
+ * <p>Valid UTF-8 with a NUL byte or a supplementary character is left for
BE too, because FE does not
+ * evaluate such string literals like BE does: StringLikeLiteral.compareTo
takes the bytes after a NUL for
+ * padding, so CAST(X'610062' AS STRING) = 'a' would fold to TRUE while BE
compares all three bytes, and
+ * VARCHAR(n) truncation and trim_in work on UTF-16 units, so
CAST(X'F09F9880' AS VARCHAR(1)) would keep
+ * half of U+1F600 where BE keeps the whole character.</p>
+ */
+ @Override
protected String castValueToString() {
- 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)) {
+ throw new AnalysisException("Cannot cast VARBINARY " +
computeToSql()
+ + " to a string type on FE: it has a NUL byte or a
supplementary character");
+ }
+ return value;
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/VarBinaryLiteral.java:
##########
@@ -73,8 +79,28 @@ public Object getValue() {
return byteValues;
}
+ /**
+ * BE casts VARBINARY to a string type by keeping the bytes as they are,
so CAST(X'616263' AS STRING) is
+ * 'abc', not the hex text '616263'. A Java string carries those bytes
only when they are valid UTF-8; for
+ * other bytes, such as X'FF', this throws, and
FoldConstantRuleOnFE.visitCast then leaves the cast for BE
+ * to evaluate (it turns only a CastException into NULL).
+ */
+ @Override
protected String castValueToString() {
- return toString();
+ return toUtf8String().orElseThrow(() -> new AnalysisException(
+ "Cannot cast VARBINARY " + computeToSql() + " to a string type
on FE: it is not valid UTF-8"));
+ }
+
+ /** 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()
Review 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. `FoldConstantRuleOnBE` matches 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, and `SELECT 'a'
= 'a\0b'` folds to true on master as well, because
`StringLikeLiteral.compareTo` treats the bytes after a NUL as padding. Before
this PR the hex text `61` happened to give BE's answer. This PR does not change
how FE compares string literals.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/VarBinaryLiteral.java:
##########
@@ -73,8 +79,28 @@ public Object getValue() {
return byteValues;
}
+ /**
+ * BE casts VARBINARY to a string type by keeping the bytes as they are,
so CAST(X'616263' AS STRING) is
+ * 'abc', not the hex text '616263'. A Java string carries those bytes
only when they are valid UTF-8; for
+ * other bytes, such as X'FF', this throws, and
FoldConstantRuleOnFE.visitCast then leaves the cast for BE
+ * to evaluate (it turns only a CastException into NULL).
+ */
+ @Override
protected String castValueToString() {
- return toString();
+ return toUtf8String().orElseThrow(() -> new AnalysisException(
Review 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 6b6e19d020c by not folding casts from VARBINARY on BE, and the
`filter` block of `test_varbinary_cast_fold` covers it in all three modes.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/VarBinaryLiteral.java:
##########
@@ -73,8 +79,28 @@ public Object getValue() {
return byteValues;
}
+ /**
+ * BE casts VARBINARY to a string type by keeping the bytes as they are,
so CAST(X'616263' AS STRING) is
+ * 'abc', not the hex text '616263'. A Java string carries those bytes
only when they are valid UTF-8; for
+ * other bytes, such as X'FF', this throws, and
FoldConstantRuleOnFE.visitCast then leaves the cast for BE
+ * to evaluate (it turns only a CastException into NULL).
+ */
+ @Override
protected String castValueToString() {
- return toString();
+ return toUtf8String().orElseThrow(() -> new AnalysisException(
+ "Cannot cast VARBINARY " + computeToSql() + " to a string type
on FE: it is not valid UTF-8"));
+ }
+
+ /** 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()
+ .onMalformedInput(CodingErrorAction.REPORT)
+ .onUnmappableCharacter(CodingErrorAction.REPORT)
+ .decode(ByteBuffer.wrap(byteValues)).toString());
Review 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 (6b6e19d020c), so FE never sees the decoded string
there either.
--
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]
