From c16f467284ccc81775823044568fc57e276b74fd Mon Sep 17 00:00:00 2001 From: Novain Shibu Date: Sun, 8 Nov 2026 09:12:43 +0000 Subject: [PATCH] GH-39149: [C++][Compute] Fix replace_substring hang on empty pattern Return Invalid for an empty pattern in replace_substring, and treat any negative max_replacements as unlimited in replace_substring_regex. Co-Authored-By: Claude Opus 5.5 --- cpp/src/arrow/compute/kernels/scalar_string_ascii.cc | 7 ++++++- cpp/src/arrow/compute/kernels/scalar_string_test.cc | 12 ++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc b/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc index dd3c507ef53e..8b86c0e6100f 100644 --- a/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc +++ b/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc @@ -2039,6 +2039,10 @@ struct PlainSubstringReplacer { static Result> Make( const ReplaceSubstringOptions& options, bool is_utf8) { + if (options.pattern.empty()) { + // An empty pattern would match at every position without advancing. + return Status::Invalid("Empty substring pattern is not supported"); + } return std::make_unique(options); } @@ -2119,7 +2123,8 @@ struct RegexSubstringReplacer { re2::StringPiece replacement(options_.replacement); // If s is empty, then it's essentially global - if (options_.max_replacements == -1 || s.empty()) { + // Any negative value means unlimited replacements. + if (options_.max_replacements < 0 || s.empty()) { std::string s_copy{s.data(), s.length()}; RE2::GlobalReplace(&s_copy, regex_replacement_, replacement); return builder->Append(reinterpret_cast(s_copy.data()), diff --git a/cpp/src/arrow/compute/kernels/scalar_string_test.cc b/cpp/src/arrow/compute/kernels/scalar_string_test.cc index 74650c1d76c5..687a2e6e41e1 100644 --- a/cpp/src/arrow/compute/kernels/scalar_string_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_string_test.cc @@ -2102,6 +2102,13 @@ TYPED_TEST(TestBaseBinaryKernels, ReplaceSubstring) { Datum input = ArrayFromJSON(this->type(), "[]"); ASSERT_RAISES(Invalid, CallFunction("replace_substring", {input})); + + // GH-39149: an empty pattern used to loop forever + input = ArrayFromJSON(this->type(), R"(["abc", "", null])"); + for (int64_t max_replacements : {-1, -3, 1}) { + options = ReplaceSubstringOptions{"", "x", max_replacements}; + ASSERT_RAISES(Invalid, CallFunction("replace_substring", {input}, &options)); + } } #ifdef ARROW_WITH_RE2 @@ -2120,6 +2127,11 @@ TYPED_TEST(TestBaseBinaryKernels, ReplaceSubstringRegex) { this->CheckUnary("replace_substring_regex", R"([""])", this->type(), R"(["x"])", &options); + // GH-39149: negative max_replacements other than -1 means unlimited + options = ReplaceSubstringOptions{"", "x", -3}; + this->CheckUnary("replace_substring_regex", R"(["abc"])", this->type(), + R"(["xaxbxcx"])", &options); + // ARROW-12774 options = ReplaceSubstringOptions{"X", "Y"}; this->CheckUnary("replace_substring_regex",