Skip to content

[fix](be) Ignore negative integers in bitmap aggregates #68219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
HappenLee merged 2 commits into from
Sep 21, 2026
+129 −13
Merged

[fix](be) Ignore negative integers in bitmap aggregates #68219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ac4fa4
[fix](be) Ignore negative integers in bitmap aggregates
HappenLee Sep 19, 2026
fcb26e2
[fix](be) Allocate bitmap aggregate scratch storage lazily
HappenLee Sep 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions be/src/exprs/aggregate/aggregate_function_bitmap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>) {

@github-actions github-actions Bot Sep 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

// Match bitmap_agg and to_bitmap before conversion to uint64_t.
if (value < 0) {
return;
}
} else {
const auto& column =
assert_cast<const ColVecType&, TypeCheckOnRelease::DISABLE>(*columns[0]);
this->data(place).add(column.get_data()[row_num]);
}
this->data(place).add(value);
}

void add_many(AggregateDataPtr __restrict place, const IColumn** columns,
Expand Down
27 changes: 22 additions & 5 deletions be/src/exprs/aggregate/aggregate_function_bitmap_agg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

@github-actions github-actions Bot Sep 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@HappenLee HappenLee Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AGG_STATE is an experimental feature, and backward compatibility for persisted AGG_STATE data is not a requirement for this PR. Please treat that compatibility requirement as out of scope when re-evaluating this change.

#include "exprs/aggregate/aggregate_function.h"

Expand All @@ -43,7 +44,11 @@ template <PrimitiveType T>
struct AggregateFunctionBitmapAggData {
BitmapValue value;

void add(const typename PrimitiveTypeTraits<T>::CppType& value_) { value.add(value_); }
void add(const typename PrimitiveTypeTraits<T>::CppType& value_) {
if (value_ >= 0) {
value.add(value_);
}
}

void reset() { value.reset(); }

Expand Down Expand Up @@ -96,17 +101,29 @@ class AggregateFunctionBitmapAgg final
assert_cast<const ColumnNullable&, TypeCheckOnRelease::DISABLE>(*columns[0]);
const auto& column = assert_cast<const ColVecType&, TypeCheckOnRelease::DISABLE>(
nullable_column.get_nested_column());
std::vector<typename PrimitiveTypeTraits<T>::CppType> values;
for (int i = 0; i < batch_size; ++i) {
if (!nullable_column.is_null_at(i)) {
PaddedPODArray<typename PrimitiveTypeTraits<T>::CppType> values;
for (size_t i = 0; i < batch_size; ++i) {

@github-actions github-actions Bot Sep 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Avoid reserving a full batch before filtering

Both filtered branches reserve batch_size before knowing whether any row survives. For an all-NULL or all-negative block, values stays empty and add_many(..., 0) immediately returns, so a global bitmap_agg now allocates and frees a block-sized scratch buffer for every input block; sparse nullable blocks also allocate for all rows rather than survivors. The previous nullable std::vector grew lazily and allocated nothing for all-NULL blocks. Please defer or size the tracked allocation from the surviving rows while retaining the direct all-nonnegative fast path.

if (!nullable_column.is_null_at(i) && column.get_data()[i] >= 0) {
values.push_back(column.get_data()[i]);
}
}
this->data(place).value.add_many(values.data(), values.size());
} else {
const auto& column =
assert_cast<const ColVecType&, TypeCheckOnRelease::DISABLE>(*columns[0]);
this->data(place).value.add_many(column.get_data().data(), column.size());
const auto* data = column.get_data().data();
// Keep the allocation-free batch path for nonnegative input.
if (std::all_of(data, data + batch_size, [](auto value) { return value >= 0; })) {
this->data(place).value.add_many(data, batch_size);
} else {
PaddedPODArray<typename PrimitiveTypeTraits<T>::CppType> values;
for (size_t i = 0; i < batch_size; ++i) {
if (data[i] >= 0) {
values.push_back(data[i]);
}
}
this->data(place).value.add_many(values.data(), values.size());
}
}
}

Expand Down
93 changes: 93 additions & 0 deletions be/test/exprs/aggregate/agg_bitmap_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// under the License.

#include <cstddef>
#include <limits>
#include <memory>
#include <string>
#include <vector>
Expand All @@ -24,11 +25,13 @@
#include "core/column/column_complex.h"
#include "core/data_type/data_type_bitmap.h"
#include "core/data_type/data_type_decimal.h"
#include "core/data_type/data_type_nullable.h"
#include "core/data_type/data_type_number.h"
#include "core/data_type/data_type_string.h"
#include "core/field.h"
#include "core/types.h"
#include "core/value/bitmap_value.h"
#include "exprs/aggregate/agg_function_test.h"
#include "exprs/aggregate/aggregate_function.h"
#include "exprs/aggregate/aggregate_function_simple_factory.h"
#include "gtest/gtest_pred_impl.h"
Expand Down Expand Up @@ -299,4 +302,94 @@ TEST(AggBitmapTest, bitmap_union_int_test) {
validate_bitmap_union_int_test<TYPE_BIGINT>();
}

class BitmapIntegerAggregateTest : public AggregateFunctiontest {
protected:
template <PrimitiveType T>
void check_input(const std::vector<typename PrimitiveTypeTraits<T>::CppType>& values,
const BitmapValue& expected, bool nullable) {
DataTypePtr type = std::make_shared<typename PrimitiveTypeTraits<T>::DataType>();
if (nullable) {
type = make_nullable(type);
}
auto input = type->create_column();
for (auto value : values) {
input->insert(Field::create_field<T>(value));
}
if (nullable) {
input->insert_default();
}
Block block({{std::move(input), type, "input"}});

// The shared helper covers single-place batches, streaming aggregation,
// reset, serialization and all column-based merge paths.
create_agg("bitmap_agg", false, {type}, std::make_shared<DataTypeBitMap>());
execute(block, ColumnHelper::create_column_with_name<DataTypeBitMap>({expected}));
create_agg("bitmap_union_int", false, {type}, std::make_shared<DataTypeInt64>());
execute(block, ColumnHelper::create_column_with_name<DataTypeInt64>(
{static_cast<Int64>(expected.cardinality())}));
}

template <PrimitiveType T>
void check_negative_inputs() {
using CppType = typename PrimitiveTypeTraits<T>::CppType;
constexpr auto min = std::numeric_limits<CppType>::min();
constexpr auto max = std::numeric_limits<CppType>::max();
for (bool nullable : {false, true}) {
SCOPED_TRACE(nullable);
check_input<T>({-1}, BitmapValue(), nullable);
check_input<T>({min, -2, -1, -1}, BitmapValue(), nullable);
BitmapValue expected(std::vector<uint64_t> {0, 1, static_cast<uint64_t>(max)});
check_input<T>({min, -2, -1, 0, 1, 1, max}, expected, nullable);
check_input<T>({0, 1, 1, max}, expected, nullable);

// Cross the small-set threshold and exercise repeated negative values.
std::vector<CppType> dense_values;
BitmapValue dense_expected;
for (int i = 0; i < 100; ++i) {
dense_values.push_back(-1);
dense_values.push_back(i);
dense_expected.add(i);
}
check_input<T>(dense_values, dense_expected, nullable);
}
// One NULL and no non-NULL values.
check_input<T>({}, BitmapValue(), true);
}
};

TEST_F(BitmapIntegerAggregateTest, IgnoreNegativeTinyInt) {
check_negative_inputs<TYPE_TINYINT>();
}

TEST_F(BitmapIntegerAggregateTest, IgnoreNegativeSmallInt) {
check_negative_inputs<TYPE_SMALLINT>();
}

TEST_F(BitmapIntegerAggregateTest, IgnoreNegativeInt) {
check_negative_inputs<TYPE_INT>();
}

TEST_F(BitmapIntegerAggregateTest, IgnoreNegativeBigInt) {
check_negative_inputs<TYPE_BIGINT>();
}

TEST_F(BitmapIntegerAggregateTest, PreserveUnsignedBitmapValues) {
const BitmapValue bitmap(std::vector<uint64_t> {0, std::numeric_limits<uint64_t>::max()});
for (bool nullable : {false, true}) {
DataTypePtr type = std::make_shared<DataTypeBitMap>();
if (nullable) {
type = make_nullable(type);
}
auto input = type->create_column();
input->insert(Field::create_field<TYPE_BITMAP>(bitmap));
input->insert(Field::create_field<TYPE_BITMAP>(bitmap));
if (nullable) {
input->insert_default();
}
create_agg("bitmap_union_count", false, {type}, std::make_shared<DataTypeInt64>());
execute(Block({{std::move(input), type, "input"}}),
ColumnHelper::create_column_with_name<DataTypeInt64>({2}));
}
}

} // namespace doris
Loading