apache / apache/datafusion-comet
Native candidate assessment: upper/lower deferred on the lack of a per-batch defer-to-dispatcher mechanism
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
## Summary
`upper` / `lower` (and their `ucase` / `lcase` aliases) have a native path already, gated off by
default behind `spark.comet.caseConversion.enabled=false`. On the face of it they look like the same
opportunity as #5351: existing kernel, blanket `Incompatible`, flip the default for a safe subset.
They are not, and the reason is worth recording rather than rediscovering. **The only subset where the
engines provably agree is all-ASCII input, which is a per-batch runtime property, and Comet chooses
between the native path and the dispatcher at plan time during serde with no mechanism to defer a
single batch.** On top of that, the non-ASCII behaviour is not one behaviour but three across the
supported version matrix, one of which depends on the host JVM's default locale.
Assessed with the `suggest-native-expression` skill. Verdict: **Deferred**, upside High, compatibility
Low for the general case. Filing so the blocker is on record, because it is shared by a whole class of
candidates rather than being specific to case conversion.
## How it runs today
`class CometCaseConversionBase[T] extends CometScalarFunction[T](function) with NativeOptInAvailable`
(`spark/src/main/scala/org/apache/comet/serde/strings.scala:50`), with
`nativeOptInConfigKeyOverride = Some(CometConf.COMET_CASE_CONVERSION_ENABLED.key)`.
- `getIncompatibleReasons()` is the blanket string "Results can vary depending on locale and character
set".
- The gate is the **global** `spark.comet.caseConversion.enabled` (default `false`), not a
per-expression `allowIncompatible`. Its doc says "Java uses locale-specific rules when converting
strings to upper or lower case and Rust does not, so we disable upper and lower by default."
- Knock-on: `ilike` is `RuntimeReplaceable` and rewrites to `Like(Lower(l), Lower(r))`, so `ilike`
inherits this path too.
## Native upside: High
Dumped the real kernel for `Upper(BoundReference(0, StringType))` over a nullable `VarCharVector` via
`CometBatchKernelCodegen.generateSource`. The emitted hot loop, on a Spark 4.1 build:
```java
if (!isNull_1) {
value_0 = CollationSupport.Upper.execBinaryICU(value_1);
}
if (isNull_0) {
output.setNull(i);
} else {
Object utfBase_0 = value_0.getBaseObject();
int utfLen_0 = value_0.numBytes();
if (utfBase_0 instanceof byte[]) {
output.setSafe(i, (byte[]) utfBase_0, ..., utfLen_0);
} else {
byte[] utfArr_0 = value_0.getBytes();
output.setSafe(i, utfArr_0, 0, utfArr_0.length);
}
}
```
Per non-null row, on the ASCII path, `UTF8String.toUpperCaseAscii()` is `convertAscii(Character::toUpperCase)`,
which allocates a fresh `byte[]` and wraps it in a new `UTF8String`: **2 allocations per row,
unconditionally**, about 16k per 8192-row batch. Unlike `replace` there is no short-circuit that
returns the input unchanged, so every row pays.
Being precise about what is **not** a cost: the output write takes the `instanceof byte[]` branch (the
result is byte[]-backed), so there is no extra copy on the way into Arrow, and the input read is
zero-copy.
`upper` and `lower` are among the most common string expressions in real SQL (normalisation,
case-insensitive matching), and `ilike` rides on `Lower`. So the upside rating is High. It is simply
not collectable, see below.
## Compatibility: Low for the general case
### There are three different non-ASCII behaviours, not one
| Spark | Path under UTF8_BINARY | Non-ASCII behaviour |
| --- | --- | --- |
| 3.4.3, 3.5.9 | `UTF8String.toUpperCase()` | ASCII fast path, else `fromString(toString().toUpperCase())` → **JVM default locale** |
| 4.0.4, 4.1.3, 4.2.0 (default) | `CollationSupport.Upper.execBinaryICU` → `CollationAwareUTF8String.toUpperCase` | `isFullAscii()` fast path, else `UCharacter.toUpperCase(...)` → **ICU root locale** |
| 4.0+ with `spark.sql.icu.caseMappings.enabled=false` | `CollationSupport.Upper.execBinary` → `UTF8String.toUpperCase()` | as 3.x, **JVM default locale** |
`spark.sql.icu.caseMappings.enabled` defaults to **true** from Spark 4.0 (`SQLConf.scala`), which is
why the dumped 4.1 kernel calls `execBinaryICU`.
A native kernel would have to reproduce ICU root-locale full case mapping on one version, the JDK's
default-locale mapping on another, and pick between them from a config. The JDK branch is
**host-dependent**: the same query on a JVM with `-Duser.language=tr` produces different output, and
nothing in the serialised plan captures that.
### The agreed subset exists but cannot be gated
Both sides agree on all-ASCII input, and notably **Spark itself branches on exactly that property**
(`UTF8String.isFullAscii()`, and `CollationAwareUTF8String.toUpperCase` checks it first). So the subset
is real and even has a name in Spark's own code.
The problem is where the check can live:
- It is not knowable at plan time. Nothing in the schema or the expression tells you whether a column
is ASCII.
- It is knowable per batch, cheaply, by scanning the values buffer.
- But `getSupportLevel` runs once during serde, and the native-versus-dispatcher decision is baked
into the serialised plan. A native kernel that discovers non-ASCII bytes mid-batch has nowhere to
go: it cannot hand that batch to the JVM dispatcher.
So the ASCII guard, which is what makes this look tractable, is not expressible in the current
architecture. That is the blocker.
### Hazard checklist
| Hazard | Applies? |
| --- | --- |
| JVM formatting / library APIs | **Yes.** ICU root-locale mapping on 4.x, JDK default-locale mapping on 3.x. Reimplementing either is the `Low` criterion. |
| Dependence on host state beyond the plan | **Yes.** The JDK path reads the JVM default locale, which is not in the plan. |
| Collation on Spark 4.0+ | **Yes.** Non-UTF8_BINARY collations route to `execLowercase` / `execICU` ([#2190](https://github.com/apache/datafusion-comet/issues/2190), [#4496](https://github.com/apache/datafusion-comet/issues/4496)). |
| Cross-version behavioural differences | **Yes.** Three behaviours across the matrix, config-selected. |
| Backreferences / regex, decimal, timezone, invalid UTF-8, lambdas, nondeterminism | No |
## What would change the answer
Either of these unblocks it. Both are bigger than this expression.
1. **A per-batch defer-to-dispatcher mechanism.** A native kernel that can decline a batch (here: "not
all ASCII") and have the operator route that batch through the JVM codegen dispatcher instead. This
is adjacent to [#4825](https://github.com/apache/datafusion-comet/issues/4825) (partial project
fallback) but per batch rather than per plan. With it, `upper` / `lower` become straightforward: the
native kernel handles ASCII, everything else falls to Spark's own code, and the result stays exact.
2. **A proof, with pinned Unicode versions, that Rust's `str::to_uppercase` / `to_lowercase` agrees
with ICU root-locale mapping for all inputs**, plus a decision that the 3.x JDK-locale path is out
of scope (for example by requiring `spark.sql.icu.caseMappings.enabled=true`). This is a narrower
claim than "Rust matches Java" and might be checkable by brute force over the codepoint range, but
it is real work and Unicode-version-sensitive.
Until one of those lands, the current default (dispatcher, exact) is correct and this issue should stay
open as the record of why.
### Note for the wider effort
The same blocker applies to every candidate whose only safe guard is a data property rather than an
expression property: `initcap` (hyphen-as-word-separator, data-dependent) and the JSON family
(`get_json_object`, `from_json`, `to_json`, `length_of_json_array`, whose divergences are
single-quoted JSON, unescaped control characters, and trailing content, all data-dependent). Landing
mechanism 1 above would unlock that whole class at once, which is likely a better investment than
attacking them one at a time.
---
Filed by the `suggest-native-expression` skill. Motivation:
[Native Coverage for Codegen-Dispatched Expressions](https://github.com/apache/datafusion-comet/blob/main/docs/source/contributor-guide/roadmap.md#native-coverage-for-codegen-dispatched-expressions).
Assessment recorded in `docs/source/contributor-guide/expression-audits/string_funcs.md` under
`## upper` and `## lower`. Earlier runs of the same skill produced #5347, #5349, and #5351.
Contributor guide
Research direction
Read spark/src/main/scala/org/apache/comet/serde/strings.scala and the assessment in docs/source/contributor-guide/expression-audits/string_funcs.md, then review the partial project fallback discussion in #4825. Determine whether a per-batch defer-to-dispatcher mechanism or pinned Unicode compatibility proof is viable; done means the blocker is resolved without changing exact current behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust, scala
- Domain
- backend-api-design, data-engineering, performance
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100