jecsand838 commented on PR #8274: URL: https://github.com/apache/arrow-rs/pull/8274#issuecomment-3259422115
> Bunch of nits to simplify the code, but otherwise ready to merge. > > > There are two items I wanted to follow-up on in future PRs: > > ``` > > 1. Further `arrow-avro/src/schema.rs` file refactoring (if possible). As I updated the file I saw other potential areas to improve. For example, I think if we introduced some sort of struct to hold context for `datatype_to_avro`, we could simplify and improve the logic. > > ``` > > Definitely future work. > > > ``` > > 2. Renaming `Nullability`. The name change will impact other areas of the code not related to the `Writer`. I'd also want to consider the impact `Union` type support will have as well when we get there. But I do think @scovich has a valid point about the current naming convention. > > ``` > > Yes. Mixing refactors with new functionality is always messy. > > I would recommend two other follow-ups: 3. Profile-guided placement of `#[inline]` markers 4. Harmonize arg ordering for the various encoding methods that take `out`; some pass it as the first arg, others as the second arg. @scovich All great call outs. I accepted your latest suggestions and pushed up changes that harmonized the arg ordering for the methods taking `out` as a parameter. As always thank you for the solid and thorough review 😄. We'll also be sure to follow-up on those PRs. @alamb @mbrobbel Would either of you be able to look over this PR if you get a chance? -- 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]
