From a57227e066c68ffc7a1cc9b3e564ce0d782858be Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 31 Mar 2022 19:11:44 +0800 Subject: [PATCH 1/8] Fix lint --- .../DeltaMerge/tests/gtest_dm_segment.cpp | 145 +++++++++--------- 1 file changed, 71 insertions(+), 74 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp index 5726cfa132d..dd9d058cc89 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp @@ -50,12 +50,10 @@ extern DMFilePtr writeIntoNewDMFile(DMContext & dm_context, // DMFileBlockOutputStream::Flags flags); namespace tests { -class Segment_test : public DB::base::TiFlashStorageTestBasic +class SegmentTest : public DB::base::TiFlashStorageTestBasic { public: - Segment_test() - : storage_pool() - {} + SegmentTest() = default; public: static void SetUpTestCase() {} @@ -63,7 +61,7 @@ class Segment_test : public DB::base::TiFlashStorageTestBasic void SetUp() override { TiFlashStorageTestBasic::SetUp(); - table_columns_ = std::make_shared(); + table_columns = std::make_shared(); segment = reload(); ASSERT_EQ(segment->segmentId(), DELTA_MERGE_FIRST_SEGMENT_ID); @@ -79,15 +77,15 @@ class Segment_test : public DB::base::TiFlashStorageTestBasic ColumnDefinesPtr cols = (!pre_define_columns) ? DMTestEnv::getDefaultColumns() : pre_define_columns; setColumns(cols); - return Segment::newSegment(*dm_context_, table_columns_, RowKeyRange::newAll(false, 1), storage_pool->newMetaPageId(), 0); + return Segment::newSegment(*dm_context, table_columns, RowKeyRange::newAll(false, 1), storage_pool->newMetaPageId(), 0); } // setColumns should update dm_context at the same time void setColumns(const ColumnDefinesPtr & columns) { - *table_columns_ = *columns; + *table_columns = *columns; - dm_context_ = std::make_unique(*db_context, + dm_context = std::make_unique(*db_context, *storage_path_pool, *storage_pool, 0, @@ -98,24 +96,24 @@ class Segment_test : public DB::base::TiFlashStorageTestBasic db_context->getSettingsRef()); } - const ColumnDefinesPtr & tableColumns() const { return table_columns_; } + const ColumnDefinesPtr & tableColumns() const { return table_columns; } - DMContext & dmContext() { return *dm_context_; } + DMContext & dmContext() { return *dm_context; } protected: /// all these var lives as ref in dm_context std::unique_ptr storage_path_pool; std::unique_ptr storage_pool; - ColumnDefinesPtr table_columns_; + ColumnDefinesPtr table_columns; DM::DeltaMergeStore::Settings settings; /// dm_context - std::unique_ptr dm_context_; + std::unique_ptr dm_context; // the segment we are going to test SegmentPtr segment; }; -TEST_F(Segment_test, WriteRead) +TEST_F(SegmentTest, WriteRead) try { const size_t num_rows_write = 100; @@ -124,11 +122,11 @@ try // write to segment segment->write(dmContext(), block); // estimate segment - auto estimatedRows = segment->getEstimatedRows(); - ASSERT_EQ(estimatedRows, block.rows()); + auto estimated_rows = segment->getEstimatedRows(); + ASSERT_EQ(estimated_rows, block.rows()); - auto estimatedBytes = segment->getEstimatedBytes(); - ASSERT_EQ(estimatedBytes, block.bytes()); + auto estimated_bytes = segment->getEstimatedBytes(); + ASSERT_EQ(estimated_bytes, block.bytes()); } { @@ -212,7 +210,7 @@ try } CATCH -TEST_F(Segment_test, WriteRead2) +TEST_F(SegmentTest, WriteRead2) try { const size_t num_rows_write = dmContext().stable_pack_rows; @@ -249,7 +247,7 @@ try } CATCH -TEST_F(Segment_test, WriteReadMultiRange) +TEST_F(SegmentTest, WriteReadMultiRange) try { const size_t num_rows_write = 100; @@ -258,11 +256,11 @@ try // write to segment segment->write(dmContext(), block); // estimate segment - auto estimatedRows = segment->getEstimatedRows(); - ASSERT_EQ(estimatedRows, block.rows()); + auto estimated_rows = segment->getEstimatedRows(); + ASSERT_EQ(estimated_rows, block.rows()); - auto estimatedBytes = segment->getEstimatedBytes(); - ASSERT_EQ(estimatedBytes, block.bytes()); + auto estimated_bytes = segment->getEstimatedBytes(); + ASSERT_EQ(estimated_bytes, block.bytes()); } { @@ -356,7 +354,7 @@ try } CATCH -TEST_F(Segment_test, ReadWithMoreAdvacedDeltaIndex) +TEST_F(SegmentTest, ReadWithMoreAdvacedDeltaIndex) try { // Test the case that reading rows with an advance DeltaIndex @@ -421,8 +419,8 @@ try } CATCH -class SegmentDeletionRelevantPlace_test - : public Segment_test +class SegmentDeletionRelevantPlaceTest + : public SegmentTest , public testing::WithParamInterface { DB::Settings getSettings() @@ -439,7 +437,7 @@ class SegmentDeletionRelevantPlace_test }; -TEST_P(SegmentDeletionRelevantPlace_test, ShareDelteRangeIndex) +TEST_P(SegmentDeletionRelevantPlaceTest, ShareDelteRangeIndex) try { const size_t num_rows_write = 300; @@ -479,15 +477,15 @@ try } CATCH -INSTANTIATE_TEST_CASE_P(WhetherEnableRelevantPlace, SegmentDeletionRelevantPlace_test, testing::Values(true, false)); +INSTANTIATE_TEST_CASE_P(WhetherEnableRelevantPlace, SegmentDeletionRelevantPlaceTest, testing::Values(true, false)); -class SegmentDeletion_test - : public Segment_test +class SegmentDeletionTest + : public SegmentTest , public testing::WithParamInterface> { }; -TEST_P(SegmentDeletion_test, DeleteDataInDelta) +TEST_P(SegmentDeletionTest, DeleteDataInDelta) try { const size_t num_rows_write = 100; @@ -565,7 +563,7 @@ try } CATCH -TEST_P(SegmentDeletion_test, DeleteDataInStable) +TEST_P(SegmentDeletionTest, DeleteDataInStable) try { const size_t num_rows_write = 100; @@ -651,7 +649,7 @@ try } CATCH -TEST_P(SegmentDeletion_test, DeleteDataInStableAndDelta) +TEST_P(SegmentDeletionTest, DeleteDataInStableAndDelta) try { const size_t num_rows_write = 100; @@ -738,9 +736,9 @@ try } CATCH -INSTANTIATE_TEST_CASE_P(WhetherReadOrMergeDeltaBeforeDeleteRange, SegmentDeletion_test, testing::Combine(testing::Bool(), testing::Bool())); +INSTANTIATE_TEST_CASE_P(WhetherReadOrMergeDeltaBeforeDeleteRange, SegmentDeletionTest, testing::Combine(testing::Bool(), testing::Bool())); -TEST_F(Segment_test, DeleteRead) +TEST_F(SegmentTest, DeleteRead) try { const size_t num_rows_write = 64; @@ -946,7 +944,7 @@ try } CATCH -TEST_F(Segment_test, Split) +TEST_F(SegmentTest, Split) try { const size_t num_rows_write_per_batch = 100; @@ -1046,7 +1044,7 @@ try } CATCH -TEST_F(Segment_test, SplitFail) +TEST_F(SegmentTest, SplitFail) try { const size_t num_rows_write = 100; @@ -1066,7 +1064,7 @@ try } CATCH -TEST_F(Segment_test, Restore) +TEST_F(SegmentTest, Restore) try { // compare will compares the given segments. @@ -1158,7 +1156,7 @@ try } CATCH -TEST_F(Segment_test, MassiveSplit) +TEST_F(SegmentTest, MassiveSplit) try { Settings settings = dmContext().db_context.getSettings(); @@ -1242,52 +1240,51 @@ try } CATCH -enum Segment_test_Mode +enum SegmentTestMode { V1_BlockOnly, V2_BlockOnly, V2_FileOnly, }; -String testModeToString(const ::testing::TestParamInfo & info) +String testModeToString(const ::testing::TestParamInfo & info) { const auto mode = info.param; switch (mode) { - case Segment_test_Mode::V1_BlockOnly: + case SegmentTestMode::V1_BlockOnly: return "V1_BlockOnly"; - case Segment_test_Mode::V2_BlockOnly: + case SegmentTestMode::V2_BlockOnly: return "V2_BlockOnly"; - case Segment_test_Mode::V2_FileOnly: + case SegmentTestMode::V2_FileOnly: return "V2_FileOnly"; default: return "Unknown"; } } -class Segment_test_2 : public Segment_test - , public testing::WithParamInterface +class SegmentTest2 : public SegmentTest + , public testing::WithParamInterface { public: - Segment_test_2() - : Segment_test() - {} + SegmentTest2() = default; + void SetUp() override { mode = GetParam(); switch (mode) { - case Segment_test_Mode::V1_BlockOnly: + case SegmentTestMode::V1_BlockOnly: setStorageFormat(1); break; - case Segment_test_Mode::V2_BlockOnly: - case Segment_test_Mode::V2_FileOnly: + case SegmentTestMode::V2_BlockOnly: + case SegmentTestMode::V2_FileOnly: setStorageFormat(2); break; } - Segment_test::SetUp(); + SegmentTest::SetUp(); } std::pair genDMFile(DMContext & context, const Block & block) @@ -1305,7 +1302,7 @@ class Segment_test_2 : public Segment_test delegator.addDTFile(file_id, dmfile->getBytesOnDisk(), store_path); - auto & pk_column = block.getByPosition(0).column; + const auto & pk_column = block.getByPosition(0).column; auto min_pk = pk_column->getInt(0); auto max_pk = pk_column->getInt(block.rows() - 1); HandleRange range(min_pk, max_pk + 1); @@ -1313,10 +1310,10 @@ class Segment_test_2 : public Segment_test return {RowKeyRange::fromHandleRange(range), {file_id}}; } - Segment_test_Mode mode; + SegmentTestMode mode; }; -TEST_P(Segment_test_2, FlushDuringSplitAndMerge) +TEST_P(SegmentTest2, FlushDuringSplitAndMerge) try { size_t row_offset = 0; @@ -1327,11 +1324,11 @@ try row_offset += 100; switch (mode) { - case Segment_test_Mode::V1_BlockOnly: - case Segment_test_Mode::V2_BlockOnly: + case SegmentTestMode::V1_BlockOnly: + case SegmentTestMode::V2_BlockOnly: segment->write(dmContext(), std::move(block)); break; - case Segment_test_Mode::V2_FileOnly: + case SegmentTestMode::V2_FileOnly: { auto delegate = dmContext().path_pool.getStableDiskDelegator(); auto file_provider = dmContext().db_context.getFileProvider(); @@ -1430,9 +1427,9 @@ try } CATCH -INSTANTIATE_TEST_CASE_P(Segment_test_Mode, // - Segment_test_2, - testing::Values(Segment_test_Mode::V1_BlockOnly, Segment_test_Mode::V2_BlockOnly, Segment_test_Mode::V2_FileOnly), +INSTANTIATE_TEST_CASE_P(SegmentTestMode, // + SegmentTest2, + testing::Values(SegmentTestMode::V1_BlockOnly, SegmentTestMode::V2_BlockOnly, SegmentTestMode::V2_FileOnly), testModeToString); enum class SegmentWriteType @@ -1440,12 +1437,12 @@ enum class SegmentWriteType ToDisk, ToCache }; -class Segment_DDL_test - : public Segment_test +class SegmentDDLTest + : public SegmentTest , public testing::WithParamInterface> { }; -String paramToString(const ::testing::TestParamInfo & info) +String paramToString(const ::testing::TestParamInfo & info) { const auto [write_type, flush_before_ddl] = info.param; @@ -1455,7 +1452,7 @@ String paramToString(const ::testing::TestParamInfo } /// Mock a col from i8 -> i32 -TEST_P(Segment_DDL_test, AlterInt8ToInt32) +TEST_P(SegmentDDLTest, AlterInt8ToInt32) try { const String column_name_i8_to_i32 = "i8_to_i32"; @@ -1627,7 +1624,7 @@ try } CATCH -TEST_P(Segment_DDL_test, AddColumn) +TEST_P(SegmentDDLTest, AddColumn) try { const String new_column_name = "i8"; @@ -1795,7 +1792,7 @@ try } CATCH -TEST_F(Segment_test, CalculateDTFileProperty) +TEST_F(SegmentTest, CalculateDTFileProperty) try { Settings settings = dmContext().db_context.getSettings(); @@ -1836,7 +1833,7 @@ try } CATCH -TEST_F(Segment_test, CalculateDTFilePropertyWithPropertyFileDeleted) +TEST_F(SegmentTest, CalculateDTFilePropertyWithPropertyFileDeleted) try { Settings settings = dmContext().db_context.getSettings(); @@ -1857,10 +1854,10 @@ try } { - auto & stable = segment->getStable(); - auto & dmfiles = stable->getDMFiles(); + const auto & stable = segment->getStable(); + const auto & dmfiles = stable->getDMFiles(); ASSERT_GT(dmfiles[0]->getPacks(), (size_t)1); - auto & dmfile = dmfiles[0]; + const auto & dmfile = dmfiles[0]; auto file_path = dmfile->path(); // check property file exists and then delete it ASSERT_EQ(Poco::File(file_path + "/property").exists(), true); @@ -1877,7 +1874,7 @@ try // calculate the StableProperty for packs in the key range [0, num_rows_write_every_round) stable->calculateStableProperty(dmContext(), range, false); ASSERT_EQ(stable->isStablePropertyCached(), true); - auto & property = stable->getStableProperty(); + const auto & property = stable->getStableProperty(); ASSERT_EQ(property.gc_hint_version, std::numeric_limits::max()); ASSERT_EQ(property.num_versions, num_rows_write_every_round); ASSERT_EQ(property.num_puts, num_rows_write_every_round); @@ -1887,7 +1884,7 @@ try CATCH INSTANTIATE_TEST_CASE_P(SegmentWriteType, - Segment_DDL_test, + SegmentDDLTest, ::testing::Combine( // ::testing::Values(SegmentWriteType::ToDisk, SegmentWriteType::ToCache), ::testing::Bool()), From 03c1db48f7be78cf21f43f32569dc0c11b4c43a3 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 7 Apr 2022 13:26:54 +0800 Subject: [PATCH 2/8] Rename dm_basic_include -> DMTestEnv --- .../Management/tests/gtest_manual_compact.cpp | 2 +- .../Storages/DeltaMerge/tests/CMakeLists.txt | 2 +- .../tests/{dm_basic_include.h => DMTestEnv.h} | 0 .../DeltaMerge/tests/MultiSegmentTestUtil.h | 5 ++--- .../tests/bank/DeltaMergeStoreProxy.h | 2 +- .../Storages/DeltaMerge/tests/bank/main.cpp | 2 +- .../DeltaMerge/tests/gtest_convert_column.cpp | 3 +-- .../DeltaMerge/tests/gtest_data_streams.cpp | 2 +- .../tests/gtest_dm_delta_merge_store.cpp | 5 ++--- .../tests/gtest_dm_delta_value_space.cpp | 3 +-- .../DeltaMerge/tests/gtest_dm_file.cpp | 3 +-- .../tests/gtest_dm_minmax_index.cpp | 4 ++-- .../DeltaMerge/tests/gtest_dm_segment.cpp | 22 +++++++++---------- .../tests/gtest_dm_segment_common_handle.cpp | 3 +-- .../tests/gtest_dm_storage_delta_merge.cpp | 4 ++-- .../DeltaMerge/tests/gtest_dm_utils.cpp | 3 +-- .../DeltaMerge/tests/gtest_version_filter.cpp | 2 +- .../DeltaMerge/tests/stress/DMStressProxy.h | 2 +- 18 files changed, 31 insertions(+), 38 deletions(-) rename dbms/src/Storages/DeltaMerge/tests/{dm_basic_include.h => DMTestEnv.h} (100%) diff --git a/dbms/src/Flash/Management/tests/gtest_manual_compact.cpp b/dbms/src/Flash/Management/tests/gtest_manual_compact.cpp index 8ec3eb54406..4527892e353 100644 --- a/dbms/src/Flash/Management/tests/gtest_manual_compact.cpp +++ b/dbms/src/Flash/Management/tests/gtest_manual_compact.cpp @@ -17,8 +17,8 @@ #include #include #include +#include #include -#include #include #include #include diff --git a/dbms/src/Storages/DeltaMerge/tests/CMakeLists.txt b/dbms/src/Storages/DeltaMerge/tests/CMakeLists.txt index 8d5854ffb5d..fd02bcebd2f 100644 --- a/dbms/src/Storages/DeltaMerge/tests/CMakeLists.txt +++ b/dbms/src/Storages/DeltaMerge/tests/CMakeLists.txt @@ -21,7 +21,7 @@ macro(grep_gtest_sources BASE_DIR DST_VAR) endmacro() # attach all dm gtest sources grep_gtest_sources(${TiFlash_SOURCE_DIR}/dbms/src/Storages/DeltaMerge/tests dm_gtest_sources) -add_executable(gtests_dm EXCLUDE_FROM_ALL ${dm_gtest_sources} dm_basic_include.h) +add_executable(gtests_dm EXCLUDE_FROM_ALL ${dm_gtest_sources} DMTestEnv.h) target_link_libraries(gtests_dm gtest_main dbms clickhouse_functions) add_check(gtests_dm) diff --git a/dbms/src/Storages/DeltaMerge/tests/dm_basic_include.h b/dbms/src/Storages/DeltaMerge/tests/DMTestEnv.h similarity index 100% rename from dbms/src/Storages/DeltaMerge/tests/dm_basic_include.h rename to dbms/src/Storages/DeltaMerge/tests/DMTestEnv.h diff --git a/dbms/src/Storages/DeltaMerge/tests/MultiSegmentTestUtil.h b/dbms/src/Storages/DeltaMerge/tests/MultiSegmentTestUtil.h index bc6b7d5c3e6..7c5b0b2416d 100644 --- a/dbms/src/Storages/DeltaMerge/tests/MultiSegmentTestUtil.h +++ b/dbms/src/Storages/DeltaMerge/tests/MultiSegmentTestUtil.h @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -30,7 +31,6 @@ #include #include -#include "dm_basic_include.h" namespace DB { @@ -43,7 +43,6 @@ namespace DM { namespace tests { - /// Helper class to test with multiple segments. /// You can call `prepareSegments` to prepare multiple segments. After that, /// you can use `verifyExpectedRowsForAllSegments` to verify the expectation for each segment. @@ -157,4 +156,4 @@ class MultiSegmentTestUtil : private boost::noncopyable } // namespace tests } // namespace DM -} // namespace DB \ No newline at end of file +} // namespace DB diff --git a/dbms/src/Storages/DeltaMerge/tests/bank/DeltaMergeStoreProxy.h b/dbms/src/Storages/DeltaMerge/tests/bank/DeltaMergeStoreProxy.h index 99264c77dc6..8edbbbc55b4 100644 --- a/dbms/src/Storages/DeltaMerge/tests/bank/DeltaMergeStoreProxy.h +++ b/dbms/src/Storages/DeltaMerge/tests/bank/DeltaMergeStoreProxy.h @@ -18,9 +18,9 @@ #include #include #include +#include #include #include -#include #include #include diff --git a/dbms/src/Storages/DeltaMerge/tests/bank/main.cpp b/dbms/src/Storages/DeltaMerge/tests/bank/main.cpp index 115e170c48b..b90ad132e25 100644 --- a/dbms/src/Storages/DeltaMerge/tests/bank/main.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/bank/main.cpp @@ -17,8 +17,8 @@ #include #include #include +#include #include -#include #include #include diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_convert_column.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_convert_column.cpp index 928a256349b..efc67de611e 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_convert_column.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_convert_column.cpp @@ -18,10 +18,9 @@ #include #include #include +#include #include -#include "dm_basic_include.h" - namespace DB { namespace DM diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_data_streams.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_data_streams.cpp index f0c2f49c30b..00f31bc97e7 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_data_streams.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_data_streams.cpp @@ -15,7 +15,7 @@ #include #include #include -#include +#include namespace DB { diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp index 35e6c3d00c6..e934f7a2049 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp @@ -27,6 +27,8 @@ #include #include #include +#include +#include #include #include #include @@ -36,9 +38,6 @@ #include #include -#include "MultiSegmentTestUtil.h" -#include "dm_basic_include.h" - namespace DB { namespace FailPoints diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_value_space.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_value_space.cpp index 2b64fd90c09..40c399353b6 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_value_space.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_value_space.cpp @@ -18,14 +18,13 @@ #include #include #include +#include #include #include #include #include -#include "dm_basic_include.h" - namespace CurrentMetrics { extern const Metric DT_SnapshotOfRead; diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_file.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_file.cpp index dfd4419fe38..23062f4ffdf 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_file.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_file.cpp @@ -20,13 +20,12 @@ #include #include #include +#include #include #include #include -#include "dm_basic_include.h" - namespace DB { namespace FailPoints diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_minmax_index.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_minmax_index.cpp index 31fd99faf01..96c0070b73b 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_minmax_index.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_minmax_index.cpp @@ -22,7 +22,7 @@ #include #include #include -#include +#include #include #include @@ -41,7 +41,7 @@ static const String DEFAULT_COL_NAME = "2020-09-26"; class DMMinMaxIndexTest : public ::testing::Test { public: - DMMinMaxIndexTest() {} + DMMinMaxIndexTest() = default; protected: static void SetUpTestCase() {} diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp index dd9d058cc89..c0ffffcf1f7 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp @@ -18,6 +18,8 @@ #include #include #include +#include +#include #include #include #include @@ -26,8 +28,6 @@ #include #include -#include "dm_basic_include.h" - namespace CurrentMetrics { extern const Metric DT_SnapshotOfRead; @@ -86,14 +86,14 @@ class SegmentTest : public DB::base::TiFlashStorageTestBasic *table_columns = *columns; dm_context = std::make_unique(*db_context, - *storage_path_pool, - *storage_pool, - 0, - /*min_version_*/ 0, - settings.not_compress_columns, - false, - 1, - db_context->getSettingsRef()); + *storage_path_pool, + *storage_pool, + 0, + /*min_version_*/ 0, + settings.not_compress_columns, + false, + 1, + db_context->getSettingsRef()); } const ColumnDefinesPtr & tableColumns() const { return table_columns; } @@ -1268,7 +1268,7 @@ class SegmentTest2 : public SegmentTest { public: SegmentTest2() = default; - + void SetUp() override { diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment_common_handle.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment_common_handle.cpp index 1cc61663a2f..6359a3db184 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment_common_handle.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment_common_handle.cpp @@ -15,14 +15,13 @@ #include #include #include +#include #include #include #include #include -#include "dm_basic_include.h" - namespace DB { namespace DM diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp index 1cb735a2b65..48fafdfdd6b 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp @@ -31,6 +31,8 @@ #include #include #include +#include +#include #include #include #include @@ -41,8 +43,6 @@ #include -#include "dm_basic_include.h" - namespace DB { namespace FailPoints diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_utils.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_utils.cpp index 340dad1ff6e..26a5b1132e4 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_utils.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_utils.cpp @@ -14,10 +14,9 @@ #include #include +#include #include -#include "dm_basic_include.h" - namespace DB { namespace DM diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_version_filter.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_version_filter.cpp index 59052e2e8b2..16b1729bea1 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_version_filter.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_version_filter.cpp @@ -15,7 +15,7 @@ #include #include #include -#include +#include namespace DB { diff --git a/dbms/src/Storages/DeltaMerge/tests/stress/DMStressProxy.h b/dbms/src/Storages/DeltaMerge/tests/stress/DMStressProxy.h index eb4d937e8d4..0571eafae83 100644 --- a/dbms/src/Storages/DeltaMerge/tests/stress/DMStressProxy.h +++ b/dbms/src/Storages/DeltaMerge/tests/stress/DMStressProxy.h @@ -18,7 +18,7 @@ #include #include #include -#include +#include #include #include From 98144f34f32a52d9a377221a4c131f1cce001934 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 7 Apr 2022 15:13:59 +0800 Subject: [PATCH 3/8] Fix broken ut for relevant place index --- dbms/src/Common/FailPoint.cpp | 4 ++- dbms/src/Interpreters/IDAsPathUpgrader.cpp | 1 + dbms/src/Storages/DeltaMerge/Segment.cpp | 14 +++++++++-- .../DeltaMerge/tests/gtest_dm_segment.cpp | 25 +++++++++++-------- 4 files changed, 30 insertions(+), 14 deletions(-) diff --git a/dbms/src/Common/FailPoint.cpp b/dbms/src/Common/FailPoint.cpp index 921dd5bf748..d522ecd1bbc 100644 --- a/dbms/src/Common/FailPoint.cpp +++ b/dbms/src/Common/FailPoint.cpp @@ -84,7 +84,9 @@ std::unordered_map> FailPointHelper::f M(force_remote_read_for_batch_cop) \ M(force_context_path) \ M(force_slow_page_storage_snapshot_release) \ - M(force_change_all_blobs_to_read_only) + M(force_change_all_blobs_to_read_only) \ + M(force_enable_dt_relevant_place) \ + M(force_disable_dt_relevant_place) #define APPLY_FOR_FAILPOINTS_ONCE_WITH_CHANNEL(M) \ M(pause_with_alter_locks_acquired) \ diff --git a/dbms/src/Interpreters/IDAsPathUpgrader.cpp b/dbms/src/Interpreters/IDAsPathUpgrader.cpp index 8c807b537e9..9aa3dcb8dd0 100644 --- a/dbms/src/Interpreters/IDAsPathUpgrader.cpp +++ b/dbms/src/Interpreters/IDAsPathUpgrader.cpp @@ -487,6 +487,7 @@ bool IDAsPathUpgrader::needUpgrade() if (db_info.engine != "TiFlash") { has_old_db_engine = true; + LOG_FMT_INFO(log, "Find old style of database engine, doing upgrade [path={}] [engine={}]", database_metadata_file, db_info.engine); } } diff --git a/dbms/src/Storages/DeltaMerge/Segment.cpp b/dbms/src/Storages/DeltaMerge/Segment.cpp index 8398fdcee40..4c58a2effaf 100644 --- a/dbms/src/Storages/DeltaMerge/Segment.cpp +++ b/dbms/src/Storages/DeltaMerge/Segment.cpp @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include #include @@ -85,6 +86,11 @@ extern const Metric DT_SnapshotOfPlaceIndex; namespace DB { +namespace FailPoints +{ +extern const char force_enable_dt_relevant_place[]; +extern const char force_disable_dt_relevant_place[]; +} // namespace FailPoints namespace ErrorCodes { extern const int LOGICAL_ERROR; @@ -1480,11 +1486,15 @@ std::pair Segment::ensurePlace(const DMContext & dm_context bool relevant_place = dm_context.enable_relevant_place; bool skippable_place = dm_context.enable_skippable_place; + fiu_do_on(FailPoints::force_enable_dt_relevant_place, relevant_place = true); + fiu_do_on(FailPoints::force_disable_dt_relevant_place, relevant_place = false); + // Note that, when enable_relevant_place is false , we cannot use the range of this segment. // Because some block / delete ranges could contain some data / range that are not belong to current segment. // If we use the range of this segment as relevant_range, fully_indexed will always be false in those cases. - RowKeyRange relevant_range = relevant_place ? mergeRanges(read_ranges, is_common_handle, rowkey_column_size) - : RowKeyRange::newAll(is_common_handle, rowkey_column_size); + RowKeyRange relevant_range + = relevant_place ? mergeRanges(read_ranges, is_common_handle, rowkey_column_size) + : RowKeyRange::newAll(is_common_handle, rowkey_column_size); auto [my_placed_rows, my_placed_deletes] = my_delta_index->getPlacedStatus(); diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp index c0ffffcf1f7..65de6ccc4bf 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp @@ -40,6 +40,11 @@ extern const Metric DT_SnapshotOfPlaceIndex; namespace DB { +namespace FailPoints +{ +extern const char force_enable_dt_relevant_place[]; +extern const char force_disable_dt_relevant_place[]; +} // namespace FailPoints namespace DM { extern DMFilePtr writeIntoNewDMFile(DMContext & dm_context, // @@ -423,16 +428,10 @@ class SegmentDeletionRelevantPlaceTest : public SegmentTest , public testing::WithParamInterface { - DB::Settings getSettings() + void SetUp() override { - DB::Settings settings; - auto enable_relevant_place = GetParam(); - - if (enable_relevant_place) - settings.set("dt_enable_relevant_place", "1"); - else - settings.set("dt_enable_relevant_place", "0"); - return settings; + // init segment + SegmentTest::SetUp(); } }; @@ -440,6 +439,10 @@ class SegmentDeletionRelevantPlaceTest TEST_P(SegmentDeletionRelevantPlaceTest, ShareDelteRangeIndex) try { + Settings my_settings; + my_settings.dt_enable_relevant_place = GetParam(); // set for test + this->reload({}, std::move(my_settings)); + const size_t num_rows_write = 300; { // write to segment @@ -472,8 +475,8 @@ try auto rows1 = get_rows(RowKeyRange::fromHandleRange(HandleRange(0, 150))); auto rows2 = get_rows(RowKeyRange::fromHandleRange(HandleRange(150, 300))); - ASSERT_EQ(rows1, (size_t)100); - ASSERT_EQ(rows2, (size_t)100); + ASSERT_EQ(rows1, 100); + ASSERT_EQ(rows2, 100); } CATCH From 27952ab995625f116589f69f553aa7b90f5db794 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 1 Jun 2022 19:38:58 +0800 Subject: [PATCH 4/8] clean Signed-off-by: JaySon-Huang --- dbms/src/Common/FailPoint.cpp | 4 +-- dbms/src/Storages/DeltaMerge/Segment.cpp | 8 ------ .../DeltaMerge/tests/gtest_dm_segment.cpp | 26 ++++++++++++++----- 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/dbms/src/Common/FailPoint.cpp b/dbms/src/Common/FailPoint.cpp index d522ecd1bbc..921dd5bf748 100644 --- a/dbms/src/Common/FailPoint.cpp +++ b/dbms/src/Common/FailPoint.cpp @@ -84,9 +84,7 @@ std::unordered_map> FailPointHelper::f M(force_remote_read_for_batch_cop) \ M(force_context_path) \ M(force_slow_page_storage_snapshot_release) \ - M(force_change_all_blobs_to_read_only) \ - M(force_enable_dt_relevant_place) \ - M(force_disable_dt_relevant_place) + M(force_change_all_blobs_to_read_only) #define APPLY_FOR_FAILPOINTS_ONCE_WITH_CHANNEL(M) \ M(pause_with_alter_locks_acquired) \ diff --git a/dbms/src/Storages/DeltaMerge/Segment.cpp b/dbms/src/Storages/DeltaMerge/Segment.cpp index 4c58a2effaf..78ddd98cb48 100644 --- a/dbms/src/Storages/DeltaMerge/Segment.cpp +++ b/dbms/src/Storages/DeltaMerge/Segment.cpp @@ -86,11 +86,6 @@ extern const Metric DT_SnapshotOfPlaceIndex; namespace DB { -namespace FailPoints -{ -extern const char force_enable_dt_relevant_place[]; -extern const char force_disable_dt_relevant_place[]; -} // namespace FailPoints namespace ErrorCodes { extern const int LOGICAL_ERROR; @@ -1486,9 +1481,6 @@ std::pair Segment::ensurePlace(const DMContext & dm_context bool relevant_place = dm_context.enable_relevant_place; bool skippable_place = dm_context.enable_skippable_place; - fiu_do_on(FailPoints::force_enable_dt_relevant_place, relevant_place = true); - fiu_do_on(FailPoints::force_disable_dt_relevant_place, relevant_place = false); - // Note that, when enable_relevant_place is false , we cannot use the range of this segment. // Because some block / delete ranges could contain some data / range that are not belong to current segment. // If we use the range of this segment as relevant_range, fully_indexed will always be false in those cases. diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp index 65de6ccc4bf..33a47e30bd6 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp @@ -28,6 +28,8 @@ #include #include +#include "common/logger_useful.h" + namespace CurrentMetrics { extern const Metric DT_SnapshotOfRead; @@ -40,11 +42,6 @@ extern const Metric DT_SnapshotOfPlaceIndex; namespace DB { -namespace FailPoints -{ -extern const char force_enable_dt_relevant_place[]; -extern const char force_disable_dt_relevant_place[]; -} // namespace FailPoints namespace DM { extern DMFilePtr writeIntoNewDMFile(DMContext & dm_context, // @@ -440,7 +437,8 @@ TEST_P(SegmentDeletionRelevantPlaceTest, ShareDelteRangeIndex) try { Settings my_settings; - my_settings.dt_enable_relevant_place = GetParam(); // set for test + const auto enable_relevant_place = GetParam(); + my_settings.dt_enable_relevant_place = enable_relevant_place; // set for test this->reload({}, std::move(my_settings)); const size_t num_rows_write = 300; @@ -473,7 +471,23 @@ try // The first call of get_rows below will place the DeleteRange into delta index. auto rows1 = get_rows(RowKeyRange::fromHandleRange(HandleRange(0, 150))); + { + auto delta = segment->getDelta(); + auto placed_rows = delta->getPlacedDeltaRows(); + auto placed_deleted = delta->getPlacedDeltaDeletes(); + ASSERT_EQ(placed_rows, num_rows_write); + ASSERT_EQ(placed_deleted, enable_relevant_place ? 0 : 1); + LOG_FMT_INFO(&Poco::Logger::get("fff"), "fffffff {} {}", placed_rows, placed_deleted); + } auto rows2 = get_rows(RowKeyRange::fromHandleRange(HandleRange(150, 300))); + { + auto delta = segment->getDelta(); + auto placed_rows = delta->getPlacedDeltaRows(); + auto placed_deleted = delta->getPlacedDeltaDeletes(); + ASSERT_EQ(placed_rows, num_rows_write); + ASSERT_EQ(placed_deleted, enable_relevant_place ? 0 : 1); + LOG_FMT_INFO(&Poco::Logger::get("fff"), "fffffff {} {}", placed_rows, placed_deleted); + } ASSERT_EQ(rows1, 100); ASSERT_EQ(rows2, 100); From 35a8c2ec086d567dd49a39d229cfbeb4557d7ef9 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 1 Jun 2022 19:39:18 +0800 Subject: [PATCH 5/8] Add comment Signed-off-by: JaySon-Huang --- dbms/src/Storages/DeltaMerge/Segment.cpp | 6 ++---- .../DeltaMerge/tests/gtest_dm_segment.cpp | 15 +++++++-------- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/Segment.cpp b/dbms/src/Storages/DeltaMerge/Segment.cpp index 78ddd98cb48..8398fdcee40 100644 --- a/dbms/src/Storages/DeltaMerge/Segment.cpp +++ b/dbms/src/Storages/DeltaMerge/Segment.cpp @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include #include #include #include @@ -1484,9 +1483,8 @@ std::pair Segment::ensurePlace(const DMContext & dm_context // Note that, when enable_relevant_place is false , we cannot use the range of this segment. // Because some block / delete ranges could contain some data / range that are not belong to current segment. // If we use the range of this segment as relevant_range, fully_indexed will always be false in those cases. - RowKeyRange relevant_range - = relevant_place ? mergeRanges(read_ranges, is_common_handle, rowkey_column_size) - : RowKeyRange::newAll(is_common_handle, rowkey_column_size); + RowKeyRange relevant_range = relevant_place ? mergeRanges(read_ranges, is_common_handle, rowkey_column_size) + : RowKeyRange::newAll(is_common_handle, rowkey_column_size); auto [my_placed_rows, my_placed_deletes] = my_delta_index->getPlacedStatus(); diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp index 33a47e30bd6..56a2815920f 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp @@ -24,12 +24,11 @@ #include #include #include +#include #include #include -#include "common/logger_useful.h" - namespace CurrentMetrics { extern const Metric DT_SnapshotOfRead; @@ -438,7 +437,7 @@ try { Settings my_settings; const auto enable_relevant_place = GetParam(); - my_settings.dt_enable_relevant_place = enable_relevant_place; // set for test + my_settings.dt_enable_relevant_place = enable_relevant_place; this->reload({}, std::move(my_settings)); const size_t num_rows_write = 300; @@ -466,18 +465,19 @@ try { HandleRange remove(100, 200); - segment->write(dmContext(), {RowKeyRange::fromHandleRange(remove)}); + segment->write(dmContext(), /*delete_range*/ {RowKeyRange::fromHandleRange(remove)}); } // The first call of get_rows below will place the DeleteRange into delta index. + // If relevant place is enabled, the placed deleted in delta-tree-index is not + // pushed forward since we do not fully apply the delete range [100, 200). auto rows1 = get_rows(RowKeyRange::fromHandleRange(HandleRange(0, 150))); { auto delta = segment->getDelta(); auto placed_rows = delta->getPlacedDeltaRows(); auto placed_deleted = delta->getPlacedDeltaDeletes(); ASSERT_EQ(placed_rows, num_rows_write); - ASSERT_EQ(placed_deleted, enable_relevant_place ? 0 : 1); - LOG_FMT_INFO(&Poco::Logger::get("fff"), "fffffff {} {}", placed_rows, placed_deleted); + EXPECT_EQ(placed_deleted, enable_relevant_place ? 0 : 1); } auto rows2 = get_rows(RowKeyRange::fromHandleRange(HandleRange(150, 300))); { @@ -485,8 +485,7 @@ try auto placed_rows = delta->getPlacedDeltaRows(); auto placed_deleted = delta->getPlacedDeltaDeletes(); ASSERT_EQ(placed_rows, num_rows_write); - ASSERT_EQ(placed_deleted, enable_relevant_place ? 0 : 1); - LOG_FMT_INFO(&Poco::Logger::get("fff"), "fffffff {} {}", placed_rows, placed_deleted); + EXPECT_EQ(placed_deleted, enable_relevant_place ? 0 : 1); } ASSERT_EQ(rows1, 100); From cec3d0f843db66005042b03aaa10e827d21ae7b3 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 1 Jun 2022 20:21:50 +0800 Subject: [PATCH 6/8] clean Signed-off-by: JaySon-Huang --- dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp index 56a2815920f..efd86276172 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp @@ -424,11 +424,6 @@ class SegmentDeletionRelevantPlaceTest : public SegmentTest , public testing::WithParamInterface { - void SetUp() override - { - // init segment - SegmentTest::SetUp(); - } }; From 5c7d7df22502cc3e79a8d60c6d681f9e216443cf Mon Sep 17 00:00:00 2001 From: JaySon Date: Mon, 6 Jun 2022 11:58:06 +0800 Subject: [PATCH 7/8] Apply suggestions from code review Co-authored-by: lidezhu <47731263+lidezhu@users.noreply.github.com> --- .../src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp index efd86276172..535aa9b79ba 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp @@ -464,23 +464,23 @@ try } // The first call of get_rows below will place the DeleteRange into delta index. - // If relevant place is enabled, the placed deleted in delta-tree-index is not + // If relevant place is enabled, the placed deletes in delta-tree-index is not // pushed forward since we do not fully apply the delete range [100, 200). auto rows1 = get_rows(RowKeyRange::fromHandleRange(HandleRange(0, 150))); { auto delta = segment->getDelta(); auto placed_rows = delta->getPlacedDeltaRows(); - auto placed_deleted = delta->getPlacedDeltaDeletes(); + auto placed_deletes = delta->getPlacedDeltaDeletes(); ASSERT_EQ(placed_rows, num_rows_write); - EXPECT_EQ(placed_deleted, enable_relevant_place ? 0 : 1); + EXPECT_EQ(placed_deletes, enable_relevant_place ? 0 : 1); } auto rows2 = get_rows(RowKeyRange::fromHandleRange(HandleRange(150, 300))); { auto delta = segment->getDelta(); auto placed_rows = delta->getPlacedDeltaRows(); - auto placed_deleted = delta->getPlacedDeltaDeletes(); + auto placed_deletes = delta->getPlacedDeltaDeletes(); ASSERT_EQ(placed_rows, num_rows_write); - EXPECT_EQ(placed_deleted, enable_relevant_place ? 0 : 1); + EXPECT_EQ(placed_deletes, enable_relevant_place ? 0 : 1); } ASSERT_EQ(rows1, 100); From ad1ae75c23bb19636d05bb099ed907892bd912f6 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 6 Jun 2022 13:12:48 +0800 Subject: [PATCH 8/8] Add more check Signed-off-by: JaySon-Huang --- .../Storages/DeltaMerge/tests/gtest_dm_segment.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp index 535aa9b79ba..deec5646d33 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_segment.cpp @@ -482,9 +482,20 @@ try ASSERT_EQ(placed_rows, num_rows_write); EXPECT_EQ(placed_deletes, enable_relevant_place ? 0 : 1); } + // Query with range [0, 300) will push the placed deletes forward no matter + // relevant place is enable or not. + auto rows3 = get_rows(RowKeyRange::fromHandleRange(HandleRange(0, 300))); + { + auto delta = segment->getDelta(); + auto placed_rows = delta->getPlacedDeltaRows(); + auto placed_deletes = delta->getPlacedDeltaDeletes(); + ASSERT_EQ(placed_rows, num_rows_write); + EXPECT_EQ(placed_deletes, 1); + } ASSERT_EQ(rows1, 100); ASSERT_EQ(rows2, 100); + ASSERT_EQ(rows3, 200); } CATCH