Copilot commented on code in PR #50769:
URL: https://github.com/apache/doris/pull/50769#discussion_r2086041656
##########
be/src/vec/functions/function_cast.h:
##########
@@ -2168,8 +2169,20 @@ class FunctionCast final : public IFunctionBase {
/// 'from_type' and 'to_type' are nested types in case of Nullable.
/// 'requested_result_is_nullable' is true if CAST to Nullable type is
requested.
- WrapperType prepare_impl(FunctionContext* context, const DataTypePtr&
from_type,
- const DataTypePtr& to_type, bool
requested_result_is_nullable) const {
+ WrapperType prepare_impl(FunctionContext* context, const DataTypePtr&
origin_from_type,
+ const DataTypePtr& origin_to_type,
+ bool requested_result_is_nullable) const {
+ auto to_type = origin_to_type;
+ auto from_type = origin_from_type;
+ if (to_type->get_primitive_type() == PrimitiveType::TYPE_AGG_STATE) {
Review Comment:
[nitpick] Consider renaming the local variable 'to_type' (and similarly
'from_type') to avoid shadowing the original parameter names, which may improve
readability.
##########
fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java:
##########
@@ -872,6 +872,8 @@ public static boolean canCastTo(Type sourceType, Type
targetType) {
return StructType.canCastTo((StructType) sourceType, (StructType)
targetType);
} else if (sourceType.isAggStateType() &&
targetType.getPrimitiveType().isCharFamily()) {
return true;
Review Comment:
Add documentation to clarify why casting from a char family type to an
AggStateType is permitted, ensuring future readers understand the design
decision.
```suggestion
return true;
// Allow casting from a char family type to an AggStateType.
// This is permitted because AggStateType can represent states that
are compatible
// with char family types, enabling specific use cases where such
conversions are needed.
```
##########
be/src/vec/exprs/vexpr.cpp:
##########
@@ -441,22 +442,24 @@ Status VExpr::check_expr_output_type(const
VExprContextSPtrs& ctxs,
"output type size not match expr size {} , expected output
size {} ", ctxs.size(),
name_and_types.size());
}
- auto check_type_can_be_converted = [](DataTypePtr& from, DataTypePtr& to)
-> bool {
- if (to->equals(*from)) {
- return true;
- }
+ auto check_type_can_be_converted = [](DataTypePtr from, DataTypePtr to) ->
bool {
Review Comment:
[nitpick] Consider adding an early equality check (as in the original
implementation) before modifying the types to optimize for the common case and
clarify intent.
```suggestion
auto check_type_can_be_converted = [](DataTypePtr from, DataTypePtr to)
-> bool {
// Early equality check to optimize for the common case
if (to->equals(*from)) {
return true;
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/check/CheckCast.java:
##########
@@ -72,6 +73,9 @@ private static boolean check(DataType originalType, DataType
targetType) {
if (originalType instanceof CharacterType && !(targetType instanceof
PrimitiveType)) {
return true;
}
Review Comment:
Consider adding a comment that explains the rationale behind allowing a cast
from AggStateType to CharacterType for clarity.
```suggestion
}
// Allow casting from AggStateType to CharacterType because
AggStateType can be
// represented as a string-like type for purposes such as
serialization or display.
```
--
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]
