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: Expressify str.slice #13747

Merged
merged 2 commits into from
Jan 15, 2024
Merged

feat: Expressify str.slice #13747

merged 2 commits into from
Jan 15, 2024

Conversation

reswqa
Copy link
Collaborator

@reswqa reswqa commented Jan 15, 2024

This comes from @cmdlineluser's nice work. It's hard to make any changes in the original commit due to too many code conflicts, but I'm listing him as a co-author to keep authorship :)

@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 15, 2024
@reswqa
Copy link
Collaborator Author

reswqa commented Jan 15, 2024

@alexander-beedie You might be interested in this :) This will work well with str.find as you mentioned in discord, we can do something like s = pl.Series(["abc12de"]) s.str.slice(s.str.find(r"[0-9]+"))then.

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Jan 15, 2024

@alexander-beedie You might be interested in this :) This will work well with str.find as you mentioned in discord, we can do something like s = pl.Series(["abc12de"]) s.str.slice(s.str.find(r"[0-9]+"))then.

Powerful combination! 😎
(And will help a lot with some additional/planned SQL support)

@@ -905,11 +905,11 @@ impl SQLFunctionVisitor<'_> {
Reverse => self.visit_unary(|e| e.str().reverse()),
Right => self.try_visit_binary(|e, length| {
Ok(e.str().slice( match length {
Expr::Literal(LiteralValue::Int64(n)) => -n,
Expr::Literal(LiteralValue::Int64(n)) => lit(-n),
Copy link
Collaborator Author

@reswqa reswqa Jan 15, 2024

Choose a reason for hiding this comment

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

@alexander-beedie I've mostly left the SQL part as it is, so feel free to push a new commit or open a new PR if you want to improve it to work better with expressions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will do; thx! :))

})
},
(len_a, 1, len_c) if len_a == len_c => {
fn infer<F: for<'a> FnMut(Option<&'a str>, Option<u64>) -> Option<&'a str>>(f: F) -> F where
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The compiler is a bit annoying, so we have to introduce this infer function to keep it happy.

Co-authored-by: cmdlineluser <99486669+cmdlineluser@users.noreply.github.com>
@reswqa reswqa marked this pull request as ready for review January 15, 2024 16:12
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.

Nice one. Thanks @mcrumiller and @reswqa for the unified effort. 😄

@ritchie46 ritchie46 merged commit 0d1b4a7 into pola-rs:main Jan 15, 2024
23 checks passed
@mcrumiller
Copy link
Contributor

@ritchie46 I think you meant to thank @cmdlineluser, I didn't contribute to this one!

@9SMTM6
Copy link

9SMTM6 commented Jan 21, 2024

I think this silently at the very least broke the API in a release that according to semver should not break API (if polars upholds semver, wasn't able to find a statement at a quick glance).

Luckily I had some assertions going around.

Issue with minimum example coming up.

IDK what precisely is going on there, but at this point it looks like its all my fault, sorry if I alarmed someone. Unless I cleared that up and found a minimal example that shows a difference between releases, I'll have to retract this.

r-brink pushed a commit to r-brink/polars that referenced this pull request Jan 24, 2024
Co-authored-by: cmdlineluser <99486669+cmdlineluser@users.noreply.github.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 python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants