-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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.normalize()
#20483
base: main
Are you sure you want to change the base?
feat: Add str.normalize()
#20483
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20483 +/- ##
=======================================
Coverage 79.01% 79.01%
=======================================
Files 1563 1564 +1
Lines 220596 220665 +69
Branches 2492 2492
=======================================
+ Hits 174306 174367 +61
- Misses 45717 45725 +8
Partials 573 573 ☔ View full report in Codecov by Sentry. |
This kernel should not be written by collecting to a temporary pub fn normalize_with<F: Fn(&str, &mut String)>(ca: &StringChunked, normalizer: F) -> StringChunked {
let mut buffer = String::new();
let mut builder = StringChunkedBuilder::new(ca.name().clone(), ca.len());
for opt_s in ca.iter() {
if let Some(s) = opt_s {
buffer.clear();
normalizer(s, &mut buffer);
builder.append_value(&buffer);
} else {
builder.append_null();
}
}
builder.finish()
}
pub fn normalize(ca: &StringChunked, form: UnicodeForm) -> StringChunked {
match form {
UnicodeForm::NFC => normalize_with(ca, |s, b| b.extend(s.nfc())),
UnicodeForm::NFKC => normalize_with(ca, |s, b| b.extend(s.nfkc())),
UnicodeForm::NFD => normalize_with(ca, |s, b| b.extend(s.nfd())),
UnicodeForm::NFKD => normalize_with(ca, |s, b| b.extend(s.nfkd())),
}
} |
Thanks @orlp, I naively followed Updated benchmark:
(Can't really explain the change in magnitude compared to the first one but the gap between polars and pandas now is consistently there) |
Contributing to the Rust part for the first time so there are probably some quirks here and there. I used the suggestion in #11455 to use the
unicode_normalization
crate and mostly followed #12878. I don't know if you want to add this function or to implement it that way but it was good training for me anyway.Note that I'm not very familiar with this method so double-checking the output and maybe adding more corner cases to the test suite would be nice.
Quick performance check after
make build-release
:A bit disappointed with the performance, maybe I missed something obvious. There are also a couple of issues on performance in the Rust crate used: https://github.com/unicode-rs/unicode-normalization/issues?q=sort%3Aupdated-desc+is%3Aissue+is%3Aopen+performance
Fixes #5799
Fixes #11455