0lai0 opened a new pull request, #5720:
URL: https://github.com/apache/datafusion-comet/pull/5720
## Which issue does this PR close?
Closes #5591. Part of #5572.
## Rationale for this change
`CometNextDay` and `CometLevenshtein` decline non-UTF8_BINARY collated
arguments but do not mix in `CodegenDispatchFallback`, so a collated argument
fails the whole projection back to Spark. Both are ordinary codegen
expressions, so the dispatcher can run Spark's own `doGenCode` inside the Comet
pipeline instead.
Collated values do reach Comet operators, but only from the query text (`col
COLLATE x` or `CAST(col AS STRING COLLATE UTF8_LCASE)`). A table schema that
declares a collated column is rejected at scan (`Unsupported schema ...
StringType(UTF8_LCASE)`). The guards are live.
Neither Spark implementation consults collation (`getDayOfWeekFromString` /
`UTF8String.levenshteinDistance` take no collation id on 4.0.1 or 4.1.1,
neither appears in `CollationSupport`). That suggested dropping the guards and
running the native kernels. I implemented that variant and benchmarked it. It
lost on every axis, so this PR does what the issue originally proposed.
## What changes are included in this PR?
- Mix `CodegenDispatchFallback` into `CometNextDay` and `CometLevenshtein`.
- Collapse `CometLevenshtein`'s two collation reason strings into one
`private val`, reworded so it does not contradict the `GenerateDocs` "runs in
the JVM" header for mixin serdes.
- `expressions.md`: both rows `Native` → `Hybrid`.
- `understanding-comet-plans.md`: `levenshtein` was wrongly listed as
`CometCodegenDispatch` with no native kernel. The example now uses `pmod`.
- Collated cases on `CometStringExpressionBenchmark` and
`CometDatetimeExpressionBenchmark` (`isSpark40Plus`; `COLLATE` does not parse
on 3.4/3.5). `next_day` had no benchmark at all, so the default-collation case
is added too.
## Benchmark
1,048,576 rows, `spark-4.0`, `local[1]`. "guard removed" is the native-path
alternative above; it is not in this PR.
| Case | Spark | guard removed | this PR |
| --- | --- | --- | --- |
| `next_day`, UTF8_LCASE | 89 ms | 125 ms (0.7X) | **70 ms (1.3X)** |
| `levenshtein`, UTF8_LCASE | 378 ms | 489 ms (0.8X) | **341 ms (1.1X)** |
| `next_day`, default (control) | 90 ms | 104 ms | 104 ms |
| `levenshtein`, default (control) | 380 ms | 439 ms | 435 ms |
Controls match between variants, so only the collated path differs.
Dispatching is also the only variant that covers `col COLLATE x`: `Collate` has
no serde, and a `Compatible` expression never reaches the dispatcher.
Added benchmark cases on a second profile / data shape: collated `next_day`
1.4X on `spark-4.1`; collated `levenshtein` 2.6X / 3.2X on `spark-4.0`.
Default-collation `next_day` is 1.0X.
## How are these changes tested?
New Spark 4.0+ SQL fixtures:
- `datetime/next_day_collation.sql`, `datetime/next_day_ansi_collation.sql`
- `string/levenshtein_collation.sql`
Column and literal arguments, `UTF8_LCASE` / `UNICODE_CI` / RTRIM
collations, non-ASCII, bad day names, ANSI errors, three-argument
`levenshtein`, and NULL in data. NULL literals are omitted:
both expressions are `nullIntolerant`, so `NullPropagation` folds them
before Comet sees them.
The load-bearing case is `levenshtein('HELLO' COLLATE UTF8_LCASE, 'hello'
COLLATE UTF8_LCASE)` returning 5, not 0. That query fails first if Spark ever
makes `levenshtein` collation-aware.
`CometCollationSuite` (4.0 and 4.1) adds a `next_day` dispatcher test and an
`allowIncompatible=true` native-path test, matching `date_format`. The latter
is the only coverage of the native kernel on collated input.
`CometStringExpressionSuite` asserts the `levenshtein` path through
`assertCodegenRan`.
Local runs, no style checks skipped: `spark-4.0` 566/0/0, `spark-4.1`
561/0/0, `spark-3.5` 539/0/1 (`assume(isSpark40Plus)` skip). Suites:
`CometCollationSuite`, `CometSqlFileTestSuite`, `CometTemporalExpressionSuite`,
`CometStringExpressionSuite`.
## Known gap
`col COLLATE utf8_lcase` works here only because the dispatcher compiles the
whole bound subtree, including `Collate`. Comet has no serde for `Collate`
(`QueryPlanSerde.scala:1009`), so any `Compatible()` expression over a
`Collate` child still fails the projection, e.g.`length(s COLLATE utf8_lcase)`.
That is wider than this issue and predates this PR.
--
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]
