Gabriel39 commented on code in PR #68532:
URL: https://github.com/apache/doris/pull/68532#discussion_r4118588806
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/CdcStreamTableValuedFunction.java:
##########
@@ -227,6 +227,8 @@ public List<Column> getTableColumns() throws
AnalysisException {
throw new AnalysisException("Table does not exist: " + table);
}
List<Column> columns = new
ArrayList<>(jdbcClient.getColumnsFromJdbc(database, table));
+ // Use the CDC transport schema, not the external JDBC catalog's
timestamp mapping.
+ columns.forEach(column ->
column.setType(StreamingJobUtils.getCdcTimestampType(column.getType())));
Review Comment:
Fixed in 59ff2383a9c. The CDC TVF and StreamingJobUtils.getColumns() now
share getCdcTransportType(): binary columns become STRING, including nested
array elements, while existing timestamp conversion and binary primary-key
handling are preserved. This matches the existing Base64 JSON carrier rather
than labeling its encoded text as raw binary.
Added TVF and destination-schema regression tests; both failed before the
fix. The TVF test also checks that byte[] {1, 2} is serialized as "AQI=". All
59 targeted FE tests and FE Checkstyle pass after the fix. A live CDC
end-to-end run was not performed locally.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -2252,9 +2282,20 @@ public static Optional<Pair<BigDecimal, BigDecimal>>
getDataTypeMinMaxValue(Data
return Optional.empty();
}
- /**
- * BE only support numeric, character, date-time and array
- */
Review Comment:
Fixed in 59ff2383a9c. processInPredicate() now rejects binary arguments
before the equal-type shortcut and common-type coercion, including binary
leaves in arrays, structs and maps. This also covers NOT IN and mixed
binary/text arguments without enabling BE binary comparisons.
Added SQL-analysis tests for equal declared types, differing lengths,
binary/text arguments in both directions, NOT IN, and nested arrays/structs
under both coercion modes. The negative cases failed before the fix and pass
afterward. Ordinary IN/NOT IN and binary CASE/IF/COALESCE tests remain green.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/types/DataType.java:
##########
@@ -386,8 +386,14 @@ public static DataType
convertPrimitiveFromStrings(List<String> types) {
dataType = VariantType.INSTANCE;
break;
case "varbinary":
- // NOTICE, Maybe. not supported create table, and varbinary do
not have len now
- dataType = VarBinaryType.INSTANCE;
+ // Keep declared byte limits in table schemas and nested
binary leaves.
+ if (types.size() == 1 || (types.size() == 2 &&
types.get(1).equals("*"))) {
+ dataType = VarBinaryType.INSTANCE;
Review Comment:
Confirmed the FE/BE mismatch for direct VARBINARY arguments, including
min/max/any_value and min_by/max_by. These aggregate signatures and BE
factories are unchanged from this PR's base (388e935b06af); optional VARBINARY
mapping could already reach these paths before this change. Making catalog
mapping mandatory increases exposure, but does not introduce aggregate support.
Per the agreed scope, this PR preserves the existing restriction on binary
computation and does not expand aggregate support or fix pre-existing aggregate
legality gaps. Deferring this finding rather than claiming the operations are
supported or fixed. The changes addressing the other comments remain limited to
the CDC transport regression and the comparison paths affected by common-type
coercion.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -772,6 +778,21 @@ private static Expression castInputs(Expression expr,
List<Optional<DataType>> c
* process BoundFunction type coercion
*/
public static Expression processBoundFunction(BoundFunction boundFunction)
{
+ if
(UNSUPPORTED_VARBINARY_COLLECTIONS.contains(boundFunction.getName())) {
+ for (Expression argument : boundFunction.children()) {
+ DataType type = argument.getDataType();
+ if (!boundFunction.getName().equals("collect_set")) {
+ while (type instanceof ArrayType) {
+ type = ((ArrayType) type).getItemType();
+ }
Review Comment:
Partially confirmed. histogram/linear_histogram, topn_array,
group_array_union/intersect with binary elements, and map_agg/map_agg_v2 with a
VARBINARY key have existing FE/BE legality gaps. Their aggregate
implementations are unchanged from the base; these pre-existing aggregate gaps
are deferred under the agreed scope.
Two details need narrowing:
- A direct FE coercion probe for topn_weighted(binary_column, 1, 2) produces
topn_weighted(CAST(binary_column AS TEXT), 1, 2), returning ARRAY<TEXT>. It
does not forward TYPE_VARBINARY through an AnyDataType signature to the BE
creator as stated here.
- The missing dispatch in the map aggregate creators is on the key argument.
A binary value with a supported key is not rejected by that creator, so the
finding should not treat every binary argument as the same failure.
This disposition does not claim new binary aggregate functionality or
end-to-end support for these operations.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -2252,9 +2282,20 @@ public static Optional<Pair<BigDecimal, BigDecimal>>
getDataTypeMinMaxValue(Data
return Optional.empty();
}
- /**
- * BE only support numeric, character, date-time and array
Review Comment:
Fixed in 59ff2383a9c. least/greatest/nullif now reject binary arguments
during FE analysis, before common-type casts can conceal them; the check also
recognizes nested binary leaves. The common binary type remains available to
value-selection expressions such as CASE/IF/COALESCE.
Added SQL-analysis coverage for homogeneous and mixed binary/text arguments,
nested binary NULLIF, both coercion modes, and ordinary scalar calls. These
negative tests failed before the fix and pass afterward. All 59 targeted FE
tests and FE Checkstyle pass.
--
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]
