fix a bug in btree lookup

btree.h

@@ -97,6 +97,7 @@ namespace persistent_data {
 			void set_max_entries(); // calculates the max for you.
 
 			size_t get_value_size() const;
+			void set_value_size(size_t);
 
 			uint64_t key_at(unsigned i) const;
 			void set_key(unsigned i, uint64_t k);
@@ -258,30 +259,6 @@ namespace persistent_data {
 			std::list<block_manager<>::write_ref> spine_;
 			block_address root_;
 		};
-
-		// FIXME: make a member of btree
-		template <typename ValueTraits>
-		optional<typename ValueTraits::value_type>
-		lookup_raw(ro_spine &spine, block_address block, uint64_t key) {
-
-			using namespace boost;
-			typedef typename ValueTraits::value_type leaf_type;
-
-			for (;;) {
-				spine.step(block);
-				node_ref<ValueTraits> leaf = spine.template get_node<ValueTraits>();
-
-				optional<unsigned> mi = leaf.exact_search(key);
-				if (!mi)
-					return optional<leaf_type>();
-
-				if (leaf.get_type() == btree_detail::LEAF)
-					return optional<leaf_type>(leaf.value_at(*mi));
-
-				node_ref<uint64_traits> internal = spine.template get_node<uint64_traits>();
-				block = internal.value_at(*mi);
-			}
-		}
 	}
 
 	template <unsigned Levels, typename ValueTraits>
@@ -346,6 +323,10 @@ namespace persistent_data {
 		void visit(typename visitor::ptr visitor) const;
 
 	private:
+		template <typename ValueTraits2, typename Search>
+		optional<typename ValueTraits2::value_type>
+		lookup_raw(btree_detail::ro_spine &spine, block_address block, uint64_t key) const;
+
 		template <typename ValueTraits2>
 		void split_node(btree_detail::shadow_spine &spine,
 				block_address parent_index,

btree.tcc

@@ -102,6 +102,13 @@ node_ref<ValueTraits>::get_value_size() const
 	return to_cpu<uint32_t>(raw_->header.value_size);
 }
 
+template <typename ValueTraits>
+void
+node_ref<ValueTraits>::set_value_size(size_t s)
+{
+	raw_->header.value_size = to_disk<__le32>(static_cast<uint32_t>(s));
+}
+
 template <typename ValueTraits>
 uint64_t
 node_ref<ValueTraits>::key_at(unsigned i) const
@@ -181,9 +188,9 @@ node_ref<ValueTraits>::copy_entries(node_ref const &rhs,
 	if ((n + count) > get_max_entries())
 		throw runtime_error("too many entries");
 
-	set_nr_entries(n + count);
 	::memcpy(key_ptr(n), rhs.key_ptr(begin), sizeof(uint64_t) * count);
 	::memcpy(value_ptr(n), rhs.value_ptr(begin), sizeof(typename ValueTraits::disk_type) * count);
+	set_nr_entries(n + count);
 }
 
 template <typename ValueTraits>
@@ -216,6 +223,9 @@ node_ref<ValueTraits>::exact_search(uint64_t key) const
 	if (i < 0 || static_cast<unsigned>(i) >= get_nr_entries())
 		return optional<unsigned>();
 
+	if (key != key_at(i))
+		return optional<unsigned>();
+
 	return optional<unsigned>(i);
 }
 
@@ -287,6 +297,7 @@ btree(typename transaction_manager::ptr tm,
 	n.set_type(btree_detail::LEAF);
 	n.set_nr_entries(0);
 	n.set_max_entries();
+	n.set_value_size(sizeof(typename ValueTraits::disk_type));
 
 	root_ = root.get_location();
 }
@@ -309,6 +320,22 @@ btree<Levels, ValueTraits>::~btree()
 
 }
 
