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

Added support for projections in reading IPC streams #1097

Merged

Conversation

joshuataylor
Copy link
Contributor

@joshuataylor joshuataylor commented Jun 23, 2022

Adds projections to IPC Streams, which is required for pola-rs/polars#3778 which in turn is required for elixir-explorer/explorer#265 馃殌

As this is my first PR, and I'm new to Rust, please give any feedback you find helpful 馃檶

Closes #1095

Backward incompatible changes

StreamReader::new has a new argument, projection. Add None to it to retain the original behavior.

@codecov
Copy link

codecov bot commented Jun 23, 2022

Codecov Report

Merging #1097 (73b2ab0) into main (5c5e727) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1097      +/-   ##
==========================================
+ Coverage   81.19%   81.22%   +0.02%     
==========================================
  Files         367      367              
  Lines       35340    35369      +29     
==========================================
+ Hits        28695    28727      +32     
+ Misses       6645     6642       -3     
Impacted Files Coverage 螖
src/io/ipc/read/stream.rs 92.77% <100.00%> (+1.53%) 猬嗭笍
src/bitmap/immutable.rs 80.86% <0.00%> (-0.62%) 猬囷笍
src/io/ipc/read/array/boolean.rs 97.91% <0.00%> (+8.33%) 猬嗭笍

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 5c5e727...73b2ab0. Read the comment docs.

@jorgecarleitao jorgecarleitao added the feature A new feature label Jun 24, 2022
Copy link
Owner

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

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

Flawless. This is ready to merge from the src and tests. Amazing work, @joshuataylor !

There is a small fix we need to apply - in integration-testing/src/bin/arrow-stream-to-file.rs:27:35

we need to add None to the call, since the API changed.

@joshuataylor
Copy link
Contributor Author

Ah, missed those, added it. Will remember to keep an eye on CI.

@jorgecarleitao
Copy link
Owner

Thanks! The CI for integration has been failing lately between C# and X. We use the same steps as apache/arrow and that is an issue from there. So, as long as there is not there all is good (I do not expect any impact - if it compiles we are good).

@jorgecarleitao jorgecarleitao changed the title Add support for projections to IPC streams Added support for projections to IPC streams Jun 24, 2022
@jorgecarleitao jorgecarleitao changed the title Added support for projections to IPC streams Added support for projections in reading IPC streams Jun 24, 2022
@jorgecarleitao jorgecarleitao merged commit b946b18 into jorgecarleitao:main Jun 24, 2022
@joshuataylor joshuataylor deleted the feature/ipc-stream-projections branch June 24, 2022 04:32
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.

Support projection to IPC streams
2 participants