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

refactor(rust): Replace DynArgs with an enum containing all its variants #18746

Merged
merged 1 commit into from
Sep 22, 2024

Conversation

3ok
Copy link
Contributor

@3ok 3ok commented Sep 14, 2024

This PR presents one way to fix #18595.

Information present in DynArgs was being lost after a serialization/deserialization roundtrip, so I had to implement Serialize and Deserialize in order to handle them.

This is my first contribution so I'm pretty excited about that!

@github-actions github-actions bot added fix Bug fix rust Related to Rust Polars labels Sep 14, 2024
Copy link

codecov bot commented Sep 14, 2024

Codecov Report

Attention: Patch coverage is 91.39785% with 8 lines in your changes missing coverage. Please review.

Project coverage is 79.86%. Comparing base (962b576) to head (4ada469).
Report is 35 commits behind head on main.

Files with missing lines Patch % Lines
...ow/src/legacy/kernels/rolling/no_nulls/quantile.rs 78.57% 3 Missing ⚠️
...arrow/src/legacy/kernels/rolling/nulls/quantile.rs 83.33% 2 Missing ⚠️
...ow/src/legacy/kernels/rolling/no_nulls/variance.rs 87.50% 1 Missing ⚠️
...arrow/src/legacy/kernels/rolling/nulls/variance.rs 83.33% 1 Missing ⚠️
...polars-core/src/frame/group_by/aggregations/mod.rs 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #18746      +/-   ##
==========================================
- Coverage   79.86%   79.86%   -0.01%     
==========================================
  Files        1517     1517              
  Lines      205542   205533       -9     
  Branches     2892     2892              
==========================================
- Hits       164156   164146      -10     
- Misses      40838    40839       +1     
  Partials      548      548              

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

@ritchie46
Copy link
Member

Hey @3ok. Thanks for the PR. Your conclusion is right. We need to support serde.

I only think we should do that on a different manner. I think we should get rid of DynArgs altogether and create a enum that supports serde, with variants for all DynArgs possibilities.

@3ok 3ok changed the title fix(rust): Implement Serialize and Deserialize for RollingOptionsFixedWindow and RollingOptionsDynamicWindow refactor(rust): Replace DynArgs with an enum containing all its variants Sep 15, 2024
@github-actions github-actions bot added the internal An internal refactor or improvement label Sep 15, 2024
@3ok
Copy link
Contributor Author

3ok commented Sep 15, 2024

Hi @ritchie46, thanks for the feedback.

I only think we should do that on a different manner. I think we should get rid of DynArgs altogether and create a enum that supports serde, with variants for all DynArgs possibilities.

I have added a new enum RollingFnParams containing all DynArgs possibilities and changed all DynArgs occurrences with Option<RollingFnParams>.

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.

Thanks!

@ritchie46 ritchie46 merged commit b618734 into pola-rs:main Sep 22, 2024
26 checks passed
@3ok 3ok deleted the impl-serde-rolling-options branch September 25, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix internal An internal refactor or improvement rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PanicException occurs when applying a deserialized rolling_quantile
2 participants