github-actions[bot] commented on code in PR #24498: URL: https://github.com/apache/doris/pull/24498#discussion_r1336776965
########## be/src/olap/rowset/segment_v2/binary_array_page.h: ########## @@ -0,0 +1,221 @@ +// 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. + +// page encoding fixed length strings +// +// The page consists of: +// Strings: +// raw strings that were written +// Trailer: +// num_elems (32-bit fixed) +// length (32-bit fixed) + +#pragma once + +#include <gen_cpp/PaloBrokerService_types.h> + +#include <cstddef> +#include <cstdint> + +#include "olap/rowset/segment_v2/options.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "util/coding.h" +#include "util/faststring.h" +namespace doris { +namespace segment_v2 { +class BinaryArrayPageBuilder : public PageBuilder { +public: + BinaryArrayPageBuilder(const PageBuilderOptions& options) : _size_estimate(0) { + _page_size = options.is_dict_page ? options.dict_page_size : options.data_page_size; + reset(); + } + + void reset() override { + _buffer.clear(); + _buffer.reserve(_page_size); + _size_estimate = sizeof(uint32_t) * 2; + _finished = false; + _elem_length = 0; + _num_elem = 0; + } + + Status add(const uint8_t* vals, size_t* count) override { + DCHECK(!_finished); + DCHECK_GT(*count, 0); + auto src = reinterpret_cast<const Slice*>(vals); + size_t num_added = 0; + if (_elem_length == 0) { + _elem_length = src->size; + } + DCHECK_EQ(_elem_length, src->size); + for (size_t i = 0; i < *count; i++) { + if (is_page_full()) { + break; + } + const Slice& s = src[i]; + _buffer.append(s.data, s.size); + _size_estimate += s.size; + num_added++; + } + *count = num_added; + _num_elem += num_added; + + return Status::OK(); + } + + OwnedSlice finish() override { + DCHECK(!_finished); + _finished = true; + put_fixed32_le(&_buffer, _num_elem); + put_fixed32_le(&_buffer, _elem_length); + return _buffer.build(); + } + + bool is_page_full() override { return _size_estimate > _page_size; } + + size_t count() const override { return _num_elem; } + + uint64_t size() const override { return _size_estimate; } Review Comment: warning: function 'size' should be marked [[nodiscard]] [modernize-use-nodiscard] ```suggestion [[nodiscard]] uint64_t size() const override { return _size_estimate; } ``` ########## be/src/olap/rowset/segment_v2/binary_array_page.h: ########## @@ -0,0 +1,221 @@ +// 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. + +// page encoding fixed length strings +// +// The page consists of: +// Strings: +// raw strings that were written +// Trailer: +// num_elems (32-bit fixed) +// length (32-bit fixed) + +#pragma once + +#include <gen_cpp/PaloBrokerService_types.h> + +#include <cstddef> +#include <cstdint> + +#include "olap/rowset/segment_v2/options.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "util/coding.h" +#include "util/faststring.h" +namespace doris { +namespace segment_v2 { +class BinaryArrayPageBuilder : public PageBuilder { +public: + BinaryArrayPageBuilder(const PageBuilderOptions& options) : _size_estimate(0) { + _page_size = options.is_dict_page ? options.dict_page_size : options.data_page_size; + reset(); + } + + void reset() override { + _buffer.clear(); + _buffer.reserve(_page_size); + _size_estimate = sizeof(uint32_t) * 2; + _finished = false; + _elem_length = 0; + _num_elem = 0; + } + + Status add(const uint8_t* vals, size_t* count) override { + DCHECK(!_finished); + DCHECK_GT(*count, 0); + auto src = reinterpret_cast<const Slice*>(vals); + size_t num_added = 0; + if (_elem_length == 0) { + _elem_length = src->size; + } + DCHECK_EQ(_elem_length, src->size); + for (size_t i = 0; i < *count; i++) { + if (is_page_full()) { + break; + } + const Slice& s = src[i]; + _buffer.append(s.data, s.size); + _size_estimate += s.size; + num_added++; + } + *count = num_added; + _num_elem += num_added; + + return Status::OK(); + } + + OwnedSlice finish() override { + DCHECK(!_finished); + _finished = true; + put_fixed32_le(&_buffer, _num_elem); + put_fixed32_le(&_buffer, _elem_length); + return _buffer.build(); + } + + bool is_page_full() override { return _size_estimate > _page_size; } + + size_t count() const override { return _num_elem; } + + uint64_t size() const override { return _size_estimate; } + + Slice get(std::size_t idx) const override { Review Comment: warning: function 'get' should be marked [[nodiscard]] [modernize-use-nodiscard] ```suggestion [[nodiscard]] Slice get(std::size_t idx) const override { ``` ########## be/src/olap/rowset/segment_v2/binary_array_page.h: ########## @@ -0,0 +1,221 @@ +// 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. + +// page encoding fixed length strings +// +// The page consists of: +// Strings: +// raw strings that were written +// Trailer: +// num_elems (32-bit fixed) +// length (32-bit fixed) + +#pragma once + +#include <gen_cpp/PaloBrokerService_types.h> + +#include <cstddef> +#include <cstdint> + +#include "olap/rowset/segment_v2/options.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "util/coding.h" +#include "util/faststring.h" +namespace doris { +namespace segment_v2 { +class BinaryArrayPageBuilder : public PageBuilder { +public: + BinaryArrayPageBuilder(const PageBuilderOptions& options) : _size_estimate(0) { + _page_size = options.is_dict_page ? options.dict_page_size : options.data_page_size; + reset(); + } + + void reset() override { + _buffer.clear(); + _buffer.reserve(_page_size); + _size_estimate = sizeof(uint32_t) * 2; + _finished = false; + _elem_length = 0; + _num_elem = 0; + } + + Status add(const uint8_t* vals, size_t* count) override { + DCHECK(!_finished); + DCHECK_GT(*count, 0); + auto src = reinterpret_cast<const Slice*>(vals); + size_t num_added = 0; + if (_elem_length == 0) { + _elem_length = src->size; + } + DCHECK_EQ(_elem_length, src->size); + for (size_t i = 0; i < *count; i++) { + if (is_page_full()) { + break; + } + const Slice& s = src[i]; + _buffer.append(s.data, s.size); + _size_estimate += s.size; + num_added++; + } + *count = num_added; + _num_elem += num_added; + + return Status::OK(); + } + + OwnedSlice finish() override { + DCHECK(!_finished); + _finished = true; + put_fixed32_le(&_buffer, _num_elem); + put_fixed32_le(&_buffer, _elem_length); + return _buffer.build(); + } + + bool is_page_full() override { return _size_estimate > _page_size; } + + size_t count() const override { return _num_elem; } + + uint64_t size() const override { return _size_estimate; } + + Slice get(std::size_t idx) const override { + DCHECK(!_finished); + DCHECK_LT(idx, _num_elem); + return Slice(&_buffer[idx * _elem_length], _elem_length); + } + +private: + faststring _buffer; + size_t _size_estimate; + bool _finished; + size_t _page_size; + + uint32_t _num_elem = 0; + uint32_t _elem_length; +}; + +class BinaryArrayPageDecoder : public PageDecoder { +public: + BinaryArrayPageDecoder(Slice data) + : _data(data), _parsed(false), _num_elems(0), _elem_length(0), _cur_idx(0) {} + + Status init() override { + CHECK(!_parsed); + + if (_data.size < sizeof(uint32_t) * 2) { + return Status::Corruption( + "file corruption: not enough bytes for trailer in BinaryArrayPageDecoder ." + "invalid data size:{}, trailer size:{}", + _data.size, sizeof(uint32_t) * 2); + } + + // Decode trailer + _num_elems = + decode_fixed32_le((const uint8_t*)&_data[_data.get_size() - sizeof(uint32_t) * 2]); + _elem_length = + decode_fixed32_le((const uint8_t*)&_data[_data.get_size() - sizeof(uint32_t)]); + _parsed = true; + + return Status::OK(); + } + + Status seek_to_position_in_page(size_t pos) override { + DCHECK_LE(pos, _num_elems); + _cur_idx = pos; + return Status::OK(); + } + + Status next_batch(size_t* n, vectorized::MutableColumnPtr& dst) override { + DCHECK(_parsed); + if (PREDICT_FALSE(*n == 0 || _cur_idx >= _num_elems)) { + *n = 0; + return Status::OK(); + } + const size_t max_fetch = std::min(*n, static_cast<size_t>(_num_elems - _cur_idx)); Review Comment: warning: variable 'max_fetch' is not initialized [cppcoreguidelines-init-variables] ```suggestion const size_t max_fetch = 0 = std::min(*n, static_cast<size_t>(_num_elems - _cur_idx)); ``` ########## be/src/olap/rowset/segment_v2/binary_array_page.h: ########## @@ -0,0 +1,221 @@ +// 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. + +// page encoding fixed length strings +// +// The page consists of: +// Strings: +// raw strings that were written +// Trailer: +// num_elems (32-bit fixed) +// length (32-bit fixed) + +#pragma once + +#include <gen_cpp/PaloBrokerService_types.h> + +#include <cstddef> +#include <cstdint> + +#include "olap/rowset/segment_v2/options.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "util/coding.h" +#include "util/faststring.h" +namespace doris { +namespace segment_v2 { +class BinaryArrayPageBuilder : public PageBuilder { +public: + BinaryArrayPageBuilder(const PageBuilderOptions& options) : _size_estimate(0) { + _page_size = options.is_dict_page ? options.dict_page_size : options.data_page_size; + reset(); + } + + void reset() override { + _buffer.clear(); + _buffer.reserve(_page_size); + _size_estimate = sizeof(uint32_t) * 2; + _finished = false; + _elem_length = 0; + _num_elem = 0; + } + + Status add(const uint8_t* vals, size_t* count) override { + DCHECK(!_finished); + DCHECK_GT(*count, 0); + auto src = reinterpret_cast<const Slice*>(vals); + size_t num_added = 0; + if (_elem_length == 0) { + _elem_length = src->size; + } + DCHECK_EQ(_elem_length, src->size); + for (size_t i = 0; i < *count; i++) { + if (is_page_full()) { + break; + } + const Slice& s = src[i]; + _buffer.append(s.data, s.size); + _size_estimate += s.size; + num_added++; + } + *count = num_added; + _num_elem += num_added; + + return Status::OK(); + } + + OwnedSlice finish() override { + DCHECK(!_finished); + _finished = true; + put_fixed32_le(&_buffer, _num_elem); + put_fixed32_le(&_buffer, _elem_length); + return _buffer.build(); + } + + bool is_page_full() override { return _size_estimate > _page_size; } + + size_t count() const override { return _num_elem; } Review Comment: warning: function 'count' should be marked [[nodiscard]] [modernize-use-nodiscard] ```suggestion [[nodiscard]] size_t count() const override { return _num_elem; } ``` ########## be/test/olap/rowset/segment_v2/binary_array_page_test.cpp: ########## @@ -0,0 +1,105 @@ +// 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 "olap/rowset/segment_v2/binary_array_page.h" + +#include <gtest/gtest.h> + +#include <cstdint> +#include <iostream> +#include <string> +#include <vector> + +#include "common/logging.h" +#include "olap/olap_common.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "olap/types.h" +#include "util/coding.h" + +namespace doris { +namespace segment_v2 { + +class BinaryArrayPageTest : public testing::Test { +public: + BinaryArrayPageTest() {} + + virtual ~BinaryArrayPageTest() {} + + void TestSmallPage() { + std::vector<Slice> slices; + slices.emplace_back("1"); + slices.emplace_back("2"); + slices.emplace_back("3"); + slices.emplace_back("4"); + + PageBuilderOptions options; + options.data_page_size = 10; + BinaryArrayPageBuilder page_builder(options); + { + size_t count = slices.size(); Review Comment: warning: variable 'count' is not initialized [cppcoreguidelines-init-variables] ```suggestion size_t count = 0 = slices.size(); ``` ########## be/test/olap/rowset/segment_v2/binary_array_page_test.cpp: ########## @@ -0,0 +1,105 @@ +// 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 "olap/rowset/segment_v2/binary_array_page.h" + +#include <gtest/gtest.h> + +#include <cstdint> +#include <iostream> +#include <string> +#include <vector> + +#include "common/logging.h" +#include "olap/olap_common.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "olap/types.h" +#include "util/coding.h" + +namespace doris { +namespace segment_v2 { + +class BinaryArrayPageTest : public testing::Test { +public: + BinaryArrayPageTest() {} + + virtual ~BinaryArrayPageTest() {} + + void TestSmallPage() { + std::vector<Slice> slices; + slices.emplace_back("1"); + slices.emplace_back("2"); + slices.emplace_back("3"); + slices.emplace_back("4"); + + PageBuilderOptions options; + options.data_page_size = 10; + BinaryArrayPageBuilder page_builder(options); + { + size_t count = slices.size(); + + Slice* ptr = &slices[0]; Review Comment: warning: variable 'ptr' is not initialized [cppcoreguidelines-init-variables] ```suggestion Slice* ptr = nullptr = &slices[0]; ``` ########## be/src/olap/rowset/segment_v2/binary_array_page.h: ########## @@ -0,0 +1,221 @@ +// 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. + +// page encoding fixed length strings +// +// The page consists of: +// Strings: +// raw strings that were written +// Trailer: +// num_elems (32-bit fixed) +// length (32-bit fixed) + +#pragma once + +#include <gen_cpp/PaloBrokerService_types.h> + +#include <cstddef> +#include <cstdint> + +#include "olap/rowset/segment_v2/options.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "util/coding.h" +#include "util/faststring.h" +namespace doris { +namespace segment_v2 { +class BinaryArrayPageBuilder : public PageBuilder { +public: + BinaryArrayPageBuilder(const PageBuilderOptions& options) : _size_estimate(0) { + _page_size = options.is_dict_page ? options.dict_page_size : options.data_page_size; + reset(); + } + + void reset() override { + _buffer.clear(); + _buffer.reserve(_page_size); + _size_estimate = sizeof(uint32_t) * 2; + _finished = false; + _elem_length = 0; + _num_elem = 0; + } + + Status add(const uint8_t* vals, size_t* count) override { + DCHECK(!_finished); + DCHECK_GT(*count, 0); + auto src = reinterpret_cast<const Slice*>(vals); + size_t num_added = 0; + if (_elem_length == 0) { + _elem_length = src->size; + } + DCHECK_EQ(_elem_length, src->size); + for (size_t i = 0; i < *count; i++) { + if (is_page_full()) { + break; + } + const Slice& s = src[i]; + _buffer.append(s.data, s.size); + _size_estimate += s.size; + num_added++; + } + *count = num_added; + _num_elem += num_added; + + return Status::OK(); + } + + OwnedSlice finish() override { + DCHECK(!_finished); + _finished = true; + put_fixed32_le(&_buffer, _num_elem); + put_fixed32_le(&_buffer, _elem_length); + return _buffer.build(); + } + + bool is_page_full() override { return _size_estimate > _page_size; } + + size_t count() const override { return _num_elem; } + + uint64_t size() const override { return _size_estimate; } + + Slice get(std::size_t idx) const override { + DCHECK(!_finished); + DCHECK_LT(idx, _num_elem); + return Slice(&_buffer[idx * _elem_length], _elem_length); + } + +private: + faststring _buffer; + size_t _size_estimate; + bool _finished; + size_t _page_size; + + uint32_t _num_elem = 0; + uint32_t _elem_length; +}; + +class BinaryArrayPageDecoder : public PageDecoder { +public: + BinaryArrayPageDecoder(Slice data) + : _data(data), _parsed(false), _num_elems(0), _elem_length(0), _cur_idx(0) {} + + Status init() override { + CHECK(!_parsed); + + if (_data.size < sizeof(uint32_t) * 2) { + return Status::Corruption( + "file corruption: not enough bytes for trailer in BinaryArrayPageDecoder ." + "invalid data size:{}, trailer size:{}", + _data.size, sizeof(uint32_t) * 2); + } + + // Decode trailer + _num_elems = + decode_fixed32_le((const uint8_t*)&_data[_data.get_size() - sizeof(uint32_t) * 2]); + _elem_length = + decode_fixed32_le((const uint8_t*)&_data[_data.get_size() - sizeof(uint32_t)]); + _parsed = true; + + return Status::OK(); + } + + Status seek_to_position_in_page(size_t pos) override { + DCHECK_LE(pos, _num_elems); + _cur_idx = pos; + return Status::OK(); + } + + Status next_batch(size_t* n, vectorized::MutableColumnPtr& dst) override { + DCHECK(_parsed); + if (PREDICT_FALSE(*n == 0 || _cur_idx >= _num_elems)) { + *n = 0; + return Status::OK(); + } + const size_t max_fetch = std::min(*n, static_cast<size_t>(_num_elems - _cur_idx)); + + uint32_t offsets[max_fetch + 1]; + for (size_t i = 0; i < max_fetch + 1; ++i) { + offsets[i] = (i + _cur_idx) * _elem_length; + } + dst->insert_many_continuous_binary_data(_data.data, offsets, max_fetch); + + *n = max_fetch; + _cur_idx += max_fetch; + return Status::OK(); + } + + size_t count() const override { Review Comment: warning: function 'count' should be marked [[nodiscard]] [modernize-use-nodiscard] ```suggestion [[nodiscard]] size_t count() const override { ``` ########## be/src/olap/rowset/segment_v2/binary_dict_page.h: ########## @@ -79,13 +79,16 @@ class BinaryDictPageBuilder : public PageBuilder { Status get_last_value(void* value) const override; + bool is_char_type() const { return _is_char_type; } Review Comment: warning: function 'is_char_type' should be marked [[nodiscard]] [modernize-use-nodiscard] ```suggestion [[nodiscard]] bool is_char_type() const { return _is_char_type; } ``` ########## be/test/olap/rowset/segment_v2/binary_array_page_test.cpp: ########## @@ -0,0 +1,105 @@ +// 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 "olap/rowset/segment_v2/binary_array_page.h" + +#include <gtest/gtest.h> + +#include <cstdint> +#include <iostream> +#include <string> +#include <vector> + +#include "common/logging.h" +#include "olap/olap_common.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "olap/types.h" +#include "util/coding.h" + +namespace doris { +namespace segment_v2 { + +class BinaryArrayPageTest : public testing::Test { +public: + BinaryArrayPageTest() {} Review Comment: warning: use '= default' to define a trivial default constructor [modernize-use-equals-default] ```suggestion BinaryArrayPageTest() = default; ``` ########## be/src/olap/rowset/segment_v2/binary_array_page.h: ########## @@ -0,0 +1,221 @@ +// 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. + +// page encoding fixed length strings +// +// The page consists of: +// Strings: +// raw strings that were written +// Trailer: +// num_elems (32-bit fixed) +// length (32-bit fixed) + +#pragma once + +#include <gen_cpp/PaloBrokerService_types.h> + +#include <cstddef> +#include <cstdint> + +#include "olap/rowset/segment_v2/options.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "util/coding.h" +#include "util/faststring.h" +namespace doris { +namespace segment_v2 { +class BinaryArrayPageBuilder : public PageBuilder { +public: + BinaryArrayPageBuilder(const PageBuilderOptions& options) : _size_estimate(0) { + _page_size = options.is_dict_page ? options.dict_page_size : options.data_page_size; + reset(); + } + + void reset() override { + _buffer.clear(); + _buffer.reserve(_page_size); + _size_estimate = sizeof(uint32_t) * 2; + _finished = false; + _elem_length = 0; + _num_elem = 0; + } + + Status add(const uint8_t* vals, size_t* count) override { + DCHECK(!_finished); + DCHECK_GT(*count, 0); + auto src = reinterpret_cast<const Slice*>(vals); + size_t num_added = 0; + if (_elem_length == 0) { + _elem_length = src->size; + } + DCHECK_EQ(_elem_length, src->size); + for (size_t i = 0; i < *count; i++) { + if (is_page_full()) { + break; + } + const Slice& s = src[i]; + _buffer.append(s.data, s.size); + _size_estimate += s.size; + num_added++; + } + *count = num_added; + _num_elem += num_added; + + return Status::OK(); + } + + OwnedSlice finish() override { + DCHECK(!_finished); + _finished = true; + put_fixed32_le(&_buffer, _num_elem); + put_fixed32_le(&_buffer, _elem_length); + return _buffer.build(); + } + + bool is_page_full() override { return _size_estimate > _page_size; } + + size_t count() const override { return _num_elem; } + + uint64_t size() const override { return _size_estimate; } + + Slice get(std::size_t idx) const override { + DCHECK(!_finished); + DCHECK_LT(idx, _num_elem); + return Slice(&_buffer[idx * _elem_length], _elem_length); + } + +private: + faststring _buffer; + size_t _size_estimate; + bool _finished; + size_t _page_size; + + uint32_t _num_elem = 0; + uint32_t _elem_length; +}; + +class BinaryArrayPageDecoder : public PageDecoder { +public: + BinaryArrayPageDecoder(Slice data) + : _data(data), _parsed(false), _num_elems(0), _elem_length(0), _cur_idx(0) {} + + Status init() override { + CHECK(!_parsed); + + if (_data.size < sizeof(uint32_t) * 2) { + return Status::Corruption( + "file corruption: not enough bytes for trailer in BinaryArrayPageDecoder ." + "invalid data size:{}, trailer size:{}", + _data.size, sizeof(uint32_t) * 2); + } + + // Decode trailer + _num_elems = + decode_fixed32_le((const uint8_t*)&_data[_data.get_size() - sizeof(uint32_t) * 2]); + _elem_length = + decode_fixed32_le((const uint8_t*)&_data[_data.get_size() - sizeof(uint32_t)]); + _parsed = true; + + return Status::OK(); + } + + Status seek_to_position_in_page(size_t pos) override { + DCHECK_LE(pos, _num_elems); + _cur_idx = pos; + return Status::OK(); + } + + Status next_batch(size_t* n, vectorized::MutableColumnPtr& dst) override { + DCHECK(_parsed); + if (PREDICT_FALSE(*n == 0 || _cur_idx >= _num_elems)) { + *n = 0; + return Status::OK(); + } + const size_t max_fetch = std::min(*n, static_cast<size_t>(_num_elems - _cur_idx)); + + uint32_t offsets[max_fetch + 1]; + for (size_t i = 0; i < max_fetch + 1; ++i) { + offsets[i] = (i + _cur_idx) * _elem_length; + } + dst->insert_many_continuous_binary_data(_data.data, offsets, max_fetch); + + *n = max_fetch; + _cur_idx += max_fetch; + return Status::OK(); + } + + size_t count() const override { + DCHECK(_parsed); + return _num_elems; + } + + size_t current_index() const override { Review Comment: warning: function 'current_index' should be marked [[nodiscard]] [modernize-use-nodiscard] ```suggestion [[nodiscard]] size_t current_index() const override { ``` ########## be/src/olap/rowset/segment_v2/binary_array_page.h: ########## @@ -0,0 +1,221 @@ +// 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. + +// page encoding fixed length strings +// +// The page consists of: +// Strings: +// raw strings that were written +// Trailer: +// num_elems (32-bit fixed) +// length (32-bit fixed) + +#pragma once + +#include <gen_cpp/PaloBrokerService_types.h> Review Comment: warning: 'gen_cpp/PaloBrokerService_types.h' file not found [clang-diagnostic-error] ```cpp #include <gen_cpp/PaloBrokerService_types.h> ^ ``` ########## be/src/olap/rowset/segment_v2/page_builder.h: ########## @@ -80,12 +80,18 @@ class PageBuilder { // Return the first value in this page. // This method could only be called between finish() and reset(). // Status::Error<ENTRY_NOT_FOUND> if no values have been added. - virtual Status get_first_value(void* value) const = 0; + virtual Status get_first_value(void* value) const { + return Status::NotSupported("get_first_value not implemented"); + } // Return the last value in this page. // This method could only be called between finish() and reset(). // Status::Error<ENTRY_NOT_FOUND> if no values have been added. - virtual Status get_last_value(void* value) const = 0; + virtual Status get_last_value(void* value) const { + return Status::NotSupported("get_last_value not implemented"); + } + + virtual Slice get(std::size_t idx) const { throw std::runtime_error("not implemented"); } Review Comment: warning: function 'get' should be marked [[nodiscard]] [modernize-use-nodiscard] ```suggestion [[nodiscard]] virtual Slice get(std::size_t idx) const { throw std::runtime_error("not implemented"); } ``` ########## be/src/olap/rowset/segment_v2/binary_plain_page.h: ########## @@ -146,7 +147,7 @@ class BinaryPlainPageBuilder : public PageBuilder { return Slice(&_buffer[_offsets[idx]], value_size); } - inline Slice get(std::size_t idx) const { return (*this)[idx]; } + Slice get(std::size_t idx) const override { return (*this)[idx]; } Review Comment: warning: function 'get' should be marked [[nodiscard]] [modernize-use-nodiscard] ```suggestion [[nodiscard]] Slice get(std::size_t idx) const override { return (*this)[idx]; } ``` ########## be/test/olap/rowset/segment_v2/binary_array_page_test.cpp: ########## @@ -0,0 +1,105 @@ +// 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 "olap/rowset/segment_v2/binary_array_page.h" + +#include <gtest/gtest.h> + +#include <cstdint> +#include <iostream> +#include <string> +#include <vector> + +#include "common/logging.h" +#include "olap/olap_common.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "olap/types.h" +#include "util/coding.h" + +namespace doris { +namespace segment_v2 { + +class BinaryArrayPageTest : public testing::Test { +public: + BinaryArrayPageTest() {} + + virtual ~BinaryArrayPageTest() {} + + void TestSmallPage() { + std::vector<Slice> slices; + slices.emplace_back("1"); + slices.emplace_back("2"); + slices.emplace_back("3"); + slices.emplace_back("4"); + + PageBuilderOptions options; Review Comment: warning: variable 'options' is not initialized [cppcoreguidelines-init-variables] ```suggestion PageBuilderOptions options = 0; ``` ########## be/test/olap/rowset/segment_v2/binary_array_page_test.cpp: ########## @@ -0,0 +1,105 @@ +// 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 "olap/rowset/segment_v2/binary_array_page.h" + +#include <gtest/gtest.h> + +#include <cstdint> +#include <iostream> +#include <string> +#include <vector> + +#include "common/logging.h" +#include "olap/olap_common.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "olap/types.h" +#include "util/coding.h" + +namespace doris { +namespace segment_v2 { + +class BinaryArrayPageTest : public testing::Test { +public: + BinaryArrayPageTest() {} + + virtual ~BinaryArrayPageTest() {} Review Comment: warning: use '= default' to define a trivial destructor [modernize-use-equals-default] ```suggestion virtual ~BinaryArrayPageTest() = default; ``` ########## be/test/olap/rowset/segment_v2/binary_array_page_test.cpp: ########## @@ -0,0 +1,105 @@ +// 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 "olap/rowset/segment_v2/binary_array_page.h" + +#include <gtest/gtest.h> + +#include <cstdint> +#include <iostream> +#include <string> +#include <vector> + +#include "common/logging.h" +#include "olap/olap_common.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "olap/types.h" +#include "util/coding.h" + +namespace doris { +namespace segment_v2 { + +class BinaryArrayPageTest : public testing::Test { +public: + BinaryArrayPageTest() {} + + virtual ~BinaryArrayPageTest() {} + + void TestSmallPage() { + std::vector<Slice> slices; + slices.emplace_back("1"); + slices.emplace_back("2"); + slices.emplace_back("3"); + slices.emplace_back("4"); + + PageBuilderOptions options; + options.data_page_size = 10; + BinaryArrayPageBuilder page_builder(options); + { + size_t count = slices.size(); + + Slice* ptr = &slices[0]; + Status ret = page_builder.add(reinterpret_cast<const uint8_t*>(ptr), &count); + EXPECT_TRUE(ret.ok()); + EXPECT_EQ(3, count); + + auto page_actual = page_builder.finish(); + std::string page_expected; Review Comment: warning: variable 'page_expected' is not initialized [cppcoreguidelines-init-variables] ```suggestion std::string page_expected = 0; ``` ########## be/test/olap/rowset/segment_v2/binary_array_page_test.cpp: ########## @@ -0,0 +1,105 @@ +// 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 "olap/rowset/segment_v2/binary_array_page.h" + +#include <gtest/gtest.h> + +#include <cstdint> +#include <iostream> +#include <string> +#include <vector> + +#include "common/logging.h" +#include "olap/olap_common.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "olap/types.h" +#include "util/coding.h" + +namespace doris { +namespace segment_v2 { + +class BinaryArrayPageTest : public testing::Test { +public: + BinaryArrayPageTest() {} + + virtual ~BinaryArrayPageTest() {} + + void TestSmallPage() { + std::vector<Slice> slices; + slices.emplace_back("1"); + slices.emplace_back("2"); + slices.emplace_back("3"); + slices.emplace_back("4"); + + PageBuilderOptions options; + options.data_page_size = 10; + BinaryArrayPageBuilder page_builder(options); + { + size_t count = slices.size(); + + Slice* ptr = &slices[0]; + Status ret = page_builder.add(reinterpret_cast<const uint8_t*>(ptr), &count); + EXPECT_TRUE(ret.ok()); + EXPECT_EQ(3, count); + + auto page_actual = page_builder.finish(); + std::string page_expected; + page_expected.push_back(slices[0][0]); + page_expected.push_back(slices[1][0]); + page_expected.push_back(slices[2][0]); + put_fixed32_le(&page_expected, count); + put_fixed32_le(&page_expected, 1); + for (size_t i = 0; i < page_expected.size(); i++) { + EXPECT_EQ(page_expected[i], page_actual.slice()[i]); + } + } + page_builder.reset(); + slices.clear(); + slices.push_back("4"); + slices.push_back("5"); + slices.push_back("6"); + { + size_t count = slices.size(); + + Slice* ptr = &slices[0]; + Status ret = page_builder.add(reinterpret_cast<const uint8_t*>(ptr), &count); + EXPECT_TRUE(ret.ok()); + EXPECT_EQ(3, count); + + auto page_actual = page_builder.finish(); + std::string page_expected; Review Comment: warning: variable 'page_expected' is not initialized [cppcoreguidelines-init-variables] ```suggestion std::string page_expected = 0; ``` ########## be/test/olap/rowset/segment_v2/binary_array_page_test.cpp: ########## @@ -0,0 +1,105 @@ +// 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 "olap/rowset/segment_v2/binary_array_page.h" + +#include <gtest/gtest.h> + +#include <cstdint> +#include <iostream> +#include <string> +#include <vector> + +#include "common/logging.h" +#include "olap/olap_common.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "olap/types.h" +#include "util/coding.h" + +namespace doris { +namespace segment_v2 { + +class BinaryArrayPageTest : public testing::Test { +public: + BinaryArrayPageTest() {} + + virtual ~BinaryArrayPageTest() {} + + void TestSmallPage() { + std::vector<Slice> slices; + slices.emplace_back("1"); + slices.emplace_back("2"); + slices.emplace_back("3"); + slices.emplace_back("4"); + + PageBuilderOptions options; + options.data_page_size = 10; + BinaryArrayPageBuilder page_builder(options); + { + size_t count = slices.size(); + + Slice* ptr = &slices[0]; + Status ret = page_builder.add(reinterpret_cast<const uint8_t*>(ptr), &count); + EXPECT_TRUE(ret.ok()); + EXPECT_EQ(3, count); + + auto page_actual = page_builder.finish(); + std::string page_expected; + page_expected.push_back(slices[0][0]); + page_expected.push_back(slices[1][0]); + page_expected.push_back(slices[2][0]); + put_fixed32_le(&page_expected, count); + put_fixed32_le(&page_expected, 1); + for (size_t i = 0; i < page_expected.size(); i++) { + EXPECT_EQ(page_expected[i], page_actual.slice()[i]); + } + } + page_builder.reset(); + slices.clear(); + slices.push_back("4"); + slices.push_back("5"); + slices.push_back("6"); + { + size_t count = slices.size(); + + Slice* ptr = &slices[0]; Review Comment: warning: variable 'ptr' is not initialized [cppcoreguidelines-init-variables] ```suggestion Slice* ptr = nullptr = &slices[0]; ``` ########## be/test/olap/rowset/segment_v2/binary_array_page_test.cpp: ########## @@ -0,0 +1,105 @@ +// 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 "olap/rowset/segment_v2/binary_array_page.h" + +#include <gtest/gtest.h> + +#include <cstdint> +#include <iostream> +#include <string> +#include <vector> + +#include "common/logging.h" +#include "olap/olap_common.h" +#include "olap/rowset/segment_v2/page_builder.h" +#include "olap/rowset/segment_v2/page_decoder.h" +#include "olap/types.h" +#include "util/coding.h" + +namespace doris { +namespace segment_v2 { + +class BinaryArrayPageTest : public testing::Test { +public: + BinaryArrayPageTest() {} + + virtual ~BinaryArrayPageTest() {} + + void TestSmallPage() { + std::vector<Slice> slices; + slices.emplace_back("1"); + slices.emplace_back("2"); + slices.emplace_back("3"); + slices.emplace_back("4"); + + PageBuilderOptions options; + options.data_page_size = 10; + BinaryArrayPageBuilder page_builder(options); + { + size_t count = slices.size(); + + Slice* ptr = &slices[0]; + Status ret = page_builder.add(reinterpret_cast<const uint8_t*>(ptr), &count); + EXPECT_TRUE(ret.ok()); + EXPECT_EQ(3, count); + + auto page_actual = page_builder.finish(); + std::string page_expected; + page_expected.push_back(slices[0][0]); + page_expected.push_back(slices[1][0]); + page_expected.push_back(slices[2][0]); + put_fixed32_le(&page_expected, count); + put_fixed32_le(&page_expected, 1); + for (size_t i = 0; i < page_expected.size(); i++) { + EXPECT_EQ(page_expected[i], page_actual.slice()[i]); + } + } + page_builder.reset(); + slices.clear(); + slices.push_back("4"); + slices.push_back("5"); + slices.push_back("6"); + { + size_t count = slices.size(); Review Comment: warning: variable 'count' is not initialized [cppcoreguidelines-init-variables] ```suggestion size_t count = 0 = slices.size(); ``` -- 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: commits-unsubscr...@doris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org For additional commands, e-mail: commits-h...@doris.apache.org
