Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spec feedback #1

Open
tov opened this issue Mar 2, 2020 · 1 comment
Open

Spec feedback #1

tov opened this issue Mar 2, 2020 · 1 comment

Comments

@tov
Copy link

tov commented Mar 2, 2020

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 .

@brucechin
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants