scovich commented on code in PR #7783: URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2167745526
########## parquet-variant/src/decoder.rs: ########## @@ -26,6 +26,11 @@ use std::num::TryFromIntError; // Makes the code a bit more readable pub(crate) const VARIANT_VALUE_HEADER_BYTES: usize = 1; +pub(crate) const MAX_UNSCALED_DECIMAL_4: i32 = 999999999; +pub(crate) const MAX_PRECISION_DECIMAL_4: u8 = 9; +pub(crate) const MAX_UNSCALED_DECIMAL_8: i64 = 999999999999999999i64; +pub(crate) const MAX_PRECISION_DECIMAL_8: u8 = 18; Review Comment: Now that https://github.com/apache/arrow-rs/pull/7776 merged, you can use `VariantDecimalXX::MAX_PRECISION` and `VariantDecimalXX::MAX_UNSCALED_VALUE` ########## parquet-variant/src/from_json.rs: ########## @@ -0,0 +1,97 @@ +use crate::variant_buffer_manager::VariantBufferManager; +use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder}; +use arrow_schema::ArrowError; +use serde_json::{Map, Value}; + +/// Eventually, internal writes should also be performed using VariantBufferManager instead of +/// ValueBuffer and MetadataBuffer so the caller has control of the memory. +/// Returns a pair <value_size, metadata_size> +pub fn json_to_variant<'a, T: VariantBufferManager>( + json: &str, + variant_buffer_manager: &'a mut T, Review Comment: ```suggestion pub fn json_to_variant<'a>( json: &str, variant_buffer_manager: &'a mut impl VariantBufferManager, ``` ########## parquet-variant/src/from_json.rs: ########## @@ -0,0 +1,97 @@ +use crate::variant_buffer_manager::VariantBufferManager; +use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder}; +use arrow_schema::ArrowError; +use serde_json::{Map, Value}; + +/// Eventually, internal writes should also be performed using VariantBufferManager instead of +/// ValueBuffer and MetadataBuffer so the caller has control of the memory. +/// Returns a pair <value_size, metadata_size> +pub fn json_to_variant<'a, T: VariantBufferManager>( + json: &str, + variant_buffer_manager: &'a mut T, +) -> Result<(usize, usize), ArrowError> { + let mut builder = VariantBuilder::new(); + let json: Value = serde_json::from_str(json) + .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {}", e)))?; + + build_json(&json, &mut builder)?; + let (metadata, value) = builder.finish(); + let value_size = value.len(); + let metadata_size = metadata.len(); + + // Write to caller's buffers - Remove this when the library internally writes to the caller's + // buffers anyway + variant_buffer_manager.ensure_metadata_buffer_size(metadata_size)?; + variant_buffer_manager.ensure_value_buffer_size(value_size)?; + let caller_metadata_buffer = variant_buffer_manager.borrow_metadata_buffer(); + caller_metadata_buffer[..metadata_size].copy_from_slice(metadata.as_slice()); + let caller_value_buffer = variant_buffer_manager.borrow_value_buffer(); + caller_value_buffer[..value_size].copy_from_slice(value.as_slice()); + Ok((metadata_size, value_size)) +} + +fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> { + match json { + Value::Null => builder.append_value(Variant::Null), + Value::Bool(b) => builder.append_value(*b), + Value::Number(n) => { + let v: Variant = n.try_into()?; + builder.append_value(v) + } + Value::String(s) => builder.append_value(s.as_str()), + Value::Array(arr) => { + let mut list_builder = builder.new_list(); + build_list(arr, &mut list_builder)?; + list_builder.finish(); + } + Value::Object(obj) => { + let mut obj_builder = builder.new_object(); + build_object(obj, &mut obj_builder)?; + obj_builder.finish(); + } + }; + Ok(()) +} + +fn build_list(arr: &Vec<Value>, builder: &mut ListBuilder) -> Result<(), ArrowError> { + for val in arr { + match val { + Value::Null => builder.append_value(Variant::Null), + Value::Bool(b) => builder.append_value(*b), + Value::Number(n) => { + let v: Variant = n.try_into()?; + builder.append_value(v) + } + Value::String(s) => builder.append_value(s.as_str()), + Value::Array(arr) => { + let mut list_builder = builder.new_list(); + build_list(arr, &mut list_builder)?; + list_builder.finish() + } + Value::Object(obj) => { + let mut obj_builder = builder.new_object(); + build_object(obj, &mut obj_builder)?; + obj_builder.finish(); + } + } + } + Ok(()) +} + +fn build_object(obj: &Map<String, Value>, builder: &mut ObjectBuilder) -> Result<(), ArrowError> { + for (key, value) in obj.iter() { + match value { + Value::Null => builder.append_value(key, Variant::Null), Review Comment: Following the above idea, we could build a simple newtype wrapper for `(&mut ObjectBuilder, &str)`, that can then impl the same trait as `VariantBuilder` and `ListBuilder`, which allows something like: ```rust for (key, value) in obj.iter() { let builder = ObjectFieldBuilder::new(builder, key); build_json(val, &mut builder); } ``` ########## parquet-variant/src/from_json.rs: ########## @@ -0,0 +1,97 @@ +use crate::variant_buffer_manager::VariantBufferManager; +use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder}; +use arrow_schema::ArrowError; +use serde_json::{Map, Value}; + +/// Eventually, internal writes should also be performed using VariantBufferManager instead of +/// ValueBuffer and MetadataBuffer so the caller has control of the memory. +/// Returns a pair <value_size, metadata_size> +pub fn json_to_variant<'a, T: VariantBufferManager>( + json: &str, + variant_buffer_manager: &'a mut T, +) -> Result<(usize, usize), ArrowError> { + let mut builder = VariantBuilder::new(); + let json: Value = serde_json::from_str(json) + .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {}", e)))?; + + build_json(&json, &mut builder)?; + let (metadata, value) = builder.finish(); + let value_size = value.len(); + let metadata_size = metadata.len(); + + // Write to caller's buffers - Remove this when the library internally writes to the caller's + // buffers anyway + variant_buffer_manager.ensure_metadata_buffer_size(metadata_size)?; + variant_buffer_manager.ensure_value_buffer_size(value_size)?; + let caller_metadata_buffer = variant_buffer_manager.borrow_metadata_buffer(); + caller_metadata_buffer[..metadata_size].copy_from_slice(metadata.as_slice()); + let caller_value_buffer = variant_buffer_manager.borrow_value_buffer(); + caller_value_buffer[..value_size].copy_from_slice(value.as_slice()); + Ok((metadata_size, value_size)) +} + +fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> { + match json { + Value::Null => builder.append_value(Variant::Null), + Value::Bool(b) => builder.append_value(*b), + Value::Number(n) => { + let v: Variant = n.try_into()?; + builder.append_value(v) + } + Value::String(s) => builder.append_value(s.as_str()), + Value::Array(arr) => { + let mut list_builder = builder.new_list(); + build_list(arr, &mut list_builder)?; + list_builder.finish(); + } + Value::Object(obj) => { + let mut obj_builder = builder.new_object(); + build_object(obj, &mut obj_builder)?; + obj_builder.finish(); + } + }; + Ok(()) +} + +fn build_list(arr: &Vec<Value>, builder: &mut ListBuilder) -> Result<(), ArrowError> { + for val in arr { + match val { + Value::Null => builder.append_value(Variant::Null), + Value::Bool(b) => builder.append_value(*b), + Value::Number(n) => { + let v: Variant = n.try_into()?; + builder.append_value(v) + } + Value::String(s) => builder.append_value(s.as_str()), + Value::Array(arr) => { + let mut list_builder = builder.new_list(); + build_list(arr, &mut list_builder)?; + list_builder.finish() + } + Value::Object(obj) => { + let mut obj_builder = builder.new_object(); + build_object(obj, &mut obj_builder)?; + obj_builder.finish(); + } Review Comment: This match is syntactically identical to the match in `build_json`. Maybe we need a trait that `VariantBuilder` and `ListBuilder` both implement, and then can just do: ```rust for val in arr { build_json(val, &mut builder); } ``` ########## parquet-variant/src/from_json.rs: ########## @@ -0,0 +1,97 @@ +use crate::variant_buffer_manager::VariantBufferManager; +use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder}; +use arrow_schema::ArrowError; +use serde_json::{Map, Value}; + +/// Eventually, internal writes should also be performed using VariantBufferManager instead of +/// ValueBuffer and MetadataBuffer so the caller has control of the memory. +/// Returns a pair <value_size, metadata_size> +pub fn json_to_variant<'a, T: VariantBufferManager>( + json: &str, + variant_buffer_manager: &'a mut T, +) -> Result<(usize, usize), ArrowError> { + let mut builder = VariantBuilder::new(); + let json: Value = serde_json::from_str(json) + .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {}", e)))?; + + build_json(&json, &mut builder)?; + let (metadata, value) = builder.finish(); + let value_size = value.len(); + let metadata_size = metadata.len(); + + // Write to caller's buffers - Remove this when the library internally writes to the caller's + // buffers anyway + variant_buffer_manager.ensure_metadata_buffer_size(metadata_size)?; + variant_buffer_manager.ensure_value_buffer_size(value_size)?; + let caller_metadata_buffer = variant_buffer_manager.borrow_metadata_buffer(); + caller_metadata_buffer[..metadata_size].copy_from_slice(metadata.as_slice()); + let caller_value_buffer = variant_buffer_manager.borrow_value_buffer(); + caller_value_buffer[..value_size].copy_from_slice(value.as_slice()); + Ok((metadata_size, value_size)) +} + +fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> { + match json { + Value::Null => builder.append_value(Variant::Null), + Value::Bool(b) => builder.append_value(*b), + Value::Number(n) => { + let v: Variant = n.try_into()?; + builder.append_value(v) + } + Value::String(s) => builder.append_value(s.as_str()), + Value::Array(arr) => { + let mut list_builder = builder.new_list(); + build_list(arr, &mut list_builder)?; + list_builder.finish(); + } + Value::Object(obj) => { + let mut obj_builder = builder.new_object(); + build_object(obj, &mut obj_builder)?; + obj_builder.finish(); + } + }; + Ok(()) +} + +fn build_list(arr: &Vec<Value>, builder: &mut ListBuilder) -> Result<(), ArrowError> { + for val in arr { + match val { + Value::Null => builder.append_value(Variant::Null), + Value::Bool(b) => builder.append_value(*b), + Value::Number(n) => { + let v: Variant = n.try_into()?; + builder.append_value(v) + } + Value::String(s) => builder.append_value(s.as_str()), + Value::Array(arr) => { + let mut list_builder = builder.new_list(); + build_list(arr, &mut list_builder)?; + list_builder.finish() + } + Value::Object(obj) => { + let mut obj_builder = builder.new_object(); + build_object(obj, &mut obj_builder)?; + obj_builder.finish(); + } + } + } + Ok(()) +} + +fn build_object(obj: &Map<String, Value>, builder: &mut ObjectBuilder) -> Result<(), ArrowError> { + for (key, value) in obj.iter() { + match value { + Value::Null => builder.append_value(key, Variant::Null), + Value::Bool(b) => builder.append_value(key, *b), + Value::Number(n) => { + let v: Variant = n.try_into()?; + builder.append_value(key, v) + } + Value::String(s) => builder.append_value(key, s.as_str()), + Value::Array(_) | Value::Object(_) => { + todo!("Nesting within objects unsupported right now."); + } Review Comment: Is there some blocker here? Or just TODO? (surprised we can nest inside arrays but not inside objects?) ########## parquet-variant/src/variant_buffer_manager.rs: ########## @@ -0,0 +1,124 @@ +use arrow_schema::ArrowError; + +pub trait VariantBufferManager { + /// Returns the slice where the variant metadata needs to be written to. This method may be + /// called several times during the construction of a new `metadata` field in a variant. The + /// implementation must make sure that on every call, all the data written to the metadata + /// buffer so far are preserved. + fn borrow_metadata_buffer(&mut self) -> &mut [u8]; + + /// Returns the slice where value needs to be written to. This method may be called several + /// times during the construction of a new `value` field in a variant. The implementation must + /// make sure that on every call, all the data written to the value buffer so far are preserved. + fn borrow_value_buffer(&mut self) -> &mut [u8]; + + /// Ensures that the next call to `borrow_metadata_buffer` returns a slice having at least + /// `size` bytes. Also ensures that the value metadata written so far are persisted - this means + /// that if `borrow_metadata_buffer` is to return a new buffer from the next call onwards, the + /// new buffer must have the contents of the old metadata buffer. + fn ensure_metadata_buffer_size(&mut self, size: usize) -> Result<(), ArrowError>; + + /// Ensures that the next call to `borrow_value_buffer` returns a slice having at least `size` + /// bytes. Also ensures that the value bytes written so far are persisted - this means that + /// if `borrow_value_buffer` is to return a new buffer from the next call onwards, the new + /// buffer must have the contents of the old value buffer. + fn ensure_value_buffer_size(&mut self, size: usize) -> Result<(), ArrowError>; + + fn get_immutable_metadata_buffer(&self) -> &[u8] { + unimplemented!("Optional method not implemented in VariantBufferManager") + } + + fn get_immutable_value_buffer(&self) -> &[u8] { + unimplemented!("Optional method not implemented in VariantBufferManager") + } +} + +pub struct SampleBoxBasedVariantBufferManager { + pub value_buffer: Box<[u8]>, + pub metadata_buffer: Box<[u8]>, Review Comment: Continuing the above thought about building up an arrow `Array`... maybe a `SampleMultiVariantBufferManager` that can stack a bunch of variants end to end, with a `len` method that allows to extract offsets? ########## parquet-variant/Cargo.toml: ########## @@ -35,7 +35,10 @@ rust-version = "1.83" [dependencies] arrow-schema = { workspace = true } chrono = { workspace = true } -serde_json = "1.0" +# "arbitrary_precision" allows us to manually parse numbers. "preserve_order" does not automatically +# sort object keys +serde_json = { version = "1.0", features = ["arbitrary_precision", "preserve_order"] } Review Comment: TIL about the (undocumented) `arbitrary_precision` feature flag and (undocumented) `Number::as_str` method it unlocks 🤯 That could solve a lot of problems for variant decimals on the to_json path as well: https://github.com/apache/arrow-rs/blob/main/parquet-variant/src/to_json.rs#L147-L162 ########## parquet-variant/src/from_json.rs: ########## @@ -0,0 +1,97 @@ +use crate::variant_buffer_manager::VariantBufferManager; +use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder}; +use arrow_schema::ArrowError; +use serde_json::{Map, Value}; + +/// Eventually, internal writes should also be performed using VariantBufferManager instead of +/// ValueBuffer and MetadataBuffer so the caller has control of the memory. +/// Returns a pair <value_size, metadata_size> +pub fn json_to_variant<'a, T: VariantBufferManager>( + json: &str, + variant_buffer_manager: &'a mut T, +) -> Result<(usize, usize), ArrowError> { + let mut builder = VariantBuilder::new(); + let json: Value = serde_json::from_str(json) + .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {}", e)))?; + + build_json(&json, &mut builder)?; + let (metadata, value) = builder.finish(); + let value_size = value.len(); + let metadata_size = metadata.len(); + + // Write to caller's buffers - Remove this when the library internally writes to the caller's + // buffers anyway + variant_buffer_manager.ensure_metadata_buffer_size(metadata_size)?; + variant_buffer_manager.ensure_value_buffer_size(value_size)?; + let caller_metadata_buffer = variant_buffer_manager.borrow_metadata_buffer(); + caller_metadata_buffer[..metadata_size].copy_from_slice(metadata.as_slice()); + let caller_value_buffer = variant_buffer_manager.borrow_value_buffer(); + caller_value_buffer[..value_size].copy_from_slice(value.as_slice()); + Ok((metadata_size, value_size)) +} + +fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> { + match json { + Value::Null => builder.append_value(Variant::Null), + Value::Bool(b) => builder.append_value(*b), + Value::Number(n) => { + let v: Variant = n.try_into()?; + builder.append_value(v) + } Review Comment: ```suggestion Value::Number(n) => builder.append_value(Variant::try_from(n)?), ``` ########## parquet-variant/src/variant_buffer_manager.rs: ########## @@ -0,0 +1,124 @@ +use arrow_schema::ArrowError; + +pub trait VariantBufferManager { Review Comment: I'm not quite sure I understand this trait. It _almost_ seems like a good building block for eventual columnar writes, if an implementation took a mutable reference to a vec, where each `VariantBufferManager` you build with it is only capable of appending new bytes to the end (leaving existing bytes unchanged). That way, one could build up an offset array along the way as part of building up arrow binary arrays for the metadata and value columns? But I don't understand the whole "may be called several times" part, which seems to return the whole slice on every call? I guess that's because the variant builder doesn't do its work all at once, but still expects to receive the overall slice? Finally, why split the ensure and borrow methods, when both take `&mut self` and it's almost certainly dangerous to "borrow" without an "ensure" first (can't guess the size in advance, it's variant data)? Why not just a pair of `ensure_and_borrow_XXX_buffer` methods that return a slice of exactly the requested size (no larger, no smaller)? ########## parquet-variant/src/variant.rs: ########## @@ -1031,6 +1033,57 @@ impl From<VariantDecimal16> for Variant<'_, '_> { } } +impl TryFrom<&Number> for Variant<'_, '_> { + type Error = ArrowError; + + fn try_from(n: &Number) -> Result<Self, Self::Error> { Review Comment: You might want to take a look at the decimal parsing code in my prototype? https://github.com/apache/arrow-rs/pull/7403/files#diff-fce219df196b3dffd6c2f6bb4924a34f299817a317943a19482289ecb8f6af69R1065-R1079 (tho that code is _not_ optimal, it might at least provide some ideas) ########## parquet-variant/tests/test_json_to_variant.rs: ########## @@ -0,0 +1,349 @@ +use arrow_schema::ArrowError; +use parquet_variant::{json_to_variant, VariantBufferManager}; + +pub struct SampleVariantBufferManager { Review Comment: How is this different from the box-based one above, and why not just use the vec-based one above instead? ########## parquet-variant/src/variant.rs: ########## @@ -1031,6 +1033,57 @@ impl From<VariantDecimal16> for Variant<'_, '_> { } } +impl TryFrom<&Number> for Variant<'_, '_> { + type Error = ArrowError; + + fn try_from(n: &Number) -> Result<Self, Self::Error> { + if n.is_i64() { + // Find minimum Integer width to fit + let i = n.as_i64().unwrap(); Review Comment: ```suggestion if let Some(i) = n.is_i64() { // Find minimum Integer width to fit ``` ########## parquet-variant/src/variant_buffer_manager.rs: ########## @@ -0,0 +1,124 @@ +use arrow_schema::ArrowError; + +pub trait VariantBufferManager { + /// Returns the slice where the variant metadata needs to be written to. This method may be + /// called several times during the construction of a new `metadata` field in a variant. The + /// implementation must make sure that on every call, all the data written to the metadata + /// buffer so far are preserved. + fn borrow_metadata_buffer(&mut self) -> &mut [u8]; + + /// Returns the slice where value needs to be written to. This method may be called several + /// times during the construction of a new `value` field in a variant. The implementation must + /// make sure that on every call, all the data written to the value buffer so far are preserved. + fn borrow_value_buffer(&mut self) -> &mut [u8]; + + /// Ensures that the next call to `borrow_metadata_buffer` returns a slice having at least + /// `size` bytes. Also ensures that the value metadata written so far are persisted - this means + /// that if `borrow_metadata_buffer` is to return a new buffer from the next call onwards, the + /// new buffer must have the contents of the old metadata buffer. + fn ensure_metadata_buffer_size(&mut self, size: usize) -> Result<(), ArrowError>; + + /// Ensures that the next call to `borrow_value_buffer` returns a slice having at least `size` + /// bytes. Also ensures that the value bytes written so far are persisted - this means that + /// if `borrow_value_buffer` is to return a new buffer from the next call onwards, the new + /// buffer must have the contents of the old value buffer. + fn ensure_value_buffer_size(&mut self, size: usize) -> Result<(), ArrowError>; + + fn get_immutable_metadata_buffer(&self) -> &[u8] { + unimplemented!("Optional method not implemented in VariantBufferManager") + } + + fn get_immutable_value_buffer(&self) -> &[u8] { + unimplemented!("Optional method not implemented in VariantBufferManager") + } +} + +pub struct SampleBoxBasedVariantBufferManager { + pub value_buffer: Box<[u8]>, + pub metadata_buffer: Box<[u8]>, Review Comment: What purpose would this serve? It seems strictly inferior to a vec for both normal and testing uses? ########## parquet-variant/src/from_json.rs: ########## @@ -0,0 +1,97 @@ +use crate::variant_buffer_manager::VariantBufferManager; +use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder}; +use arrow_schema::ArrowError; +use serde_json::{Map, Value}; + +/// Eventually, internal writes should also be performed using VariantBufferManager instead of +/// ValueBuffer and MetadataBuffer so the caller has control of the memory. +/// Returns a pair <value_size, metadata_size> +pub fn json_to_variant<'a, T: VariantBufferManager>( + json: &str, + variant_buffer_manager: &'a mut T, +) -> Result<(usize, usize), ArrowError> { + let mut builder = VariantBuilder::new(); + let json: Value = serde_json::from_str(json) + .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {}", e)))?; + + build_json(&json, &mut builder)?; + let (metadata, value) = builder.finish(); + let value_size = value.len(); + let metadata_size = metadata.len(); + + // Write to caller's buffers - Remove this when the library internally writes to the caller's + // buffers anyway + variant_buffer_manager.ensure_metadata_buffer_size(metadata_size)?; + variant_buffer_manager.ensure_value_buffer_size(value_size)?; + let caller_metadata_buffer = variant_buffer_manager.borrow_metadata_buffer(); + caller_metadata_buffer[..metadata_size].copy_from_slice(metadata.as_slice()); + let caller_value_buffer = variant_buffer_manager.borrow_value_buffer(); + caller_value_buffer[..value_size].copy_from_slice(value.as_slice()); + Ok((metadata_size, value_size)) +} + +fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> { + match json { + Value::Null => builder.append_value(Variant::Null), + Value::Bool(b) => builder.append_value(*b), + Value::Number(n) => { + let v: Variant = n.try_into()?; + builder.append_value(v) + } + Value::String(s) => builder.append_value(s.as_str()), + Value::Array(arr) => { + let mut list_builder = builder.new_list(); + build_list(arr, &mut list_builder)?; + list_builder.finish(); + } + Value::Object(obj) => { + let mut obj_builder = builder.new_object(); + build_object(obj, &mut obj_builder)?; + obj_builder.finish(); + } + }; + Ok(()) +} + +fn build_list(arr: &Vec<Value>, builder: &mut ListBuilder) -> Result<(), ArrowError> { Review Comment: I'm surprised clippy didn't flag this ```suggestion fn build_list(arr: &[Value], builder: &mut ListBuilder) -> Result<(), ArrowError> { ``` ########## parquet-variant/src/variant.rs: ########## @@ -1031,6 +1033,57 @@ impl From<VariantDecimal16> for Variant<'_, '_> { } } +impl TryFrom<&Number> for Variant<'_, '_> { + type Error = ArrowError; + + fn try_from(n: &Number) -> Result<Self, Self::Error> { + if n.is_i64() { + // Find minimum Integer width to fit + let i = n.as_i64().unwrap(); + if i as i8 as i64 == i { + Ok((i as i8).into()) + } else if i as i16 as i64 == i { + Ok((i as i16).into()) + } else if i as i32 as i64 == i { + Ok((i as i32).into()) + } else { + Ok(i.into()) + } Review Comment: Interesting approach. That double `.. as .. as ..` definitely caused a double take, but I guess it could be cheaper than TryFrom? ```suggestion let value = i8::try_from(i) .map(Variant::from) .or_else(|_| i16::try_from(i).map(Variant::from)) .or_else(|_| i32::try_from(i).map(Variant::from)) .unwrap_or_else(|| Variant::from(i)); Ok(value) ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