+namespace {
+	template <typename ValueTraits>
+	struct lower_bound_search {
+		static optional<unsigned> search(btree_detail::node_ref<ValueTraits> n, uint64_t key) {
+			return n.lower_bound(key);
+		}
+	};
+
+	template <typename ValueTraits>
+	struct exact_search {
+		static optional<unsigned> search(btree_detail::node_ref<ValueTraits> n, uint64_t key) {
+			return n.exact_search(key);
+		}
+	};
+}
+
 template <unsigned Levels, typename ValueTraits>
 typename btree<Levels, ValueTraits>::maybe_value
 btree<Levels, ValueTraits>::lookup(key const &key) const
@@ -320,14 +347,14 @@ btree<Levels, ValueTraits>::lookup(key const &key) const
 
         for (unsigned level = 0; level < Levels - 1; ++level) {
 		optional<block_address> mroot =
-			lookup_raw<uint64_traits>(spine, root, key[level]);
+			lookup_raw<uint64_traits, lower_bound_search<uint64_traits> >(spine, root, key[level]);
 		if (!mroot)
 			return maybe_value();
 
 		root = *mroot;
         }
 
-	return lookup_raw<ValueTraits>(spine, root, key[Levels - 1]);
+	return lookup_raw<ValueTraits, exact_search<ValueTraits> >(spine, root, key[Levels - 1]);
 }
 
 template <unsigned Levels, typename ValueTraits>
@@ -422,6 +449,38 @@ btree<Levels, ValueTraits>::destroy()
 }
 #endif
 
+template <unsigned Levels, typename _>
+template <typename ValueTraits, typename Search>
+optional<typename ValueTraits::value_type>
+btree<Levels, _>::
+lookup_raw(ro_spine &spine, block_address block, uint64_t key) const
+{
+	using namespace boost;
+	typedef typename ValueTraits::value_type leaf_type;
+
+	for (;;) {
+		spine.step(block);
+		node_ref<ValueTraits> leaf = spine.template get_node<ValueTraits>();
+
+		optional<unsigned> mi;
+		if (leaf.get_type() == btree_detail::LEAF) {
+			mi = Search::search(leaf, key);
+			if (!mi)
+				return optional<leaf_type>();
+			return optional<leaf_type>(leaf.value_at(*mi));
+
+		}
+
+		mi = leaf.lower_bound(key);
+		if (!mi || *mi < 0)
+			return optional<leaf_type>();
+
+		node_ref<uint64_traits> internal = spine.template get_node<uint64_traits>();
+		block = internal.value_at(*mi);
+	}
+}
+
+
 template <unsigned Levels, typename _>
 template <typename ValueTraits>
 void
