From 1ac4fa4b5766f360033e3df7d439b58a85605e59 Mon Sep 17 00:00:00 2001 From: happenlee Date: Sat, 19 Sep 2026 14:04:01 +0800 Subject: [PATCH 1/2] [fix](be) Ignore negative integers in bitmap aggregates ### What problem does this PR solve? Problem Summary: `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, companion English/Chinese development-version documentation updates clarify 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. --- .../aggregate/aggregate_function_bitmap.h | 22 +++-- .../aggregate/aggregate_function_bitmap_agg.h | 29 +++++- be/test/exprs/aggregate/agg_bitmap_test.cpp | 93 +++++++++++++++++++ 3 files changed, 131 insertions(+), 13 deletions(-) diff --git a/be/src/exprs/aggregate/aggregate_function_bitmap.h b/be/src/exprs/aggregate/aggregate_function_bitmap.h index 296433df8b65bc..e60ab9769d4ea2 100644 --- a/be/src/exprs/aggregate/aggregate_function_bitmap.h +++ b/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(*columns[0]); - if (!nullable_column.is_null_at(row_num)) { - const auto& column = assert_cast( - 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(*data_column) + .get_data()[row_num]; + if constexpr (!std::is_same_v) { + // Match bitmap_agg and to_bitmap before conversion to uint64_t. + if (value < 0) { + return; } - } else { - const auto& column = - assert_cast(*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 212698f7dbd60c..1111b7a301e8a1 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 struct AggregateFunctionBitmapAggData { BitmapValue value; - void add(const typename PrimitiveTypeTraits::CppType& value_) { value.add(value_); } + void add(const typename PrimitiveTypeTraits::CppType& value_) { + if (value_ >= 0) { + value.add(value_); + } + } void reset() { value.reset(); } @@ -96,9 +101,10 @@ class AggregateFunctionBitmapAgg final assert_cast(*columns[0]); const auto& column = assert_cast( nullable_column.get_nested_column()); - std::vector::CppType> values; - for (int i = 0; i < batch_size; ++i) { - if (!nullable_column.is_null_at(i)) { + PaddedPODArray::CppType> values; + values.reserve(batch_size); + 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 +112,20 @@ class AggregateFunctionBitmapAgg final } else { const auto& column = assert_cast(*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::CppType> values; + values.reserve(batch_size); + 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 f0b4addcabde8d..e2ebb5b211bcb7 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 +#include #include #include #include @@ -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(); } +class BitmapIntegerAggregateTest : public AggregateFunctiontest { +protected: + template + void check_input(const std::vector::CppType>& values, + const BitmapValue& expected, bool nullable) { + DataTypePtr type = std::make_shared::DataType>(); + if (nullable) { + type = make_nullable(type); + } + auto input = type->create_column(); + for (auto value : values) { + input->insert(Field::create_field(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()); + execute(block, ColumnHelper::create_column_with_name({expected})); + create_agg("bitmap_union_int", false, {type}, std::make_shared()); + execute(block, ColumnHelper::create_column_with_name( + {static_cast(expected.cardinality())})); + } + + template + void check_negative_inputs() { + using CppType = typename PrimitiveTypeTraits::CppType; + constexpr auto min = std::numeric_limits::min(); + constexpr auto max = std::numeric_limits::max(); + for (bool nullable : {false, true}) { + SCOPED_TRACE(nullable); + check_input({-1}, BitmapValue(), nullable); + check_input({min, -2, -1, -1}, BitmapValue(), nullable); + BitmapValue expected(std::vector {0, 1, static_cast(max)}); + check_input({min, -2, -1, 0, 1, 1, max}, expected, nullable); + check_input({0, 1, 1, max}, expected, nullable); + + // Cross the small-set threshold and exercise repeated negative values. + std::vector 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(dense_values, dense_expected, nullable); + } + // One NULL and no non-NULL values. + check_input({}, BitmapValue(), true); + } +}; + +TEST_F(BitmapIntegerAggregateTest, IgnoreNegativeTinyInt) { + check_negative_inputs(); +} + +TEST_F(BitmapIntegerAggregateTest, IgnoreNegativeSmallInt) { + check_negative_inputs(); +} + +TEST_F(BitmapIntegerAggregateTest, IgnoreNegativeInt) { + check_negative_inputs(); +} + +TEST_F(BitmapIntegerAggregateTest, IgnoreNegativeBigInt) { + check_negative_inputs(); +} + +TEST_F(BitmapIntegerAggregateTest, PreserveUnsignedBitmapValues) { + const BitmapValue bitmap(std::vector {0, std::numeric_limits::max()}); + for (bool nullable : {false, true}) { + DataTypePtr type = std::make_shared(); + if (nullable) { + type = make_nullable(type); + } + auto input = type->create_column(); + input->insert(Field::create_field(bitmap)); + input->insert(Field::create_field(bitmap)); + if (nullable) { + input->insert_default(); + } + create_agg("bitmap_union_count", false, {type}, std::make_shared()); + execute(Block({{std::move(input), type, "input"}}), + ColumnHelper::create_column_with_name({2})); + } +} + } // namespace doris From fcb26e2e742a1d77cbb61e856b98cbe9de2f5460 Mon Sep 17 00:00:00 2001 From: happenlee Date: Sat, 19 Sep 2026 17:15:39 +0800 Subject: [PATCH 2/2] [fix](be) Allocate bitmap aggregate scratch storage lazily ### What problem does this PR solve? Related PR: #68219 Problem Summary: Both filtered bitmap_agg batch paths reserved scratch storage for the complete input batch before filtering. All-NULL or all-negative input therefore allocated and freed unused storage for each batch. Remove the two eager reserve calls so the existing tracked PaddedPODArray allocates only when a nonnegative, non-NULL value survives. The non-nullable all-nonnegative add_many fast path is unchanged. This eliminates the one scratch allocation per fully filtered batch; no end-to-end performance improvement has been measured. ### Release note None ### Check List (For Author) - Test: Manual validation: clang-format 16, check-format, BE header hygiene, git diff --check, and syntax-only compilation of the bitmap_agg registration translation unit passed. Existing tests cover all-NULL, all-negative, mixed-sign and nonnegative batches for all four signed integer widths. Unit tests were not rerun: the prior local ASAN UT build is blocked by a missing aws/core/auth/GeneralHTTPCredentialsProvider.h dependency. clang-tidy was attempted and remains blocked by pre-existing diagnostics, including an unmatched NOLINTEND in core/types.h. - Behavior changed: No; only temporary allocation timing changes. - Does this need documentation: No --- be/src/exprs/aggregate/aggregate_function_bitmap_agg.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/be/src/exprs/aggregate/aggregate_function_bitmap_agg.h b/be/src/exprs/aggregate/aggregate_function_bitmap_agg.h index 1111b7a301e8a1..8d6af09106664e 100644 --- a/be/src/exprs/aggregate/aggregate_function_bitmap_agg.h +++ b/be/src/exprs/aggregate/aggregate_function_bitmap_agg.h @@ -102,7 +102,6 @@ class AggregateFunctionBitmapAgg final const auto& column = assert_cast( nullable_column.get_nested_column()); PaddedPODArray::CppType> values; - values.reserve(batch_size); 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]); @@ -118,7 +117,6 @@ class AggregateFunctionBitmapAgg final this->data(place).value.add_many(data, batch_size); } else { PaddedPODArray::CppType> values; - values.reserve(batch_size); for (size_t i = 0; i < batch_size; ++i) { if (data[i] >= 0) { values.push_back(data[i]);