From 990b52e95b3b2d7bac72ef468a3f5665497145c3 Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Tue, 18 Sep 2018 12:06:59 -0700 Subject: [PATCH] Unit test for custom comparator RangeDelAggregator (#4388) Summary: Add a unit test for range collapsing when non-default comparator is used. This exposes the bug fixed in #4386. Pull Request resolved: https://github.com/facebook/rocksdb/pull/4388 Differential Revision: D9918252 Pulled By: ajkr fbshipit-source-id: 99501b96b251eab41791a7e33b27055ee36c5c39 --- db/range_del_aggregator.cc | 2 +- db/range_del_aggregator_test.cc | 21 +++++++++++++++------ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/db/range_del_aggregator.cc b/db/range_del_aggregator.cc index e343cf611..1854377e4 100644 --- a/db/range_del_aggregator.cc +++ b/db/range_del_aggregator.cc @@ -173,7 +173,7 @@ class CollapsedRangeDelMap : public RangeDelMap { const Comparator* ucmp_; public: - CollapsedRangeDelMap(const Comparator* ucmp) + explicit CollapsedRangeDelMap(const Comparator* ucmp) : rep_(stl_wrappers::LessOfComparator(ucmp)), ucmp_(ucmp) { InvalidatePosition(); diff --git a/db/range_del_aggregator_test.cc b/db/range_del_aggregator_test.cc index ab18f6d29..16b68b7c4 100644 --- a/db/range_del_aggregator_test.cc +++ b/db/range_del_aggregator_test.cc @@ -27,7 +27,7 @@ enum Direction { kReverse, }; -static auto icmp = InternalKeyComparator(BytewiseComparator()); +static auto bytewise_icmp = InternalKeyComparator(BytewiseComparator()); void AddTombstones(RangeDelAggregator* range_del_agg, const std::vector& range_dels, @@ -66,8 +66,8 @@ void VerifyRangeDels( const std::vector& range_dels_in, const std::vector& expected_points, const std::vector& expected_collapsed_range_dels, - const InternalKey* smallest = nullptr, - const InternalKey* largest = nullptr) { + const InternalKey* smallest = nullptr, const InternalKey* largest = nullptr, + const InternalKeyComparator& icmp = bytewise_icmp) { // Test same result regardless of which order the range deletions are added // and regardless of collapsed mode. for (bool collapsed : {false, true}) { @@ -164,6 +164,14 @@ TEST_F(RangeDelAggregatorTest, OverlapAboveMiddle) { {{"a", "b", 5}, {"b", "c", 10}, {"c", "d", 5}}); } +TEST_F(RangeDelAggregatorTest, OverlapAboveMiddleReverse) { + VerifyRangeDels({{"d", "a", 5}, {"c", "b", 10}}, + {{"z", 0}, {"d", 5}, {"c", 10}, {"b", 5}, {"a", 0}}, + {{"d", "c", 5}, {"c", "b", 10}, {"b", "a", 5}}, + nullptr /* smallest */, nullptr /* largest */, + InternalKeyComparator(ReverseBytewiseComparator())); +} + TEST_F(RangeDelAggregatorTest, OverlapFully) { VerifyRangeDels({{"a", "d", 10}, {"b", "c", 5}}, {{" ", 0}, {"a", 10}, {"d", 0}}, {{"a", "d", 10}}); @@ -235,14 +243,14 @@ TEST_F(RangeDelAggregatorTest, AlternateMultipleAboveBelow) { TEST_F(RangeDelAggregatorTest, MergingIteratorAllEmptyStripes) { for (bool collapsed : {true, false}) { - RangeDelAggregator range_del_agg(icmp, {1, 2}, collapsed); + RangeDelAggregator range_del_agg(bytewise_icmp, {1, 2}, collapsed); VerifyRangeDelIter(range_del_agg.NewIterator().get(), {}); } } TEST_F(RangeDelAggregatorTest, MergingIteratorOverlappingStripes) { for (bool collapsed : {true, false}) { - RangeDelAggregator range_del_agg(icmp, {5, 15, 25, 35}, collapsed); + RangeDelAggregator range_del_agg(bytewise_icmp, {5, 15, 25, 35}, collapsed); AddTombstones( &range_del_agg, {{"d", "e", 10}, {"aa", "b", 20}, {"c", "d", 30}, {"a", "b", 10}}); @@ -253,7 +261,8 @@ TEST_F(RangeDelAggregatorTest, MergingIteratorOverlappingStripes) { } TEST_F(RangeDelAggregatorTest, MergingIteratorSeek) { - RangeDelAggregator range_del_agg(icmp, {5, 15}, true /* collapsed */); + RangeDelAggregator range_del_agg(bytewise_icmp, {5, 15}, + true /* collapsed */); AddTombstones(&range_del_agg, {{"a", "c", 10}, {"b", "c", 11}, {"f", "g", 10},