Refine IpcSchemaEncoder API and docs #1
Conversation
|
|
| } | ||
|
|
||
| /// Specify a dictionary tracker to use | ||
| pub fn with_dictionary_tracker( |
There was a problem hiding this comment.
Since the lower level build_field takes an optional dictionary tracker it seems like we should permit that still
There was a problem hiding this comment.
It actually seems like many of the crate private APIs like build_field could be pulled into this IpcSchemaEncoder but I don't want to go too crazy now. Maybe some future PR
There was a problem hiding this comment.
yeah I was thinking the same, I wanted to keep the changes small
| /// | ||
| /// See also [`fb_to_schema`] for the reverse operation | ||
| /// | ||
| /// # Example |
There was a problem hiding this comment.
I always found these APIs confusing to use, so I figured I would make them easier to use here
| pub fn schema_to_fb(schema: &Schema) -> FlatBufferBuilder<'_> { | ||
| let mut fbb = FlatBufferBuilder::new(); |
There was a problem hiding this comment.
Here and below, I removed the duplicate implementation and call into the new encoder -- so we can keep the old API the same but the code is shared
| @@ -208,8 +208,9 @@ impl IpcDataGenerator { | |||
| ) -> EncodedData { | |||
| let mut fbb = FlatBufferBuilder::new(); | |||
| let schema = { | |||
| let mut converter = IpcSchemaConverter::new(dictionary_tracker); | |||
| let fb = converter.schema_to_fb_offset(&mut fbb, schema); | |||
| let fb = IpcSchemaEncoder::new() | |||
There was a problem hiding this comment.
I think this usage looks a bit nicer
Note this targets apache#6444
While reviewing the (very nice and very fast) work from @brancz I wanted to help improve the documentation and refine the API a little as part of the review. I figured it would be easier to propose these changes as a PR rather than comments