[GitHub] [arrow-rs] Joseph-Rance commented on a diff in pull request #4773: Add `RecordReader` trait and proc macro to implement it for a struct

["Joseph-Rance (via GitHub)" <gi...@apache.org>]

Joseph-Rance commented on code in PR #4773:
URL: https://github.com/apache/arrow-rs/pull/4773#discussion_r1328661595


##########
parquet_derive/src/parquet_field.rs:
##########
@@ -682,6 +838,66 @@ impl Type {
     }
 }
 
+// returns quote of function needed to convert from parquet
+// types back into the types used in the struct fields.
+pub fn get_convertable_quote() -> proc_macro2::TokenStream {

Review Comment:
   Yes we could. The issue I was facing is that the current `Field` struct contains a fairly complex `ty` field, so if we want to convert based on the stored type, we are going to have to do something like the code that follows this line:
   
   https://github.com/apache/arrow-rs/blob/175c7765939c0738defc736426c0b0a93b00bfa8/parquet_derive/src/parquet_field.rs#L87
   
   Except we also need to match on every type path as well.
   
   Since the compiler knows what type we want to return anyway, I figured this might save us another 200 lines of match statement.
   
   I understand the concern with including this with every quote. However, this trait will be inserted inside the `read_from_row_group` function, so the interface exposed to the end user is the same (i.e. the user can't access this trait when using the macro). I guess there is the further issue of this making the code somewhat more difficult to extend, but based on what you have said, there aren't many planned upgrades here anyway.
   
   Happy to instead use the match (or something else if you have any ideas), if you think that would be better?



-- 
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: github-unsubscribe@arrow.apache.org

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