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 |
KushShriv
marked this pull request as ready for review
October 15, 2026 08:07
Contributor
Author
|
Hey @zhouyuan, could you please have a look at this PR, thank you. |
|
Run Gluten Clickhouse CI on x86 |
Contributor
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.
| } | ||
| } else if (i + 1 < n) { | ||
| val next = pattern.charAt(i + 1) | ||
| if (next >= '1' && next <= '9') return None // backreference |
Comment on lines
+33
to
+34
| /// Converts Java \uXXXX Unicode escapes to RE2's \x{XXXX} syntax. | ||
| /// Returns the input unchanged if no \uXXXX sequences are found (idempotent). |
Comment on lines
+999
to
+1001
| .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.")) |
Comment on lines
+83
to
+84
| val next = pattern.charAt(i + 1) | ||
| if (next >= '1' && next <= '9') return None // backreference |
| // \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(); |
Copilot stopped reviewing on behalf of
zhouyuan due to an error
October 15, 2026 08:51
|
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 |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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