andygrove commented on PR #5350: URL: https://github.com/apache/datafusion-comet/pull/5350#issuecomment-5286725412
Fourth run, and the first **Deferred** verdict: **[#5353: `upper`/`lower` deferred on the lack of a per-batch defer-to-dispatcher mechanism](https://github.com/apache/datafusion-comet/issues/5353)**. Recorded under both `## upper` and `## lower` in `string_funcs.md`. `upper`/`lower` look like #5351 all over again: existing native kernel, blanket `Incompatible` reason, gated off by `spark.comet.caseConversion.enabled=false`. They are the most common string expressions in the pool, `ilike` rewrites to `Lower`, and the emitted kernel allocates 2 objects per non-null row unconditionally (`toUpperCaseAscii` is `convertAscii`, so a fresh `byte[]` plus a `UTF8String` wrapper, ~16k per batch). High upside on every axis. It still doesn't work, for two reasons that only showed up by dumping the kernel and reading `CollationSupport`: - **The kernel calls `execBinaryICU`, not `execBinary`.** `spark.sql.icu.caseMappings.enabled` defaults to `true` from Spark 4.0, so on 4.x the non-ASCII path is **ICU root-locale** mapping, while on 3.x (and on 4.x with that config off) it is `toString().toUpperCase()` under the **JVM default locale**. That is three behaviours across the pinned matrix, one of which is host-dependent: the same query on a JVM started with `-Duser.language=tr` produces different output, and nothing in the serialised plan captures that. - **The obvious guard is not expressible.** Both engines agree on all-ASCII input, and Spark itself branches on exactly that property (`UTF8String.isFullAscii()`, checked first inside `CollationAwareUTF8String.toUpperCase`). But that is a per-batch runtime property, and Comet fixes the native-versus-dispatcher choice at plan time during serde. A native kernel that finds non-ASCII bytes mid-batch has nowhere to hand them. So the issue records the blocker and two concrete unblocking conditions rather than proposing work that cannot land. The reason it is worth filing at all is that the blocker is **shared**: `initcap` and the JSON family (`get_json_object`, `from_json`, `to_json`, `length_of_json_array`) are all gated on data-dependent divergences too. A per-batch defer-to-dispatcher mechanism, adjacent to #4825 but per batch, would unlock the whole class at once — likely a better investment than picking them off individually. Two asides from this run: - I ranked `replace` second and dropped it, but it is probably the best *next* candidate: its entire divergence is "empty search string", which is plan-time detectable when the search argument is a literal. What stopped me recommending it is the upside, not the compatibility — `UTF8String.replace` returns `this` with zero allocation when there is no match, so unlike the other candidates the allocation saving only materialises on matching rows. That needs a match-density measurement before anyone commits to it. - `CometInitCap.getIncompatibleReasons()` cites #1052, which is **closed**. That string is user-facing (it lands in the Compatibility Guide and in EXPLAIN output), so it is a stale link in shipped docs. Not touching it here, but worth a small follow-up. This PR now carries four assessments across three category pages. -- 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]
