Skip to content

ARROW-9753: [Rust] [DataFusion] Replaced Arc<Mutex<>> by Box<>#8307

Closed
jorgecarleitao wants to merge 1 commit intoapache:masterfrom
jorgecarleitao:box_iterator
Closed

ARROW-9753: [Rust] [DataFusion] Replaced Arc<Mutex<>> by Box<>#8307
jorgecarleitao wants to merge 1 commit intoapache:masterfrom
jorgecarleitao:box_iterator

Conversation

@jorgecarleitao
Copy link
Member

@jorgecarleitao jorgecarleitao commented Sep 30, 2020

This PR is built on top of #8225 and Replaces Arc<Mutex<dyn ...>> by Box<dyn ...>.

In the TopK example, I had to move some functions away from the impl. This is because self cannot be borrowed as mutable and immutable at the same time, and, during iteration, it was being borrowed as mutable (to update the BTree) and as immutable (to access the input). There is probably a better way of achieving this e.g. via interior mutability.

@github-actions
Copy link

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW the idea of getting rid of the mutex looks good to me. I am curious if you had a reason to prefer Box over Arc? If we use a Box, then anyone who wants to share results will have to either convert from Box --> Arc or will have to copy the entire Box.

@jorgecarleitao
Copy link
Member Author

I think that the reason is that an iterator like RecordBatchReader needs to be mutable to be iterated upon (regardless of whether we use next_batch or into_iter), while an Arc would require it to be immutable. Box implements BorrowMut, which addresses this.

@jorgecarleitao jorgecarleitao marked this pull request as ready for review October 2, 2020 03:25
Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants