andygrove opened a new issue, #5353:
URL: https://github.com/apache/datafusion-comet/issues/5353
## 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.
--
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]
