philo-he commented on code in PR #13278: URL: https://github.com/apache/gluten/pull/13278#discussion_r4485117810
########## gluten-substrait/src/main/scala/org/apache/gluten/expression/Re2PatternUtils.scala: ########## @@ -0,0 +1,169 @@ +/* + * 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 org.apache.gluten.exception.GlutenNotSupportException + +import org.apache.spark.sql.catalyst.expressions.{Expression, Literal} +import org.apache.spark.sql.types.StringType +import org.apache.spark.unsafe.types.UTF8String + +import java.util.Locale + +/** + * Utilities for translating regex patterns from Java syntax to RE2 syntax. Used by any backend + * whose native engine is RE2 (Velox, Bolt); see + * [[org.apache.gluten.backendsapi.SparkPlanExecApi#rewriteRegexPattern]]. + */ +object Re2PatternUtils { + + /** + * Implements [[org.apache.gluten.backendsapi.SparkPlanExecApi#rewriteRegexPattern]] for RE2 + * backends. A string literal pattern is translated to RE2 syntax; patterns with constructs that + * have no RE2 equivalent (lookaheads, lookbehinds, backreferences, unpaired surrogates) throw + * [[GlutenNotSupportException]] to trigger fallback. Non-literal patterns are returned unchanged. + */ + def rewriteRegexPattern(pattern: Expression): Expression = pattern match { + case Literal(p: UTF8String, StringType) => + val translated = translateJavaPatternToRe2(p.toString).getOrElse( + throw new GlutenNotSupportException( + s"Regex pattern ${describePattern(p.toString)} uses constructs " + + "(lookahead/lookbehind/backreference) or unpaired surrogates that RE2 cannot " + + "handle. Falling back to Spark.")) + Literal(UTF8String.fromString(translated), StringType) + case other => other + } + + private def isHex(c: Char): Boolean = + (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F') + + /** + * Renders a pattern for error messages: quoted, control characters escaped, and truncated so that + * very long patterns do not bloat fallback reasons and logs. + */ + def describePattern(pattern: String, maxLen: Int = 100): String = { + val sb = new StringBuilder("'") + var i = 0 + while (i < pattern.length && sb.length < maxLen) { + pattern.charAt(i) match { + case '\n' => sb.append("\\n") + case '\r' => sb.append("\\r") + case '\t' => sb.append("\\t") + case ch if ch < ' ' || ch == '\u007f' => sb.append("\\x%02X".format(ch.toInt)) + case ch => sb.append(ch) + } + i += 1 + } + if (i < pattern.length) sb.append(s"...(truncated, ${pattern.length} chars total)") + sb.append("'").toString + } + + /** 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. + * - Every backslash followed by a digit 1-9 is treated as a backreference and rejected (Java + * parses it as a group reference, whatever follows), including inside a character class, + * where Java itself raises an error. + * - Named groups `(?<name>...)` are supported by RE2 and are not rejected. + */ + def translateJavaPatternToRe2(pattern: String): Option[String] = { Review Comment: `\Q...\E` quoted sections aren't handled, so their content is still scanned as regex syntax: - `\Q\u0041\E` is translated to `\Q\x{0041}\E`. In Java it matches the literal text `\u0041`, but RE2 also supports `\Q...\E`, so the translated pattern compiles and matches the literal text `\x{0041}` instead. Nothing falls back, and the result differs from Spark. - `\Q(?=\E` and `\Q\1\E` are rejected, so they fall back unnecessarily. Could we copy everything from `\Q` up to the next `\E` (or the end of the pattern) verbatim, and add tests for these three cases? ########## backends-velox/src/test/scala/org/apache/gluten/execution/VeloxStringFunctionsSuite.scala: ########## @@ -678,4 +680,55 @@ class VeloxStringFunctionsSuite extends VeloxWholeStageTransformerSuite { s"select l_orderkey, unbase64(base64(l_comment)) " + s"from $LINEITEM_TABLE limit $LENGTH")(checkGlutenPlan[ProjectExecTransformer]) } + + // Regression test: Java \\uXXXX Unicode escapes must be translated to RE2 \\x{XXXX} syntax so + // that CJK Unicode range patterns run natively on Velox instead of falling back to the JVM. + test("rlike with Java Unicode escape runs natively") { + val pattern = "[\\u0041-\\u005A]+" + runQueryAndCompare( + s"select l_orderkey, rlike(l_comment, '$pattern') " + + s"from $LINEITEM_TABLE limit $LENGTH")(checkGlutenPlan[ProjectExecTransformer]) Review Comment: I believe this pattern may never reach the translator. `"[\\u0041-\\u005A]+"` puts `[\u0041-\u005A]+` into the SQL text, and with the default `spark.sql.parser.escapedStringLiterals=false`, Spark's parser decodes `\u0041` while reading the literal, so Gluten would receive `[A-Z]+`. The surrogate-pair test below doubles the backslashes for this reason. Could you apply the same escaping here and check that the test fails without this change? Also, the comment mentions CJK ranges, but the test uses `A-Z`. ########## backends-velox/src/test/scala/org/apache/gluten/execution/VeloxStringFunctionsSuite.scala: ########## @@ -678,4 +680,55 @@ class VeloxStringFunctionsSuite extends VeloxWholeStageTransformerSuite { s"select l_orderkey, unbase64(base64(l_comment)) " + s"from $LINEITEM_TABLE limit $LENGTH")(checkGlutenPlan[ProjectExecTransformer]) } + + // Regression test: Java \\uXXXX Unicode escapes must be translated to RE2 \\x{XXXX} syntax so + // that CJK Unicode range patterns run natively on Velox instead of falling back to the JVM. + test("rlike with Java Unicode escape runs natively") { + val pattern = "[\\u0041-\\u005A]+" + runQueryAndCompare( + s"select l_orderkey, rlike(l_comment, '$pattern') " + + s"from $LINEITEM_TABLE limit $LENGTH")(checkGlutenPlan[ProjectExecTransformer]) + } + + // Java surrogate pair escapes (backslash-u high + low) must be combined into one RE2 code point. + test("rlike with Java surrogate pair escape runs natively") { + // Back the input with parquet: a LocalRelation is folded by the optimizer + // (ConvertToLocalRelation) before the projection ever reaches Gluten. + withTempPath { + dir => + // scalastyle:off nonascii + val data = Seq("a\uD83D\uDE00b", "plain", "\uD83D\uDE4F") + // scalastyle:on nonascii + data.toDF("s").write.parquet(dir.getCanonicalPath) + withTempView("emoji_t") { + spark.read.parquet(dir.getCanonicalPath).createOrReplaceTempView("emoji_t") + // SQL literal parsing consumes one level of backslashes, so double them. + val pattern = "[\\\\uD83D\\\\uDE00-\\\\uD83D\\\\uDE4F]" + runQueryAndCompare(s"select s, rlike(s, '$pattern') from emoji_t")( + checkGlutenPlan[ProjectExecTransformer]) + } + } + } + + // An escaped backslash followed by u0041 is literal text, not a unicode escape. + test("rlike with escaped backslash before u is not translated") { + withTempPath { + dir => + Seq("\\u0041", "A").toDF("s").write.parquet(dir.getCanonicalPath) + withTempView("bs_t") { + spark.read.parquet(dir.getCanonicalPath).createOrReplaceTempView("bs_t") + runQueryAndCompare("select s, rlike(s, '\\\\\\\\u0041') from bs_t")( + checkGlutenPlan[ProjectExecTransformer]) + } + } + } + + // Regression test: lookbehind patterns have no RE2 equivalent and must still fall back + // gracefully to the Spark JVM engine rather than crashing or producing wrong results. + test("rlike with lookbehind falls back to JVM") { + runQueryAndCompare( + s"""select rlike(l_comment, "(?<=x)y") from $LINEITEM_TABLE limit 5""", Review Comment: With `noFallBack = false` and an empty check, this test would also pass if the pattern ran natively. Could we assert the fallback explicitly, e.g. that the plan contains no `ProjectExecTransformer` for this projection, or that a fallback event with the expected reason is emitted? ########## gluten-substrait/src/main/scala/org/apache/gluten/expression/Re2PatternUtils.scala: ########## @@ -0,0 +1,169 @@ +/* + * 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 org.apache.gluten.exception.GlutenNotSupportException + +import org.apache.spark.sql.catalyst.expressions.{Expression, Literal} +import org.apache.spark.sql.types.StringType +import org.apache.spark.unsafe.types.UTF8String + +import java.util.Locale + +/** + * Utilities for translating regex patterns from Java syntax to RE2 syntax. Used by any backend + * whose native engine is RE2 (Velox, Bolt); see + * [[org.apache.gluten.backendsapi.SparkPlanExecApi#rewriteRegexPattern]]. + */ +object Re2PatternUtils { + + /** + * Implements [[org.apache.gluten.backendsapi.SparkPlanExecApi#rewriteRegexPattern]] for RE2 + * backends. A string literal pattern is translated to RE2 syntax; patterns with constructs that + * have no RE2 equivalent (lookaheads, lookbehinds, backreferences, unpaired surrogates) throw + * [[GlutenNotSupportException]] to trigger fallback. Non-literal patterns are returned unchanged. + */ + def rewriteRegexPattern(pattern: Expression): Expression = pattern match { + case Literal(p: UTF8String, StringType) => + val translated = translateJavaPatternToRe2(p.toString).getOrElse( + throw new GlutenNotSupportException( + s"Regex pattern ${describePattern(p.toString)} uses constructs " + + "(lookahead/lookbehind/backreference) or unpaired surrogates that RE2 cannot " + Review Comment: Nit: the message lists every possible cause. The translator already knows the actual one when it rejects, so returning `Either[String, String]` instead of `Option[String]` would let the fallback reason be specific, which helps when debugging fallbacks. ########## backends-velox/src/test/scala/org/apache/gluten/execution/FallbackSuite.scala: ########## @@ -397,12 +397,25 @@ class FallbackSuite extends VeloxWholeStageTransformerSuite with AdaptiveSparkPl assert(nestedLoopJoin.isDefined) val fallbackReasons = events.flatMap(_.fallbackNodeToReason.values) assert(fallbackReasons.nonEmpty) - assert(fallbackReasons.forall(_.contains("regexp_extract due to Pattern"))) + // The lookbehind/lookahead pattern is now rejected at the JVM level by + // rewriteRegexPattern before the native validator sees it, so the + // fallback reason contains the GlutenNotSupportException message instead + // of the old native "regexp_extract due to Pattern ..." message. + assert( + fallbackReasons.exists( + r => + r.contains("regexp_extract due to Pattern") || + r.contains("uses constructs (lookahead/lookbehind/backreference)")), Review Comment: Since the lookaround pattern is now always rejected on the JVM side by `rewriteRegexPattern`, could we assert only the new message? Accepting either message lets the test pass whichever path is taken. ########## gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala: ########## @@ -987,6 +1016,21 @@ object ExpressionConverter extends SQLConfHelper with Logging { } } + /** + * Lets the backend rewrite the pattern argument of a regex expression (e.g. translate it to RE2 + * syntax) before it is written into the Substrait literal. Backends reject patterns they cannot + * express by throwing [[GlutenNotSupportException]], which triggers fallback. + */ + private def withRewrittenRegexPattern( + patternChild: Expression, + attributeSeq: Seq[Attribute], + expressionsMap: Map[Class[_], String]): ExpressionTransformer = { + BackendsApiManager.getSparkPlanExecApiInstance.rewriteRegexPattern(patternChild) match { + case l: Literal => LiteralTransformer(l) + case other => replaceWithExpressionTransformer0(other, attributeSeq, expressionsMap) Review Comment: Nit: is the `case l: Literal => LiteralTransformer(l)` branch needed? Passing the rewritten expression directly to `replaceWithExpressionTransformer0` would send every pattern through the normal conversion path. If there's a reason for the special case, a short comment would help. -- 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]
