-
Notifications
You must be signed in to change notification settings - Fork 5
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
Groups API + Tests + Example v2 #69
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a lot of the comments I left on the previous PR still apply here.
577ca5b
to
8d8818d
Compare
tiledb/api/src/group/mod.rs
Outdated
} | ||
|
||
pub fn consolidate_metadata<S>( | ||
&self, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This C API doc doesn't mention anything about the group open mode but this sure sounds like a mutating function call...
Anyway, as I've been mulling over the whole "some methods are &mut self
and some aren't" thing, it's kind of occurred to me that &mut self
to represent a write mode method doesn't really work because the API allows you to just get another handle, and then you have ownership of that second handle and can call whatever you want anyway. And this can go in the direction of "just let the C API give back the error and don't worry about it" or in the direction of "embed the permissions to call certain APIs in the type system". If we had infinite time I'd want the latter but let's move ahead with the former for now.
c04d7f3
to
26a6d66
Compare
Added a story for consolidate and vacuum tests: https://app.shortcut.com/tiledb-inc/story/46102/write-tests-for-group-consolidate-and-group-vacuum |
Float64Value(Vec<f64>), | ||
//StringAsciiValue(CString), | ||
//BooleanValue(bool), | ||
// maybe blobs? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets remove commented out code here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skimming this, I'm not seeing anything of concern. Just the one minor nit. I'll defer to @rroelke to approve since he's reviewed this more than me.
.capi_return(unsafe { | ||
ffi::tiledb_group_is_open(c_context, raw_group.ffi, &mut c_open) | ||
}) | ||
.expect("TileDB internal error when checking for open group."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect
isn't appropriate here since we can return Result
also I think you can remove this if you want, your comment yesterday (earlier this week? something) in Drop
convinced me that you did it correctly the first time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All righty!
Looks like there are two final comments but they should be pretty trivial to address.
Let's get this in there, nice work!
No description provided.