Jefffrey commented on code in PR #10647:
URL: https://github.com/apache/arrow-rs/pull/10647#discussion_r3766517866
##########
arrow-ipc/src/convert.rs:
##########
@@ -158,52 +158,75 @@ pub fn schema_to_fb_offset<'a>(
}
/// Convert an IPC Field to Arrow Field
+///
+/// This panics on malformed input; every reader path uses the fallible
+/// conversion instead. kept for backwards compatibility only.
impl From<crate::Field<'_>> for Field {
fn from(field: crate::Field) -> Field {
- let arrow_field = if let Some(dictionary) = field.dictionary() {
- #[allow(deprecated)]
- Field::new_dict(
- field.name().unwrap_or_default(),
- get_data_type(field, true),
- field.nullable(),
- dictionary.id(),
- dictionary.isOrdered(),
- )
- } else {
- Field::new(
- field.name().unwrap_or_default(),
- get_data_type(field, true),
- field.nullable(),
- )
- };
+ try_field_from(field).expect("invalid IPC field")
+ }
+}
- let mut metadata_map = HashMap::default();
- if let Some(list) = field.custom_metadata() {
- for kv in list {
- if let (Some(k), Some(v)) = (kv.key(), kv.value()) {
- metadata_map.insert(k.to_string(), v.to_string());
- }
+/// Fallible conversion of an IPC Field to an Arrow Field.
+fn try_field_from(field: crate::Field) -> Result<Field, ArrowError> {
+ let arrow_field = if let Some(dictionary) = field.dictionary() {
+ #[allow(deprecated)]
+ Field::new_dict(
+ field.name().unwrap_or_default(),
+ get_data_type(field, true)?,
+ field.nullable(),
+ dictionary.id(),
+ dictionary.isOrdered(),
+ )
+ } else {
+ Field::new(
+ field.name().unwrap_or_default(),
+ get_data_type(field, true)?,
+ field.nullable(),
+ )
+ };
+
+ let mut metadata_map = HashMap::default();
+ if let Some(list) = field.custom_metadata() {
+ for kv in list {
+ if let (Some(k), Some(v)) = (kv.key(), kv.value()) {
+ metadata_map.insert(k.to_string(), v.to_string());
}
}
-
- arrow_field.with_metadata(metadata_map)
}
+
+ Ok(arrow_field.with_metadata(metadata_map))
}
/// Deserialize an ipc [`crate::Schema`] from flat buffers to an arrow
[Schema].
+///
+/// This panics on malformed input; prefer the fallible [`try_fb_to_schema`].
+/// kept for backwards compatibility only.
pub fn fb_to_schema(fb: crate::Schema) -> Schema {
+ try_fb_to_schema(fb).expect("invalid IPC schema")
+}
+
+/// Deserialize an ipc [`crate::Schema`] from flat buffers to an arrow
[Schema].
+///
+/// Unlike [`fb_to_schema`], returns an error instead of panicking on schema
+/// messages that the flatbuffer verifier accepts but that are not valid Arrow.
+pub fn try_fb_to_schema(fb: crate::Schema) -> Result<Schema, ArrowError> {
Review Comment:
```suggestion
/// Returns an error on schema messages that the flatbuffer
/// verifier accepts but that are not valid Arrow.
```
keep the doc self contained, especially if we decide to deprecate the old one
##########
arrow-ipc/src/convert.rs:
##########
@@ -305,16 +340,23 @@ pub(crate) fn get_data_type(field: crate::Field,
may_be_dictionary: bool) -> Dat
(32, false) => DataType::UInt32,
(64, true) => DataType::Int64,
(64, false) => DataType::UInt64,
- _ => panic!("Unexpected bitwidth and signed"),
+ _ => {
+ return Err(ArrowError::ParseError(
+ "Unexpected bitwidth and signed".to_string(),
+ ));
Review Comment:
```suggestion
return Err(ArrowError::ParseError(format!(
"Index type with bit width of {} and signed of {} not
supported",
z.0, z.1
)));
```
##########
arrow-ipc/src/convert.rs:
##########
@@ -158,52 +158,75 @@ pub fn schema_to_fb_offset<'a>(
}
/// Convert an IPC Field to Arrow Field
+///
+/// This panics on malformed input; every reader path uses the fallible
+/// conversion instead. kept for backwards compatibility only.
impl From<crate::Field<'_>> for Field {
fn from(field: crate::Field) -> Field {
- let arrow_field = if let Some(dictionary) = field.dictionary() {
- #[allow(deprecated)]
- Field::new_dict(
- field.name().unwrap_or_default(),
- get_data_type(field, true),
- field.nullable(),
- dictionary.id(),
- dictionary.isOrdered(),
- )
- } else {
- Field::new(
- field.name().unwrap_or_default(),
- get_data_type(field, true),
- field.nullable(),
- )
- };
+ try_field_from(field).expect("invalid IPC field")
+ }
+}
- let mut metadata_map = HashMap::default();
- if let Some(list) = field.custom_metadata() {
- for kv in list {
- if let (Some(k), Some(v)) = (kv.key(), kv.value()) {
- metadata_map.insert(k.to_string(), v.to_string());
- }
+/// Fallible conversion of an IPC Field to an Arrow Field.
+fn try_field_from(field: crate::Field) -> Result<Field, ArrowError> {
+ let arrow_field = if let Some(dictionary) = field.dictionary() {
+ #[allow(deprecated)]
+ Field::new_dict(
+ field.name().unwrap_or_default(),
+ get_data_type(field, true)?,
+ field.nullable(),
+ dictionary.id(),
+ dictionary.isOrdered(),
+ )
+ } else {
+ Field::new(
+ field.name().unwrap_or_default(),
+ get_data_type(field, true)?,
+ field.nullable(),
+ )
+ };
+
+ let mut metadata_map = HashMap::default();
+ if let Some(list) = field.custom_metadata() {
+ for kv in list {
+ if let (Some(k), Some(v)) = (kv.key(), kv.value()) {
+ metadata_map.insert(k.to_string(), v.to_string());
}
}
-
- arrow_field.with_metadata(metadata_map)
}
+
+ Ok(arrow_field.with_metadata(metadata_map))
}
/// Deserialize an ipc [`crate::Schema`] from flat buffers to an arrow
[Schema].
+///
+/// This panics on malformed input; prefer the fallible [`try_fb_to_schema`].
+/// kept for backwards compatibility only.
Review Comment:
we can deprecate this function
--
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]
