[GitHub] [arrow] paddyhoran commented on pull request #7666: ARROW-8559: [Rust] Consolidate Record Batch reader traits in main arrow crate

2020-07-11 Thread GitBox 


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

2020-07-11 Thread GitBox 


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

2020-07-10 Thread GitBox 


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

2020-07-09 Thread GitBox 


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

2020-07-07 Thread GitBox 


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