Copilot commented on code in PR #66545:
URL: https://github.com/apache/doris/pull/66545#discussion_r3727821898
##########
be/test/load/memtable/memtable_sort_test.cpp:
##########
@@ -17,67 +17,235 @@
#include <gtest/gtest.h>
+#include <memory>
+#include <string>
+#include <vector>
Review Comment:
`strlen()` is used but `<cstring>` isn't included. This can break
compilation depending on transitive includes.
##########
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:
`DescriptorTbl::create()` failure should abort the helper early;
`EXPECT_TRUE` will continue and can lead to null dereference when accessing
`desc_tbl`/`tuple_desc`. Use `ASSERT_*` in this test helper.
This issue also appears in the following locations of the same file:
- line 149
- line 153
##########
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:
The tie-break within equal-key ranges is on a performance-sensitive path,
but it currently uses `std::sort` even though the surrounding sort
infrastructure uses `pdqsort`/`HybridSorter`. Consider switching this to
`pdqsort` to avoid a potential regression on large duplicate-key groups.
--
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]
