-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: Reduce memcopy in parquet #19350
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19350 +/- ##
==========================================
- Coverage 80.24% 80.21% -0.04%
==========================================
Files 1523 1523
Lines 209545 209790 +245
Branches 2434 2434
==========================================
+ Hits 168148 168281 +133
- Misses 40842 40954 +112
Partials 555 555 ☔ View full report in Codecov by Sentry. |
crates/polars-io/src/mmap.rs
Outdated
match self { | ||
ReaderBytes::Borrowed(v) => MemSlice::from_static(v), | ||
ReaderBytes::Owned(v) => MemSlice::from_vec(v), | ||
ReaderBytes::Mapped(v, _) => MemSlice::from_mmap(Arc::new(v)), | ||
ReaderBytes::Owned(v) => MemSlice::from_static(unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't safe if the self is dropped before the MemSlice
.
I see that MemSlice
has an inner
that can have a bytes::Bytes
variant. If we have an ReaderBytes::Owned
we can move that vec into the MemSlice
.
I think we should also change the type form Vec<u8>
to Bytes
so we can freely clone it.
It should be sound since it matches what we did a long time ago
polars/crates/polars-io/src/parquet/read/read_impl.rs
Line 496 in 04714b9