[Variant] Rename batch_json_string_to_variant and batch_variant_to_json_string
#8161
Conversation
| fn with_json(&mut self, json: &str) -> Result<(), ArrowError>; | ||
| } | ||
|
|
||
| fn build_json(json: &Value, builder: &mut impl VariantBuilderExt) -> Result<(), ArrowError> { |
There was a problem hiding this comment.
Removed redundant build_json
| use base64::{engine::general_purpose, Engine as _}; | ||
| use chrono::Timelike; | ||
| use parquet_variant::{Variant, VariantList, VariantObject}; | ||
| use serde_json::Value; |
There was a problem hiding this comment.
Changes in this file are basically moving the doc to trait and function body to impl
There was a problem hiding this comment.
Thank you @liamzwbao -- I think this is a really nice improvement (using traits is 👨🍳 👌 )
I took the liberty of renaming with_json to be append_json to be consistent with the other APIs but really this was very nicely done
cc @harshmotw-db and @scovich
|
|
||
| build_json(&json, builder)?; | ||
| Ok(()) | ||
| pub trait JsonToVariant { |
| Ok(()) | ||
| pub trait JsonToVariant { | ||
| /// Create a Variant from a JSON string | ||
| fn with_json(&mut self, json: &str) -> Result<(), ArrowError>; |
There was a problem hiding this comment.
Can we pleae call this append_json to be consistent with VariantBuilderExt::append_value ?
Also I think the convention of using with_* in these crates is for builder / fluent style APIs that take self (rather than &mut self)
|
Resolved conflicts, should be good to go now |
|
Thank you @liamzwbao and @scovich |
3 participants
Which issue does this PR close?
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.
batch_json_string_to_variantandbatch_variant_to_json_stringjson_to_variant #8144.Rationale for this change
What changes are included in this PR?
Use extension traits to wrap the json variant conversion function, and rename batch function to a more common name.
Are these changes tested?
Yes
Are there any user-facing changes?
The APIs of parquet-variant-json are changed