-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add replace operator #897
Add replace operator #897
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes generally look good. Minor suggestions
dask_sql/physical/rex/core/call.py
Outdated
if isinstance(s, str): | ||
return s.replace(pat, repl) | ||
elif isinstance(s, dd.Series): | ||
return s.str.replace(pat, repl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestion: Can replace the is instance
checks with something similar to this
dask-sql/dask_sql/physical/rex/core/call.py
Lines 481 to 482 in 2dfa5f7
if is_frame(s): | |
s = s.str |
REPLACE(a, 'r', 'l') as w, | ||
REPLACE('Another String', 'th', 'b') as x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add some tests to the unit/test_call::test_string_operations
unit tests. Preferable with some edge cases if possible
Codecov Report
@@ Coverage Diff @@
## main #897 +/- ##
==========================================
+ Coverage 75.20% 75.38% +0.17%
==========================================
Files 72 72
Lines 3779 3786 +7
Branches 674 675 +1
==========================================
+ Hits 2842 2854 +12
+ Misses 804 795 -9
- Partials 133 137 +4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Closes #895