andygrove opened a new issue, #6633:
URL: https://github.com/apache/datafusion-comet/issues/6633
Part of #6615.
## What / Why
This issue moves the Spark 4 collation suites to Comet SQL tests:
- `spark-4.0/CometCollationSuite`
- `spark-4.1/CometCollationSuite`
- `spark-4.x/CometSortCollationSuite`
- `spark-4.x/CometCastCollatedStringSuite`
Static analysis on `main` at 3bc2faa934 found the following:
| Suite | Covered | Convert | Blocked | Split | Keep | Total |
|---|---|---|---|---|---|---|
| `CometCastCollatedStringSuite` | 0 | 14 | 0 | 4 | 3 | 21 |
| `spark-4.0/CometCollationSuite` | 3 | 15 | 1 | 0 | 6 | 25 |
| `spark-4.1/CometCollationSuite` | 3 | 15 | 1 | 0 | 1 | 20 |
| `spark-4.x/CometSortCollationSuite` | 0 | 5 | 0 | 0 | 2 | 7 |
| **Total** | **6** | **49** | **2** | **4** | **12** | **73** |
The 4.1 collation suite is the 4.0 suite minus its five join-converter
tests, so the other 20 tests are identical in both. Converted into fixtures
gated `-- MinSparkVersion: 4.0`, they run on 4.0, 4.1 and 4.2, which takes most
of the duplication that #4770 is about off the table.
It also adds coverage. The shuffle and sort fallback reasons have never been
checked on Spark 4.2, because the 4.0 and 4.1 suites don't run there. If 4.2
reports different reasons, split the fixture into a `MaxSparkVersion: 4.1` /
`MinSparkVersion: 4.2` pair.
Collated tests in other suites (`rlike`, `unix_timestamp`, string
concat/reverse) are in the regex, datetime and string issues.
## Facts that affect the conversion
1. **Collated literals.** `'x' COLLATE c` parses to `Collate(Literal, ...)`.
With ConstantFolding excluded it stays a `Collate` node, which has no Comet
serde. `CAST('MON' AS STRING COLLATE UTF8_LCASE)` works instead, because
CometCast folds a cast of a literal into a collated `Literal`.
- That is enough for `next_day`'s native path (4.0 L386 / 4.1 L207).
- It is not enough for `date_format`, because
`CometDateFormat.nativeApplicable` needs a Spark-level `Literal` format. So 4.0
L403 / 4.1 L224 wait on #6618.
2. **Collated datetime fallbacks record two reasons.** When a
`CodegenDispatchFallback` serde reports `Incompatible` for a collation and the
dispatcher is disabled, Comet records both `<fn>:
spark.comet.exec.scalaUDF.codegen.enabled=false` and `... does not support
non-UTF8_BINARY collations`. `routing_datetime_collation_disabled.sql` asserts
the first and the Scala tests assert the second, so each expression needs a
second `expect_fallback` query.
3. **Cast eval modes.** For `CometCastCollatedStringSuite`, the cast values
never raise. So `-- ConfigMatrix: spark.sql.ansi.enabled=false,true` plus
`try_cast` covers all three eval modes that `isSupported` checks.
## Work
Collation-suite line numbers are given as "4.0 Lx / 4.1 Ly".
**Datetime collation routing (existing fixtures)**
- `datetime/routing_datetime_collation_disabled.sql`
- Add a second query per expression asserting the collation-specific
reason: `next_day` (L292 / L113), `convert_timezone` (L359 / L180), `trunc`
(L366 / L187), `date_trunc` (L372 / L193), and `date_format` with a collated
*column* format (L379 / L200).
- Add `from_unixtime` with `expect_fallback(Only the default datetime
format pattern)` (L315 / L136).
- Add `expect_native(unix_timestamp)` for `unix_timestamp(ts, fmt_ts
COLLATE UTF8_LCASE)` (L299 / L120), using a new `fmt_ts` column.
- `datetime/routing_datetime_collation_enabled.sql`
- The same `unix_timestamp` query with codegen on.
- `expect_dispatch(date_format)` with a collated column format (L426 /
L247). The existing query uses a foldable `Collate(Literal)` format, which
takes `DateFormatClass`'s precomputed-formatter branch instead of the per-row
one.
- `datetime/unix_timestamp.sql` (L440 / L261): add the two-argument
`unix_timestamp(ts, fmt_column)` with `expect_native`.
**New fixtures (all `MinSparkVersion: 4.0`)**
- `datetime/next_day_collation_allow_incompatible.sql` (L386 / L207)
- Codegen off and `NextDay.allowIncompatible=true`.
- `expect_native(next_day)` over `next_day(d, CAST('MON' AS STRING COLLATE
UTF8_LCASE))`.
- `string/collation_shuffle_fallback.sql` (L47/L35, L55/L43, L62/L50,
L70/L58, L77/L65)
- listagg DISTINCT, DISTINCT and GROUP BY on a collated string, each with
`expect_fallback(unsupported hash partitioning data type for columnar shuffle)`.
- ORDER BY on a collated string, with `expect_fallback(unsupported range
partitioning data type for columnar shuffle)`.
- A native control over Parquet.
- `windows/window_sort_collation.sql` (sort L51, L118, and the window half
of L182), with AQE off
- `expect_fallback(Sort does not support non-default string collation)`
for keys `c, _1`, `_1, c, _2` and `struct(array(c)), _2`.
- Native window controls.
- `string/collation_sort.sql` (sort L160 and the TopK half of L182): TopK
`ORDER BY c, _1 LIMIT 2` over a `REPARTITION(1)` subquery.
- `string/collation_sort_round_robin.sql` (sort L135): jvm shuffle, with
`SELECT /*+ REPARTITION(2) */ ... SORT BY c, _1`.
- `cast/routing_cast_collation_disabled.sql` and
`cast/routing_cast_collation_enabled.sql` (`CometCastCollatedStringSuite`)
- Table `(_1 string, _2 int)` with `('123', 1)` and `('456', 2)`, plus the
ANSI ConfigMatrix.
- Disabled file: codegen off, and `expect_fallback(Cast involving a
non-default string collation is not supported)` for L98, L102, L109, L113,
L117, L131, L140, L161, L170, L174, the convertible halves of L146, L151 and
L156, and the end-to-end tests L224 and L243.
- Enabled file: `expect_dispatch(cast)` for the same expressions, the
end-to-end tests L234 and L261, and `expect_native(cast)` for the uncollated
half of L183.
- Things to check:
- L109: Spark must type `AS STRING` as UTF8_BINARY.
- L151 and L170: Spark 4 must accept a collated map key in `CreateMap`.
- L174: the suite says no SQL builds `ArrayType(NullType) ->
ARRAY<STRING COLLATE UTF8_LCASE>`, but with ConstantFolding excluded
`CAST(array(NULL) AS ...)` should reach `isSupported`.
**Blocked**
- **4.0 L403 / 4.1 L224** (`date_format` with a collated literal format):
needs ConstantFolding, #6618.
## Stays in Scala
- **4.0 L119, L149, L175, L203, L228** call the join operator serde on
hand-built plans; the test comment says end-to-end SQL cannot reach the join
converter. The 4.1 copy dropped these, although they build plans directly and
could run on 4.1+. Consider moving them to a 4.x shim while deleting the rest
of these suites.
- **Sort L65, L93** call `QueryPlanSerde.supportedSortType` on synthetic
SortExec plans: map keys and values, which SQL cannot sort by, and the struct
single-key versus multi-key rule.
- **4.0 L328 / 4.1 L149** pin explain text (#6182). Their dispatch/fallback
assertions duplicate `routing_datetime_collation_{enabled,disabled}.sql` L57-61.
- **The identity casts in `CometCastCollatedStringSuite`** (L121, L135,
L189, and the identity halves of L146, L151, L156 and L183). `SimplifyCasts`
drops them before Comet sees them.
## 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.
What stays in Scala afterwards (the explain-text tests and the
join-converter serde tests) is small enough to move into one `spark-4.x` suite.
That would retire both version-specific `CometCollationSuite` files and resolve
#4770.
<details><summary>Every Scala test in scope, with its verdict and target
fixture</summary>
| Suite | Line | Test | Verdict | Target |
|---|---|---|---|---|
| `CometCastCollatedStringSuite` | 98 | cast collated string to IntegerType
has no native path | convert | cast/routing_cast_collation_disabled.sql (new);
cast/routing_cast_collation_enabled.sql (new) |
| `CometCastCollatedStringSuite` | 102 | cast IntegerType to collated string
has no native path | convert | cast/routing_cast_collation_disabled.sql (new);
cast/routing_cast_collation_enabled.sql (new) |
| `CometCastCollatedStringSuite` | 109 | cast collated string to
default-collation StringType has no native path | convert |
cast/routing_cast_collation_disabled.sql (new);
cast/routing_cast_collation_enabled.sql (new) |
| `CometCastCollatedStringSuite` | 113 | cast default-collation StringType
to collated string has no native path | convert |
cast/routing_cast_collation_disabled.sql (new);
cast/routing_cast_collation_enabled.sql (new) |
| `CometCastCollatedStringSuite` | 117 | cast between two different
collations has no native path | convert |
cast/routing_cast_collation_disabled.sql (new);
cast/routing_cast_collation_enabled.sql (new) |
| `CometCastCollatedStringSuite` | 121 | cast collated string to the same
collation has no native path | keep | |
| `CometCastCollatedStringSuite` | 131 | cast array of collated strings to
another collation has no native path | convert |
cast/routing_cast_collation_disabled.sql (new);
cast/routing_cast_collation_enabled.sql (new) |
| `CometCastCollatedStringSuite` | 135 | cast array of collated strings to
the same collation has no native path | keep | |
| `CometCastCollatedStringSuite` | 140 | cast array of collated strings to
StringType has no native path | convert |
cast/routing_cast_collation_disabled.sql (new);
cast/routing_cast_collation_enabled.sql (new) |
| `CometCastCollatedStringSuite` | 146 | cast struct with a collated field
has no native path | split | cast/routing_cast_collation_disabled.sql (new);
cast/routing_cast_collation_enabled.sql (new) |
| `CometCastCollatedStringSuite` | 151 | cast map with a collated key has no
native path | split | cast/routing_cast_collation_disabled.sql (new);
cast/routing_cast_collation_enabled.sql (new) |
| `CometCastCollatedStringSuite` | 156 | cast map with a collated value has
no native path | split | cast/routing_cast_collation_disabled.sql (new);
cast/routing_cast_collation_enabled.sql (new) |
| `CometCastCollatedStringSuite` | 161 | cast struct whose collated field is
unchanged while a sibling field is cast | convert |
cast/routing_cast_collation_disabled.sql (new);
cast/routing_cast_collation_enabled.sql (new) |
| `CometCastCollatedStringSuite` | 170 | cast map whose collated key is
unchanged while the value type is cast | convert |
cast/routing_cast_collation_disabled.sql (new);
cast/routing_cast_collation_enabled.sql (new) |
| `CometCastCollatedStringSuite` | 174 | cast array of nulls to array of
collated strings has no native path | convert |
cast/routing_cast_collation_disabled.sql (new) |
| `CometCastCollatedStringSuite` | 183 | default-collation string casts are
untouched by the collation guard | split |
cast/routing_cast_collation_enabled.sql (new) |
| `CometCastCollatedStringSuite` | 189 | nested default-collation string
casts are untouched by the collation guard | keep | |
| `CometCastCollatedStringSuite` | 224 | cast from a collated string falls
back to Spark when codegen dispatch is off | convert |
cast/routing_cast_collation_disabled.sql (new) |
| `CometCastCollatedStringSuite` | 234 | cast from a collated string routes
through the codegen dispatcher when it is on | convert |
cast/routing_cast_collation_enabled.sql (new) |
| `CometCastCollatedStringSuite` | 243 | cast of a struct carrying a
collated field has no native path end to end | convert |
cast/routing_cast_collation_disabled.sql (new) |
| `CometCastCollatedStringSuite` | 261 | cast of a struct carrying a
collated field routes through the codegen dispatcher | convert |
cast/routing_cast_collation_enabled.sql (new) |
| `spark-4.0/CometCollationSuite` | 47 | listagg DISTINCT with utf8_lcase
collation (issue #1947) | convert | string/collation_shuffle_fallback.sql (new)
|
| `spark-4.0/CometCollationSuite` | 55 | DISTINCT on utf8_lcase collated
string groups case-insensitively | convert |
string/collation_shuffle_fallback.sql (new) |
| `spark-4.0/CometCollationSuite` | 62 | GROUP BY utf8_lcase collated string
groups case-insensitively | convert | string/collation_shuffle_fallback.sql
(new) |
| `spark-4.0/CometCollationSuite` | 70 | ORDER BY utf8_lcase collated string
sorts case-insensitively | convert | string/collation_shuffle_fallback.sql
(new) |
| `spark-4.0/CometCollationSuite` | 77 | default UTF8_BINARY string still
runs through Comet | convert | string/collation_shuffle_fallback.sql (new) |
| `spark-4.0/CometCollationSuite` | 119 | CometBroadcastHashJoinExec rejects
non-default collated join keys | keep | |
| `spark-4.0/CometCollationSuite` | 149 | CometHashJoinExec rejects
non-default collated join keys | keep | |
| `spark-4.0/CometCollationSuite` | 175 | CometBroadcastHashJoinExec still
accepts default UTF8_BINARY string keys | keep | |
| `spark-4.0/CometCollationSuite` | 203 | CometSortMergeJoinExec rejects
non-default collated join keys | keep | |
| `spark-4.0/CometCollationSuite` | 228 | CometSortMergeJoinExec still
accepts default UTF8_BINARY string keys | keep | |
| `spark-4.0/CometCollationSuite` | 292 | next_day rejects non-UTF8_BINARY
collated dayOfWeek (issue #4646) | convert |
datetime/routing_datetime_collation_disabled.sql |
| `spark-4.0/CometCollationSuite` | 299 | unix_timestamp stays native with a
collated format for timestamp input (issue #4646) | convert |
datetime/routing_datetime_collation_disabled.sql +
datetime/routing_datetime_collation_enabled.sql |
| `spark-4.0/CometCollationSuite` | 315 | from_unixtime rejects
non-UTF8_BINARY collated format (issue #4646) | convert |
datetime/routing_datetime_collation_disabled.sql |
| `spark-4.0/CometCollationSuite` | 328 | $functionName with collated
arguments has no native opt-in (codegen=$codegenEnabled, issue #6080) [loop:
make_timestamp\|to_unix_timestamp x codegen false\|true = 4 instances] | keep |
|
| `spark-4.0/CometCollationSuite` | 359 | convert_timezone rejects
non-UTF8_BINARY collated timezone (issue #4646) | convert |
datetime/routing_datetime_collation_disabled.sql |
| `spark-4.0/CometCollationSuite` | 366 | trunc falls back with collated
format when codegen is disabled (issue #4646) | convert |
datetime/routing_datetime_collation_disabled.sql |
| `spark-4.0/CometCollationSuite` | 372 | date_trunc falls back with
collated format when codegen is disabled (issue #4646) | convert |
datetime/routing_datetime_collation_disabled.sql |
| `spark-4.0/CometCollationSuite` | 379 | date_format falls back with
collated format when codegen is disabled (issue #4646) | convert |
datetime/routing_datetime_collation_disabled.sql |
| `spark-4.0/CometCollationSuite` | 386 | next_day uses native path with
collated dayOfWeek when allowIncompatible is enabled | convert |
datetime/next_day_collation_allow_incompatible.sql (new) |
| `spark-4.0/CometCollationSuite` | 403 | date_format uses native path with
collated format when allowIncompatible is enabled | blocked: #6618 |
datetime/date_format_collation_allow_incompatible.sql (new) |
| `spark-4.0/CometCollationSuite` | 415 | trunc routes collated format
through codegen dispatcher (issue #4646) | covered |
datetime/routing_datetime_collation_enabled.sql |
| `spark-4.0/CometCollationSuite` | 420 | date_trunc routes collated format
through codegen dispatcher (issue #4646) | covered |
datetime/routing_datetime_collation_enabled.sql |
| `spark-4.0/CometCollationSuite` | 426 | date_format routes collated format
through codegen dispatcher (issue #4646) | convert |
datetime/routing_datetime_collation_enabled.sql |
| `spark-4.0/CometCollationSuite` | 432 | next_day routes collated dayOfWeek
through codegen dispatcher (issue #5591) | covered |
datetime/routing_datetime_collation_enabled.sql |
| `spark-4.0/CometCollationSuite` | 440 | datetime expressions still run
with default UTF8_BINARY collation (issue #4646) | convert |
datetime/unix_timestamp.sql |
| `spark-4.1/CometCollationSuite` | 35 | listagg DISTINCT with utf8_lcase
collation (issue #1947) | convert | string/collation_shuffle_fallback.sql (new)
|
| `spark-4.1/CometCollationSuite` | 43 | DISTINCT on utf8_lcase collated
string groups case-insensitively | convert |
string/collation_shuffle_fallback.sql (new) |
| `spark-4.1/CometCollationSuite` | 50 | GROUP BY utf8_lcase collated string
groups case-insensitively | convert | string/collation_shuffle_fallback.sql
(new) |
| `spark-4.1/CometCollationSuite` | 58 | ORDER BY utf8_lcase collated string
sorts case-insensitively | convert | string/collation_shuffle_fallback.sql
(new) |
| `spark-4.1/CometCollationSuite` | 65 | default UTF8_BINARY string still
runs through Comet | convert | string/collation_shuffle_fallback.sql (new) |
| `spark-4.1/CometCollationSuite` | 113 | next_day rejects non-UTF8_BINARY
collated dayOfWeek (issue #4646) | convert |
datetime/routing_datetime_collation_disabled.sql |
| `spark-4.1/CometCollationSuite` | 120 | unix_timestamp stays native with a
collated format for timestamp input (issue #4646) | convert |
datetime/routing_datetime_collation_disabled.sql +
datetime/routing_datetime_collation_enabled.sql |
| `spark-4.1/CometCollationSuite` | 136 | from_unixtime rejects
non-UTF8_BINARY collated format (issue #4646) | convert |
datetime/routing_datetime_collation_disabled.sql |
| `spark-4.1/CometCollationSuite` | 149 | $functionName with collated
arguments has no native opt-in (codegen=$codegenEnabled, issue #6080) [loop:
make_timestamp\|to_unix_timestamp x codegen false\|true = 4 instances] | keep |
|
| `spark-4.1/CometCollationSuite` | 180 | convert_timezone rejects
non-UTF8_BINARY collated timezone (issue #4646) | convert |
datetime/routing_datetime_collation_disabled.sql |
| `spark-4.1/CometCollationSuite` | 187 | trunc falls back with collated
format when codegen is disabled (issue #4646) | convert |
datetime/routing_datetime_collation_disabled.sql |
| `spark-4.1/CometCollationSuite` | 193 | date_trunc falls back with
collated format when codegen is disabled (issue #4646) | convert |
datetime/routing_datetime_collation_disabled.sql |
| `spark-4.1/CometCollationSuite` | 200 | date_format falls back with
collated format when codegen is disabled (issue #4646) | convert |
datetime/routing_datetime_collation_disabled.sql |
| `spark-4.1/CometCollationSuite` | 207 | next_day uses native path with
collated dayOfWeek when allowIncompatible is enabled | convert |
datetime/next_day_collation_allow_incompatible.sql (new) |
| `spark-4.1/CometCollationSuite` | 224 | date_format uses native path with
collated format when allowIncompatible is enabled | blocked: #6618 |
datetime/date_format_collation_allow_incompatible.sql (new) |
| `spark-4.1/CometCollationSuite` | 236 | trunc routes collated format
through codegen dispatcher (issue #4646) | covered |
datetime/routing_datetime_collation_enabled.sql |
| `spark-4.1/CometCollationSuite` | 241 | date_trunc routes collated format
through codegen dispatcher (issue #4646) | covered |
datetime/routing_datetime_collation_enabled.sql |
| `spark-4.1/CometCollationSuite` | 247 | date_format routes collated format
through codegen dispatcher (issue #4646) | convert |
datetime/routing_datetime_collation_enabled.sql |
| `spark-4.1/CometCollationSuite` | 253 | next_day routes collated dayOfWeek
through codegen dispatcher (issue #5591) | covered |
datetime/routing_datetime_collation_enabled.sql |
| `spark-4.1/CometCollationSuite` | 261 | datetime expressions still run
with default UTF8_BINARY collation (issue #4646) | convert |
datetime/unix_timestamp.sql |
| `spark-4.x/CometSortCollationSuite` | 51 | window sort with UTF8_LCASE and
multiple keys falls back to Spark (issue #6158) | convert |
../windows/window_sort_collation.sql (new) |
| `spark-4.x/CometSortCollationSuite` | 65 | sort type check rejects
non-default collation in every key and nested type | keep | |
| `spark-4.x/CometSortCollationSuite` | 93 | sort type check preserves
default collation and existing single-key restrictions | keep | |
| `spark-4.x/CometSortCollationSuite` | 118 | window sort rejects collated
keys after another key and inside nested types | convert |
../windows/window_sort_collation.sql (new) |
| `spark-4.x/CometSortCollationSuite` | 135 | local sort after round-robin
shuffle rejects non-default collation | convert |
string/collation_sort_round_robin.sql (new) |
| `spark-4.x/CometSortCollationSuite` | 160 | top-k with multiple keys
rejects non-default collation | convert | string/collation_sort.sql (new) |
| `spark-4.x/CometSortCollationSuite` | 182 | default UTF8_BINARY string and
multi-key struct sorting remain native | convert |
../windows/window_sort_collation.sql (new) + string/collation_sort.sql (new) |
</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]
