You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "SMason1441 (via GitHub)" <gi...@apache.org> on 2023/12/18 02:26:58 UTC
[PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
SMason1441 opened a new pull request, #39264:
URL: https://github.com/apache/arrow/pull/39264
### Rationale for this change
There was a bug in replace_substring that caused it to hang if the pattern was empty.
### What changes are included in this PR?
Followed the guidance given in the comment by Alenka in https://github.com/apache/arrow/issues/39149. However after some experimentation I noticed that the issue was that the function ReplaceString would hang forever if the pattern was empty. This is because the update rule in the while loop is i += size of pattern (which is 0). Now, when the pattern is empty, the resulting string will remain the same.
I am also adding the file scalar_string_tests.cc to the CMakeLists.txt in the cpp/arrow/src/compute/kernels directory since I noticed the tests were missing.
### Are these changes tested?
I added extended the existing tests to include cases where the string was empty and where the pattern was empty.
### Are there any user-facing changes?
Closes #39149
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #39264:
URL: https://github.com/apache/arrow/pull/39264#issuecomment-1881449427
Perhaps we can start by returning an error, and we can later change our mind if we find a clear reason of favoring one behavior over another?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
Posted by "mason142 (via GitHub)" <gi...@apache.org>.
mason142 commented on PR #39264:
URL: https://github.com/apache/arrow/pull/39264#issuecomment-1865367720
@AlenkaF, should be good now hopefully.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on PR #39264:
URL: https://github.com/apache/arrow/pull/39264#issuecomment-1881422694
I personally wasn't aware of this specific behaviour of Python, and I don't think we necessarily need to replicate it exactly. Is there a typical use case for this?
For another data point: Postgres doesn't seem to have this behaviour, but rather matches nothing for an empty string:
```
test_db=# SELECT replace('test', 't', 'z');
replace
---------
zesz
(1 row)
test_db=# SELECT replace('test', '', 'z');
replace
---------
test
(1 row)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
Posted by "mason142 (via GitHub)" <gi...@apache.org>.
mason142 commented on PR #39264:
URL: https://github.com/apache/arrow/pull/39264#issuecomment-1865365809
@github-actions autotune
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #39264:
URL: https://github.com/apache/arrow/pull/39264#issuecomment-1881446640
Also Java has the same behavior as Python :-)
https://stackoverflow.com/questions/33960364/replace-with-empty-string-replaces-newchar-around-all-the-characters-in-original
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic closed pull request #39264: GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements
URL: https://github.com/apache/arrow/pull/39264
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
Posted by "mason142 (via GitHub)" <gi...@apache.org>.
mason142 commented on PR #39264:
URL: https://github.com/apache/arrow/pull/39264#issuecomment-1863869986
@github-actions autotune
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
Posted by "mason142 (via GitHub)" <gi...@apache.org>.
mason142 commented on PR #39264:
URL: https://github.com/apache/arrow/pull/39264#issuecomment-1865342072
@github-actions autotune
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #39264:
URL: https://github.com/apache/arrow/pull/39264#issuecomment-1881379139
Sorry for the delay here.
If we want these semantics, we should make sure that non-ASCII data works as expected for utf8 strings:
```python
>>> "àà".replace("", "z")
'zàzàz'
```
I'm not sure the current code ensures this...
An alternative would be to simply return an error if the pattern is empty.
What do you think @wjones127 @jorisvandenbossche ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #39264:
URL: https://github.com/apache/arrow/pull/39264#issuecomment-3547437995
Thank you for your contribution. Unfortunately, this
pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label
or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you
do not have repository permissions to reopen the PR, please tag a maintainer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39264:
URL: https://github.com/apache/arrow/pull/39264#issuecomment-1859445553
:warning: GitHub issue #39149 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39264:
URL: https://github.com/apache/arrow/pull/39264#discussion_r1434318576
##########
cpp/src/arrow/compute/kernels/CMakeLists.txt:
##########
@@ -72,6 +72,12 @@ add_arrow_compute_test(scalar_utility_test
EXTRA_LINK_LIBS
arrow_compute_kernels_testing)
+add_arrow_compute_test(scalar_string_test
+ SOURCES
+ scalar_string_test.cc
Review Comment:
This file is already compiled into `scalar_type_test` above, so you don't need to add a new test declaration for it.
##########
cpp/src/arrow/compute/kernels/scalar_string_test.cc:
##########
@@ -1743,12 +1743,20 @@ TYPED_TEST(TestStringKernels, Utf8ReplaceSlice) {
TYPED_TEST(TestBaseBinaryKernels, ReplaceSubstring) {
ReplaceSubstringOptions options{"foo", "bazz"};
- this->CheckUnary("replace_substring", R"(["foo", "this foo that foo", null])",
- this->type(), R"(["bazz", "this bazz that bazz", null])", &options);
+ this->CheckUnary("replace_substring", R"(["foo", "this foo that foo", "", null])",
+ this->type(), R"(["bazz", "this bazz that bazz", "", null])", &options);
options = ReplaceSubstringOptions{"foo", "bazz", 1};
- this->CheckUnary("replace_substring", R"(["foo", "this foo that foo", null])",
- this->type(), R"(["bazz", "this bazz that foo", null])", &options);
+ this->CheckUnary("replace_substring", R"(["foo", "this foo that foo", "", null])",
+ this->type(), R"(["bazz", "this bazz that foo", "", null])", &options);
+
+ options = ReplaceSubstringOptions{"", "bazz"};
+ this->CheckUnary("replace_substring", R"(["foo", "this foo that foo", "", null])",
+ this->type(), R"(["foo", "this foo that foo", "", null])", &options);
Review Comment:
Is this the desired semantics? Python does this:
```python
>>> s = "abcde"
>>> s.replace("", "zz")
'zzazzbzzczzdzzezz'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
Posted by "mason142 (via GitHub)" <gi...@apache.org>.
mason142 commented on PR #39264:
URL: https://github.com/apache/arrow/pull/39264#issuecomment-1868140023
> Thanks for submitting this PR @mason142 . Here are a couple comments, I'm not sure the solution is right here.
Hi @pitrou, thank you very much for looking at my PR and giving your thoughts. I rewrote my solution to better follow the semantics of .replace() in python. I was a little unsure on how to handle the case of "".replace("", any_string) so I hardcoded that edge case at the beginning of ReplaceSubstring. Any other advice would be greatly appreciated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39149: [C++] replace_substring_regex hangs indefinitely for negative max_replacements [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on PR #39264:
URL: https://github.com/apache/arrow/pull/39264#issuecomment-1881433529
And then R seems to have the same behaviour as Python (`sub` only replaces the first occurrence):
```R
> sub("test", "t", "z")
[1] "z"
> sub("t", "z", "test")
[1] "zest"
> sub("", "z", "test")
[1] "ztest"
> gsub("t", "z", "test")
[1] "zesz"
> gsub("", "z", "test")
[1] "ztzezsztz"
```
And the tidyverse version of this function raises an error:
```R
> library(stringr)
> str_replace("test", "t", "z")
[1] "zest"
> str_replace("test", "", "z")
Error in `str_replace()`:
! `pattern` can't be the empty string (`""`).
```
So we already have examples of three different behaviours .. (replace (insert) every other string, replace nothing, error)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org