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: Add str.head and str.tail #14425

Merged
merged 13 commits into from Apr 13, 2024
Merged

Conversation

mcrumiller
Copy link
Contributor

Resolves #10337.

@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 Feb 11, 2024
@mcrumiller mcrumiller marked this pull request as ready for review February 11, 2024 22:44
@JulianCologne
Copy link
Contributor

nice addition! 😃

I would recommend a small change in the test

the check for "foobar" with

  • head(-3) == "foo"
  • tail(-3) == "bar"

is a little confusing because this would also work if the function just took the absolute value.

"abcde" with

  • head(2) == "ab" & head(-2) == "abc"
  • tail(2) == "de" & tail(-2) == "cde"

would be a little clearer to understand and is not ambiguous

@@ -526,6 +526,26 @@ impl StringNameSpace {
)
}

/// Take the first `n` characters of the string values.
pub fn head(self, n: Expr) -> Expr {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we can't just define these in terms of a slice operation - that would save a lot of code bloat. But that might not work with negative indices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @stinodego -- this was my initial intent, when I said I would piggyback on @reswqa's implementation of str.slice. I realized soon after that the negative indexing for head requires calculation of the string length to determine the end of the slice.

Here are the operations and their slice equivalents:

s.str.head(3)   # s.str.slice(3, None)
s.str.head(-3)  # no equivalent: must know string length for start offset

s.str.tail(3)   # s.str.slice(-3, None)
s.str.tail(-3)  # s.str.slice(3, None)

So for tail, we could do it because slice can run to the end of the string by itself, but for head we have no recourse. I suppose this would save us a little bit of bloat but it does make the code a bit asymmetric, but on the other hand the tail implementation is a bit more performant than slice because we have one fewer parameters, and so we have more fast paths. So it's a tradeoff here. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I guess you can use slice in combination with len_chars, but clearly it will be a bit more efficient to have a dedicated implementation like in this PR. I'll leave it to Ritchie to be the judge here.

Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

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

Thanks! I left a few comments, I'll leave the proper review to Ritchie or Orson.

Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

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

The docstrings could use a bit more love (you can probably copy paste some stuff from slice) and a doc entry is missing in the API reference.

If you could address this, I'll approve and leave it to someone to judge the Rust side of things.

py-polars/polars/series/string.py Outdated Show resolved Hide resolved
py-polars/polars/series/string.py Outdated Show resolved Hide resolved
@mcrumiller
Copy link
Contributor Author

mcrumiller commented Feb 12, 2024

@stinodego I've updated the docstrings with more detail and more examples.

I cannot for the life of me figure out why the CI doctest is failing with an "unexpected indentation" error. My doctests pass fine locally and I can't determine which part is causing the error.

I do note that when I run code locally, Series show 8 spaces of indentation:

>>> import polars as pl
>>> s = pl.Series(["pear", None, "papaya", "dragonfruit"])
>>> s.str.head(-3)
shape: (4,)
Series: '' [str]
[
        "p"
        null
        "pap"
        "dragonfr"
]

And the examples in string.py are a hodgepodge of 4 or 8 spaces. str.explode, for example, has 8 spaces in its docstring examples, but those do not seem to cause an error, whereas str.contains has only 4 spaces in its examples, and also does not cause an error. Could this be the issue?

Edit: I suspect this is the case, as my local doctest does not complain. I've reduced to 4 and we'll see how that fares.
Edit2: nope, still failing.

@stinodego
Copy link
Member

It's probably an unclosed backtick. I can take a look. There are some issues with the docstring formatting anyway that I can see won't render.

py-polars/polars/series/string.py Outdated Show resolved Hide resolved
crates/polars-ops/src/chunked_array/strings/substring.rs Outdated Show resolved Hide resolved
Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

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

All right, Python side looks good to me. Leaving the Rust side review to @ritchie46

@coolstudio1678
Copy link

When can it be used in rust?

@mcrumiller
Copy link
Contributor Author

It needs to be approved first. @ritchie46 would you mind taking a look?

@ritchie46
Copy link
Member

I hope to get to this today. I am a bit worried about sliced that are within char boundaries.

@mcrumiller
Copy link
Contributor Author

I am a bit worried about sliced that are within char boundaries.

Do we have reason not to trust str.len(), or do you mean that we must be very careful?

(_, 1) => {
// SAFETY: `n` was verified to have at least 1 element.
let n = unsafe { n.get_unchecked(0) };
unary_elementwise(ca, |str_val| head_binary(str_val, n)).with_name(ca.name())
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be changed, string head/tail must be defined in terms of codepoints, not bytes! Otherwise you get illegal UTF-8 and general nonsense. Please change this and add a test-case that tests this, for example:

import polars as pl

df = pl.DataFrame({"s": ["你好世界"]})
head = pl.DataFrame({"s": ["你好"]})
tail = pl.DataFrame({"s": ["世界"]})
assert_frame_equal(df.select(pl.col.s.str.head(2)), head)
assert_frame_equal(df.select(pl.col.s.str.tail(2)), tail)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @orlp, may have to make a few changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI I am noticing that str.slice does not properly index codepoints with negative indexes. Using your example:

s = pl.Series(["你好世界"])
tail = "界"
s.str.slice(-1)  # should be equivalent to "tail"
# shape: (1,)
# Series: '' [str]
# [
#         ""
# ]

I'll see if I can address this as a separate issue once I have finished with this one. Edit: opened #15136.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orlp the new impl respects code points instead of bytes. I added some specific code point tests using your example. Let me know if anything looks off to you!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good now.

@mcrumiller mcrumiller marked this pull request as draft March 18, 2024 14:34
@mcrumiller mcrumiller marked this pull request as ready for review March 18, 2024 19:55
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

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

Project coverage is 81.15%. Comparing base (dcee934) to head (0b9f353).
Report is 4 commits behind head on main.

Files Patch % Lines
...rates/polars-plan/src/dsl/function_expr/strings.rs 80.76% 5 Missing ⚠️
py-polars/src/expr/general.rs 50.00% 2 Missing ⚠️
.../polars-ops/src/chunked_array/strings/substring.rs 98.52% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14425      +/-   ##
==========================================
+ Coverage   81.14%   81.15%   +0.01%     
==========================================
  Files        1363     1363              
  Lines      175282   175408     +126     
  Branches     2527     2527              
==========================================
+ Hits       142236   142360     +124     
- Misses      32568    32571       +3     
+ Partials      478      477       -1     

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

(_, 1) => {
// SAFETY: `n` was verified to have at least 1 element.
let n = unsafe { n.get_unchecked(0) };
unary_elementwise(ca, |str_val| head_binary(str_val, n)).with_name(ca.name())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good now.

@mcrumiller mcrumiller requested a review from reswqa as a code owner April 8, 2024 15:59
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. Thanks @mcrumiller. Sorry for the delay on this one.

@ritchie46 ritchie46 merged commit 429d3dd into pola-rs:main Apr 13, 2024
24 checks passed
@mcrumiller mcrumiller deleted the str-head-tail branch April 13, 2024 15:43
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.

Add .str.head and .str.tail
6 participants