Skip to content
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

feat(rust): Implement IpcReaderAsync #14984

Merged
merged 7 commits into from
Mar 13, 2024

Conversation

mickvangelderen
Copy link
Contributor

@mickvangelderen mickvangelderen commented Mar 11, 2024

  • implement basics
  • support passing options when reading data
  • integrate in default engine

@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature rust Related to Rust Polars labels Mar 11, 2024
Copy link

codecov bot commented Mar 11, 2024

Codecov Report

Attention: Patch coverage is 66.46526% with 111 lines in your changes are missing coverage. Please review.

Project coverage is 80.95%. Comparing base (78dc628) to head (a146d06).
Report is 15 commits behind head on main.

Files Patch % Lines
crates/polars-io/src/ipc/ipc_reader_async.rs 52.56% 74 Missing ⚠️
...es/polars-plan/src/logical_plan/functions/count.rs 62.00% 19 Missing ⚠️
crates/polars-plan/src/logical_plan/builder.rs 75.00% 8 Missing ⚠️
py-polars/src/lazyframe/mod.rs 73.07% 7 Missing ⚠️
...olars-lazy/src/physical_plan/executors/scan/ipc.rs 93.75% 2 Missing ⚠️
crates/polars-io/src/ipc/ipc_file.rs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14984      +/-   ##
==========================================
- Coverage   80.99%   80.95%   -0.05%     
==========================================
  Files        1333     1338       +5     
  Lines      173175   173612     +437     
  Branches     2458     2460       +2     
==========================================
+ Hits       140260   140544     +284     
- Misses      32448    32599     +151     
- Partials      467      469       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mickvangelderen mickvangelderen force-pushed the implement-ipc-async-reader branch 2 times, most recently from 90f4831 to 3ecb4e3 Compare March 11, 2024 17:22
Copy link
Contributor Author

@mickvangelderen mickvangelderen left a comment

Choose a reason for hiding this comment

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

This PR is now in a state where the core seems to work. At the same time the tests are inadequate and this PR also introduces a lot of duplication. Together, this makes it hard to be confident about this change working well for more than the most basic cases.

crates/polars-io/src/ipc/ipc_file.rs Outdated Show resolved Hide resolved
crates/polars-io/src/ipc/ipc_reader_async.rs Outdated Show resolved Hide resolved
crates/polars-io/src/ipc/ipc_reader_async.rs Outdated Show resolved Hide resolved
py-polars/tests/unit/io/test_lazy_ipc.py Outdated Show resolved Hide resolved
py-polars/src/lazyframe/mod.rs Show resolved Hide resolved
@mickvangelderen mickvangelderen force-pushed the implement-ipc-async-reader branch 2 times, most recently from 0d2de57 to d05ae08 Compare March 11, 2024 17:37
Copy link
Member

@ritchie46 ritchie46 left a comment

Choose a reason for hiding this comment

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

Alright, I've left some comments. I think it looks good. We can clean it up and have a second pass. 👍

crates/polars-io/src/ipc/ipc_file.rs Outdated Show resolved Hide resolved
crates/polars-io/src/ipc/ipc_reader_async.rs Show resolved Hide resolved
crates/polars-io/src/ipc/ipc_reader_async.rs Outdated Show resolved Hide resolved
crates/polars-io/src/ipc/ipc_reader_async.rs Outdated Show resolved Hide resolved
crates/polars-io/src/ipc/ipc_reader_async.rs Outdated Show resolved Hide resolved
crates/polars-plan/src/logical_plan/builder.rs Outdated Show resolved Hide resolved
crates/polars-plan/src/logical_plan/builder.rs Outdated Show resolved Hide resolved
crates/polars-io/src/ipc/ipc_reader_async.rs Show resolved Hide resolved
@ritchie46 ritchie46 merged commit bbaf341 into pola-rs:main Mar 13, 2024
24 checks passed
@ritchie46
Copy link
Member

Great!

@mickvangelderen mickvangelderen deleted the implement-ipc-async-reader branch March 13, 2024 09:23
@c-peters c-peters added the accepted Ready for implementation label Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation enhancement New feature or an improvement of an existing feature rust Related to Rust Polars
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants