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, cli): Add ignore_nulls for list.join #13701

Merged
merged 1 commit into from
Jan 21, 2024

Conversation

reswqa
Copy link
Collaborator

@reswqa reswqa commented Jan 13, 2024

This should be a breaking change as null value is no longer be treated as a string null.

@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 Jan 13, 2024
@reswqa reswqa changed the title feat(rust, python, cli!): Add ignore_nulls for list.join feat(rust, python, cli)!: Add ignore_nulls for list.join Jan 13, 2024
@github-actions github-actions bot added the breaking Change that breaks backwards compatibility label Jan 13, 2024
@alexander-beedie alexander-beedie force-pushed the list_join_ignore_nulls branch 2 times, most recently from 619de08 to 4fae592 Compare January 14, 2024 20:54
@reswqa reswqa marked this pull request as ready for review January 15, 2024 02:57
@reswqa
Copy link
Collaborator Author

reswqa commented Jan 17, 2024

Rebased on master to resolve the conflict. And @stinodego fancy taking a look at this? The changes are relatively minor, but breaking compatibility.

@ritchie46
Copy link
Member

I think we must wait until a breaking release then. :/

@reswqa
Copy link
Collaborator Author

reswqa commented Jan 19, 2024

I think we must wait until a breaking release then.

Make sense, we should merge this when 1.0 is about to be released. Are we close to it?

@stinodego
Copy link
Member

stinodego commented Jan 19, 2024

Just to be clear, the existing behavior is:

import polars as pl

s = pl.Series([["a", None, "b"], ["c", None]])
result = s.list.join("")
print(result)
shape: (2,)
Series: '' [str]
[
        "anullb"
        "cnull"
]

That looks broken to me and I can't imagine anyone relies on this behavior. In my opinion, this PR can be merged as a fix.

@alexander-beedie
Copy link
Collaborator

@reswqa: looks like the latest polars-sql merges created a conflict; I can fix that when I get home tonight 👌

@reswqa
Copy link
Collaborator Author

reswqa commented Jan 19, 2024

That looks broken to me and I can't imagine anyone relies on this behavior. In my opinion, this PR can be merged as a fix.

Ohh, It occurs to me that our introduction of the ignore_nulls to str.concat is also treated as a bug-fix RP.

@alexander-beedie
Copy link
Collaborator

Resolved merge conflicts / rebased 👍

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Jan 19, 2024

That looks broken to me and I can't imagine anyone relies on this behavior. In my opinion, this PR can be merged as a fix.

@stinodego: Yup; it also enables a definite fix on the SQL side (for slightly different reasons - concat and concat_ws both require use of the new ignore_nulls=True option to match standard SQL behaviour, although we might still need to hook that up - will check that now); I'd say go for it.

@alexander-beedie alexander-beedie added fix Bug fix and removed breaking Change that breaks backwards compatibility labels Jan 20, 2024
@alexander-beedie alexander-beedie changed the title feat(rust, python, cli)!: Add ignore_nulls for list.join feat(rust, python, cli): Add ignore_nulls for list.join Jan 20, 2024
Co-authored-by: alexander-beedie <alexander.m.beedie@icloud.com>
@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Jan 21, 2024

And one last rebase (following #13877).
Once CI succeeds, let's merge 👍

@alexander-beedie alexander-beedie merged commit 3d688ce into pola-rs:main Jan 21, 2024
23 checks passed
r-brink pushed a commit to r-brink/polars that referenced this pull request Jan 24, 2024
…13701)

Co-authored-by: alexander-beedie <alexander.m.beedie@icloud.com>
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 fix Bug fix 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