[GitHub] [arrow-datafusion] retikulum commented on pull request #4168: Improve Error Handling and Readibility for downcasting `Decimal128Array`

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

retikulum commented on PR #4168:
URL: https://github.com/apache/arrow-datafusion/pull/4168#issuecomment-1310881295

   > Thank you for these changes, much appreciated!
   > 
   > I saw two issues with code style/structure and left inline comments. I think your other PRs may have similar issues in them too. It would be great if you can improve code quality by applying these suggestions to those code pieces too.
   > 
   > Short PRs are typically preferable to long PRs, but IMO this is a case where the opposite is true. Since the same kind of work was broken down to many PRs, the suggestions you get in one of those PRs will probably end up being applicable to many of them. Therefore, it sometimes is better to have one cohesive PR in cases like this.
   
   Thanks for review @ozankabak. These recommendations are great for improving code quality. I will change the suggested pieces. 
   
   However, I was confused with PR sizes. While we discussed implementation in the issue, @alamb also [suggested](https://github.com/apache/arrow-datafusion/issues/3152#issuecomment-1292654025) that these PRs should be small ( but he suggested a few functions unlike my one function per PR). Rather than implementing one function at one PR, it seems better to implement 3-5 (or more) functions for a PR. My next PRs will include more function implementation.


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