This is an automated email from the ASF dual-hosted git repository.
HappenLee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new d93137b9f13 [fix](be) Ignore negative integers in bitmap aggregates
(#68219)
d93137b9f13 is described below
commit d93137b9f134b1526dc6ec6aa13dc1709e627180
Author: HappenLee <[email protected]>
AuthorDate: Mon Sep 21 12:00:07 2026 +0800
[fix](be) Ignore negative integers in bitmap aggregates (#68219)
`bitmap_agg` and `bitmap_union_int` currently pass signed negative
integers into `BitmapValue`, which converts them to unsigned bitmap
members. For example, `bitmap_agg(-1)` can contain
`18446744073709551615` and `bitmap_union_int(-1)` returns 1, while
`to_bitmap(-1)` produces an empty bitmap.
Filter negative integers before insertion in both aggregates. Cover
scalar insertion, nullable and non-nullable `bitmap_agg` batches, and
the integer-only branch of the shared bitmap-count aggregate. Preserve
the allocation-free batch path for all-nonnegative input and use a
tracked temporary buffer for filtered batches. Generic bitmap operations
and the full uint64 domain of `bitmap_union_count` are unchanged.
With this change:
- `bitmap_count(bitmap_agg(-1))` and `bitmap_union_int(-1)` return 0.
- Input `[-1, 0, 1, 1, NULL]` contributes exactly the members `{0, 1}`.
- All-negative/all-NULL groups produce an empty bitmap or a count of 0.
### Release note
`bitmap_agg` and `bitmap_union_int` now ignore negative integers,
consistently with `to_bitmap`. In particular, `bitmap_union_int` counts
distinct nonnegative integers rather than all distinct integers. Queries
containing negative inputs can return smaller counts after upgrading.
### Check List (For Author)
- Test: add BE unit tests covering TINYINT/SMALLINT/INT/BIGINT,
nullable/non-nullable input, negative minima, positive maxima, zero,
duplicates, all-negative/all-NULL groups, dense batches, reset,
streaming serialization and column merge paths. Also protect UINT64_MAX
in `bitmap_union_count`.
- Passed: clang-format 16 check, BE build-hygiene checks and `git diff
--check`.
- Passed: compiler syntax checks of both aggregate
registration/instantiation translation units and the modified test
translation unit, using the generated ASAN-UT compilation settings.
- Attempted: `run-be-ut.sh -j48 --run
--filter='BitmapIntegerAggregateTest.*:AggBitmapTest.*'`. The build
stops in unchanged `common/cpp/aws_common.cpp` because the local
thirdparty installation lacks
`aws/core/auth/GeneralHTTPCredentialsProvider.h`; tests have not
executed locally.
- Attempted: repository clang-tidy script. Analysis is blocked by an
existing unmatched NOLINTEND in `core/types.h` and existing warnings in
the touched files outside the new logic. No clean clang-tidy run is
claimed.
- Behavior changed: Yes, negative integers no longer contribute bitmap
members or counts. NULL handling, result types, function signatures and
intermediate/storage formats remain unchanged.
- Does this need documentation: Yes:
https://github.com/apache/doris-website/pull/4154 updates the
English/Chinese development-version documentation and clarifies the
changed `bitmap_union_int` semantics.
### Review notes
The existing aggregate test helper exercises the batch, streaming and
merge paths without adding a new framework. Grouped additions use the
same scalar `add` entry point. No new locks, configuration, persistence
changes or FE/BE fields are introduced. Filtering is applied only to
integer inputs; already-materialized bitmaps are preserved. During a
rolling upgrade, old BEs can still contribute old partial states, so
uniform negative-input semantics require all executing BEs to contain
this fix.
---
be/src/exprs/aggregate/aggregate_function_bitmap.h | 22 +++--
.../aggregate/aggregate_function_bitmap_agg.h | 27 +++++--
be/test/exprs/aggregate/agg_bitmap_test.cpp | 93 ++++++++++++++++++++++
3 files changed, 129 insertions(+), 13 deletions(-)
diff --git a/be/src/exprs/aggregate/aggregate_function_bitmap.h
b/be/src/exprs/aggregate/aggregate_function_bitmap.h
index 296433df8b6..e60ab9769d4 100644
--- a/be/src/exprs/aggregate/aggregate_function_bitmap.h
+++ b/be/src/exprs/aggregate/aggregate_function_bitmap.h
@@ -351,19 +351,25 @@ public:
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>) {
+ // 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,
diff --git a/be/src/exprs/aggregate/aggregate_function_bitmap_agg.h
b/be/src/exprs/aggregate/aggregate_function_bitmap_agg.h
index 212698f7dbd..8d6af091066 100644
--- a/be/src/exprs/aggregate/aggregate_function_bitmap_agg.h
+++ b/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"
#include "exprs/aggregate/aggregate_function.h"
@@ -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(); }
@@ -96,9 +101,9 @@ public:
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) {
+ if (!nullable_column.is_null_at(i) && column.get_data()[i] >=
0) {
values.push_back(column.get_data()[i]);
}
}
@@ -106,7 +111,19 @@ public:
} 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());
+ }
}
}
diff --git a/be/test/exprs/aggregate/agg_bitmap_test.cpp
b/be/test/exprs/aggregate/agg_bitmap_test.cpp
index f0b4addcabd..e2ebb5b211b 100644
--- a/be/test/exprs/aggregate/agg_bitmap_test.cpp
+++ b/be/test/exprs/aggregate/agg_bitmap_test.cpp
@@ -16,6 +16,7 @@
// under the License.
#include <cstddef>
+#include <limits>
#include <memory>
#include <string>
#include <vector>
@@ -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"
@@ -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
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
