Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

feat: allow memory mapping a slice into a primitive array. #1369

Closed
wants to merge 5 commits into from

Conversation

ritchie46
Copy link
Collaborator

@ritchie46 ritchie46 commented Jan 20, 2023

I often need to an arrow kernel without owning the data. This allows you to create a PrimitiveArray of the slice data, use the needed kernel and drop the array immediately all while the slice is in scope.

Example usage in the wild

    pub unsafe fn take_unchecked_from_slice(
        &self,
        idx: &[IdxSize],
    ) -> PolarsResult<Series> {
        let idx = IdxCa::borrowed_from_slice("", idx);
        self.take_unchecked(&idx)
    }

@codecov
Copy link

codecov bot commented Jan 20, 2023

Codecov Report

Base: 83.34% // Head: 83.31% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (ff3121e) compared to base (ca14ef0).
Patch coverage: 79.38% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1369      +/-   ##
==========================================
- Coverage   83.34%   83.31%   -0.04%     
==========================================
  Files         371      373       +2     
  Lines       40321    40340      +19     
==========================================
+ Hits        33607    33610       +3     
- Misses       6714     6730      +16     
Impacted Files Coverage Δ
src/ffi/mmap/slice.rs 0.00% <0.00%> (ø)
src/ffi/mod.rs 100.00% <ø> (ø)
src/ffi/mmap/mod.rs 98.41% <98.41%> (ø)
src/ffi/mmap/ipc.rs 96.49% <100.00%> (ø)
src/io/ipc/read/file.rs 96.87% <0.00%> (-0.45%) ⬇️
src/bitmap/utils/slice_iterator.rs 98.78% <0.00%> (+1.21%) ⬆️
src/io/ipc/read/array/boolean.rs 98.11% <0.00%> (+5.66%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jorgecarleitao
Copy link
Owner

jorgecarleitao commented Jan 25, 2023

Interesting idea! What do you think of a slightly different API: https://github.com/jorgecarleitao/arrow2/compare/added_mmap_slice?expand=1 (second commit is the relevant change)

Essentially:

  • Not expose it in PrimitiveArray, but on arrow2::ffi::mmap::slice, to make it obvious that we are mmapping
  • Avoid the extra clone by calling try_from_ffi directly
  • Avoid the transmute explicitly via bytemuck::cast_slice
  • Re-organize the modules so that the IPC-specific mmap is in another module

@ritchie46
Copy link
Collaborator Author

Oh, I didn't see you already had a mockup PR. 🙈

Let me know what you want to do. I can rebase to previous commit and merge yours?

@jorgecarleitao jorgecarleitao added the feature A new feature label Feb 2, 2023
@jorgecarleitao
Copy link
Owner

Oh, I didn't see you already had a mockup PR. 🙈

Let me know what you want to do. I can rebase to previous commit and merge yours?

I mocked after looking at yours - was trying to understand the gist of this PR and modifying it as I was reading it. ^^

I think probably merging mine on this one is easiest, but I have no preference.

@ritchie46
Copy link
Collaborator Author

Alright, I just PRed your mockup, I will close this one.

@ritchie46 ritchie46 closed this Feb 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants