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): Add null_on_oob parameter to expr.list.get #15395

Merged
merged 6 commits into from
Apr 1, 2024

Conversation

JamesCE2001
Copy link
Contributor

@JamesCE2001 JamesCE2001 commented Mar 29, 2024

Addresses first part of #15240 (first time contributing so I figured I'd add 1 and get feedback before embarking on the rest, especially as they don't seem to overlap as much as I'd expected.)

For now leaves the default behavior to null_on_oob=True (see discussion below; we will revisit the default behavior for once it has been implemented for all functions in #15240).

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

codecov bot commented Mar 29, 2024

Codecov Report

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

Project coverage is 81.14%. Comparing base (39b486e) to head (8004174).
Report is 42 commits behind head on main.

Files Patch % Lines
crates/polars-plan/src/dsl/function_expr/list.rs 80.00% 3 Missing ⚠️
crates/polars-plan/src/dsl/list.rs 50.00% 2 Missing ⚠️
crates/polars-sql/src/functions.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15395      +/-   ##
==========================================
- Coverage   81.37%   81.14%   -0.24%     
==========================================
  Files        1363     1361       -2     
  Lines      176803   174683    -2120     
  Branches     2531     2531              
==========================================
- Hits       143869   141738    -2131     
- Misses      32449    32459      +10     
- Partials      485      486       +1     

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

@reswqa
Copy link
Collaborator

reswqa commented Mar 31, 2024

It looks like we changed the default behavior of list.get, this should be a breaking change. What I wish was that we had an explicit discussion of the default value for null_on_oob in #15240 first.

@ritchie46
Copy link
Member

It looks like we changed the default behavior of list.get, this should be a breaking change

Yes, we shouldn't change the default behaviour yet. We can do that after all operations in #15240 f are supported and we go to 1.0.

@ritchie46
Copy link
Member

@JamesCE2001 can you default to null_on_oob = True. We can make these consistent later.

@JamesCE2001
Copy link
Contributor Author

@JamesCE2001 can you default to null_on_oob = True. We can make these consistent later.

Done, apologies for jumping the gun. Updated original PR comment to reflect.

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.

Looks good @JamesCE2001.Thank you!

@ritchie46 ritchie46 merged commit 2c9b6c1 into pola-rs:main Apr 1, 2024
25 checks passed
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.

4 participants