github-actions[bot] commented on code in PR #67822:
URL: https://github.com/apache/doris/pull/67822#discussion_r4002164428
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/ArrayCompact.java:
##########
@@ -61,10 +61,17 @@ private ArrayCompact(ScalarFunctionParams functionParams) {
*/
@Override
public void checkLegalityBeforeTypeCoercion() {
- DataType argType = ((ArrayType)
getArgument(0).getDataType()).getItemType();
- if (argType.isMapType() || argType.isStructType()) {
+ DataType dataType = getArgument(0).getDataType();
+ if (dataType.isNullType()) {
+ return;
+ }
+ if (!dataType.isArrayType()) {
+ throw new AnalysisException("array_compact requires an ARRAY
argument, but got " + dataType.toSql());
+ }
+ DataType itemType = ((ArrayType) dataType).getItemType();
+ if (itemType.isMapType() || itemType.isStructType()) {
Review Comment:
[P2] Reject element columns that cannot perform compact's comparison
This still accepts metric elements such as `array(to_bitmap(1),
to_bitmap(1))`, because `BitmapType` is neither map nor struct.
`FunctionArrayCompact::_execute` unconditionally calls `compare_at` for
adjacent elements, but `ColumnComplexType` defines that override only under
`BE_TEST`; production falls back to `IColumn::compare_at` and throws
`NOT_IMPLEMENTED_ERROR`. Please reject BITMAP/HLL/QUANTILE_STATE (and any other
non-comparable element family) during analysis or implement their intended
equality semantics, with negative regression coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/types/DataType.java:
##########
@@ -781,6 +781,12 @@ public boolean canBeCalculatedInArray() {
return isNumericType() || isBooleanType() || isStringLikeType() ||
isNullType();
}
+ /** Whether this type has a BE column implementation supported by
hash-based array set operations. */
+ public boolean canBeUsedInArraySetOperation() {
+ return isNumericType() || isBooleanType() || isStringLikeType() ||
isVarBinaryType()
Review Comment:
[P1] Match this allowlist to the BE dispatch it describes
`isVarBinaryType()` materializes as `ColumnVarbinary`, and `isTimeType()`
materializes as the distinct `ColumnTimeV2`; neither class is present in
`ALL_COLUMNS_SIMPLE`, which is the only pack tried by `ArrayMapImpl`. Calls
with two identical VARBINARY or TIME arguments therefore pass this new FE check
and signature coercion, then fall into the BE unsupported-type path (which
currently also triggers the null dereference noted separately). Please either
remove these families until support exists or add the correct BE
specializations—VARBINARY in particular needs safe ownership/deep-copy
behavior—and cover both families in union/intersect regression tests.
##########
be/src/exprs/function/array/function_array_map.h:
##########
@@ -171,7 +171,8 @@ struct ArrayMapImpl {
res_ptr = assemble_column_array(dst);
return Status::OK();
}
- return Status::RuntimeError("Unexpected columns");
+ return Status::InvalidArgument("Unsupported array element type {}",
+ datas[0].nested_type->get_name());
Review Comment:
[P1] Avoid dereferencing an unset type on this error path
`FunctionArrayNary::execute_impl` value-initializes each
`ColumnArrayExecutionData` and only calls `extract_column_array_info`; that
helper fills the physical array/offset/null-map fields but never assigns
`nested_type`. Any element column that misses `ALL_COLUMNS_SIMPLE` (for example
a BITMAP plan sent by an old FE during a rolling upgrade, or one of the
mismatched new-FE types) therefore reaches this line with `nested_type ==
nullptr`, so formatting the intended `InvalidArgument` can fault the BE
instead. Please format this from the populated `nested_col`, or explicitly
populate/validate the logical type, and add a BE-level unsupported-column test
that does not rely on new-FE rejection.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/types/DataType.java:
##########
@@ -781,6 +781,12 @@ public boolean canBeCalculatedInArray() {
return isNumericType() || isBooleanType() || isStringLikeType() ||
isNullType();
}
+ /** Whether this type has a BE column implementation supported by
hash-based array set operations. */
+ public boolean canBeUsedInArraySetOperation() {
Review Comment:
[P2] Apply the corrected support check to the parallel `array_except` path
`ArrayExcept` has the same indexed `ARRAY<AnyDataType>` shape and its BE
implementation dispatches `ArraySetImpl` over the same `ALL_COLUMNS_SIMPLE`
set, but it has no legality check using this predicate. As a result,
`array_except(array(to_bitmap(1)), array(to_bitmap(1)))` still passes FE
analysis and surfaces the backend's generic `Unexpected columns` failure—the
parallel instance of the root cause this helper addresses. Please apply the
corrected physical-support validation to `ArrayExcept` and add a matching
negative regression case.
--
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]
