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.

Sign up for GitHub

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

Jump to bottom

[FEAT] Adds str.length_bytes() function #2775

Merged
merged 3 commits into from
Sep 5, 2024
Merged

[FEAT] Adds str.length_bytes() function #2775

merged 3 commits into from
Sep 5, 2024

Conversation

thomasjpfan
Copy link
Contributor

Resolves #2770

@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request labels Aug 31, 2024
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Aug 31, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #2775 will improve performances by ×5.9

Comparing thomasjpfan:adds_length_bytes (718ec41) with main (7fe3dbc)

Summary

⚡ 1 improvements
✅ 15 untouched benchmarks

Benchmarks breakdown

Benchmark main thomasjpfan:adds_length_bytes Change
⚡ test_show[100 Small Files] 296.6 ms 50 ms ×5.9

jaychia
jaychia reviewed Sep 1, 2024
View reviewed changes
Copy link
Contributor

@jaychia jaychia left a comment

Choose a reason for hiding this comment

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

Thanks! This is looking pretty good, just some extra tests and it should be good to go :)

tests/series/test_utf8_ops.py Outdated
def test_series_utf8_length_bytes() -> None:
s = Series.from_arrow(pa.array(["😉test", "hey̆", "baz"]))
result = s.str.length_bytes()
assert result.to_pylist() == [8, 5, 3]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also add some tests for empty string and nulls?

@netlify Netlify
Copy link

netlify bot commented Sep 3, 2024 •
edited
Loading

❌ Deploy Preview for daft-docs failed. Why did it fail? →

Name Link
🔨 Latest commit e4aa748
🔍 Latest deploy log https://app.netlify.com/sites/daft-docs/deploys/66d7857b2c49340008a8c219

jaychia
jaychia approved these changes Sep 5, 2024
View reviewed changes
Copy link
Contributor

@jaychia jaychia left a comment

Choose a reason for hiding this comment

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

Looking good, thanks for the contribution and congrats on first PR! 🚀 🚀 🚀 🚀 🚀 🚀

@jaychia jaychia merged commit 6fe408c into Eventual-Inc:main Sep 5, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaychia jaychia jaychia approved these changes

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UTF-8 string length_bytes()
2 participants
@thomasjpfan @jaychia