Repository navigation
Conversation
…ed constructs
Translate Java-style patterns to RE2 syntax for backends that use RE2
(Velox, Bolt), gated by SparkPlanExecApi.needsRe2PatternTranslation so
other backends (e.g. ClickHouse) are unaffected.
- Add Re2PatternUtils: an escape-aware scanner that rewrites \uXXXX to
\x{XXXX}, combines surrogate pairs into one code point, and returns
None (so the plan falls back to Spark) for lookaround, backreferences
and unpaired surrogates.
- Apply it to RLike, RegExpExtract and RegExpExtractAll literal patterns
in ExpressionConverter.
- Add the same escape-pair and surrogate handling to the C++ safeguard
translateJavaUnicodeToRe2 in cpp/velox/utils/Common.cc.
- Add Re2PatternUtilsSuite, C++ CommonTest cases, and rlike cases in
VeloxStringFunctionsSuite; tighten the FallbackSuite assertion.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Use Locale.ROOT for toUpperCase, wrap long/non-ASCII lines, and import testImplicits for toDF in VeloxStringFunctionsSuite. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Run Gluten Clickhouse CI on x86 |
Resolve FallbackSuite.scala conflict: keep main's ANSI-disabled wrapper around the nested loop join test and our updated lookaround fallback-reason assertion. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ded to LocalRelation Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
3 similar comments
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
…ranslated copy
validatePattern translated \uXXXX to \x{XXXX} only for its own RE2 compile,
but the Substrait plan keeps the original literal and Velox compiles that
untranslated at execution time. A raw \uXXXX that reached the native side
(any path that bypasses withRe2PatternTranslation) was therefore approved by
the validator and then failed at runtime with "invalid escape sequence: \u"
instead of falling back to Spark.
Validate the pattern exactly as Velox will compile it. Patterns translated on
the Scala side (\x{XXXX}) still pass, so native coverage is unchanged; an
untranslated \uXXXX now falls back as it did on main.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Hey @zhouyuan, could you please have a look at this PR, thank you. |
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open findings
\uXXXXis present, but the… · New$patStr) directly. For very long patterns or… · New\1–\9, which is broader than the stated… · New
What changed in this PR
Adds Java→RE2 regex literal translation (mainly \uXXXX and surrogate pairs) for RE2-based backends to avoid unnecessary Spark fallbacks, while keeping a native-side RE2 compilation validator as a strict safeguard.
Changes:
- Add Scala-side regex literal translation + rejection of unsupported constructs (lookaround/backrefs/unpaired surrogates) gated by a new backend hook.
- Add Velox C++ helper for Java unicode escape translation (tested) and make
validatePattern()compile the raw pattern as-is. - Extend test coverage across Scala unit tests, Velox C++ tests, and Velox integration/fallback suites.
| File | Description |
|---|---|
| gluten-substrait/src/test/scala/org/apache/gluten/expression/Re2PatternUtilsSuite.scala | Scala unit tests for Java→RE2 translation and unsupported-construct detection |
| gluten-substrait/src/main/scala/org/apache/gluten/expression/Re2PatternUtils.scala | New Java-regex scanner/translator to RE2-compatible syntax |
| gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala | Translate regex literal arguments before writing Substrait literals (RE2 backends only) |
| gluten-substrait/src/main/scala/org/apache/gluten/backendsapi/SparkPlanExecApi.scala | Add needsRe2PatternTranslation hook (default false) |
| cpp/velox/utils/Common.h | Declare unicode-translation helper + clarify validator behavior |
| cpp/velox/utils/Common.cc | Implement unicode translation helper; validate raw pattern without translation |
| cpp/velox/tests/CommonTest.cc | Add C++ unit tests for translator + validator behavior |
| cpp/velox/tests/CMakeLists.txt | Register new common_test target |
| backends-velox/src/test/scala/org/apache/gluten/execution/VeloxStringFunctionsSuite.scala | Add integration tests for unicode escapes/surrogate pairs and fallback on lookbehind |
| backends-velox/src/test/scala/org/apache/gluten/execution/FallbackSuite.scala | Update fallback assertions + add regression ensuring unicode-escape rlike doesn’t fallback |
| backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala | Enable translation for Velox backend |
| backends-bolt/src/main/scala/org/apache/gluten/backendsapi/bolt/BoltSparkPlanExecApi.scala | Enable translation for Bolt backend |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Run Gluten Clickhouse CI on x86 |
- Document that translateJavaUnicodeToRe2 is lexical only: unpaired surrogate escapes are left as \uXXXX on purpose, so the output may be partly translated and is not guaranteed to compile in RE2. - Document that every backslash followed by 1-9 is rejected as a backreference, including inside a character class where Java itself rejects it, and add tests for [\1] and [\12]. - Quote, escape control characters in, and truncate the pattern (and RE2's own error text) in the Scala fallback reason and the native validator error. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
philo-he
left a comment
philo-he
left a comment
There was a problem hiding this comment.
Thanks. One comment. Please check whether it make sense.
| s"that RE2 cannot handle. Falling back to Spark.")) | ||
| LiteralTransformer(Literal(UTF8String.fromString(translated), StringType)) | ||
| case other => | ||
| replaceWithExpressionTransformer0(other, attributeSeq, expressionsMap) |
There was a problem hiding this comment.
Can we move this code to backend API? Then we only keep that and can remove needsRe2PatternTranslation.
// VeloxSparkPlanExecApi (and BoltSparkPlanExecApi)
override def rewriteRegexPattern(pattern: Expression): Expression = pattern match {
case Literal(p: UTF8String, StringType) =>
val translated = Re2PatternUtils
.translateJavaPatternToRe2(p.toString)
.getOrElse(throw new GlutenNotSupportException(
s"Regex pattern ${Re2PatternUtils.describePattern(p.toString)} uses constructs " +
"that RE2 cannot handle. Falling back to Spark."))
Literal(UTF8String.fromString(translated), StringType)
case other => other
}
There was a problem hiding this comment.
Thanks @philo-he, done in aeb75ef. needsRe2PatternTranslation is gone and SparkPlanExecApi now has rewriteRegexPattern(pattern: Expression): Expression, which returns the pattern unchanged by default. Velox and Bolt override it. Since the two bodies would be identical, the override delegates to a shared Re2PatternUtils.rewriteRegexPattern with your snippet (I kept the existing fallback wording so the FallbackSuite assertion still matches). ExpressionConverter now just calls the hook and wraps a literal result in LiteralTransformer; ClickHouse is unaffected because the default is a no-op.
Replace SparkPlanExecApi.needsRe2PatternTranslation with rewriteRegexPattern(pattern), which defaults to a no-op. Velox and Bolt override it to translate Java patterns to RE2 via Re2PatternUtils.rewriteRegexPattern, and ExpressionConverter no longer knows about RE2. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
philo-he
left a comment
philo-he
left a comment
There was a problem hiding this comment.
@KushShriv, thanks for the update.
My understanding is, this rewrite only works for literal regex pattern. Do you plan to support patterns from a column as well? I am thinking whether we can rewrite at a unified place in Velox for all regex functions, regardless of literal pattern or not. Does this make sense?
|
@philo-he Thanks. Yes, this only rewrites literal patterns. The translation, and the decision to fall back on constructs RE2 cannot express (lookaround, backreferences, unpaired surrogates), both happen at plan time, when the pattern value is known. Patterns from a column are not affected by this PR. Moving the rewrite into Velox would let us translate |
Problem
Velox and Bolt execute regexes with RE2, which doesn't understand Java's Unicode escapes (e.g.,
\uXXXX). A pattern like\u4e00-\u9fa5fails RE2 compilation, so the whole operator falls back to Spark (tracked inFallbackSuite). Supplementary characters written as surrogate pairs (\uD83D\uDE00) have the same problem.Solution
Translate to RE2 syntax in Scala, and make the C++ validator a strict safeguard.
ExpressionConverter.withRe2PatternTranslationrewrites literal patterns before they are written into the Substrait plan, forRegExpReplace,RLike,RegExpExtract,RegExpExtractAll,RegExpInstr, andStringSplit. It usesRe2PatternUtils.translateJavaPatternToRe2, a single escape-aware scanner:\uXXXX\x{XXXX}; a high+low surrogate pair\x{cp}\\u0041(escaped backslash +u0041) is left alone.None(GlutenNotSupportException1–9, including inside a character class, where Java itself rejects it), and unpaired surrogates. Escaped occurrences (\(?=,\\1) and lookaround markers inside a character class ([(?=]) are not flagged.[\1], so falling back lets Spark report it. Letting it through would be unsafe: RE2 reads[\12]as an octal escape and would run it natively where Spark throws.(?<n>...)are allowed.SparkPlanExecApi.needsRe2PatternTranslationis true. It is overridden to true for Velox and Bolt (both RE2-based) and defaults to false, so ClickHouse is unaffected. Bolt has no C++ validator change and relies on the Scala-side rewrite alone.validatePattern()compiles the pattern with RE2 exactly as given, with no translation. A raw\uthat reaches the native side (for example, a plan not built by Gluten's converter, or a new regex function added without Scala wiring) is rejected at validation, so the operator falls back to Spark instead of failing at runtime withVeloxUserError: invalid escape sequence: \u.translateJavaUnicodeToRe2is kept as a public, tested helper but the validator no longer uses it. It is a lexical rewrite only: an unpaired surrogate escape is left as\uXXXXon purpose so RE2 rejects it, so its output is not guaranteed to compile.Changes
gluten-substrait/.../Re2PatternUtils.scaladescribePatternfor error messagesgluten-substrait/.../ExpressionConverter.scalawithRe2PatternTranslation; explicitRLike/RegExpExtract/RegExpExtractAllcases; wired into all 6 regex expressionsgluten-substrait/.../SparkPlanExecApi.scalaneedsRe2PatternTranslationhook (defaultfalse)backends-velox/.../VeloxSparkPlanExecApi.scala,backends-bolt/.../BoltSparkPlanExecApi.scalatruecpp/velox/utils/Common.{h,cc}translateJavaUnicodeToRe2helper;validatePatterncompiles and checks the pattern as given (rejects raw\u) and reports a quoted, truncated pattern on failurecpp/velox/tests/CommonTest.cc,CMakeLists.txtcommon_testgluten-substrait/.../Re2PatternUtilsSuite.scalabackends-velox/.../FallbackSuite.scala\u4e00join-filter test now asserts native execution; lookbehind fallback assertion tightened to the specific reasonbackends-velox/.../VeloxStringFunctionsSuite.scalarliketests: Unicode escape, surrogate pair, escaped backslash run natively; lookbehind falls backTesting
Unit and integration tests (Spark 3.5, Velox):
common_test(C++)Re2PatternUtilsSuiteVeloxStringFunctionsSuiteFallbackSuiteEnd-to-end comparison on a 6,000-row table, with results checked against vanilla Spark and an independent Java regex oracle. Ten Unicode escape query shapes were run:
rlike,regexp_replace,regexp_extract,regexp_extract_all,regexp_instr,split, surrogate pairs, and escaped backslashes.Safeguard check: I ran the main Scala layer (no Phase 1) against this branch's native library to simulate a raw
\ureaching the C++ side.validatePatternand then failed withVeloxUserError ... invalid escape sequence: \u.VeloxUserError.Style checks:
scalastyle,checkstyle, andspotlesspass forbackends-veloxandgluten-substrait.Notes
[a|b],[a&&[b]]) is unaffected:ensureRegexIsCompatiblesees a string with the same bracket depth.StringTypeexactly, so such a pattern may skip translation. The C++ safeguard makes it fall back instead of failing at runtime.str_to_map(s, '\\u0020', '\\u0031')runs natively and fails at runtime (entryDelimiter's size should be 1) on both main and this branch.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-sonnet-5.5