github-actions[bot] commented on code in PR #68532:
URL: https://github.com/apache/doris/pull/68532#discussion_r4118372974
##########
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:
[P1] `CdcStreamTableValuedFunction` now returns the JDBC client's VARBINARY
columns directly, but this TVF sends `format=json` rows from
`fetchRecordStream`. The CDC client serializes binary `byte[]` values as base64
JSON strings, and the BE VARBINARY JSON serde inserts that string's bytes
verbatim (it does not base64/hex-decode JSON), so e.g. `0x01 0x02` arrives as
ASCII `AQI=` and is corrupted. Please apply the CDC-specific
VARBINARY-to-STRING normalization used by `StreamingJobUtils.getColumns()`
(including nested binary types if supported) before returning the TVF schema,
or add a matching binary decoder to the JSON transport.
##########
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:
[P1] `findCommonBinaryType()` now permits VARBINARY to become the common
type for `IN`/`NOT IN`, and `processInPredicate()` treats that type as
supported through `supportCompare()`. The resulting plan reaches the BE `in`
function, whose `open()` explicitly returns `NotSupported("VARBINARY IN/NOT IN
is not supported")`. Please reject VARBINARY in `processInPredicate()`
(including the equal-type fast path) or implement the BE binary set path before
exposing this 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:
[P1] The new VARBINARY guard covers only selected array functions and
`collect_set`, but generic aggregates such as `histogram`/`linear_histogram`,
`map_agg`/`map_agg_v2`, `topn_array`/`topn_weighted`, and
`group_array_union`/`group_array_intersect` still accept VARBINARY (or
Array<VARBINARY>) through AnyDataType signatures. Their BE creators omit
TYPE_VARBINARY/VARBINARY leaf dispatch and return null or unsupported when the
plan opens. Please reject VARBINARY (including nested array leaves) for each
unsupported aggregate, or add the corresponding BE dispatch before making
external binary columns available.
##########
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:
[P1] The new VARBINARY common-type path also admits scalar callers whose BE
kernels still reject binary. `least()`/`greatest()` now emit VARBINARY
signatures, but `least_greast.cpp` has no TYPE_VARBINARY dispatch and throws
`not support type ColumnVarbinary`; `nullif()` similarly reaches its hidden
`eq` call, whose predicate factory throws `VARBINARY predicates are not
supported`. Please reject VARBINARY in these common-type signatures (or add
byte-wise least/greatest and equality kernels) before exposing this coercion.
##########
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:
[P1] This newly exposes external binary columns as Nereids VARBINARY, but
`Min`/`Max` and `AnyValue` accept that type while the shared BE aggregate
factory's `TYPE_VARBINARY` branch throws `NOT_IMPLEMENTED_ERROR` (the same
factory is used by `any_value`). Thus `min()`/`max()`/`any_value()` over a
binary column can analyze successfully and fail only when the plan opens.
Please reject VARBINARY in these aggregate legality/signature paths (and audit
min_by/max_by) or add a supported byte-order aggregate before enabling this
mapping.
--
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]
