github-actions[bot] commented on code in PR #65421:
URL: https://github.com/apache/doris/pull/65421#discussion_r3576144588


##########
be/src/core/value/bitmap_value.h:
##########
@@ -1980,51 +2475,34 @@ class BitmapValue {
                 throw Exception(ErrorCode::INTERNAL_ERROR,
                                 "bitmap value with incorrect set count, count: 
{}", count);
             }
-            _set.reserve(count);
-            for (uint8_t i = 0; i != count; ++i, src += sizeof(uint64_t)) {
-                _set.insert(decode_fixed64_le(reinterpret_cast<const 
uint8_t*>(src)));
-            }
-            if (_set.size() != count) {
+            if (!_set.read(src, count)) {
                 throw Exception(ErrorCode::INTERNAL_ERROR,
-                                "bitmap value with incorrect set count, count: 
{}, set size: {}",
-                                count, _set.size());
+                                "bitmap value with duplicated set values, 
count: {}", count);
             }
+            src += sizeof(uint64_t) * count;
 
             if (!config::enable_set_in_bitmap_value) {
-                _prepare_bitmap_for_write();
-                for (auto v : _set) {
-                    _bitmap->add(v);
-                }
-                _type = BITMAP;
-                _set.clear();
+                _convert_set_to_bitmap();

Review Comment:
   This path can still reuse an old inactive `_bitmap` when `deserialize()` is 
called on an existing `BitmapValue`. `DataTypeBitMap::deserialize()` resizes 
the existing `ColumnBitmap` vector and then calls 
`data[i].deserialize(data_ptr)`, so an element that previously held a BITMAP 
can be overwritten by a SET payload without dropping the old roaring map. If 
SET storage is disabled here, or if the freshly deserialized SET is later 
promoted, `_convert_set_to_bitmap()` only calls `_prepare_bitmap_for_write()` 
and then `addMany(_set...)`, so it appends the new SET values into the stale 
bitmap instead of replacing it. For example, a reused value that had BITMAP 
`{0..40}` and then deserializes SET `{100,101}` can become `{0..40,100,101}` 
after conversion. Please reset/clear inactive representation state at the start 
of `deserialize()` or make the deserialize SET-to-BITMAP conversion build from 
an empty roaring map, and add a reuse test for BITMAP-to-SET deserialization 
followed by SET prom
 otion.
   



-- 
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]

Reply via email to