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

[VL] Validate unsupported rewrite string in Re2 #6312

Closed
wants to merge 3 commits into from
Closed

[VL] Validate unsupported rewrite string in Re2 #6312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b10378f
check re2 rewrite string
kecookier Jul 1, 2024
fe9cf78
fix
kecookier Jul 3, 2024
e447192
fix
kecookier Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,19 @@ bool SubstraitToVeloxPlanValidator::validateRegexExpr(
LOG_VALIDATION_MSG("Pattern is not string literal for " + name);
return false;
}

const auto& pattern = patternArg.literal().string();

std::string rewrite;
if (name == "regexp_replace " && scalarFunction.arguments().size() > 2) {
const auto& rewriteArg = scalarFunction.arguments()[2].value();
if (!rewriteArg.has_literal() || !rewriteArg.literal().has_string()) {
LOG_VALIDATION_MSG("Rewrite is not string literal for " + name);
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

This check will make this function always fall back if non-constant replacement string is used.

Please check whether this comment makes sense: #6224 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the explanation. I will modify the code according to your suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kecookier, if the proposal is feasible, you can file a drafted pr in velox to see whether the community accepts that. Thanks!

}
rewrite = rewriteArg.literal().string();
}
std::string error;
if (!validatePattern(pattern, error)) {
if (!validateRe2Function(pattern, rewrite, error)) {
LOG_VALIDATION_MSG(name + " due to " + error);
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion cpp/velox/substrait/SubstraitToVeloxPlanValidator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ class SubstraitToVeloxPlanValidator {

/// Validates regex functions.
/// Ensures the second pattern argument is a literal string.
/// Check if the pattern can pass with RE2 compilation.
/// Check if the pattern can pass with RE2 compilation and check rewriteString of regexp_replace is validate
bool validateRegexExpr(const std::string& name, const ::substrait::Expression::ScalarFunction& scalarFunction);

/// Validate Substrait scarlar function.
Expand Down
14 changes: 12 additions & 2 deletions cpp/velox/utils/Common.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,23 @@ std::unique_ptr<re2::RE2> compilePattern(const std::string& pattern) {
return std::make_unique<re2::RE2>(re2::StringPiece(pattern), RE2::Quiet);
}

bool validatePattern(const std::string& pattern, std::string& error) {
bool validateRe2Function(const std::string& pattern, const std::string& rewrite, std::string& error) {
auto re2 = compilePattern(pattern);
if (!re2->ok()) {
error = "Pattern " + pattern + " compilation failed in RE2. Reason: " + re2->error();
return false;
}
return ensureRegexIsCompatible(pattern, error);

if (!ensureRegexIsCompatible(pattern, error)) {
return false;
}

std::string err;
if (!rewrite.empty() && !re2->CheckRewriteString(re2::StringPiece(rewrite), &err)) {
error = "Rewrite " + rewrite + "check failed in RE2. Reason: " + err;
return false;
}
return true;
}

} // namespace gluten
2 changes: 1 addition & 1 deletion cpp/velox/utils/Common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ namespace gluten {
// Compile the given pattern and return the RE2 object.
inline std::unique_ptr<re2::RE2> compilePattern(const std::string& pattern);

bool validatePattern(const std::string& pattern, std::string& error);
bool validateRe2Function(const std::string& pattern, const std::string& rewrite, std::string& error);

static inline void fastCopy(void* dst, const void* src, size_t n) {
facebook::velox::simd::memcpy(dst, src, n);
Expand Down
Loading