github-actions[bot] commented on code in PR #68219:
URL: https://github.com/apache/doris/pull/68219#discussion_r4052456846
##########
be/src/exprs/aggregate/aggregate_function_bitmap_agg.h:
##########
@@ -27,6 +27,7 @@
#include "core/assert_cast.h"
#include "core/data_type/data_type_bitmap.h"
+#include "core/pod_array.h"
#include "core/value/bitmap_value.h"
Review Comment:
[P1] Handle persisted states when changing these aggregate semantics
Both functions can be persisted through `_state`/`_merge` in user
`AGG_STATE` columns and synchronous MVs, while asynchronous MTMVs can retain
their finalized `BITMAP`/`BIGINT` output. Before this change, evaluating `-1`
stores `UINT64_MAX` (or count 1); this guard only affects new raw input, while
merge paths and already materialized outputs keep the old result. Because the
diff adds no function-semantic version or MV invalidation, after all BEs
upgrade an old object can still disagree with fresh evaluation
(`bitmap_union_int_merge(old_state) = 1` versus `bitmap_union_int(-1) = 0`).
Please version or reject old aggregate states and invalidate, rebuild, or
migrate affected synchronous/asynchronous MVs and `AGG_STATE` tables, with
upgrade regressions for each persisted form.
##########
be/src/exprs/aggregate/aggregate_function_bitmap.h:
##########
@@ -351,19 +351,25 @@ class AggregateFunctionBitmapCount final
void add(AggregateDataPtr __restrict place, const IColumn** columns,
ssize_t row_num,
Arena&) const override {
+ const IColumn* data_column = columns[0];
if constexpr (arg_is_nullable) {
const auto& nullable_column =
assert_cast<const ColumnNullable&,
TypeCheckOnRelease::DISABLE>(*columns[0]);
- if (!nullable_column.is_null_at(row_num)) {
- const auto& column = assert_cast<const ColVecType&,
TypeCheckOnRelease::DISABLE>(
- nullable_column.get_nested_column());
- this->data(place).add(column.get_data()[row_num]);
+ if (nullable_column.is_null_at(row_num)) {
+ return;
+ }
+ data_column = &nullable_column.get_nested_column();
+ }
+ const auto& value =
+ assert_cast<const ColVecType&,
TypeCheckOnRelease::DISABLE>(*data_column)
+ .get_data()[row_num];
+ if constexpr (!std::is_same_v<ColVecType, ColumnBitmap>) {
Review Comment:
[P1] Version the new aggregate semantics for rolling upgrades
During a supported rolling upgrade, both the base and head BEs accept
`be_exec_version` 15, but an old leaf turns `-1` into `UINT64_MAX` while a new
leaf drops it. Both serialize the same unversioned `BitmapValue`, and the final
merge preserves an old leaf's member, so `bitmap_union_int` can return 0 or 1
(and `bitmap_agg` empty or `{UINT64_MAX}`) depending on fragment placement.
Please version this semantic change and select the old implementation for the
old execution version, or reject/gate execution until every participant uses
the new semantics; add a mixed-version partial-state compatibility test.
--
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]
