andygrove opened a new issue, #6626:
URL: https://github.com/apache/datafusion-comet/issues/6626
Part of #6615.
## What / Why
This issue moves string expression coverage to Comet SQL tests under
`sql-tests/expressions/string/`. The sources are `CometStringExpressionSuite`,
the Spark 3.x `CometStringDecodeSuite`, and the string tests in
`CometExpressionSuite` (substring, like, contains, char/varchar, read-side
padding). Regex tests have their own issue. Static analysis on `main` at
3bc2faa934 found the following:
| Suite | Covered | Convert | Blocked | Split | Keep | Total |
|---|---|---|---|---|---|---|
| `CometStringExpressionSuite` | 12 | 27 | 0 | 0 | 1 | 40 |
| `CometStringDecodeSuite` | 0 | 1 | 0 | 0 | 0 | 1 |
| `CometExpressionSuite` | 6 | 8 | 1 | 0 | 0 | 15 |
| **Total** | **18** | **36** | **1** | **0** | **1** | **56** |
Most of the work extends existing fixtures, which are thinner than the Scala
tests. They have:
- no genuinely decomposed or combining characters (the Scala "combining"
rows are themselves precomposed; see below);
- no binary `lpad`/`rpad`;
- no Parquet string-predicate pushdown;
- no `concat_ws` with scalar subqueries or native array arguments;
- no case-sensitivity check for `contains`;
- no escapes or Contains rewrite in `like`.
## Work
Line numbers refer to `CometStringExpressionSuite`, except L902, L914,
L1269, L1306, L1321, L1349, L1573, L1952 and L2909, which are in
`CometExpressionSuite`.
**Existing fixtures to extend**
- `string_lpad.sql` / `string_rpad.sql` (L66, L70, L74)
- Add decomposed `'é'` and Telugu rows, also with a NULL length and with
length 4.
- Add literal non-positive lengths with a column string (`lpad(s, -6,
'x')`, `lpad(s, 0)`) and the multi-character pad `'zzz'` with a column length.
- `split_rust.sql` (L196, L207, L228)
- Add limits 3 and 0, and the regex separators `'\\s+'` and `'\\d+'`. The
backslashes must be doubled; see the regex issue.
- Add UTF-8 rows: CJK, emoji, a genuinely decomposed accent, RTL scripts,
supplementary characters, mixed text with `'[a-z]+'`, and a zero-width space.
- `chr.sql` (L334)
- Add negative and large column values (61231, -61231, -1700, 512) and the
literals 555, -555 and `cast(0.5 as int)`.
- `init_cap_enabled.sql` (L354): add `'james ähtäri'`.
- `string_trim.sql` (L378)
- Add `trim('SL', s)`, `ltrim('SL', s)` and `rtrim('SL', s)`.
- Add `btrim(s)`, `btrim('SL', s)` (where the column is the trim set) and
`btrim(s, 'SL')`.
- Add `upper(trim(s))` and its variants.
- `string_instr.sql`, `string_replace.sql`, `string_translate_enabled.sql`
(L409)
- Add `instr(s, '手机')` over `'苹果手机'`, the two-argument `replace(s,
'SQL')`, and a native `translate(s, 'SL苹', '123')`.
- `length.sql` (L423): add an `X'00FF'` row to the binary table.
- `substring.sql`
- Covers nine `CometStringExpressionSuite` tests (L577-L759) and
`CometExpressionSuite` L902 and L914.
- Rows: `'a'`, `'ab'`, `'x'`, `'hello'`, `'world!'`, mixed-width `'aé苹😀'`,
`'苹果手机'`, `'😀🎉🔥'`, and real decomposed, precomposed and Telugu strings.
- Add the missing start/length combinations, the `-2147483647` cases, the
literal `(-300, 3)`, `substring(s, -2, -2)`, `substring(s, 0, 0)`, the
two-argument `substring(s, 0)`, and four multi-byte literals.
- `like.sql`
- L1321: a `'%..%'` pattern (the Contains rewrite) and use in a WHERE
clause over mixed-case data.
- L1349: `LIKE '%\_%'` and `LIKE '%$_%' ESCAPE '$'`. Spark's unescaping
keeps `\_` and `\%` as written.
- `contains.sql` (L1573): rows that differ only in case, a needle at the
start of the string, and use in a WHERE clause.
- `misc/codegen_dispatch_disabled_fallback.sql` (L1952)
- Add `initcap` with
`expect_fallback(spark.comet.exec.scalaUDF.codegen.enabled=false)`.
- The opt-in half is already in `init_cap_enabled.sql`.
- `decode.sql`, plus a new `decode_disabled.sql` (`MaxSparkVersion: 3.5`),
for the `CometStringDecodeSuite` test
- Set `spark.comet.expression.StringDecode.enabled=false`.
- Assert `expect_fallback(Expression support is disabled. Set
spark.comet.expression.StringDecode.enabled=true to enable it.)`.
**New fixtures**
- `binary_pad.sql` (L95, L99): binary `lpad`/`rpad` with column and literal
str/len/pad combinations. These go through the dispatcher as `StaticInvoke`.
- `like_parquet_pushdown.sql` (L456, L496)
- Use a ConfigMatrix over
`spark.sql.parquet.filterPushdown.stringPredicate` (and
`parquet.enable.dictionary`), plus `parquet.block.size=512`.
- Write one-file tables with `range(0, n, 1, 1)` and add LIKE / NOT LIKE
filters.
- `concat_ws_scalar_subquery.sql` (L782): dispatcher off, a 32-row
single-file table, and six scalar-subquery queries.
- `concat_ws_native.sql` (L799)
- Turn the dispatcher off and set `StringSplit.allowIncompatible=true`, so
a plain `query` proves native execution.
- Add `concat_ws('', arr, s, array('x', NULL, 'y'))`.
- `char_varchar.sql`
- L1269: `char(7)`, `char(8)` and `varchar(6)` columns under `substring`.
- L2909: a `CHAR(2)` column holding `'é'`, `'é'`, `''` and `'ab'`.
- `collation_native_opt_in.sql` (L43, `MinSparkVersion: 4.0`)
- Dispatcher off, with `Concat.allowIncompatible=true` and
`Reverse.allowIncompatible=true`.
- `expect_native(concat)` / `expect_native(reverse)` over `CAST('abc' AS
STRING COLLATE UTF8_LCASE)`. Because CometCast folds a literal cast, this does
produce the collated `Literal` the Scala test builds.
**Blocked**
- **`CometExpressionSuite` L1306 (char/varchar over-length values).** It
needs a table over files that already contain over-length values. A fixture
cannot point a table at a location, and the write-side length check rejects
such values. Keep it in Scala unless the framework gains `LOCATION` support.
## Stays in Scala
- **L444** reads `test-data/dictionary-string-binary.parquet`, an Arrow
dictionary-typed file that SQL cannot write.
## Notes on the Scala tests
- **"split string with UTF-8 characters".** Its "combining" row is
byte-identical to the precomposed row: both use U+00E9 and U+00EF.
- **"substring - unicode multi-byte characters".** `"café" // combining
accent` is precomposed. Neither test covers combining characters, so the
fixtures add real ones.
- **"Upper and Lower" and "trim".** They set `Upper.allowIncompatible` /
`Lower.allowIncompatible`, which do nothing: the native path is gated on
`spark.comet.caseConversion.enabled`. Both tests therefore run the dispatcher.
- **"lpad/rpad with NULL length"** says the generator never emits NULL
integers. It does now.
- **All-literal pad combinations.** Spark folds them in the Scala suite.
They really run in Comet once in a fixture.
- **`concat_ws.sql:44-45`** uses `spark_answer_only` on all-literal
arguments, and its comment says the plan dispatches. Pin it with
`expect_dispatch(concat_ws)`.
## 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` | 902 | string type and substring | convert |
string/substring.sql |
| `CometExpressionSuite` | 914 | substring with start < 1 | convert |
string/substring.sql |
| `CometExpressionSuite` | 925 | substring with dictionary | covered |
string/substring.sql |
| `CometExpressionSuite` | 1269 | charvarchar | convert |
string/char_varchar.sql (new) |
| `CometExpressionSuite` | 1306 | char varchar over length values | blocked:
other | string/char_varchar.sql (new) |
| `CometExpressionSuite` | 1321 | like (LikeSimplification enabled) |
convert | string/like.sql |
| `CometExpressionSuite` | 1349 | like with custom escape | convert |
string/like.sql |
| `CometExpressionSuite` | 1573 | contains | convert | string/contains.sql |
| `CometExpressionSuite` | 1613 | contains with both columns | covered |
string/contains.sql |
| `CometExpressionSuite` | 1621 | startswith | covered |
string/starts_with.sql |
| `CometExpressionSuite` | 1637 | endswith | covered | string/ends_with.sql |
| `CometExpressionSuite` | 1952 | enable incompat expression using dynamic
config | convert | misc/codegen_dispatch_disabled_fallback.sql |
| `CometExpressionSuite` | 2049 | unhex | covered | string/unhex.sql |
| `CometExpressionSuite` | 2909 | readSidePadding | convert |
string/char_varchar.sql (new) |
| `CometExpressionSuite` | 3752 | test length function | covered |
string/length.sql |
| `CometStringDecodeSuite` | 32 | StringDecode honors
spark.comet.expression.StringDecode.enabled | convert | string/decode.sql;
string/decode_disabled.sql (new) |
| `CometStringExpressionSuite` | 43 | collated strings preserve native
opt-in routing | convert | string/collation_native_opt_in.sql (new) |
| `CometStringExpressionSuite` | 66 | lpad string | convert |
string/string_lpad.sql |
| `CometStringExpressionSuite` | 70 | rpad string | convert |
string/string_rpad.sql |
| `CometStringExpressionSuite` | 74 | lpad/rpad with NULL length | convert |
string/string_lpad.sql; string/string_rpad.sql |
| `CometStringExpressionSuite` | 95 | lpad binary | convert |
string/binary_pad.sql (new) |
| `CometStringExpressionSuite` | 99 | rpad binary | convert |
string/binary_pad.sql (new) |
| `CometStringExpressionSuite` | 186 | split string basic | covered |
string/split_rust.sql |
| `CometStringExpressionSuite` | 196 | split string with limit | convert |
string/split_rust.sql |
| `CometStringExpressionSuite` | 207 | split string with regex patterns |
convert | string/split_rust.sql |
| `CometStringExpressionSuite` | 220 | split string edge cases | covered |
string/split_rust.sql |
| `CometStringExpressionSuite` | 228 | split string with UTF-8 characters |
convert | string/split_rust.sql |
| `CometStringExpressionSuite` | 286 | Various String scalar functions |
covered | string/ascii.sql; string/bit_length.sql; string/octet_length.sql |
| `CometStringExpressionSuite` | 298 | Upper and Lower | covered |
string/upper.sql; string/lower.sql |
| `CometStringExpressionSuite` | 313 | Chr | covered | string/chr.sql |
| `CometStringExpressionSuite` | 323 | Chr with null character | covered |
string/chr.sql |
| `CometStringExpressionSuite` | 334 | Chr with negative and large value |
convert | string/chr.sql |
| `CometStringExpressionSuite` | 354 | InitCap compatible cases | convert |
string/init_cap_enabled.sql |
| `CometStringExpressionSuite` | 368 | InitCap incompatible cases | covered
| string/init_cap.sql |
| `CometStringExpressionSuite` | 378 | trim | convert |
string/string_trim.sql |
| `CometStringExpressionSuite` | 400 | string repeat | covered |
string/string_repeat.sql |
| `CometStringExpressionSuite` | 409 | length, reverse, instr, replace,
translate | convert | string/string_instr.sql; string/string_replace.sql;
string/string_translate_enabled.sql |
| `CometStringExpressionSuite` | 423 | length on binary input runs natively
| convert | string/length.sql |
| `CometStringExpressionSuite` | 444 | length on dictionary-typed string and
binary columns from an Arrow-written file | keep | |
| `CometStringExpressionSuite` | 456 | string predicate filter | convert |
string/like_parquet_pushdown.sql (new) |
| `CometStringExpressionSuite` | 496 | filter pushdown - StringPredicate |
convert | string/like_parquet_pushdown.sql (new) |
| `CometStringExpressionSuite` | 537 | string_space | covered |
string/string_space.sql |
| `CometStringExpressionSuite` | 543 | string_space with dictionary |
covered | string/string_space.sql |
| `CometStringExpressionSuite` | 553 | substring | covered |
string/substring.sql |
| `CometStringExpressionSuite` | 577 | substring - negative start boundary
cases | convert | string/substring.sql |
| `CometStringExpressionSuite` | 598 | substring - negative start with zero
and negative length | convert | string/substring.sql |
| `CometStringExpressionSuite` | 614 | substring - single character and
empty strings | convert | string/substring.sql |
| `CometStringExpressionSuite` | 627 | substring - unicode multi-byte
characters | convert | string/substring.sql |
| `CometStringExpressionSuite` | 656 | substring - decomposed and combining
unicode characters | convert | string/substring.sql |
| `CometStringExpressionSuite` | 686 | substring - large start and length
values | convert | string/substring.sql |
| `CometStringExpressionSuite` | 698 | substring - dictionary encoded
strings | convert | string/substring.sql |
| `CometStringExpressionSuite` | 731 | substring - scalar inputs | convert |
string/substring.sql |
| `CometStringExpressionSuite` | 759 | substring - scalar inputs with
multi-byte | convert | string/substring.sql |
| `CometStringExpressionSuite` | 782 | concat_ws with scalar subqueries over
a multi-row batch | convert | string/concat_ws_scalar_subquery.sql (new) |
| `CometStringExpressionSuite` | 799 | concat_ws with array<string>
arguments | convert | string/concat_ws_native.sql (new); string/concat_ws.sql |
| `CometStringExpressionSuite` | 840 | levenshtein routes collated strings
through the codegen dispatcher (issue #5591) | covered |
string/levenshtein_collation.sql; string/routing_string_collation_enabled.sql |
</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]
