Rust-LSM-KV icon indicating copy to clipboard operation
Rust-LSM-KV copied to clipboard

Spec feedback

Open tov opened this issue 5 years ago • 1 comments

Thanks for submitting your spec. It’s a good start, but it’s missing a lot of detail. First, in order to describe the API, you should be describing it in Rust, since that will include important details that otherwise aren’t mentioned. So it should look like:

#[derive(Debug)]  // Does Clone make sense?
struct LsmMap<K, V> { … }

pub trait LsmValue : Serialize + Deserialize { }
impl<T: Serialize + Deserialize> LsmValue for T { }

pub trait LsmKey : Eq + Ord + LsmValue { }
impl<T: Eq + Ord + LsmValue> LsmKey for T { }

impl<K, V> LsmMap<K, V>
where
    K: LsmKey,
    V: LsmValue
{
    pub fn new(path: &Path) -> io::Result<Self> { … }
    pub fn open(path: &Path, options: &std::fs::OpenOptions) -> io::Result<Self> { … }
    pub fn flush(&self) -> io::Result<()> { … }
}

impl<'a, K, V, Q> std::ops::Index<&'a Q> for LsmMap<K, V>
where
    K: LsmKey + Borrow<Q>,
    V: LsmValue,
    Q: Eq + Ord
{
    type Output = V;
    …
}

impl<'a, K, V, Q> std::ops::IndexMut<&'a Q> for LsmMap<K, V>
where
    K: LsmKey + Borrow<Q>,
    V: LsmValue,
    Q: Eq + Ord { … }

That would also help me understand some of these operations that currently I don’t:

deconstructor() search_buffer(key) search_disk(key)

What are the actual signatures?

I’m also wondering—is the Bloom filter part of your public API? Or is it an implementation detail? It seems like the latter, but you listed it like the former.

As far as tests and examples go, you need to give me code! It‘s really not that hard, and it makes things much clearer. Here’s something I would imagine should work:

#[test]
fn persists_across_close() {
    let filename = Path::new("persists_across_close.dat");

    {
        let mut map = LsmMap::new(filename)?;
        map["one"] = 1;
        map["two"] = 2;
        map["three"] = 3;
        map.remove("two");
    } // map is closed here

    let map = LsmMap::open(filename, OpenOptions::default())?;
    assert_eq!( map.get(&"one"), Some(&1) );
    assert_eq!( map.get(&"two"), None );
    assert_eq!( map.get(&"three"), Some(&3) );
}

Because there’s not enough detail here, this is a .

tov avatar Mar 02 '20 19:03 tov

Thanks for your advice! We will update our specification later.

brucechin avatar Mar 02 '20 21:03 brucechin