[GitHub] [arrow] alamb commented on a change in pull request #8784: ARROW-10674: [Rust] Fix BigDecimal to be little endian; Add IPC Reader/Writer for Decimal type to allow integration tests

[GitBox <gi...@apache.org>]

alamb commented on a change in pull request #8784:
URL: https://github.com/apache/arrow/pull/8784#discussion_r535138494



##########
File path: rust/arrow/src/ipc/convert.rs
##########
@@ -97,6 +97,12 @@ pub fn fb_to_schema(fb: ipc::Schema) -> Schema {
     let len = c_fields.len();
     for i in 0..len {
         let c_field: ipc::Field = c_fields.get(i);
+        match c_field.type_type() {

Review comment:
       I see the problem -- yes, there since the `endianness` is on the shema object, not the field, since the field is all that is passed around there is no way to know what the details of the schema are.
   
   I personally think this code is fine, if a bit un-indeal and could be cleaned up in the future. My only worry is that it would get lost / broken during such cleanup
   
   What would you think about adding a test that triggers the error? Then we could be sure that any future cleanups will not break the check?

##########
File path: rust/arrow/src/ipc/convert.rs
##########
@@ -97,6 +97,12 @@ pub fn fb_to_schema(fb: ipc::Schema) -> Schema {
     let len = c_fields.len();
     for i in 0..len {
         let c_field: ipc::Field = c_fields.get(i);
+        match c_field.type_type() {

Review comment:
       I see the problem -- yes, there since the `endianness` is on the shema object, not the field, since the field is all that is passed around there is no way to know what the details of the schema are.
   
   I personally think this code is fine, if a bit un-indeal and could be cleaned up in the future. My only worry is that it would get lost / broken during such cleanup
   
   What would you think about adding a test that triggers the error? Then we could be sure that any future cleanups will not break the check?
   
   Thanks again @sweb 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org