Add support for opening a column family with options

[garyttierney]

I've hit an issue when I create a column family with a custom compare function and then try to
re-open the same database. Since I can't pass in options to open_cf, my existing column familes
are opened with the default options and I get an error.

This adds a new DB::open_cf_descriptors method that allows passing in Options for
each column family, and a new ColumnFamilyDescriptor type was added to contain
the congfiguration for each column family.

[vmx]

This change is similar to #130. I might even like this one better. Though I would make it a breaking change and change open_cf_descriptors() to just open_cf().

[garyttierney]

Apologies @vmx, I completely missed your open pull request or I'd have pulled that instead! I'd be happy to update this to be a breaking change if you'd rather go with this PR over your own :).

[vmx]

It would be great to get feedback from others, especially @spacejam on which approach he prefers and if he agrees with me that this should be a breaking change or not.

[spacejam]

I like coupling column family identifiers to their options, reducing the chance of messing up the order by users. I see it being relatively inexpensive in maintenance costs to keep open_cf_descriptors as well as open_cf and it's nice that this doesn't break existing code. Maybe if more people feel strongly about there being one, we can merge this as-is for now, and open an issue for collapsing them into a single endpoint in the next breaking release? I'd like to get more people's support for that kind of breaking change I think, since it's frustrating to have to go back and change existing code for users.

[spacejam]

Could you please add a test exercising the new open_cf_descriptors method?

[spacejam]

Could you please add a short comment describing this public struct?

[vmx]

Would it make sense to use ColumnFamilyDescriptor::new(name, Options::defaut()) instead?

[vmx]

@spacejam My point about making this one the open_cf() is that I don't really see a use case for the original function. As soon as you need any options you can't use it. And at least I use column families especially because i needed different options.

So I'm in favour for keeping the API as small as possible, especially since we are before 1.0.

[garyttierney]

I've made the requested changes and added some crate level documentation on ColumnFamilyDescriptors.

With regards to folding open_cf_descriptors into open_cf, an automatic conversion from &str to a ColumnFamilyDescriptor with default options could be done to keep backwards compatibility I think:

pub fn open_cf<P, C>(opts: &Options, path: P, cfs: &[C]) -> Result<DB, Error>
       where P: AsRef<Path>, C: Into<ColumnFamilyDescriptor> {}

impl From<&str> for ColumnFamilyDescriptor {
    fn from(name: &str) -> Self {
        ColumnFamilyDescriptor::new(name, Options::default())
    }
}

Though, I think in most cases when you want to open a ColumnFamily you also want to define a set of per-cf options.

[spacejam]

@garyttierney I like the idea of using an automatic conversion to support the new use case while preserving backwards compatibility, and reduce API bloat. The implementation may require a couple more lifetimes, but it's feasible. @vmx what do you think about this approach?

[vmx]

That sounds like a good solution, I wasn't even aware that things like that are possible with Rust.

[spacejam]

Awesome! That feels like a good way forward to me at this point. Great idea, @garyttierney ! Would you mind implementing open_cf such that it accepts an Into<ColumnFamilyDescriptor>? And removing open_cf_descriptors in its favor.

When playing around with the idea I made this quick PoC of migrating a &[&str to an Into<_>, which is a bit ugly, but in case it's helpful here it is! https://play.rust-lang.org/?gist=401075448daa3d550e4d29d726788b4b&version=stable

[garyttierney]

@spacejam yep, happy to go ahead and implement this :-) I'll get some changes pushed later on tonight for review.

[vmx]

@garyttierney Can I help somehow with this pull request? If yes, tell me which part is missing and I'll be working on it.

[garyttierney]

@vmx sorry for the delay. I got a bit stuck keeping backwards compatibility for the open_cfs method!

Currently open_cf_descriptors takes a Vec<ColumnFamilyDescriptor> and would be able to take a Vec<Into<ColumnFamilyDescriptor>>. However, this method signature isn't compatible with existing calls to open_cf which accepts &[&str].

The solution there might be to accept &[C] where C: Into<ColumnFamilyDescriptor> but I've had some issues with this also:

pub fn open_cf<P, C>(opts: &Options, path: P, cfs: &[C]) -> Result<DB, Error>
       where P: AsRef<Path>, C: Into<ColumnFamilyDescriptor> 
{
    for cf in cfs.into_iter() {  // cf = &ColumnFamilyDescriptor, need ColumnFamilyDescriptor
    }
}

