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

fix: Capture groups should be ignored in replace_all when literal=True #19366

Merged
merged 2 commits into from
Oct 22, 2024

Conversation

corwinjoy
Copy link
Contributor

Fixes #18238.
From the docs, (https://docs.pola.rs/api/python/stable/reference/expressions/api/polars.Expr.str.replace_all.html#polars-expr-str-replace-all), when literal=True, then capture groups should be ignored. As the docs say:

The dollar sign ($) is a special character related to capture groups. To refer to a literal dollar sign, use $$ instead or set literal to True.

Fix the Rust logic which did not apply this logic for multi-row replacement.

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars labels Oct 22, 2024
reg.replace_all(s, val)
}
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still a bit new at Rust. I ended up putting an if in the lambda when what I really wanted was to create the lambda I need once based on the literal. This is probably slightly slower, but I doubt it matters.

Copy link
Member

Choose a reason for hiding this comment

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

The lambda's will be different. So you must either put them behind an indirection or call iter_and_replace in the branches.

But here it won't really matter indeed.

@corwinjoy
Copy link
Contributor Author

@adamreeve

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.20%. Comparing base (00650b9) to head (0b506c5).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
...rates/polars-plan/src/dsl/function_expr/strings.rs 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #19366      +/-   ##
==========================================
+ Coverage   80.02%   80.20%   +0.17%     
==========================================
  Files        1528     1523       -5     
  Lines      209871   209859      -12     
  Branches     2419     2434      +15     
==========================================
+ Hits       167954   168314     +360     
+ Misses      41366    40990     -376     
- Partials      551      555       +4     

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

@corwinjoy corwinjoy changed the title fix: Capture groups should be ignored in repalce_all when literal=True fix: Capture groups should be ignored in replace_all when literal=True Oct 22, 2024
@ritchie46 ritchie46 merged commit c731d11 into pola-rs:main Oct 22, 2024
26 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

ExprStringNameSpace replace / replace_all literal flag ignored for dataframes with multiple rows
2 participants