[GitHub] [arrow] paddyhoran commented on pull request #7666: ARROW-8559: [Rust] Consolidate Record Batch reader traits in main arrow crate
paddyhoran commented on pull request #7666: URL: https://github.com/apache/arrow/pull/7666#issuecomment-657110158 I adopted `SchemaRef` throughout the codebase also. I figure if we are going to use `SchemaRef` in some places we might as well adopt it universally. 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: us...@infra.apache.org
[GitHub] [arrow] paddyhoran commented on pull request #7666: ARROW-8559: [Rust] Consolidate Record Batch reader traits in main arrow crate
paddyhoran commented on pull request #7666: URL: https://github.com/apache/arrow/pull/7666#issuecomment-657109850 Ok, I think we are all in agreement. Thanks. 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: us...@infra.apache.org
[GitHub] [arrow] paddyhoran commented on pull request #7666: ARROW-8559: [Rust] Consolidate Record Batch reader traits in main arrow crate
paddyhoran commented on pull request #7666: URL: https://github.com/apache/arrow/pull/7666#issuecomment-656856701 > I think the `schema` method is already returning a `Arc`, where is the `Rc` being used? IMHO if it is not too difficult to do we should consolidate on a single trait now instead of having to change it later again. It did occur to me that we could just have a single trait that uses `Arc` and not bother with the `Rc` version. I'm not sure what the performance of `Arc` is versus `Rc`. Obviously, there has to be some difference but is this important in the context of Arrow? I'm up for converting to one trait and using `Arc`. If users request it then we can add the `Rc` version later. What do others think? The `Rc` version is used in a few places but again I don't think it's that important to use `Rc` over `Arc`. 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: us...@infra.apache.org
[GitHub] [arrow] paddyhoran commented on pull request #7666: ARROW-8559: [Rust] Consolidate Record Batch reader traits in main arrow crate
paddyhoran commented on pull request #7666: URL: https://github.com/apache/arrow/pull/7666#issuecomment-656306027 cc @maxburke @mcassels you might have opinions on this. 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: us...@infra.apache.org
[GitHub] [arrow] paddyhoran commented on pull request #7666: ARROW-8559: [Rust] Consolidate Record Batch reader traits in main arrow crate
paddyhoran commented on pull request #7666: URL: https://github.com/apache/arrow/pull/7666#issuecomment-655100102 cc @houqp 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: us...@infra.apache.org