github-actions[bot] commented on code in PR #65031:
URL: https://github.com/apache/doris/pull/65031#discussion_r3518057181
##########
be/src/exprs/aggregate/aggregate_function_min_max.h:
##########
@@ -378,9 +385,10 @@ struct SingleValueDataString {
void insert_result_into(IColumn& to) const {
if (has()) {
- assert_cast<ColumnString&>(to).insert_data(get_data(), size);
+ assert_cast<ColumnString&,
TypeCheckOnRelease::DISABLE>(to).insert_data(get_data(),
+
size);
Review Comment:
The new guard for this aggregate is still only the default logical type
check. For string `min`/`max`/`any`, `DataTypeString::check_column()` accepts
`ColumnString64`, but `SingleValueDataString` still reads inputs via
`assert_cast<const ColumnString&, TypeCheckOnRelease::DISABLE>` and writes
results through this `ColumnString&` cast. That means the new type-check layer
can approve a `ColumnString64` block and the hot path then has undefined/wrong
physical access. Please add explicit `ColumnString` input/result checks for the
string single-value aggregate, or support `ColumnString64`, and cover it with a
BE test.
##########
be/src/exprs/aggregate/aggregate_function_count_by_enum.h:
##########
@@ -229,7 +229,8 @@ class AggregateFunctionCountByEnum final
void insert_result_into(ConstAggregateDataPtr __restrict place, IColumn&
to) const override {
const std::string json_arr = this->data(place).get();
- assert_cast<ColumnString&>(to).insert_data(json_arr.c_str(),
json_arr.length());
+ assert_cast<ColumnString&,
TypeCheckOnRelease::DISABLE>(to).insert_data(json_arr.c_str(),
+
json_arr.length());
Review Comment:
This still relies on the default logical string checker even though the
implementation requires the physical `ColumnString` layout.
`DataTypeString::check_column()` accepts `ColumnString64`, so the new
`AggFnEvaluator` guard can pass a `ColumnString64` argument/result here and
then this release-disabled cast, and the input casts in `add()`, will treat it
as `ColumnString`. Please add explicit physical checks for `count_by_enum`
including nullable nested input, or make the implementation handle
`ColumnString64`, with a BE test for both input and result.
--
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]
