github-actions[bot] commented on code in PR #65492: URL: https://github.com/apache/doris/pull/65492#discussion_r3565929076
########## be/test/storage/key/row_key_encoder_test.cpp: ########## @@ -0,0 +1,645 @@ +// 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. + +// White-box access to TabletColumn/TabletSchema private fields: the test +// builds schema objects field by field, the same way the existing storage +// unit tests do (tablet_schema_helper.cpp does the same). The macros wrap +// only this include and are #undef-ed right away, so gtest, standard and +// other Doris headers below are parsed with their access specifiers intact. +#if defined(__clang__) +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wkeyword-macro" +#endif +#define private public +#define protected public +#include "storage/tablet/tablet_schema.h" // IWYU pragma: keep +#undef private +#undef protected +#if defined(__clang__) +#pragma clang diagnostic pop +#endif + +#include <gtest/gtest.h> + +#include <array> +#include <optional> +#include <string> +#include <vector> + +#include "common/consts.h" +#include "core/block/block.h" +#include "storage/iterator/olap_data_convertor.h" +#include "storage/key/row_key_encoder.h" +#include "storage/olap_common.h" +#include "storage/tablet/tablet_schema_helper.h" +#include "storage/utils.h" + +namespace doris { +namespace { + +constexpr uint8_t kNull = KeyConsts::KEY_NULL_FIRST_MARKER; // 0x01 +constexpr uint8_t kNormal = KeyConsts::KEY_NORMAL_MARKER; // 0x02 +constexpr uint8_t kMinimal = KeyConsts::KEY_MINIMAL_MARKER; // 0x00 + +// A hidden sequence column is identified by its reserved name. +TabletColumnPtr create_seq_col(int32_t uid) { + auto c = std::make_shared<TabletColumn>(); + c->_unique_id = uid; + c->_col_name = SEQUENCE_COL; + c->_type = FieldType::OLAP_FIELD_TYPE_INT; + c->_is_key = false; + c->_is_nullable = true; + c->_length = 4; + c->_index_length = 4; + return c; +} + +void fill_int(MutableColumns& cols, uint32_t cid, const std::vector<std::optional<int32_t>>& vals) { + for (const auto& v : vals) { + if (v.has_value()) { + int32_t x = *v; + cols[cid]->insert_data(reinterpret_cast<const char*>(&x), sizeof(x)); + } else { + cols[cid]->insert_default(); // NULL for a nullable column + } + } +} + +void fill_char(MutableColumns& cols, uint32_t cid, const std::vector<std::string>& vals) { + for (const auto& s : vals) { + cols[cid]->insert_data(s.data(), s.size()); + } +} + +uint8_t byte_at(const std::string& s, size_t i) { + return static_cast<uint8_t>(s[i]); +} + +// key(0), key(1), value(2): two int key columns. +TabletSchemaSPtr two_int_key_schema() { + auto s = std::make_shared<TabletSchema>(); + s->append_column(*create_int_key(0)); + s->append_column(*create_int_key(1)); + s->append_column(*create_int_value(2)); + s->_keys_type = DUP_KEYS; + s->_num_short_key_columns = 2; + return s; +} + +// char(8) key (index_length 1) + value: tests short-key truncation. +TabletSchemaSPtr char_key_schema() { + auto s = std::make_shared<TabletSchema>(); + s->append_column(*create_char_key(0, /*is_nullable=*/true, /*length=*/8)); + s->append_column(*create_int_value(1)); + s->_keys_type = DUP_KEYS; + s->_num_short_key_columns = 1; + return s; +} + +// key(0), value(1), seq(2): unique-key table with a sequence column. +TabletSchemaSPtr seq_schema() { + auto s = std::make_shared<TabletSchema>(); + s->append_column(*create_int_key(0)); + s->append_column(*create_int_value(1)); + s->append_column(*create_seq_col(2)); + s->_keys_type = UNIQUE_KEYS; + s->_num_short_key_columns = 1; + return s; +} + +// key(uid 0), value(uid 1) with the value column declared as the cluster key. +TabletSchemaSPtr cluster_key_schema() { + auto s = std::make_shared<TabletSchema>(); + s->append_column(*create_int_key(0)); + s->append_column(*create_int_value(1)); + s->_keys_type = UNIQUE_KEYS; + s->_cluster_key_uids = {1}; + s->_num_short_key_columns = 1; + return s; +} + +std::string to_hex(const std::string& s) { + static constexpr char kDigits[] = "0123456789abcdef"; + std::string out; + out.reserve(s.size() * 2); + for (unsigned char c : s) { + out.push_back(kDigits[c >> 4]); + out.push_back(kDigits[c & 0xf]); + } + return out; +} + +template <typename T> +void fill_raw(MutableColumns& cols, uint32_t cid, T v) { + cols[cid]->insert_data(reinterpret_cast<const char*>(&v), sizeof(v)); +} + +TabletColumnPtr make_key_column(int32_t uid, FieldType type, int32_t length, int32_t index_length, + int32_t precision = 0, int32_t frac = 0) { + auto c = std::make_shared<TabletColumn>(); + c->_unique_id = uid; + c->_col_name = "k" + std::to_string(uid); + c->_type = type; + c->_is_key = true; + c->_is_nullable = false; + c->_length = length; + c->_index_length = index_length; + c->_precision = precision; + c->_frac = frac; + return c; +} + +// One key column of the given type + one int value column, unique keys so the +// primary-key view exists. +TabletSchemaSPtr single_key_schema(const TabletColumnPtr& key_col) { + auto s = std::make_shared<TabletSchema>(); + s->append_column(*key_col); + s->append_column(*create_int_value(100)); + s->_keys_type = UNIQUE_KEYS; + s->_num_short_key_columns = 1; + return s; +} + +} // namespace + +// A small holder so the convertor (which the accessors point into) and the +// source block stay alive until after the encode calls. +class RowKeyEncoderTest : public testing::Test { +protected: + void build(const TabletSchemaSPtr& schema, size_t num_rows, + const std::function<void(MutableColumns&)>& fill) { + _schema = schema; + _num_rows = num_rows; + _block = schema->create_block(); + { + auto guard = _block.mutate_columns_scoped(); + fill(guard.mutable_columns()); + } + _convertor = std::make_unique<OlapBlockDataConvertor>(schema.get()); + _convertor->set_source_content(&_block, 0, num_rows); + } + + IOlapColumnDataAccessor* acc(uint32_t cid) { + auto [st, accessor] = _convertor->convert_column_data(cid); + EXPECT_TRUE(st.ok()) << st; + return accessor; + } + + // Encode one row through all three views and compare against literal hex + // goldens. The goldens pin the on-disk key byte layout: any coder change + // or CPU-architecture (endianness) drift must fail these expectations. + void check_golden(const TabletSchemaSPtr& schema, + const std::function<void(MutableColumns&)>& fill, const std::string& full_hex, + const std::string& primary_hex, const std::string& short_hex) { + build(schema, 1, fill); + RowKeyEncoder enc(*_schema, /*mow=*/true); + std::vector<IOlapColumnDataAccessor*> keys {acc(0)}; + EXPECT_EQ(to_hex(enc.full_encode(keys, 0)), full_hex); + EXPECT_EQ(to_hex(enc.full_encode_primary_keys(keys, 0)), primary_hex); + EXPECT_EQ(to_hex(enc.encode_short_keys(keys, 0)), short_hex); + } + + TabletSchemaSPtr _schema; + Block _block; + std::unique_ptr<OlapBlockDataConvertor> _convertor; + size_t _num_rows = 0; +}; + +// Each non-null key column is one marker byte + the encoded value. +TEST_F(RowKeyEncoderTest, FullEncodeIntKeyLayout) { + build(two_int_key_schema(), 1, [](MutableColumns& c) { + fill_int(c, 0, {1}); + fill_int(c, 1, {2}); + fill_int(c, 2, {0}); + }); + RowKeyEncoder enc(*_schema, /*mow=*/false); + std::vector<IOlapColumnDataAccessor*> keys {acc(0), acc(1)}; + std::string k = enc.full_encode(keys, 0); + + ASSERT_EQ(k.size(), 1u + 4u + 1u + 4u); + EXPECT_EQ(byte_at(k, 0), kNormal); + EXPECT_EQ(byte_at(k, 5), kNormal); +} + +// A null key value is a single null marker with no value bytes. +TEST_F(RowKeyEncoderTest, NullKeyUsesNullMarker) { + build(two_int_key_schema(), 1, [](MutableColumns& c) { + fill_int(c, 0, {std::nullopt}); + fill_int(c, 1, {7}); + fill_int(c, 2, {0}); + }); + RowKeyEncoder enc(*_schema, /*mow=*/false); + std::vector<IOlapColumnDataAccessor*> keys {acc(0), acc(1)}; + std::string k = enc.full_encode(keys, 0); + + ASSERT_EQ(k.size(), 1u + (1u + 4u)); + EXPECT_EQ(byte_at(k, 0), kNull); + EXPECT_EQ(byte_at(k, 1), kNormal); +} + +// The encoded key is a sortable byte string: byte-by-byte order over the +// encodings must match the multi-column key order, and nulls sort first. +TEST_F(RowKeyEncoderTest, FullEncodePreservesAscendingOrder) { + build(two_int_key_schema(), 5, [](MutableColumns& c) { + fill_int(c, 0, {std::nullopt, -5, -5, 2, 2}); + fill_int(c, 1, {0, 3, 9, -100, 7}); + fill_int(c, 2, {0, 0, 0, 0, 0}); + }); + RowKeyEncoder enc(*_schema, /*mow=*/false); + std::vector<IOlapColumnDataAccessor*> keys {acc(0), acc(1)}; + + std::vector<std::string> encoded; + for (size_t pos = 0; pos < _num_rows; ++pos) { + encoded.push_back(enc.full_encode(keys, pos)); + } + for (size_t i = 0; i + 1 < encoded.size(); ++i) { + EXPECT_LT(encoded[i], encoded[i + 1]) << "rows " << i << " and " << i + 1; + } +} + +// Without cluster keys the sort key and the schema (primary) key are identical. +TEST_F(RowKeyEncoderTest, PrimaryKeysEqualFullEncodeWithoutClusterKey) { + build(two_int_key_schema(), 1, [](MutableColumns& c) { + fill_int(c, 0, {3}); + fill_int(c, 1, {4}); + fill_int(c, 2, {0}); + }); + RowKeyEncoder enc(*_schema, /*mow=*/true); + std::vector<IOlapColumnDataAccessor*> keys {acc(0), acc(1)}; + EXPECT_EQ(enc.full_encode(keys, 0), enc.full_encode_primary_keys(keys, 0)); +} + +// With cluster keys the segment sorts by the cluster key columns, while the +// primary key index is still built over the schema key columns. +TEST_F(RowKeyEncoderTest, ClusterKeySortDiffersFromPrimaryKeys) { Review Comment: This test does not actually catch the primary-vs-cluster coder mixup that the refactor is trying to protect against. `cluster_key_schema()` makes both the primary key and the cluster key INT columns, and this assertion only proves that encoded value `5` differs from encoded value `99`. If `full_encode_primary_keys()` accidentally used the cluster/sort-key coder vector, the test would still pass because the supplied primary-key accessor is also INT. Please make this case heterogeneous, for example INT primary key plus CHAR/VARCHAR cluster key, and assert the exact hex for `full_encode_primary_keys(primary_keys)`, `full_encode(sort_keys)`, and the short key so a wrong coder vector fails deterministically. -- 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]
