github-actions[bot] commented on code in PR #67473:
URL: https://github.com/apache/doris/pull/67473#discussion_r3925703747
##########
be/src/core/column/column_complex.h:
##########
@@ -63,11 +63,10 @@ class ColumnComplexType final : public COWHelper<IColumn,
ColumnComplexType<T>>
void insert_binary_data(const char* pos, size_t length) {
insert_default();
- value_type* pvalue = &get_element(size() - 1);
if (!length) {
Review Comment:
[P1] Please avoid silently turning every zero-length aggregate payload into
an empty value unless the legacy encoding contract is made explicit. Current
writers serialize empty BITMAP/HLL/QUANTILE_STATE values as 1/1/5 bytes and
skip nullable rows. HLL has a documented release-0.10 dirty-data recovery rule,
but no equivalent producer/version contract was found for BITMAP or
QUANTILE_STATE. As written, a truncated cell for those types becomes an empty
query result and can hide/rewrite corruption; reject it at the decoder boundary
or scope, document, and test compatibility recovery per type.
##########
be/test/core/column/column_complex_test.cpp:
##########
@@ -0,0 +1,145 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "core/column/column_complex.h"
+
+#include <gtest/gtest.h>
+
+#include <cstdint>
+#include <cstring>
+#include <vector>
+
+#include "core/value/bitmap_value.h"
+#include "core/value/hll.h"
+#include "core/value/quantile_state.h"
+#include "core/string_ref.h"
Review Comment:
[P2] Please apply the repository clang-format 16 include order. The
mandatory format check currently fails on this added file; `core/string_ref.h`
needs to sort before the `core/value/...` headers.
##########
be/test/core/column/column_complex_test.cpp:
##########
@@ -0,0 +1,145 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "core/column/column_complex.h"
+
+#include <gtest/gtest.h>
+
+#include <cstdint>
+#include <cstring>
+#include <vector>
+
+#include "core/value/bitmap_value.h"
+#include "core/value/hll.h"
+#include "core/value/quantile_state.h"
+#include "core/string_ref.h"
+
+namespace doris {
+
+static std::vector<char> make_garbage_buffer(size_t n) {
+ std::vector<char> buf(n);
+ for (size_t i = 0; i < n; ++i) {
+ buf[i] = static_cast<char>(0xAB);
+ }
+ return buf;
+}
+
+TEST(ColumnComplexTest, InsertBinaryDataZeroLengthBitmap) {
+ auto col = ColumnBitmap::create();
+ auto garbage = make_garbage_buffer(64);
+
+ col->insert_binary_data(garbage.data(), 0);
+
+ ASSERT_EQ(col->size(), 1);
+ EXPECT_EQ(col->get_element(0).cardinality(), 0);
Review Comment:
[P2] Please make this assert a canonical empty `BitmapValue`. On the base
implementation, the zero-length branch copies an invalid `_type` from the
garbage buffer, but `BitmapValue::cardinality()` falls through and still
returns 0, so this test can pass without the fix. Assert `get_type_code() ==
BitmapTypeCode::EMPTY` (or mutate/read the value afterward) so the BITMAP
specialization itself fails on the old code.
--
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]
