zanmato1984 commented on code in PR #50616:
URL: https://github.com/apache/arrow/pull/50616#discussion_r3641408162
##########
cpp/src/arrow/compute/kernels/scalar_string_ascii.cc:
##########
@@ -1500,27 +1495,59 @@ std::string MakeLikeRegex(const MatchSubstringOptions&
options) {
return like_pattern;
}
+struct MatchLikeConstants {
+ // NOTE: avoid making those constants global to avoid compiling regexes at
startup
+ RE2::Options regex_options;
+ // A LIKE pattern matching this regex can be translated into a substring
search.
+ RE2 like_pattern_is_substring_match;
+ // A LIKE pattern matching this regex can be translated into a prefix search.
+ RE2 like_pattern_is_starts_with;
+ // A LIKE pattern matching this regex can be translated into a suffix search.
+ RE2 like_pattern_is_ends_with;
+ Status init_status;
+
+ static Result<const MatchLikeConstants*> Instance(bool is_utf8) {
+ static const auto constants = MakeAll();
+ return constants[is_utf8].Map([](const auto& ptr) { return ptr.get(); });
+ }
+
+ private:
+ static Result<std::unique_ptr<MatchLikeConstants>> Make(bool is_utf8) {
+ auto constants = std::unique_ptr<MatchLikeConstants>(new
MatchLikeConstants(is_utf8));
+ if (constants->init_status.ok()) {
+ return constants;
+ } else {
+ return constants->init_status;
+ }
+ }
+
+ static std::array<Result<std::unique_ptr<MatchLikeConstants>>, 2> MakeAll() {
+ return {Make(false), Make(true)};
+ }
+
+ explicit MatchLikeConstants(bool is_utf8)
+ : regex_options(MakeRE2Options(is_utf8)),
+ like_pattern_is_substring_match(R"(%+([^%_]*[^\\%_])?%+)",
regex_options),
+ like_pattern_is_starts_with(R"(([^%_]*[^\\%_])?%+)", regex_options),
+ like_pattern_is_ends_with(R"(%+([^%_]*))", regex_options),
+ init_status(RegexStatus(like_pattern_is_substring_match) &
+ RegexStatus(like_pattern_is_starts_with) &
+ RegexStatus(like_pattern_is_ends_with)) {}
+};
+
// Evaluate a SQL-like LIKE pattern by translating it to a regexp or
// substring search as appropriate. See what Apache Impala does:
//
https://github.com/apache/impala/blob/9c38568657d62b6f6d7b10aa1c721ba843374dd8/be/src/exprs/like-predicate.cc
-template <typename StringType>
+template <typename PhysicalType>
struct MatchLike {
+ static_assert(!is_string_or_string_view(PhysicalType::type_id),
+ "should only codegen on physical types");
+
static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
- // NOTE: avoid making those constants global to avoid compiling regexes at
startup
- static const RE2::Options kRE2Options = MakeRE2Options<StringType>();
- // A LIKE pattern matching this regex can be translated into a substring
search.
- static const RE2 kLikePatternIsSubstringMatch(R"(%+([^%_]*[^\\%_])?%+)",
kRE2Options);
- // A LIKE pattern matching this regex can be translated into a prefix
search.
- static const RE2 kLikePatternIsStartsWith(R"(([^%_]*[^\\%_])?%+)",
kRE2Options);
- // A LIKE pattern matching this regex can be translated into a suffix
search.
- static const RE2 kLikePatternIsEndsWith(R"(%+([^%_]*))", kRE2Options);
- static bool global_checked = false;
- if (ARROW_PREDICT_FALSE(!global_checked)) {
- RETURN_NOT_OK(RegexStatus(kLikePatternIsSubstringMatch));
- RETURN_NOT_OK(RegexStatus(kLikePatternIsStartsWith));
- RETURN_NOT_OK(RegexStatus(kLikePatternIsEndsWith));
- global_checked = true;
- }
+ ARROW_ASSIGN_OR_RAISE(
+ const auto like_constants,
+ MatchLikeConstants::Instance(
+ /*is_utf8=*/is_string_or_string_view(batch[0].type()->id())));
Review Comment:
Could we add a targeted binary-vs-utf8 test for this runtime selection?
Since string and binary now share the same physical `MatchLike` implementation,
this cache is the one place where accidentally selecting the wrong UTF-8 /
Latin-1 slot would be hard to catch with ASCII-only inputs.
For example, `pattern = "%ÀB%"` with `ignore_case = true` should not match
lowercase `àb` on binary input, but should match it on utf8 input.
##########
cpp/src/arrow/compute/kernels/scalar_string_ascii.cc:
##########
@@ -1500,27 +1495,59 @@ std::string MakeLikeRegex(const MatchSubstringOptions&
options) {
return like_pattern;
}
+struct MatchLikeConstants {
+ // NOTE: avoid making those constants global to avoid compiling regexes at
startup
+ RE2::Options regex_options;
+ // A LIKE pattern matching this regex can be translated into a substring
search.
+ RE2 like_pattern_is_substring_match;
+ // A LIKE pattern matching this regex can be translated into a prefix search.
+ RE2 like_pattern_is_starts_with;
+ // A LIKE pattern matching this regex can be translated into a suffix search.
+ RE2 like_pattern_is_ends_with;
+ Status init_status;
Review Comment:
Optional nit: could `init_status` be a local in `Make()` instead of a
`MatchLikeConstants` member? It appears to be construction-only state, so
narrowing its scope would make the struct state tighter.
##########
cpp/src/arrow/compute/kernels/scalar_string_ascii.cc:
##########
@@ -1500,27 +1495,59 @@ std::string MakeLikeRegex(const MatchSubstringOptions&
options) {
return like_pattern;
}
+struct MatchLikeConstants {
+ // NOTE: avoid making those constants global to avoid compiling regexes at
startup
+ RE2::Options regex_options;
+ // A LIKE pattern matching this regex can be translated into a substring
search.
+ RE2 like_pattern_is_substring_match;
+ // A LIKE pattern matching this regex can be translated into a prefix search.
+ RE2 like_pattern_is_starts_with;
+ // A LIKE pattern matching this regex can be translated into a suffix search.
+ RE2 like_pattern_is_ends_with;
+ Status init_status;
+
+ static Result<const MatchLikeConstants*> Instance(bool is_utf8) {
+ static const auto constants = MakeAll();
+ return constants[is_utf8].Map([](const auto& ptr) { return ptr.get(); });
+ }
+
+ private:
+ static Result<std::unique_ptr<MatchLikeConstants>> Make(bool is_utf8) {
+ auto constants = std::unique_ptr<MatchLikeConstants>(new
MatchLikeConstants(is_utf8));
+ if (constants->init_status.ok()) {
+ return constants;
+ } else {
+ return constants->init_status;
+ }
+ }
+
+ static std::array<Result<std::unique_ptr<MatchLikeConstants>>, 2> MakeAll() {
+ return {Make(false), Make(true)};
+ }
+
+ explicit MatchLikeConstants(bool is_utf8)
+ : regex_options(MakeRE2Options(is_utf8)),
+ like_pattern_is_substring_match(R"(%+([^%_]*[^\\%_])?%+)",
regex_options),
Review Comment:
Optional readability nit: could we use default member initializers for these
`RE2` members so each regex literal stays next to the comment explaining it?
The comments and regexes are tightly coupled because each one classifies a LIKE
pattern that can be rewritten as substring / prefix / suffix search. Keeping
them together would make these hard-to-read regexes easier to audit.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