@@ -456,14 +515,20 @@ split_beneath(btree_detail::shadow_spine &spine,
 	node_ref<ValueTraits> l = to_node<ValueTraits>(left);
 	l.set_nr_entries(0);
 	l.set_max_entries();
+	l.set_value_size(sizeof(typename ValueTraits::disk_type));
 
 	write_ref right = tm_->new_block();
 	node_ref<ValueTraits> r = to_node<ValueTraits>(right);
 	r.set_nr_entries(0);
 	r.set_max_entries();
+	r.set_value_size(sizeof(typename ValueTraits::disk_type));
 
 	{
 		node_ref<ValueTraits> p = spine.template get_node<ValueTraits>();
+
+		if (p.get_value_size() != sizeof(typename ValueTraits::disk_type))
+			throw std::runtime_error("bad value_size");
+
 		nr_left = p.get_nr_entries() / 2;
 		nr_right = p.get_nr_entries() - nr_left;
 		type = p.get_type();
@@ -480,8 +545,8 @@ split_beneath(btree_detail::shadow_spine &spine,
 		internal_node p = spine.template get_node<uint64_traits>();
 		p.set_type(btree_detail::INTERNAL);
 		p.set_nr_entries(2);
+		p.set_value_size(sizeof(typename uint64_traits::disk_type));
 
-		// FIXME: set the value_size
 		p.overwrite_at(0, l.key_at(0), left.get_location());
 		p.overwrite_at(1, r.key_at(0), right.get_location());
 	}

unit-tests/btree_t.cc

@@ -1,5 +1,5 @@
 #include "transaction_manager.h"
-#include "core_map.h"
+#include "space_map_core.h"
 #include "btree.h"
 
 #define BOOST_TEST_MODULE BTreeTests
@@ -16,7 +16,7 @@ namespace {
 
 	transaction_manager::ptr
 	create_tm() {
-		block_manager<>::ptr bm(new block_manager<>("./test.data", NR_BLOCKS));
+		block_manager<>::ptr bm(new block_manager<>("./test.data", NR_BLOCKS, 4, true));
 		space_map::ptr sm(new core_map(NR_BLOCKS));
 		transaction_manager::ptr tm(new transaction_manager(bm, sm));
 		return tm;
@@ -36,17 +36,26 @@ namespace {
 	//
 	class constraint_visitor : public btree<1, uint64_traits>::visitor {
 	public:
-	  bool visit_internal(unsigned level, bool is_root, btree_detail::node_ref<uint64_traits> const &n) {
+		typedef btree_detail::node_ref<uint64_traits> internal_node;
+		typedef btree_detail::node_ref<uint64_traits> leaf_node;
+
+		bool visit_internal(unsigned level, bool sub_root,
+				    boost::optional<uint64_t> key,
+				    internal_node const &n) {
 			check_duplicate_block(n.get_location());
 			return true;
 		}
 
-	  bool visit_internal_leaf(unsigned level, bool is_root, btree_detail::node_ref<uint64_traits> const &n) {
+	  bool visit_internal_leaf(unsigned level, bool sub_root,
+				   boost::optional<uint64_t> key,
+				   internal_node const &n) {
 			check_duplicate_block(n.get_location());
 			return true;
 		}
 
-	  bool visit_leaf(unsigned level, bool is_root, btree_detail::node_ref<uint64_traits> const &n) {
+	  bool visit_leaf(unsigned level, bool sub_root,
+			  boost::optional<uint64_t> key,
+			  leaf_node const &n) {
 			check_duplicate_block(n.get_location());
 			return true;
 		}
@@ -77,7 +86,7 @@ namespace {
 
 BOOST_AUTO_TEST_CASE(empty_btree_contains_nothing)
 {
-	auto tree = create_btree();
+	btree<1, uint64_traits>::ptr tree = create_btree();
 	check_constraints(tree);
 
 	for (uint64_t i = 0; i < 1000; i++) {
@@ -90,19 +99,46 @@ BOOST_AUTO_TEST_CASE(insert_works)
 {
 	unsigned const COUNT = 100000;
 
-	auto tree = create_btree();
+	btree<1, uint64_traits>::ptr tree = create_btree();
 	for (uint64_t i = 0; i < COUNT; i++) {
 		uint64_t key[1] = {i * 7};
 		uint64_t value = i;
 
 		tree->insert(key, value);
 
-		auto l = tree->lookup(key);
+		btree<1, uint64_traits>::maybe_value l = tree->lookup(key);
-		BOOST_CHECK(l);
+		BOOST_REQUIRE(l);
 		BOOST_CHECK_EQUAL(*l, i);
 	}
 
 	check_constraints(tree);
 }
 
+BOOST_AUTO_TEST_CASE(insert_does_not_insert_imaginary_values)
+{
+	btree<1, uint64_traits>::ptr tree = create_btree();
+	uint64_t key[1] = {0};
+	uint64_t value = 100;
+
+	btree<1, uint64_traits>::maybe_value l = tree->lookup(key);
+	BOOST_CHECK(!l);
+
+	key[0] = 1;
+	l = tree->lookup(key);
+	BOOST_CHECK(!l);
+
+	key[0] = 0;
+	tree->insert(key, value);
+
+	l = tree->lookup(key);
+	BOOST_REQUIRE(l);
+	BOOST_CHECK_EQUAL(*l, 100);
+
+	key[0] = 1;
+	l = tree->lookup(key);
+	BOOST_CHECK(!l);
+
+	check_constraints(tree);
+}
+
 //----------------------------------------------------------------