morningman commented on code in PR #69025:
URL: https://github.com/apache/doris/pull/69025#discussion_r4285193371
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/CheckAfterRewrite.java:
##########
@@ -192,20 +193,27 @@ private void checkMetricTypeIsUsedCorrectly(Plan plan) {
} else if (plan instanceof LogicalJoin) {
LogicalJoin<?, ?> join = (LogicalJoin<?, ?>) plan;
for (Expression conjunct : join.getHashJoinConjuncts()) {
- if (conjunct.anyMatch(e -> ((Expression)
e).getDataType().isVarBinaryType())) {
+ if (comparesVarBinary(conjunct)) {
throw new AnalysisException(
"varbinary type could not in join equal
conditions: " + conjunct.toSql());
}
}
for (Expression conjunct : join.getMarkJoinConjuncts()) {
- if (conjunct.anyMatch(e -> ((Expression)
e).getDataType().isVarBinaryType())) {
+ if (comparesVarBinary(conjunct)) {
throw new AnalysisException(
"varbinary type could not in join equal
conditions: " + conjunct.toSql());
}
}
}
}
+ // 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()
Review 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.
--
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]
