github-actions[bot] commented on code in PR #65421:
URL: https://github.com/apache/doris/pull/65421#discussion_r3567639674
##########
be/src/exprs/function/function_bitmap_min_or_max.h:
##########
@@ -68,20 +68,24 @@ class FunctionBitmapSingle : public IFunction {
private:
void execute_straight(const ColumnBitmap* date_column, ColumnInt64*
result_column,
NullMap& result_null_map, size_t input_rows_count)
const {
+ const auto& data = date_column->get_data();
+ auto& result_data = result_column->get_data();
+ result_data.resize(input_rows_count);
+
for (size_t i = 0; i < input_rows_count; i++) {
if (result_null_map[i]) {
- result_column->insert_default();
+ result_data[i] = 0;
continue;
}
- BitmapValue value = date_column->get_element(i);
- if (!value.cardinality()) {
+ const BitmapValue& value = data[i];
+ if (value.empty()) {
Review Comment:
This should keep the old cardinality check rather than using `empty()`.
`empty()` only means `_type == EMPTY`, but deserialization can produce a
zero-cardinality non-EMPTY bitmap: for example TypeCode `SET` with count 0
leaves `_type = SET`, and empty roaring payloads leave `_type = BITMAP`. The
old `!value.cardinality()` returned NULL for those values; now this branch
falls through to `minimum()`/`maximum()`, which returns 0, so
`bitmap_min`/`bitmap_max` can report 0 for an empty persisted/base64 bitmap.
Please restore the cardinality-based emptiness check here.
##########
be/src/core/value/bitmap_value.h:
##########
@@ -989,32 +1401,16 @@ class BitmapValue {
break;
}
_is_shared = other._is_shared;
+ other._type = EMPTY;
+ other._is_shared = false;
return *this;
}
// Construct a bitmap from given elements.
- explicit BitmapValue(const std::vector<uint64_t>& bits) :
_is_shared(false) {
- if (bits.size() == 0) {
- _type = EMPTY;
- return;
- }
-
- if (bits.size() == 1) {
- _type = SINGLE;
- _sv = bits[0];
- return;
- }
-
- if (!config::enable_set_in_bitmap_value || bits.size() >
SET_TYPE_THRESHOLD) {
- _type = BITMAP;
- _prepare_bitmap_for_write();
- _bitmap->addMany(bits.size(), &bits[0]);
- } else {
- _type = SET;
- for (auto v : bits) {
- _set.insert(v);
- }
- }
+ template <typename T, typename = std::enable_if_t<std::is_integral_v<T>>>
Review Comment:
This templated replacement drops the initializer-list path that existing
code still uses. With only `template <typename T> BitmapValue(const
std::vector<T>&)`, C++ cannot deduce `T` from a braced list, so calls like
`BitmapValue({1, 2, 200})` in the tests, and existing BE tests under
`be/test/exprs/function` / `be/test/exec/operator`, become compile errors.
Please keep a non-templated `std::vector<uint64_t>` overload or add an
initializer-list constructor that forwards into `add_many`, while preserving
the new typed-vector fast path.
--
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]
