[PR] [WIP][CORE] Avoid allocation and boxing in UTF8String ASCII case conversion [spark]

["david-mollitor-db (via GitHub)" <gi...@apache.org>]

david-mollitor-db opened a new pull request, #59198:
URL: https://github.com/apache/spark/pull/59198

   ### What changes were proposed in this pull request?
   
   `UTF8String.toLowerCase()` / `toUpperCase()` dispatch to the ASCII fast paths
   `toLowerCaseAscii()` / `toUpperCaseAscii()` once `isFullAscii()` holds. This PR rewrites
   those fast paths:
   
   - Convert bytes with direct ASCII arithmetic (`A`-`Z` <-> `a`-`z` by +/- 32) instead of
     routing each byte through an autoboxed `Function<Character, Character>` and
     `Character.toLowerCase`/`toUpperCase` (a full Unicode-table lookup). Over ASCII code
     points the two are identical.
   - Allocate the result array lazily, only when a byte actually changes case, bulk-copying
     the unchanged prefix with `Platform.copyMemory`. A string already in the target case
     returns `this` with no allocation.
   - Tag the converted result as full ASCII (`IsFullAscii.FULL_ASCII`), since ASCII case
     folding stays ASCII, so downstream operations skip re-scanning for ASCII-ness.
   - Remove the now-unused `convertAscii` helper and its `java.util.function.Function` import,
     and add an `assert isFullAscii()` precondition to both methods (callers already gate on
     `isFullAscii()`).
   
   Also adds `UTF8StringBenchmark` exercising `toLowerCase`/`toUpperCase` over all-lowercase,
   all-uppercase, and mixed-case ASCII inputs.
   
   NOTE (WIP): the benchmark result file is not yet committed; it will be regenerated on
   GitHub Actions for a consistent environment.
   
   ### Why are the changes needed?
   
   `toLowerCase`/`toUpperCase` back the SQL `lower()`/`upper()` functions and are hot in
   predicates such as `lower(col) LIKE '%...%'`. The previous fast path always allocated a new
   array and `UTF8String` and did per-byte autoboxing plus a Unicode case-mapping lookup, even
   though the string was already known to be ASCII. For strings already in the target case this
   is pure overhead; when conversion is needed, the Unicode lookup is unnecessary for ASCII.
   
   ### Does this PR introduce _any_ user-facing change?
   
   No. Results are unchanged for every input. One internal nuance: on the no-change path the
   returned value now aliases the input's backing memory rather than being a fresh copy, which
   is consistent with `UTF8String`'s existing immutability / copy-on-retain contract.
   
   ### How was this patch tested?
   
   Covered by existing `UTF8StringSuite` assertions for `toLowerCase`/`toUpperCase`, including
   the already-cased cases that exercise the new return-`this` path.
   
   Added `UTF8StringBenchmark`. Preliminary local numbers (JDK 17, indicative only; the
   committed result file will be regenerated on GitHub Actions per the PR template,
   https://spark.apache.org/developer-tools.html#github-workflow-benchmarks), best-time ns/row
   over 16000 strings of 64 ASCII chars, master vs. this PR:
   
   | operation | input | master (ns/row) | this PR (ns/row) | speedup |
   |---|---|---|---|---|
   | toLowerCase | all lowercase (no-change fast path) | 101.9 | 71.7 | 1.42x |
   | toLowerCase | all uppercase (full convert) | 126.7 | 113.6 | 1.12x |
   | toLowerCase | mixed case | 413.0 | 381.5 | 1.08x |
   | toUpperCase | all lowercase (full convert) | 143.7 | 113.6 | 1.26x |
   | toUpperCase | all uppercase (no-change fast path) | 124.6 | 71.1 | 1.75x |
   | toUpperCase | mixed case | 459.3 | 370.6 | 1.24x |
   
   The largest gain is on already-cased inputs, which return `this` with no allocation; the
   conversion path is faster too from dropping the per-byte boxing and Unicode lookup. Fixed
   harness overhead (the benchmark passes the operation as a function, which blocks inlining)
   dilutes the relative numbers, so the per-call improvement is larger than shown.
   
   ### Was this patch authored or co-authored using generative AI tooling?
   
   Generated-by: Isaac
   
   This pull request and its description were written by Isaac.
   


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59951][CORE] Avoid allocation and boxing in UTF8String ASCII case conversion [spark]

["HyukjinKwon (via GitHub)" <gi...@apache.org>]

HyukjinKwon commented on code in PR #59198:
URL: https://github.com/apache/spark/pull/59198#discussion_r4166090199


##########
common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java:
##########
@@ -821,8 +808,36 @@ public UTF8String toUpperCase() {
     return isFullAscii() ? toUpperCaseAscii() : toUpperCaseSlow();
   }
 
+  /**
+   * Returns the upper case of this ASCII string. The caller must ensure the string is full
+   * ASCII; the normal entry points reach this only after {@link #isFullAscii()} returns true.
+   *
+   * The result array is allocated lazily, only once a byte actually changes case: a string
+   * that is already upper case returns {@code this} with no allocation. Because upper-casing
+   * ASCII stays ASCII, the result is tagged full ASCII so later operations can skip re-scanning.
+   */
   public UTF8String toUpperCaseAscii() {
-    return convertAscii(Character::toUpperCase);
+    assert isFullAscii();
+    byte[] bytes = null;
+    for (int i = 0; i < numBytes; i++) {
+      byte b = getByte(i);
+      byte upper = (b >= 'a' && b <= 'z') ? (byte) (b - 32) : b;
+      if (upper != b && bytes == null) {
+        // First byte to change case: allocate the result and copy the clean prefix [0, i).
+        bytes = new byte[numBytes];
+        copyMemory(base, offset, bytes, BYTE_ARRAY_OFFSET, i);

Review Comment:
   **Nit (P3):** This new lazy-allocation branch bulk-copies the already-correct prefix `[0, i)` with `copyMemory` before the first case-changing byte, but no deterministic test exercises it with a non-empty prefix. `UTF8StringSuite.upperAndLower` only uses full-flip ASCII inputs (`ABCXYZ`/`abcxyz`, first change at index 0, so the prefix length is always 0) and non-ASCII inputs that take the slow path, and `UTF8StringPropertyCheckSuite` generates full-Unicode strings that essentially never land on the ASCII fast path. A future off-by-one or wrong-offset regression in this prefix copy would silently corrupt `lower()`/`upper()` output for the common mixed-case input (e.g. `toUpperCase("ABCdef")`) with no failing assertion. Consider adding a one-line assertion for a full-ASCII string whose first case-changing byte is past index 0, for both methods -- e.g. `assertEquals(fromString("ABCDEF"), fromString("ABCdef").toUpperCase())` and its lowercase mirror.
   
   **Verification:**
   - **Regression:** A unit assertion that corrupting the preserved clean prefix (wrong copy length/offset) would fail: for an ASCII input such as an already-uppercase prefix + lowercase suffix, toUpperCase returns the full uppercase string, and the lowercase mirror holds.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org