SEPURI-SAI-KRISHNA commented on PR #29004: URL: https://github.com/apache/flink/pull/29004#issuecomment-5383506363
Thanks for the review, all three are addressed in the new commit. `StringBuilder` is gone; the size of the `char[]` is computed up front rather than assumed to be `len`, and it now measures slightly faster than the current implementation on the common case and much faster on truncation. Numbers are in the inline reply. Comments are cut back to one line per helper. I have also added null cases, which the test set was missing: null base, null pad, a null INT length, plus empty pad and negative length, for both LPAD and RPAD. While re-checking I also confirmed this is inconsistent with the neighbouring functions rather than just with the documentation: `CHAR_LENGTH(E)` is 1 and `SUBSTR(E, 1, 1)` returns E intact, both via `BinaryStringData`, while `LPAD(E, 1, 'x')` returns half of the surrogate pair. LPAD and RPAD are the only two affected: SUBSTRING, LEFT, RIGHT and OVERLAY are generated against `BinaryStringData`, `SqlFunctionUtils#subString` and `#overlay` are not wired to any operator, and `initcap` rewrites only ASCII ranges so it never splits a pair. I have added that to the PR description. Both `sql_functions.yml` and `sql_functions_zh.yml` already describe the length as characters, so no documentation change is needed here. Verification on the new revision: reverting only the `SqlFunctionUtils` change fails 36 of the `StringFunctionsITCase` assertions and passes all 613 with it applied; `ScalarFunctionsTest` 99/99; the full `flink-table-runtime` suite 1813/1813; 1M randomized input combinations (2M lpad/rpad comparisons) match a code-point reference implementation exactly, and 500k BMP-only combinations match the current implementation exactly. -- 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]
