From 8ec9f162bb682b33ae2561bca7ababee9c66fbde Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Tue, 28 Apr 2015 16:31:16 -0400 Subject: [PATCH 1/3] sstables: make read_index a private function Signed-off-by: Glauber Costa --- sstables/sstables.hh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sstables/sstables.hh b/sstables/sstables.hh index 650a3e980b..ca38ac0000 100644 --- a/sstables/sstables.hh +++ b/sstables/sstables.hh @@ -105,6 +105,10 @@ private: future read_indexes(uint64_t position, uint64_t quantity); + future read_indexes(uint64_t position) { + return read_indexes(position, _summary.header.sampling_level); + } + input_stream data_stream_at(uint64_t pos); // Read exactly the specific byte range from the data file (after // uncompression, if the file is compressed). This can be used to read @@ -144,10 +148,6 @@ public: static version_types version_from_sstring(sstring& s); static format_types format_from_sstring(sstring& s); - future read_indexes(uint64_t position) { - return read_indexes(position, _summary.header.sampling_level); - } - future<> load(); future<> store(); From 198f55dc5c915582500a84b871389a151e281cd7 Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Tue, 28 Apr 2015 16:47:41 -0400 Subject: [PATCH 2/3] sstables: don't expose summary binary search There is no need to expose binary search. It can be an internal function that is accessible for test only. Also, in the end, the implementation of the summary version was such a simple one, that there is no need to have a specific method for that. We can just pass the summary entries as a parameter. Some header file massage is needed to keep it compiling Signed-off-by: Glauber Costa --- sstables/partition.cc | 9 +++++---- sstables/sstables.hh | 4 +++- sstables/types.hh | 2 -- tests/urchin/sstable_test.cc | 18 ++++++++++++------ 4 files changed, 20 insertions(+), 13 deletions(-) diff --git a/sstables/partition.cc b/sstables/partition.cc index cf5ca09d0d..e0c4778b68 100644 --- a/sstables/partition.cc +++ b/sstables/partition.cc @@ -5,6 +5,7 @@ #include "sstables.hh" #include "types.hh" #include "core/future-util.hh" +#include "key.hh" #include "dht/i_partitioner.hh" @@ -24,7 +25,7 @@ namespace sstables { // This code should work in all kinds of vectors in whose's elements is possible to aquire // a key view. template -int binary_search(const T& entries, const key& sk) { +int sstable::binary_search(const T& entries, const key& sk) { int low = 0, mid = entries.size(), high = mid - 1, result = -1; auto& partitioner = dht::global_partitioner(); @@ -58,7 +59,7 @@ int binary_search(const T& entries, const key& sk) { return -mid - (result < 0 ? 1 : 2); } -int summary::binary_search(const key& sk) { - return sstables::binary_search(entries, sk); -} +// Force generation, so we make it available outside this compilation unit without moving that +// much code to .hh +template int sstable::binary_search<>(const std::vector& entries, const key& sk); } diff --git a/sstables/sstables.hh b/sstables/sstables.hh index ca38ac0000..c0202fd340 100644 --- a/sstables/sstables.hh +++ b/sstables/sstables.hh @@ -19,6 +19,7 @@ #include "row.hh" namespace sstables { +class key; class malformed_sstable_exception : public std::exception { sstring _msg; @@ -118,7 +119,8 @@ private: // for iteration through all the rows. future> data_read(uint64_t pos, size_t len); - + template + int binary_search(const T& entries, const key& sk); public: // Read one or few rows at the given byte range from the data file, // feeding them into the consumer. This function reads the entire given diff --git a/sstables/types.hh b/sstables/types.hh index ae2d5c99bf..ed4350996b 100644 --- a/sstables/types.hh +++ b/sstables/types.hh @@ -2,7 +2,6 @@ #include "core/enum.hh" #include "bytes.hh" -#include "sstables/key.hh" namespace sstables { @@ -115,7 +114,6 @@ struct summary_la { // filled with the same data. It's too early to judge that the data is useless. // However, it was tested that Cassandra loads successfully a Summary file with // this structure removed from it. Anyway, let's pay attention to it. - int binary_search(const key& sk); }; using summary = summary_la; diff --git a/tests/urchin/sstable_test.cc b/tests/urchin/sstable_test.cc index d7f1b723b0..fa5cdb385f 100644 --- a/tests/urchin/sstable_test.cc +++ b/tests/urchin/sstable_test.cc @@ -11,7 +11,9 @@ #include "core/align.hh" #include "core/do_with.hh" #include "sstables/sstables.hh" +#include "sstables/key.hh" #include "tests/test-utils.hh" +#include "schema.hh" #include using namespace sstables; @@ -68,6 +70,10 @@ public: return _sst->_components; } + template + int binary_search(const T& entries, const key& sk) { + return _sst->binary_search(entries, sk); + } }; } @@ -773,7 +779,7 @@ SEASTAR_TEST_CASE(find_key_map) { kk.push_back(map); auto key = sstables::key::from_deeply_exploded(*s, kk); - BOOST_REQUIRE(summary.binary_search(key) == 0); + BOOST_REQUIRE(sstables::test(sstp).binary_search(summary.entries, key) == 0); }); } @@ -793,7 +799,7 @@ SEASTAR_TEST_CASE(find_key_set) { kk.push_back(set); auto key = sstables::key::from_deeply_exploded(*s, kk); - BOOST_REQUIRE(summary.binary_search(key) == 0); + BOOST_REQUIRE(sstables::test(sstp).binary_search(summary.entries, key) == 0); }); } @@ -813,7 +819,7 @@ SEASTAR_TEST_CASE(find_key_list) { kk.push_back(list); auto key = sstables::key::from_deeply_exploded(*s, kk); - BOOST_REQUIRE(summary.binary_search(key) == 0); + BOOST_REQUIRE(sstables::test(sstp).binary_search(summary.entries, key) == 0); }); } @@ -831,7 +837,7 @@ SEASTAR_TEST_CASE(find_key_composite) { kk.push_back(boost::any(b2)); auto key = sstables::key::from_deeply_exploded(*s, kk); - BOOST_REQUIRE(summary.binary_search(key) == 0); + BOOST_REQUIRE(sstables::test(sstp).binary_search(summary.entries, key) == 0); }); } @@ -842,7 +848,7 @@ SEASTAR_TEST_CASE(all_in_place) { int idx = 0; for (auto& e: summary.entries) { auto key = sstables::key::from_bytes(e.key); - BOOST_REQUIRE(summary.binary_search(key) == idx++); + BOOST_REQUIRE(sstables::test(sstp).binary_search(summary.entries, key) == idx++); } }); } @@ -861,6 +867,6 @@ SEASTAR_TEST_CASE(not_find_key_composite_bucket0) { auto key = sstables::key::from_deeply_exploded(*s, kk); // (result + 1) * -1 -1 = 0 - BOOST_REQUIRE(summary.binary_search(key) == -2); + BOOST_REQUIRE(sstables::test(sstp).binary_search(summary.entries, key) == -2); }); } From 34d099dfbf29f2f37ade7524a2534a2bbbc152cd Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Tue, 28 Apr 2015 17:29:25 -0400 Subject: [PATCH 3/3] sstables: make read_summary_entry private Signed-off-by: Glauber Costa --- sstables/sstables.hh | 4 ++-- tests/urchin/sstable_test.cc | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/sstables/sstables.hh b/sstables/sstables.hh index c0202fd340..3ea0eb707a 100644 --- a/sstables/sstables.hh +++ b/sstables/sstables.hh @@ -121,6 +121,8 @@ private: template int binary_search(const T& entries, const key& sk); + + future read_summary_entry(size_t i); public: // Read one or few rows at the given byte range from the data file, // feeding them into the consumer. This function reads the entire given @@ -153,8 +155,6 @@ public: future<> load(); future<> store(); - future read_summary_entry(size_t i); - void set_generation(unsigned long generation) { _generation = generation; } // Allow the test cases from sstable_test.cc to test private methods. We use diff --git a/tests/urchin/sstable_test.cc b/tests/urchin/sstable_test.cc index fa5cdb385f..e122f3d2c0 100644 --- a/tests/urchin/sstable_test.cc +++ b/tests/urchin/sstable_test.cc @@ -58,6 +58,10 @@ public: return _sst->read_summary(); } + future read_summary_entry(size_t i) { + return _sst->read_summary_entry(i); + } + summary& get_summary() { return _sst->_summary; } @@ -207,7 +211,7 @@ SEASTAR_TEST_CASE(composite_index_read_0_21_20) { template future<> summary_query(sstring path, int generation) { return reusable_sst(path, generation).then([] (sstable_ptr ptr) { - return ptr->read_summary_entry(Position).then([ptr] (auto entry) { + return sstables::test(ptr).read_summary_entry(Position).then([ptr] (auto entry) { BOOST_REQUIRE(entry.position == EntryPosition); BOOST_REQUIRE(entry.key.size() == EntryKeySize); return make_ready_future<>();