liaoxin01 commented on code in PR #66545: URL: https://github.com/apache/doris/pull/66545#discussion_r3729020963
########## be/test/load/memtable/memtable_sort_test.cpp: ########## @@ -17,67 +17,235 @@ #include <gtest/gtest.h> +#include <memory> +#include <string> +#include <vector> Review Comment: Added `#include <cstring>`. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ ########## be/test/load/memtable/memtable_sort_test.cpp: ########## @@ -17,67 +17,235 @@ #include <gtest/gtest.h> +#include <memory> +#include <string> +#include <vector> + +#include "core/block/block.h" +#include "core/column/column_nullable.h" +#include "core/column/column_string.h" +#include "core/column/column_vector.h" +#include "load/delta_writer/delta_writer_context.h" #include "load/memtable/memtable.h" +#include "runtime/descriptor_helper.h" +#include "runtime/descriptors.h" +#include "runtime/memory/mem_tracker_limiter.h" +#include "runtime/workload_management/resource_context.h" +#include "storage/tablet/tablet_schema.h" namespace doris { -class MemTableSortTest : public ::testing::Test {}; - -TEST_F(MemTableSortTest, Tie) { - auto t0 = Tie {0, 0}; - EXPECT_FALSE(t0.iter().next()); +namespace { + +// Schema used by every case: k1 INT (key), k2 VARCHAR (key, nullable), v INT. +// Two key columns are needed so the equal-range refinement between key columns +// is exercised, and k2 is nullable so the ColumnNullable sort path is covered. +TabletSchemaSPtr create_schema(KeysType keys_type) { + TabletSchemaPB pb; + pb.set_keys_type(keys_type); + + auto add = [&](const std::string& name, const std::string& type, bool is_key, bool nullable, + int32_t length, const std::string& agg) { + ColumnPB* c = pb.add_column(); + c->set_unique_id(pb.column_size()); + c->set_name(name); + c->set_type(type); + c->set_is_key(is_key); + c->set_is_nullable(nullable); + c->set_length(length); + c->set_aggregation(agg); + c->set_is_bf_column(false); + }; + add("k1", "INT", true, false, 4, "NONE"); + add("k2", "VARCHAR", true, true, 20, "NONE"); + // value aggregation only matters for AGG_KEYS; UNIQUE_KEYS always replaces + add("v", "INT", false, false, 4, keys_type == KeysType::AGG_KEYS ? "SUM" : "REPLACE"); + + auto schema = std::make_shared<TabletSchema>(); + schema->init_from_pb(pb); + return schema; +} - auto tie = Tie {0, 1}; - EXPECT_FALSE(tie.iter().next()); +TDescriptorTable create_descriptor_table() { + TDescriptorTableBuilder dtb; + TTupleDescriptorBuilder tuple_builder; + tuple_builder.add_slot(TSlotDescriptorBuilder() + .type(TYPE_INT) + .nullable(false) + .column_name("k1") + .column_pos(0) + .build()); + tuple_builder.add_slot(TSlotDescriptorBuilder() + .string_type(20) + .nullable(true) + .column_name("k2") + .column_pos(1) + .build()); + tuple_builder.add_slot(TSlotDescriptorBuilder() + .type(TYPE_INT) + .nullable(false) + .column_name("v") + .column_pos(2) + .build()); + tuple_builder.build(&dtb); + return dtb.desc_tbl(); +} - auto t = Tie {10, 30}; - for (int i = 10; i < 30; i++) { - EXPECT_EQ(t[i], 1); +struct Row { + int32_t k1; + const char* k2; // nullptr means SQL NULL + int32_t v; +}; + +} // namespace + +class MemTableSortTest : public testing::Test { +protected: + // Feeds `rows` through a MemTable in `batches` insert() calls and returns the + // flushed block. Going through to_block() means the real _sort() runs. + std::unique_ptr<Block> run(KeysType keys_type, const std::vector<Row>& rows, size_t batches) { + TabletSchemaSPtr schema = create_schema(keys_type); + TDescriptorTable tdesc = create_descriptor_table(); + ObjectPool pool; + DescriptorTbl* desc_tbl = nullptr; + EXPECT_TRUE(DescriptorTbl::create(&pool, tdesc, &desc_tbl).ok()); + TupleDescriptor* tuple_desc = desc_tbl->get_tuple_descriptor(0); + auto resource_ctx = ResourceContext::create_shared(); Review Comment: Changed the helper to `void run(..., std::unique_ptr<Block>* out)` so `ASSERT_*` is usable inside it, switched all four checks (`DescriptorTbl::create`, `insert`, `to_block`, plus a new non-null check on the block) to `ASSERT_*`, and wrapped every call site in `ASSERT_NO_FATAL_FAILURE` so a failure aborts the test instead of dereferencing a null block. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ ########## be/src/load/memtable/memtable.cpp: ########## @@ -388,38 +390,80 @@ Status MemTable::_put_into_output(Block& in_block) { row_pos_vec.data() + in_block.rows()); } +// ColumnSorter keeps an inline copy of the key next to the row id, so a +// comparison no longer chases a row pointer nor pays a virtual +// IColumn::compare_at, which is what dominated the previous kernel. +size_t MemTable::_sort_permutation_by_key_columns(MutableBlock& block, + const std::vector<int>& key_col_idx, + IColumn::Permutation& perm) { + const size_t num_rows = perm.size(); + if (num_rows == 0) { + return 0; + } + EqualFlags flags(num_rows, 1); + EqualRange range {0, static_cast<int>(num_rows)}; + HybridSorter hybrid_sorter; + for (int idx : key_col_idx) { + ColumnWithSortDescription col {block.get_column_by_position(idx).get(), + SortColumnDescription(idx, 1 /*asc*/, -1 /*nulls first*/)}; + ColumnSorter sorter(col, hybrid_sorter, 0 /*no limit*/); + // last_column is deliberately never true: the equal ranges are still + // needed below for the row position tie-break. + sorter(flags, perm, range, false); + } + + size_t same_keys_num = 0; + EqualRangeIterator iter(flags, range.first, range.second); + while (iter.next()) { + // perm elements are row positions, so a plain sort restores insertion order + std::sort(perm.begin() + iter.range_begin, perm.begin() + iter.range_end); Review Comment: Switched both branches to `pdqsort`, which is also what the code being replaced used here. _🤖 Addressed by [Claude Code](https://claude.com/claude-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]