I'm relatively new to Rust, so if you have any ideas there I'd appreciate the help. Thanks!

[vmx]

I'm still trying to get this working and I think I'll get this working. Though I really wonder if it's worth it.

I still don't see the point of the original API, I think if you use Column Families, the usual case is to have custom options. I would make it a breaking change with a note in the README saying:

If you want to use column families like prior to this release you can do so with converting the Column Family names into ColumnFamilyDescriptors via:
your_cfs_as_str.to_vec().iter().map(|name| ColumnFamilyDescriptor::new(*name, Options::default())).collect();

Or opening a bug with the exact error message and then posting this line so you can find it if you hit the problem.

[vmx]

Here's the code I came up with. The problem is that it needs to clone the elements of the colum family slice even if it is of type ColumFamilyDescriptor already.

diff --git a/src/db.rs b/src/db.rs
index 6660863..f9c3995 100644
--- a/src/db.rs
+++ b/src/db.rs
@@ -569,6 +569,12 @@ impl ColumnFamilyDescriptor {
     }
 }
 
+impl<'a> From<&'a str> for ColumnFamilyDescriptor {
+    fn from(name: &'a str) -> ColumnFamilyDescriptor {
+        ColumnFamilyDescriptor::new(name, Options::default())
+    }
+}
+
 impl DB {
     /// Open a database with default options.
     pub fn open_default<P: AsRef<Path>>(path: P) -> Result<DB, Error> {
@@ -579,14 +585,16 @@ impl DB {
 
     /// Open the database with the specified options.
     pub fn open<P: AsRef<Path>>(opts: &Options, path: P) -> Result<DB, Error> {
-        DB::open_cf(opts, path, &[])
+        let empty: &[&str] = &[];
+        DB::open_cf(opts, path, empty)
     }
 
     /// Open a database with the given database options and column family names.
     ///
     /// Column families opened using this function will be created with default `Options`.
-    pub fn open_cf<P: AsRef<Path>>(opts: &Options, path: P, cfs: &[&str]) -> Result<DB, Error> {
-        let cfs_v = cfs.to_vec().iter().map(|name| ColumnFamilyDescriptor::new(*name, Options::default())).collect();
+    pub fn open_cf<P, C>(opts: &Options, path: P, cfs: &[C]) -> Result<DB, Error>
+            where P: AsRef<Path>, C: Clone + Into<ColumnFamilyDescriptor> {
+        let cfs_v = cfs.into_iter().map(|cf| cf.clone().into()).collect();
 
         DB::open_cf_descriptors(opts, path, cfs_v)
     }
diff --git a/src/lib.rs b/src/lib.rs
index 788e97a..07c947b 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -81,6 +81,7 @@ pub struct DB {
 /// A descriptor for a RocksDB column family.
 ///
 /// A description of the column family, containing the name and `Options`.
+#[derive(Clone)]
 pub struct ColumnFamilyDescriptor {
     name: String,
     options: Options,
@@ -165,6 +166,7 @@ pub struct BlockBasedOptions {
 ///    DB::open(&opts, path).unwrap()
 /// }
 /// ```
+#[derive(Clone)]
 pub struct Options {
     inner: *mut ffi::rocksdb_options_t,
 }

[vmx]

Although the above code compiles, it doesn't work (it segfaults), probably due to the cloning of the Options.

I think all boils down to that open_cf gets the strings as slice (kind of borrowed) and that open_cf_descriptor gets the vector of ColumFamilyDescriptors moved (which makes sense). So with keeping API compatibility you'd borrow the vector and then internally clone it. I think that's counter intuitive.

[vmx]

I've create a version of open_cf_descriptor which borrows the vector of ColumnFamilyDescriptor (https://github.com/vmx/rust-rocksdb/tree/feature/open-cfs-with-options). I haven't found a clean way to convert the &[&str] into a &Vec<ColumnFamilyDescriptor> while not doing any conversion if it's a &Vec<ColumnFamilyDescriptor>.

[vmx]

I've spent another few hours. This time I come to the conclusion that converting a reference to another one — in our case a &[&str]] into a &Vec<ColumnFamilyDescriptor> (or &[ColumnFamilyDescriptor]) — is not possible with the From trait as something needs to hold the actual ColumnFamilydescriptors. Anything created within the fn from() won't live long enough.

[vmx]

This pull request relates to the discussions at #145.

[spacejam]

crate version 0.9.0 has been cut with these changes included!