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

Replaced MutableBuffer by std::Vec #693

Merged
merged 1 commit into from Dec 21, 2021
Merged

Replaced MutableBuffer by std::Vec #693

merged 1 commit into from Dec 21, 2021

Conversation

jorgecarleitao
Copy link
Owner

@jorgecarleitao jorgecarleitao commented Dec 20, 2021

This PR replaces

  • cache_aligned cargo feature
  • the custom implementation of Vec under the feature gate
  • the custom allocator
  • MutableBuffer

by Rust's std::vec::Vec.

We still need Bytes for the ffi, but this significantly simplifies the APIs for Rust users.

Backward incompatible changes

To migrate:

  1. replace Buffer::from(slice) by Buffer::from_slice(slice) (from is now from Vec)
  2. replace .from_trusted_len_iter and the like by .extend (since the std lib supports specialization and will pick the trusted len implementation by default)
  3. replace MutableBuffer by Vec
  4. replace a.extend_constant(additional, value) by a.resize(a.len() + additional, value)

@codecov
Copy link

codecov bot commented Dec 20, 2021

Codecov Report

Merging #693 (7db249a) into main (f64339c) will increase coverage by 0.67%.
The diff coverage is 84.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #693      +/-   ##
==========================================
+ Coverage   69.77%   70.45%   +0.67%     
==========================================
  Files         303      300       -3     
  Lines       16855    16589     -266     
==========================================
- Hits        11761    11688      -73     
+ Misses       5094     4901     -193     
Impacted Files Coverage Δ
src/array/map/mod.rs 42.62% <0.00%> (ø)
src/compute/sort/mod.rs 26.82% <0.00%> (+0.63%) ⬆️
src/compute/take/list.rs 95.00% <ø> (ø)
src/io/ipc/read/array/binary.rs 48.27% <0.00%> (ø)
src/io/ipc/read/array/list.rs 25.00% <0.00%> (ø)
src/io/ipc/read/array/map.rs 25.00% <0.00%> (ø)
src/io/ipc/read/array/utf8.rs 48.27% <0.00%> (ø)
src/io/parquet/read/binary/basic.rs 68.32% <0.00%> (ø)
src/io/parquet/read/binary/nested.rs 50.00% <ø> (ø)
src/io/parquet/read/binary/utils.rs 100.00% <ø> (ø)
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f64339c...7db249a. Read the comment docs.

@ritchie46
Copy link
Collaborator

This is awesome! <3

@jorgecarleitao
Copy link
Owner Author

@sundy-li , would this be ok for you?

@sundy-li
Copy link
Collaborator

sundy-li commented Dec 21, 2021

That's ok since they have similar pub methods.

Is there any motivation to have MutableBuffer instead of vec at first?

@jorgecarleitao
Copy link
Owner Author

MutableBuffer was needed due to the custom allocator aligned to cache lines, under the feature flag cache_aligned. This PR proposes removing the custom allocator (and consequently the MutableBuffer). I never saw any difference between them, and using Vec is safer and much lower maintenance (imo).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants