andygrove opened a new issue, #6627:
URL: https://github.com/apache/datafusion-comet/issues/6627
Part of #6615.
## What / Why
This issue moves the regex coverage in `CometRegExpJvmSuite`,
`CometRegexParitySuite` and the `rlike` tests in `CometExpressionSuite` to
Comet SQL tests under `sql-tests/expressions/string/`. Static analysis on
`main` at 3bc2faa934 found the following:
| Suite | Covered | Convert | Blocked | Split | Keep | Total |
|---|---|---|---|---|---|---|
| `CometRegExpJvmSuite` | 12 | 53 | 8 | 0 | 1 | 74 |
| `CometRegexSuite` | 0 | 0 | 0 | 0 | 14 | 14 |
| `CometRegexParitySuite` | 0 | 3 | 0 | 0 | 0 | 3 |
| `CometExpressionSuite` | 2 | 2 | 0 | 0 | 0 | 4 |
| **Total** | **14** | **58** | **8** | **0** | **15** | **95** |
Only 14 of the 95 are covered by a fixture today, and none of those depends
on `\d`, `\w` or a backreference. The reason is the first item below.
## Fix first: seven regex fixtures don't run the patterns they name
Spark's SQL string-literal unescaping (`unescapeSQLString`, with
`spark.sql.parser.escapedStringLiterals=false`) drops the backslash before any
character that is not a recognized escape. So `'\d'` becomes `d`, `'\w'`
becomes `w`, `'\1'` becomes `1` and `'\+'` becomes `+`.
These fixtures write single backslashes:
| Fixture | Lines |
| --- | --- |
| `regexp_extract.sql` | 28, 32, 36, 44, 48, 56 |
| `regexp_extract_all.sql` | 28, 32, 36, 44, 52 |
| `regexp_instr.sql` | 28, 32, 40, 48 |
| `regexp_replace_java.sql` | 27, 30, 34, 38, 42, 50 |
| `regexp_replace_rust.sql` | 29, 32, 36 |
| `rlike_java.sql` | 27, 37, 41, 49 |
| `rlike_auto_native.sql` | 38 |
Examples:
- `rlike_java.sql:27` runs `^d+$`.
- The "backreference (Java-only)" queries run `^(w)1$`, which contains no
backreference.
- `regexp_replace_java.sql:34` (`'(\d+)-(\d+)'`) matches no row, so its
`$2-$1` replacement is never applied.
- `rlike_auto_native.sql:38` (`'a\+b'`) is `a+b`, i.e. one or more `a`
followed by `b`, not the literal string.
The two `*_enabled.sql` regex fixtures already write `'\\d'` correctly. Use
`'\\d'` or a raw literal `r'\d'`; raw literals work on every supported Spark
version.
The Scala tests have the same bug. In `CometExpressionSuite`'s `rlike` test
(L1507), none of the classes and anchors (`\d \D \w \W \b \B \h \H \s \S \v \V
\A \Z \z`) ever reached a regex engine. In `rlike whitespace` (L1473), six of
the 13 patterns are literals that can never match. So the conversion has to fix
the patterns, not copy them.
## Work
Line numbers refer to `CometRegExpJvmSuite` unless marked; "parity" means
`CometRegexParitySuite`.
The full per-test `add` text is in the analysis; this is the condensed
version. Use `expect_dispatch(rlike)` / `expect_native(rlike)` wherever the
Scala test pins the engine. The explain-text checks `contains("JVM codegen
dispatcher: rlike")` map one-to-one onto these. `assertCodegenRan` is weaker:
it passes if anything in the block compiled a kernel.
**Existing fixtures to fix and extend**
- `rlike_java.sql`
- Covers L65-L155.
- Fix lines 27, 37, 41 and 49.
- Add `'\\d+'` and `'^(\\w)\\1$'` in a projection and in a filter,
`'foo(?=bar)'`, `'(?<=foo)bar'`, `'(?<digit>\\d)'`, `'^$'`, an all-NULL table,
and the backreference filter with GROUP BY.
- All of these patterns are outside the native subset, so pin them with
`expect_dispatch(rlike)`.
- `rlike_auto_native.sql`
- Covers L224 and L606 (4.0+).
- Line 28 becomes `expect_native(rlike)`. The file currently says routing
is asserted in the Scala suites this issue deletes.
- `rlike_rust.sql`
- Covers L501, plus `CometExpressionSuite` L1473 and L1507.
- Add `expect_native(rlike)` for `'\\d+'` under the opt-in.
- Add CR, LF and CRLF rows with `^` / `$` anchors.
- Add the curated class, qualifier and anchor set with doubled backslashes.
- `routing_regex_enabled.sql`
- Covers L488 and L943.
- Add `expect_dispatch(rlike)` for `'\\d+'`, and
`expect_dispatch(regexp_instr)`, which no fixture pins today.
- `routing_regex_disabled.sql`
- Covers L419 and L549.
- Add `expect_fallback(rlike:
spark.comet.exec.scalaUDF.codegen.enabled=false)` for the 64-branch pattern and
for `'\\d+'`.
- `regexp_extract.sql`
- Covers L682-L722.
- Fix the backslashes, add rows, and add all-NULL input.
- `regexp_extract_all.sql`
- Covers L730-L757.
- Fix the backslashes, and add `'([a-z])(\\d)'` with groups 1 and 2, and
`'foo(?=ba[rz])'`.
- `regexp_replace_java.sql`
- Covers L766-L797.
- Fix the backslashes.
- Add `'(\\w+) (\\w+)'` with `'$2 $1'`, `'foo(?=bar)'`, the empty pattern,
and all-NULL input.
- `regexp_instr.sql`
- Covers L852-L871.
- Fix the backslashes, and add groups 1 and 2 and a lookahead.
- `split_java.sql`
- Covers L879, L885 and L909.
- Add `'x,,y'`, `'\\d+'` and all-NULL input.
**New fixtures**
- `rlike_java_batches.sql`: L169, 5000 generated rows across several batches.
- `rlike_routing.sql`
- Covers the analyzer-budget tests: L237-L465 and L531.
- Write each pattern end to end as `expect_dispatch(rlike)` or
`expect_native(rlike)`.
- Patterns up to 16,388 characters must be spelled out in full; `repeat()`
would make the pattern non-literal and change the route.
- `rlike_native_parity.sql`
- Covers parity L129 and L136.
- 43 subjects × 30 corpus patterns, in a projection and in a filter, all
`expect_native(rlike)`.
- Write control characters and line terminators as `\uXXXX`.
- `rlike_native_batches.sql`: parity L163, with `spark.comet.batchSize=64`
and 5000 rows.
- `rlike_collation.sql` and `rlike_collation_opt_in.sql` (`MinSparkVersion:
4.0`)
- L620 and L648 use a collated subject with `expect_dispatch(rlike)`.
- RLike is not in Spark's collation-coercion list, so the pattern stays a
plain literal.
- `regexp_extract_batches.sql` (L917) and `regexp_combined.sql` (L929).
- `regexp_replace_errors.sql` and `regexp_replace_errors_row_filter.sql`
(4.0+)
- L803 and L828. These are blocked on #6617.
- `expect_error(INVALID_REGEXP_REPLACE)` can go in now as an interim check.
For the invalid-pattern tests (L144, L272, L579), check the `expect_error`
substring on each profile. Spark's codegen path throws a raw
`PatternSyntaxException` ("Unclosed character class") from generated init code,
while the interpreted path wraps it in `INVALID_PARAMETER_VALUE.PATTERN`.
**Blocked**
- **#6617 (error parity):** L272, L579, L803, L828.
- **#6618 (optimizer rules):**
- L531 needs `NullPropagation` excluded.
- L634 and L664 need a collated pattern literal, which only exists when
ConstantFolding runs.
- A mid-file `statement SET spark.sql.optimizer.excludedRules=...` may
work today; this was reasoned from the code but not run.
- **L440:** JDK 21 only, with a 303,103-character pattern. There is no
JDK-version directive. L419 covers the same route with a 64-branch pattern, and
`CometRegexSuite` L222 unit-tests the analyzer verdict, so this can stay in
Scala.
## Stays in Scala
- **All 14 `CometRegexSuite` tests.** They are unit tests of
`CometRegex.supportLevel`, the plan-time analyzer, with no Spark session.
Several of the patterns don't compile in Java at all.
- **L585.** It pins a Comet-only failure: the opt-in sends a lookahead to
the Rust engine, which rejects it, while Spark succeeds. `expect_error` needs
both engines to fail.
## Notes on the Scala tests
- **L144** ("invalid pattern falls back to Spark") never checks a fallback.
It is a weaker duplicate of L579.
- **L943** ("always run via the JVM dispatcher") never pins the dispatcher.
Its queries duplicate L682, L730 and L852.
- **Covered by other tests:** L606 is a subset of L224, and parity L129 is
implied by L136.
## Done when
- Every test in the table below is accounted for. Either a fixture covers it
(the same inputs, configs and assertion strength, or stronger), the fixture
that already covers it is cited, or it stays in Scala for the reason given
above.
- Each converted Scala test is deleted in the PR that adds its coverage. A
suite left empty is deleted and removed from the suite lists in
`.github/workflows/pr_build_linux.yml` and `pr_build_macos.yml`.
- The new and changed fixtures pass on every Spark profile. Apply
`run-all-spark-profiles`, because pull requests run only the default profile.
<details><summary>Every Scala test in scope, with its verdict and target
fixture</summary>
| Suite | Line | Test | Verdict | Target |
|---|---|---|---|---|
| `CometExpressionSuite` | 1367 | rlike simple case | covered |
string/rlike_rust.sql |
| `CometExpressionSuite` | 1462 | rlike with non-scalar pattern runs via
codegen dispatcher | covered | string/routing_regex_enabled.sql |
| `CometExpressionSuite` | 1473 | rlike whitespace | convert |
string/rlike_rust.sql |
| `CometExpressionSuite` | 1507 | rlike | convert | string/rlike_rust.sql |
| `CometRegExpJvmSuite` | 65 | rlike: projection produces Java regex
semantics with null handling | convert | string/rlike_java.sql |
| `CometRegExpJvmSuite` | 72 | rlike: predicate filters rows using Java
regex semantics | convert | string/rlike_java.sql |
| `CometRegExpJvmSuite` | 79 | rlike: backreference in projection (Java-only
construct) | convert | string/rlike_java.sql |
| `CometRegExpJvmSuite` | 90 | rlike: backreference in predicate (Java-only
construct) | convert | string/rlike_java.sql |
| `CometRegExpJvmSuite` | 101 | rlike: lookahead pattern (Java-only
construct) | convert | string/rlike_java.sql |
| `CometRegExpJvmSuite` | 108 | rlike: lookbehind pattern (Java-only
construct) | convert | string/rlike_java.sql |
| `CometRegExpJvmSuite` | 114 | rlike: embedded case-insensitive flag
(Java-only construct) | covered | string/rlike_java.sql |
| `CometRegExpJvmSuite` | 120 | rlike: named groups (Java-only construct) |
convert | string/rlike_java.sql |
| `CometRegExpJvmSuite` | 126 | rlike: empty pattern matches every non-null
row | covered | string/rlike_java.sql |
| `CometRegExpJvmSuite` | 132 | rlike: empty subject string is handled
correctly | convert | string/rlike_java.sql |
| `CometRegExpJvmSuite` | 138 | rlike: all-null subject column produces
all-null result | convert | string/rlike_java.sql |
| `CometRegExpJvmSuite` | 144 | rlike: invalid pattern falls back to Spark |
convert | string/rlike_java.sql |
| `CometRegExpJvmSuite` | 155 | rlike: combines with filter, projection, and
aggregate | convert | string/rlike_java.sql |
| `CometRegExpJvmSuite` | 169 | rlike: many rows spanning multiple batches |
convert | string/rlike_java_batches.sql (new) |
| `CometRegExpJvmSuite` | 224 | rlike: safe literal pattern takes the native
path by default | convert | string/rlike_auto_native.sql |
| `CometRegExpJvmSuite` | 237 | rlike: Rust class set operations stay on the
dispatcher | convert | string/rlike_routing.sql (new) |
| `CometRegExpJvmSuite` | 258 | rlike: leading-bracket class ranges stay on
the dispatcher | convert | string/rlike_routing.sql (new) |
| `CometRegExpJvmSuite` | 272 | rlike: raw [ range endpoint stays on the
dispatcher and preserves Spark error | blocked: #6617 |
string/rlike_routing.sql (new) |
| `CometRegExpJvmSuite` | 287 | rlike: over-budget counted repetition stays
on the dispatcher | convert | string/rlike_routing.sql (new) |
| `CometRegExpJvmSuite` | 329 | rlike: compile-budget boundary stays native
| convert | string/rlike_routing.sql (new) |
| `CometRegExpJvmSuite` | 342 | rlike: exact-zero counted repetitions stay
native | convert | string/rlike_routing.sql (new) |
| `CometRegExpJvmSuite` | 356 | rlike: aggregate expansion budget stays on
the dispatcher | convert | string/rlike_routing.sql (new) |
| `CometRegExpJvmSuite` | 370 | rlike: nested quantified stars stay on the
dispatcher | convert | string/rlike_routing.sql (new) |
| `CometRegExpJvmSuite` | 384 | rlike: capturing groups copied by counted
repetition stay on the dispatcher | convert | string/rlike_routing.sql (new) |
| `CometRegExpJvmSuite` | 406 | rlike: uncounted capturing groups stay on
the dispatcher | convert | string/rlike_routing.sql (new) |
| `CometRegExpJvmSuite` | 419 | rlike: uncounted capturing groups fall back
to Spark when the dispatcher is disabled | convert |
string/routing_regex_disabled.sql |
| `CometRegExpJvmSuite` | 440 | rlike: full uncounted-capture reproducer
falls back to Spark when JVM dispatcher is disabled | blocked: other |
string/routing_regex_disabled_large.sql (new) |
| `CometRegExpJvmSuite` | 465 | rlike: repeated complex character classes
stay on the dispatcher | convert | string/rlike_routing.sql (new) |
| `CometRegExpJvmSuite` | 488 | rlike: unsafe literal pattern stays on the
JVM dispatcher by default | convert | string/routing_regex_enabled.sql |
| `CometRegExpJvmSuite` | 501 | rlike: unsafe but Rust-accepted pattern is
native after opt-in | convert | string/rlike_rust.sql |
| `CometRegExpJvmSuite` | 516 | rlike: non-literal pattern stays on the
dispatcher | covered | string/routing_regex_enabled.sql |
| `CometRegExpJvmSuite` | 531 | rlike: null literal pattern stays on the
dispatcher | blocked: #6618 | string/rlike_routing.sql (new) |
| `CometRegExpJvmSuite` | 549 | rlike: unsafe pattern falls back to Spark
when the dispatcher is disabled | convert | string/routing_regex_disabled.sql |
| `CometRegExpJvmSuite` | 559 | rlike: in-subset pattern stays native when
the dispatcher is disabled | covered | string/routing_regex_disabled.sql |
| `CometRegExpJvmSuite` | 579 | rlike: invalid pattern preserves Spark
exception type and message | blocked: #6617 | string/rlike_java.sql |
| `CometRegExpJvmSuite` | 585 | rlike: Java-only pattern with
allowIncompatible keeps existing opt-in behavior | keep | |
| `CometRegExpJvmSuite` | 606 | rlike: UTF8_BINARY safe literal is native on
Spark 4 | convert | string/rlike_auto_native.sql |
| `CometRegExpJvmSuite` | 620 | rlike: non-default collation on the subject
stays on the dispatcher | convert | string/rlike_collation.sql (new) |
| `CometRegExpJvmSuite` | 634 | rlike: non-default collation on the pattern
stays on the dispatcher | blocked: #6618 | string/rlike_collation.sql (new) |
| `CometRegExpJvmSuite` | 648 | rlike: collated subject stays on dispatcher
even with allowIncompatible | convert | string/rlike_collation_opt_in.sql (new)
|
| `CometRegExpJvmSuite` | 664 | rlike: collated pattern stays on dispatcher
even with allowIncompatible | blocked: #6618 |
string/rlike_collation_opt_in.sql (new) |
| `CometRegExpJvmSuite` | 682 | regexp_extract: basic group extraction |
convert | string/regexp_extract.sql |
| `CometRegExpJvmSuite` | 691 | regexp_extract: group 0 returns entire match
| convert | string/regexp_extract.sql |
| `CometRegExpJvmSuite` | 697 | regexp_extract: no match returns empty
string | covered | string/regexp_extract.sql |
| `CometRegExpJvmSuite` | 703 | regexp_extract: backreference pattern
(Java-only) | convert | string/regexp_extract.sql |
| `CometRegExpJvmSuite` | 709 | regexp_extract: lookahead pattern
(Java-only) | convert | string/regexp_extract.sql |
| `CometRegExpJvmSuite` | 715 | regexp_extract: embedded flags (Java-only) |
covered | string/regexp_extract.sql |
| `CometRegExpJvmSuite` | 722 | regexp_extract: all-null column | convert |
string/regexp_extract.sql |
| `CometRegExpJvmSuite` | 730 | regexp_extract_all: basic extraction of all
matches | convert | string/regexp_extract_all.sql |
| `CometRegExpJvmSuite` | 736 | regexp_extract_all: group 0 returns full
matches | covered | string/regexp_extract_all.sql |
| `CometRegExpJvmSuite` | 742 | regexp_extract_all: multiple groups |
convert | string/regexp_extract_all.sql |
| `CometRegExpJvmSuite` | 751 | regexp_extract_all: no matches returns empty
array | covered | string/regexp_extract_all.sql |
| `CometRegExpJvmSuite` | 757 | regexp_extract_all: lookahead pattern
(Java-only) | convert | string/regexp_extract_all.sql |
| `CometRegExpJvmSuite` | 766 | regexp_replace: basic replacement | convert
| string/regexp_replace_java.sql |
| `CometRegExpJvmSuite` | 772 | regexp_replace: backreference in pattern
(Java-only) | convert | string/regexp_replace_java.sql |
| `CometRegExpJvmSuite` | 778 | regexp_replace: backreference in replacement
| convert | string/regexp_replace_java.sql |
| `CometRegExpJvmSuite` | 785 | regexp_replace: lookahead pattern
(Java-only) | convert | string/regexp_replace_java.sql |
| `CometRegExpJvmSuite` | 791 | regexp_replace: empty pattern replaces
between characters | convert | string/regexp_replace_java.sql |
| `CometRegExpJvmSuite` | 797 | regexp_replace: all-null column | convert |
string/regexp_replace_java.sql |
| `CometRegExpJvmSuite` | 803 | regexp_replace: invalid group reference
surfaces the original Spark exception | blocked: #6617 |
string/regexp_replace_errors.sql (new) |
| `CometRegExpJvmSuite` | 828 | regexp_replace: invalid group reference in a
filter surfaces the original exception | blocked: #6617 |
string/regexp_replace_errors_row_filter.sql (new) |
| `CometRegExpJvmSuite` | 852 | regexp_instr: basic position finding |
convert | string/regexp_instr.sql |
| `CometRegExpJvmSuite` | 858 | regexp_instr: specific group position |
convert | string/regexp_instr.sql |
| `CometRegExpJvmSuite` | 865 | regexp_instr: no match returns 0 | covered |
string/regexp_instr.sql |
| `CometRegExpJvmSuite` | 871 | regexp_instr: lookahead (Java-only) |
convert | string/regexp_instr.sql |
| `CometRegExpJvmSuite` | 879 | split: basic regex split | convert |
string/split_java.sql |
| `CometRegExpJvmSuite` | 885 | split: regex pattern | convert |
string/split_java.sql |
| `CometRegExpJvmSuite` | 891 | split: with limit | covered |
string/split_java.sql |
| `CometRegExpJvmSuite` | 897 | split: limit -1 returns all | covered |
string/split_java.sql |
| `CometRegExpJvmSuite` | 903 | split: lookahead pattern (Java-only) |
covered | string/split_java.sql |
| `CometRegExpJvmSuite` | 909 | split: all-null column | convert |
string/split_java.sql |
| `CometRegExpJvmSuite` | 917 | regexp_extract: many rows spanning multiple
batches | convert | string/regexp_extract_batches.sql (new) |
| `CometRegExpJvmSuite` | 929 | all regexp expressions combined in one query
| convert | string/regexp_combined.sql (new) |
| `CometRegExpJvmSuite` | 943 | expressions with no native path always run
via the JVM dispatcher | convert | string/routing_regex_enabled.sql |
| `CometRegexParitySuite` | 129 | analyzer admits every corpus pattern |
convert | string/rlike_native_parity.sql (new) |
| `CometRegexParitySuite` | 136 | native rlike matches Spark for every
admitted pattern | convert | string/rlike_native_parity.sql (new) |
| `CometRegexParitySuite` | 163 | native rlike matches Spark across multiple
Arrow batches | convert | string/rlike_native_batches.sql (new) |
| `CometRegexSuite` | 38 | admits ASCII literals, classes, quantifiers,
groups, and alternation | keep | |
| `CometRegexSuite` | 69 | admits lexer-boundary patterns that a substring
search would misclassify | keep | |
| `CometRegexSuite` | 78 | rejects constructs that diverge from Java regex
or are unrecognized | keep | |
| `CometRegexSuite` | 110 | rejects scanner edge cases the whitelist must
not silently admit | keep | |
| `CometRegexSuite` | 131 | rejects Rust-only character-class set operations
| keep | |
| `CometRegexSuite` | 135 | rejects unescaped ] used as a character-class
atom or range endpoint | keep | |
| `CometRegexSuite` | 141 | rejects unescaped [ used as a character-class
range endpoint | keep | |
| `CometRegexSuite` | 146 | rejects counted or nested patterns that can
exceed the Rust compile budget | keep | |
| `CometRegexSuite` | 163 | rejects aggregate patterns that exceed the Rust
compile budget | keep | |
| `CometRegexSuite` | 179 | rejects nested quantified patterns that can
exceed the Rust compile budget | keep | |
| `CometRegexSuite` | 192 | rejects capturing groups duplicated by counted
repetition past the compile budget | keep | |
| `CometRegexSuite` | 208 | rejects repeated complex character classes that
can exceed the Rust compile budget | keep | |
| `CometRegexSuite` | 222 | rejects uncounted capturing groups that exceed
the compile budget | keep | |
| `CometRegexSuite` | 230 | capture cost propagates through uncounted
repetition and composition | keep | |
</details>
--
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]
