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,python): Bitwise groupby aggregations #10260

Closed
wants to merge 4 commits into from

Conversation

jg2562
Copy link

@jg2562 jg2562 commented Aug 3, 2023

A very primitive attempt at implementing #5009 allowing for bitwise xor/or/and on a lazy group by. Implementation is mirrored off of the LazyGroupBy sum implemenation.

Several parts are unimplemented as they seem to require a change to arrow2 as well to match the SIMD requirements.

@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature rust Related to Rust Polars labels Aug 3, 2023
@jg2562 jg2562 changed the title feat(rust): Bitwise groupby aggregations feat(rust,python): Bitwise groupby aggregations Aug 7, 2023
@github-actions github-actions bot added the python Related to Python Polars label Aug 7, 2023
@jg2562
Copy link
Author

jg2562 commented Aug 22, 2023

@stinodego Curious what my next steps should be to get this in shape for an actual PR

Some questions I have:

  1. Where should I be putting tests to verify the functionality for adding the lazy expressions?
  2. Which chunk arrays should I be implementing?
  3. For the SIMD stuff, should I make a PR to pyarrow2? Or do I implement it in a different way?
  4. In the aggregation functions, im not sure I understand what GroupsProxy::Slice is or how it gets used.
  5. I want to double check, this should be an expression not a operation right?

I know this is a pretty rough PR but hopefully I can polish it up. From what I can tell the remaining things I need to do are:

  • Add tests
  • Add examples
  • Fix any logic I didn't fully understand
  • Implement the correct chunk arrays
  • Add SIMD capabilities?
  • Rebase onto main

Please let me know if there's anything more or anything else needs to be revised in how I'm going about this, thanks!

@jg2562
Copy link
Author

jg2562 commented Sep 4, 2023

@ritchie46 Would you be able to let me know what I need to do to progress this draft to a PR?

@stinodego
Copy link
Member

Thank you for the work here @jg2562 . Unfortunately, we didn't get to review this in time. By now it has gathered a bunch of conflicts, so I'll close this PR. If you're still interested in implementing this, please open a fresh PR and we can take a look. The linked issue has been accepted.

@stinodego stinodego closed this Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants