Copilot commented on code in PR #13278: URL: https://github.com/apache/gluten/pull/13278#discussion_r4280880185
########## 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 backreference rejection doesn’t honor the documented behavior of ignoring “in-class occurrences”: since backslash handling runs before the `inClass` branch, a pattern like `"[\\1]"` will currently be rejected even though `\1` inside a character class is not a backreference in Java regex (and the PR description explicitly calls out in-class occurrences as not flagged). Make the backreference check conditional on `!inClass`, and add a unit test to lock this down. ########## 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: This exception message embeds the full raw pattern (`$patStr`) directly. For very long patterns or patterns containing newlines/control characters, this can make fallback reasons noisy and harder to parse in logs/UI. Consider truncating/escaping the rendered pattern (e.g., limit length and replace newlines) while still preserving enough context to debug. ########## 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: The comment implies a “pure translation” + idempotence when no `\uXXXX` is present, but the implementation can also intentionally leave certain `\uXXXX` sequences untouched (e.g., unpaired surrogates) so RE2 rejects them later, and may produce partially translated output. Please document that behavior explicitly (including the unpaired-surrogate case) to prevent misuse by callers who might assume the output is always RE2-compilable. ########## 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: Similar to the Scala-side exception, this error string concatenates the full raw pattern without quoting/escaping. Consider quoting and truncating the pattern (and/or escaping control characters) to keep native-side failure reasons readable and to avoid log bloat when patterns are large. ########## 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 implementation rejects any escape starting with `\1`–`\9`, which is broader than the stated scope (“backreferences `\1`–`\19`” in the PR description / doc comment). Either (a) narrow the detection to the intended range, or (b) update the Scaladoc/PR description/tests to explicitly state the stricter rule so users aren’t surprised by additional fallbacks (e.g., octal-like escapes or digit escapes beyond `\19`). -- 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]
