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..8d6af09106664e 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,9 @@ 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; + 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 @@ 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; + 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