[GitHub] [arrow] sunchao commented on a change in pull request #7969: ARROW-9753: [Rust] [DataFusion] Remove use of Mutex from DataFusion Partition trait

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

sunchao commented on a change in pull request #7969:
URL: https://github.com/apache/arrow/pull/7969#discussion_r471017707



##########
File path: rust/arrow/src/ipc/reader.rs
##########
@@ -473,7 +476,7 @@ pub struct FileReader<R: Read + Seek> {
     blocks: Vec<ipc::Block>,
 
     /// A counter to keep track of the current block that should be read
-    current_block: usize,
+    current_block: AtomicUsize,

Review comment:
       curious why this is an atomic var now - will there by multiple threads accessing the same `FileReader` instance? 

##########
File path: rust/arrow/src/ipc/reader.rs
##########
@@ -783,21 +787,22 @@ impl<R: Read> RecordBatchReader for StreamReader<R> {
         self.schema.clone()
     }
 
-    fn next_batch(&mut self) -> Result<Option<RecordBatch>> {
-        if self.finished {
+    fn next_batch(&self) -> Result<Option<RecordBatch>> {
+        if self.finished.load(Ordering::SeqCst) {
             return Ok(None);
         }
         // determine metadata length
         let mut meta_size: [u8; 4] = [0; 4];
 
-        match self.reader.read_exact(&mut meta_size) {
+        let mut reader = self.reader.borrow_mut();

Review comment:
       this will panic when someone else is holding the reference, so we need to be careful - not sure if `try_borrow_mut` makes more sense here though.




----------------------------------------------------------------
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:
users@infra.apache.org