From 4ae502eaa6e2b00789fdfd5c058d760f00af0f75 Mon Sep 17 00:00:00 2001 From: alesapin Date: Thu, 20 Jun 2019 11:48:56 +0300 Subject: [PATCH] Fix bug with wrong granularity detection --- .../MergeTree/IMergedBlockOutputStream.cpp | 11 ++++++----- .../MergeTree/IMergedBlockOutputStream.h | 1 - .../gtest_aux_funcs_for_adaptive_granularity.cpp | 16 ++++++++-------- .../00152_insert_different_granularity.reference | 1 - 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.cpp b/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.cpp index e2a63fc6d4b..f87ab634224 100644 --- a/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.cpp +++ b/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.cpp @@ -28,7 +28,6 @@ IMergedBlockOutputStream::IMergedBlockOutputStream( , max_compress_block_size(max_compress_block_size_) , aio_threshold(aio_threshold_) , marks_file_extension(storage.canUseAdaptiveGranularity() ? getAdaptiveMrkExtension() : getNonAdaptiveMrkExtension()) - , mark_size_in_bytes(storage.canUseAdaptiveGranularity() ? getAdaptiveMrkSize() : getNonAdaptiveMrkSize()) , blocks_are_granules_size(blocks_are_granules_size_) , index_granularity(index_granularity_) , compute_granularity(index_granularity.empty()) @@ -99,11 +98,12 @@ void fillIndexGranularityImpl( size_t fixed_index_granularity_rows, bool blocks_are_granules, size_t index_offset, - MergeTreeIndexGranularity & index_granularity) + MergeTreeIndexGranularity & index_granularity, + bool can_use_adaptive_index_granularity) { size_t rows_in_block = block.rows(); size_t index_granularity_for_block; - if (index_granularity_bytes == 0) + if (!can_use_adaptive_index_granularity) index_granularity_for_block = fixed_index_granularity_rows; else { @@ -139,7 +139,8 @@ void IMergedBlockOutputStream::fillIndexGranularity(const Block & block) storage.settings.index_granularity, blocks_are_granules_size, index_offset, - index_granularity); + index_granularity, + storage.canUseAdaptiveGranularity()); } void IMergedBlockOutputStream::writeSingleMark( @@ -170,7 +171,7 @@ void IMergedBlockOutputStream::writeSingleMark( writeIntBinary(stream.plain_hashing.count(), stream.marks); writeIntBinary(stream.compressed.offset(), stream.marks); - if (storage.settings.index_granularity_bytes) + if (storage.canUseAdaptiveGranularity()) writeIntBinary(number_of_rows, stream.marks); }, path); } diff --git a/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.h b/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.h index 995e7567fa9..b9d083f3a19 100644 --- a/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.h +++ b/dbms/src/Storages/MergeTree/IMergedBlockOutputStream.h @@ -134,7 +134,6 @@ protected: size_t current_mark = 0; const std::string marks_file_extension; - const size_t mark_size_in_bytes; const bool blocks_are_granules_size; MergeTreeIndexGranularity index_granularity; diff --git a/dbms/src/Storages/tests/gtest_aux_funcs_for_adaptive_granularity.cpp b/dbms/src/Storages/tests/gtest_aux_funcs_for_adaptive_granularity.cpp index fc1166b18bb..85df83be2de 100644 --- a/dbms/src/Storages/tests/gtest_aux_funcs_for_adaptive_granularity.cpp +++ b/dbms/src/Storages/tests/gtest_aux_funcs_for_adaptive_granularity.cpp @@ -30,14 +30,14 @@ TEST(AdaptiveIndexGranularity, FillGranularityToyTests) EXPECT_EQ(block1.bytes(), 80); { /// Granularity bytes are not set. Take default index_granularity. MergeTreeIndexGranularity index_granularity; - fillIndexGranularityImpl(block1, 0, 100, false, 0, index_granularity); + fillIndexGranularityImpl(block1, 0, 100, false, 0, index_granularity, false); EXPECT_EQ(index_granularity.getMarksCount(), 1); EXPECT_EQ(index_granularity.getMarkRows(0), 100); } { /// Granule size is less than block size. Block contains multiple granules. MergeTreeIndexGranularity index_granularity; - fillIndexGranularityImpl(block1, 16, 100, false, 0, index_granularity); + fillIndexGranularityImpl(block1, 16, 100, false, 0, index_granularity, true); EXPECT_EQ(index_granularity.getMarksCount(), 5); /// First granule with 8 rows, and second with 1 row for (size_t i = 0; i < index_granularity.getMarksCount(); ++i) EXPECT_EQ(index_granularity.getMarkRows(i), 2); @@ -46,7 +46,7 @@ TEST(AdaptiveIndexGranularity, FillGranularityToyTests) { /// Granule size is more than block size. Whole block (and maybe more) can be placed in single granule. MergeTreeIndexGranularity index_granularity; - fillIndexGranularityImpl(block1, 512, 100, false, 0, index_granularity); + fillIndexGranularityImpl(block1, 512, 100, false, 0, index_granularity, true); EXPECT_EQ(index_granularity.getMarksCount(), 1); for (size_t i = 0; i < index_granularity.getMarksCount(); ++i) EXPECT_EQ(index_granularity.getMarkRows(i), 64); @@ -55,7 +55,7 @@ TEST(AdaptiveIndexGranularity, FillGranularityToyTests) { /// Blocks with granule size MergeTreeIndexGranularity index_granularity; - fillIndexGranularityImpl(block1, 1, 100, true, 0, index_granularity); + fillIndexGranularityImpl(block1, 1, 100, true, 0, index_granularity, true); EXPECT_EQ(index_granularity.getMarksCount(), 1); for (size_t i = 0; i < index_granularity.getMarksCount(); ++i) EXPECT_EQ(index_granularity.getMarkRows(i), block1.rows()); @@ -63,7 +63,7 @@ TEST(AdaptiveIndexGranularity, FillGranularityToyTests) { /// Shift in index offset MergeTreeIndexGranularity index_granularity; - fillIndexGranularityImpl(block1, 16, 100, false, 6, index_granularity); + fillIndexGranularityImpl(block1, 16, 100, false, 6, index_granularity, true); EXPECT_EQ(index_granularity.getMarksCount(), 2); for (size_t i = 0; i < index_granularity.getMarksCount(); ++i) EXPECT_EQ(index_granularity.getMarkRows(i), 2); @@ -79,7 +79,7 @@ TEST(AdaptiveIndexGranularity, FillGranularitySequenceOfBlocks) auto block3 = getBlockWithSize(65536, 8); MergeTreeIndexGranularity index_granularity; for (const auto & block : {block1, block2, block3}) - fillIndexGranularityImpl(block, 1024, 8192, false, 0, index_granularity); + fillIndexGranularityImpl(block, 1024, 8192, false, 0, index_granularity, true); EXPECT_EQ(index_granularity.getMarksCount(), 192); /// granules for (size_t i = 0; i < index_granularity.getMarksCount(); ++i) @@ -92,7 +92,7 @@ TEST(AdaptiveIndexGranularity, FillGranularitySequenceOfBlocks) EXPECT_EQ(block1.rows() + block2.rows() + block3.rows(), 3136); MergeTreeIndexGranularity index_granularity; for (const auto & block : {block1, block2, block3}) - fillIndexGranularityImpl(block, 1024, 8192, false, 0, index_granularity); + fillIndexGranularityImpl(block, 1024, 8192, false, 0, index_granularity, true); EXPECT_EQ(index_granularity.getMarksCount(), 98); /// granules for (size_t i = 0; i < index_granularity.getMarksCount(); ++i) @@ -110,7 +110,7 @@ TEST(AdaptiveIndexGranularity, FillGranularitySequenceOfBlocks) size_t index_offset = 0; for (const auto & block : {block1, block2, block3}) { - fillIndexGranularityImpl(block, 16384, 8192, false, index_offset, index_granularity); + fillIndexGranularityImpl(block, 16384, 8192, false, index_offset, index_granularity, true); index_offset = index_granularity.getLastMarkRows() - block.rows(); } EXPECT_EQ(index_granularity.getMarksCount(), 1); /// granules diff --git a/dbms/tests/queries/1_stateful/00152_insert_different_granularity.reference b/dbms/tests/queries/1_stateful/00152_insert_different_granularity.reference index d548ab089d0..c573f1c3072 100644 --- a/dbms/tests/queries/1_stateful/00152_insert_different_granularity.reference +++ b/dbms/tests/queries/1_stateful/00152_insert_different_granularity.reference @@ -1,3 +1,2 @@ 8873918 -8873898 8873998