Scaffolding to support generating arbitrary filter pipelines #23
Conversation
…ialize, Deserialize
|
This doesn't quite work yet, the |
There was a problem hiding this comment.
Whoa. Yeah, that' turned out way better than I was expecting when we discussed possible approaches on the Thursday call. It really shines when looking at the proptest definitions as well.
Most things I had were either minor style nits or general questions that don't need answering for this PR. The only real thing I saw was that c_uchar vs c_uint question.
Otherwise, +1 once CI is green.
| } | ||
| if let Some(reinterpret_datatype) = reinterpret_datatype { | ||
| let c_datatype = | ||
| reinterpret_datatype.capi_enum() as std::ffi::c_uchar; |
There was a problem hiding this comment.
is std::ffi::c_uchar correct here? I would have expected u32 or std::ffi::c_uint.
There was a problem hiding this comment.
Surprisingly yes - it expects just one byte inside of libtiledb
There was a problem hiding this comment.
Well that's awkward. In a note to future selves, I tracked this down to the fact that most of our uses of Datatype refelect the public C API typedef enum { ... } tiledb_datatype_t definition which is apparently backed by a u32 according to bindgen. However, the Filter::set_option pipes the void* all the way to CompressionFilter::set_option_impl which is casting it to a tiledb::sm::Datatype which is enum class Datatype : uint8_t { ... } definition which means it really does want a single byte ini this case.
| @@ -67,6 +68,12 @@ impl<'ctx> FilterList<'ctx> { | |||
| } | |||
| } | |||
|
|
|||
| pub fn to_vec(&self) -> TileDBResult<Vec<Filter<'ctx>>> { | |||
| (0..self.get_num_filters()?) | |||
| .map(|f| self.get_filter(f)) | |||
There was a problem hiding this comment.
Huh. TIL that collect() returns the first error or the whole list. That's awesome!
The missing piece from
Attribute::eqwas the filter pipeline. To get this in there with the testing infrastructure we have and/or will need, we need to doDebug,Eq,SerializeandDeserializeforFilterListandFilter.That turns out to require a bunch of things:
syscrate with minimal dependencies, I movedDatatypeto theapicrate where it can#[derive(Deserialize, Serialize)]. This also is a big step towards keepingffi::out of the public APIs.enum FilterDatais really useful because it can#[derive(PartialEq)]which helps prevent us developers from making mistakes. Now those mistakes can live inFilter::createand/orFilter::filter_datainstead.FilterData::transform_datatypeis an internal C API, but we need to use it for property-based testing to make sure that we construct valid filter pipelines. The output of each filter in the pipeline needs to be a valid input to the next one.