KushShriv commented on code in PR #13278: URL: https://github.com/apache/gluten/pull/13278#discussion_r4333418364
########## gluten-substrait/src/main/scala/org/apache/gluten/expression/Re2PatternUtils.scala: ########## @@ -0,0 +1,120 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.gluten.expression + +import java.util.Locale + +/** + * Utilities for translating regex patterns from Java syntax to RE2 syntax. Used by + * ExpressionConverter for any backend whose native engine is RE2 (Velox, Bolt); see + * [[org.apache.gluten.backendsapi.SparkPlanExecApi#needsRe2PatternTranslation]]. + */ +object Re2PatternUtils { + + private def isHex(c: Char): Boolean = + (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F') + + /** Returns the code unit of a backslash-u escape with 4 hex digits starting at `i`, or -1. */ + private def unicodeEscapeAt(p: String, i: Int): Int = { + if ( + i + 6 <= p.length && p.charAt(i) == '\\' && p.charAt(i + 1) == 'u' && + (2 until 6).forall(k => isHex(p.charAt(i + k))) + ) { + Integer.parseInt(p.substring(i + 2, i + 6), 16) + } else { + -1 + } + } + + /** + * Attempts to translate a Java regex pattern to RE2-compatible syntax. + * + * Returns Some(translated) if all constructs have RE2 equivalents and the translation is + * lossless. Returns None if the pattern contains constructs that RE2 cannot represent + * (lookaheads, lookbehinds, backreferences) or an unpaired UTF-16 surrogate escape; the caller + * should throw [[org.apache.gluten.exception.GlutenNotSupportException]] to trigger fallback. + * + * Translation rules: + * - backslash-u followed by 4 hex digits becomes `\x{XXXX}`; a high+low surrogate escape pair + * becomes a single `\x{code point}`. + * - Escaped characters (e.g. an escaped backslash followed by `u0041`, or an escaped + * parenthesis) are copied verbatim and never reinterpreted. + * - Lookaround markers are ignored inside character classes, where `(` is a literal. + * - Named groups `(?<name>...)` are supported by RE2 and are not rejected. + */ + def translateJavaPatternToRe2(pattern: String): Option[String] = { + val sb = new StringBuilder(pattern.length) + val n = pattern.length + var i = 0 + var inClass = false + while (i < n) { + val c = pattern.charAt(i) + if (c == '\\') { + val cp = unicodeEscapeAt(pattern, i) + if (cp >= 0) { + if (Character.isHighSurrogate(cp.toChar)) { + val lo = unicodeEscapeAt(pattern, i + 6) + if (lo < 0 || !Character.isLowSurrogate(lo.toChar)) return None + val full = Character.toCodePoint(cp.toChar, lo.toChar) + val hex = Integer.toHexString(full).toUpperCase(Locale.ROOT) + sb.append("\\x{").append(hex).append("}") + i += 12 + } else if (Character.isLowSurrogate(cp.toChar)) { + return None + } else { + sb.append("\\x{").append(pattern.substring(i + 2, i + 6)).append("}") + i += 6 + } + } else if (i + 1 < n) { + val next = pattern.charAt(i + 1) + if (next >= '1' && next <= '9') return None // backreference Review Comment: Thanks, you are right that the PR description was misleading here. The in-class exemption only ever applied to lookaround markers (`[(?=]`), not to backreferences, and I have corrected the description. I am keeping the check unconditional on purpose. Java rejects `\1`–`\9` inside a character class (`Illegal/unsupported escape sequence`), so falling back to Spark gives the same error the user would get without Gluten. If we only rejected outside a class, `[\12]` would reach Velox, where RE2 reads it as an octal escape and runs it, while Spark would throw. I have added a Scaladoc note and tests for `[\1]` and `[\12]` to lock this in. ########## cpp/velox/utils/Common.h: ########## @@ -30,6 +30,12 @@ namespace gluten { // Compile the given pattern and return the RE2 object. inline std::unique_ptr<re2::RE2> compilePattern(const std::string& pattern); +/// Converts Java \uXXXX Unicode escapes to RE2's \x{XXXX} syntax. +/// Returns the input unchanged if no \uXXXX sequences are found (idempotent). Review Comment: Good catch, I have reworded the doc. It now says the rewrite is lexical only, that a high+low surrogate pair becomes one `\x{code point}`, and that an unpaired surrogate escape is left as `\uXXXX` on purpose so RE2 rejects it. The output may therefore be partly translated and is not guaranteed to compile, so callers must still validate it (see `validatePattern`). ########## gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala: ########## @@ -949,6 +979,32 @@ object ExpressionConverter extends SQLConfHelper with Logging { } } + /** + * Translates the pattern argument of a regex expression to RE2 syntax before it is written into + * the Substrait literal. If the pattern is a string literal, Java Unicode escapes (backslash-u + * followed by 4 hex digits) are converted to RE2's `\x{XXXX}` form. Patterns that contain + * constructs with no RE2 equivalent (lookaheads, lookbehinds, backreferences) throw + * [[GlutenNotSupportException]] to trigger graceful fallback. Non-literal (column-referenced) + * patterns, and all patterns on backends that do not use RE2, are passed through unchanged. + */ + private def withRe2PatternTranslation( + patternChild: Expression, + attributeSeq: Seq[Attribute], + expressionsMap: Map[Class[_], String]): ExpressionTransformer = { + patternChild match { + case Literal(patStr: UTF8String, StringType) + if BackendsApiManager.getSparkPlanExecApiInstance.needsRe2PatternTranslation => + val translated = Re2PatternUtils + .translateJavaPatternToRe2(patStr.toString) + .getOrElse(throw new GlutenNotSupportException( + s"Regex pattern '$patStr' uses constructs (lookahead/lookbehind/backreference) " + + s"or unpaired surrogates that RE2 cannot handle. Falling back to Spark.")) Review Comment: Agreed. I added `Re2PatternUtils.describePattern`, which quotes the pattern, escapes newlines and other control characters, and truncates at about 100 characters with the total length. The fallback reason now uses it. ########## gluten-substrait/src/main/scala/org/apache/gluten/expression/Re2PatternUtils.scala: ########## @@ -0,0 +1,120 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.gluten.expression + +import java.util.Locale + +/** + * Utilities for translating regex patterns from Java syntax to RE2 syntax. Used by + * ExpressionConverter for any backend whose native engine is RE2 (Velox, Bolt); see + * [[org.apache.gluten.backendsapi.SparkPlanExecApi#needsRe2PatternTranslation]]. + */ +object Re2PatternUtils { + + private def isHex(c: Char): Boolean = + (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F') + + /** Returns the code unit of a backslash-u escape with 4 hex digits starting at `i`, or -1. */ + private def unicodeEscapeAt(p: String, i: Int): Int = { + if ( + i + 6 <= p.length && p.charAt(i) == '\\' && p.charAt(i + 1) == 'u' && + (2 until 6).forall(k => isHex(p.charAt(i + k))) + ) { + Integer.parseInt(p.substring(i + 2, i + 6), 16) + } else { + -1 + } + } + + /** + * Attempts to translate a Java regex pattern to RE2-compatible syntax. + * + * Returns Some(translated) if all constructs have RE2 equivalents and the translation is + * lossless. Returns None if the pattern contains constructs that RE2 cannot represent + * (lookaheads, lookbehinds, backreferences) or an unpaired UTF-16 surrogate escape; the caller + * should throw [[org.apache.gluten.exception.GlutenNotSupportException]] to trigger fallback. + * + * Translation rules: + * - backslash-u followed by 4 hex digits becomes `\x{XXXX}`; a high+low surrogate escape pair + * becomes a single `\x{code point}`. + * - Escaped characters (e.g. an escaped backslash followed by `u0041`, or an escaped + * parenthesis) are copied verbatim and never reinterpreted. + * - Lookaround markers are ignored inside character classes, where `(` is a literal. + * - Named groups `(?<name>...)` are supported by RE2 and are not rejected. + */ + def translateJavaPatternToRe2(pattern: String): Option[String] = { + val sb = new StringBuilder(pattern.length) + val n = pattern.length + var i = 0 + var inClass = false + while (i < n) { + val c = pattern.charAt(i) + if (c == '\\') { + val cp = unicodeEscapeAt(pattern, i) + if (cp >= 0) { + if (Character.isHighSurrogate(cp.toChar)) { + val lo = unicodeEscapeAt(pattern, i + 6) + if (lo < 0 || !Character.isLowSurrogate(lo.toChar)) return None + val full = Character.toCodePoint(cp.toChar, lo.toChar) + val hex = Integer.toHexString(full).toUpperCase(Locale.ROOT) + sb.append("\\x{").append(hex).append("}") + i += 12 + } else if (Character.isLowSurrogate(cp.toChar)) { + return None + } else { + sb.append("\\x{").append(pattern.substring(i + 2, i + 6)).append("}") + i += 6 + } + } else if (i + 1 < n) { + val next = pattern.charAt(i + 1) + if (next >= '1' && next <= '9') return None // backreference Review Comment: The behavior is intentional, and the description was what was wrong. Java treats any backslash followed by `1`–`9` as a group reference, whatever follows, so there is no upper bound to narrow to, and a tighter range would let some backreferences through. I have updated the Scaladoc and the PR description to state the rule as "backslash followed by `1`–`9`", and the tests cover multi-digit cases such as `\12`. ########## cpp/velox/utils/Common.cc: ########## @@ -52,7 +56,70 @@ std::unique_ptr<re2::RE2> compilePattern(const std::string& pattern) { return std::make_unique<re2::RE2>(re2::StringPiece(pattern), RE2::Quiet); } +namespace { +// Parses a Java \uXXXX escape at pattern[i]; returns the code unit, or -1 if there is none. +int parseJavaUnicodeEscape(const std::string& pattern, size_t i) { + if (i + 6 > pattern.size() || pattern[i] != '\\' || pattern[i + 1] != 'u') { + return -1; + } + for (size_t k = 2; k < 6; ++k) { + if (!std::isxdigit(static_cast<unsigned char>(pattern[i + k]))) { + return -1; + } + } + return static_cast<int>(std::stoul(pattern.substr(i + 2, 4), nullptr, 16)); +} +} // namespace + +std::string translateJavaUnicodeToRe2(const std::string& pattern) { + std::string result; + result.reserve(pattern.size()); + size_t i = 0; + while (i < pattern.size()) { + if (pattern[i] != '\\') { + result += pattern[i++]; + continue; + } + const int cu = parseJavaUnicodeEscape(pattern, i); + if (cu < 0) { + // Any other escape (including an escaped backslash) is copied verbatim as a pair, so that + // "\\\\u0041" (literal backslash followed by text) is not mistaken for a unicode escape. + result += pattern[i++]; + if (i < pattern.size()) { + result += pattern[i++]; + } + continue; + } + if (cu >= 0xD800 && cu <= 0xDBFF) { + const int lo = parseJavaUnicodeEscape(pattern, i + 6); + if (lo >= 0xDC00 && lo <= 0xDFFF) { + const unsigned cp = + 0x10000 + ((static_cast<unsigned>(cu) - 0xD800) << 10) + (static_cast<unsigned>(lo) - 0xDC00); + char buf[16]; + std::snprintf(buf, sizeof(buf), "\\x{%X}", cp); + result += buf; + i += 12; + continue; + } + } + if (cu >= 0xD800 && cu <= 0xDFFF) { + // Unpaired surrogate: leave untouched so that RE2 rejects it and the caller falls back. + result.append(pattern, i, 6); + } else { + result += "\\x{"; + result.append(pattern, i + 2, 4); + result += '}'; + } + i += 6; + } + return result; +} + bool validatePattern(const std::string& pattern, std::string& error) { + // Validate the pattern exactly as Velox will compile it at execution time. The plan keeps the + // original literal, so translating a private copy here would approve patterns (e.g. a raw + // \uXXXX) that Velox then fails to compile at runtime instead of falling back to Spark. Java + // \uXXXX escapes are rewritten on the Scala side before the plan is built. auto re2 = compilePattern(pattern); if (!re2->ok()) { error = "Pattern " + pattern + " compilation failed in RE2. Reason: " + re2->error(); Review Comment: Same as the Scala side. I added a small `describePattern` helper in `Common.cc` that quotes, escapes control characters and truncates, and `validatePattern` uses it. RE2's own error text repeats the whole pattern, so I cap that too. There is a new `CommonTest` case checking the escaped and truncated message. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
