Repository navigation
Closed
Labels
enhancementNew feature or requestNew feature or requestfunctionsChanges to functions implementationChanges to functions implementationhelp wantedExtra attention is neededExtra attention is neededtech debtCode complexity, duplication or structure that makes the codebase harder to changeCode complexity, duplication or structure that makes the codebase harder to change
Description
Is your feature request related to a problem or challenge?
lpad and rpad contain four copies of the same padding loop:
lpad_impl(unicode/lpad.rs#L366-L513) has one loop for an explicit fill array (about L376-L445) and a near-identical one for the default space fill (about L447-L505). Each repeats thei32::MAXcheck, negative-length clamp, zero-length shortcut, ASCII fast path and char-boundary fallback (nesting depth 7, cognitive complexity 111).rpad_impl(unicode/rpad.rs#L365-L516) is the mirror image. The only differences are whether the padding is written before or after the string, and the function name in the error text.
The copies match one for one today, but any fix to length validation or Unicode handling has to be made in four places.
Describe the solution you'd like
- Treat the default-space case as the fill case with
fill = " "(monomorphized, so the hot loop is unchanged), removing the second loop in each function. - Extract a shared
parse_target_len(target_len, fn_name)for thei32::MAX/ negative clamp. - Move a single pad kernel, generic over the pad side (e.g. a
PadSide::{Left, Right}const generic), into a shared module underdatafusion/functions/src/unicode, leavinglpad.rs/rpad.rsas thin wrappers.
These functions were recently optimized (#20277, #20655, #21060), so the change must keep the ASCII fast path and be checked against the existing pad benchmarks.
Describe alternatives you've considered
Only merge the with-fill and no-fill loops within each function, without sharing code between lpad and rpad.
Additional context
Line references are pinned to commit 0d79252.
Metadata
Metadata
Assignees
Labels
enhancementNew feature or requestNew feature or requestfunctionsChanges to functions implementationChanges to functions implementationhelp wantedExtra attention is neededExtra attention is neededtech debtCode complexity, duplication or structure that makes the codebase harder to changeCode complexity, duplication or structure that makes the codebase harder to change
Activity
shazilkhan commentedon Oct 15, 2026
take
shazilkhan commentedon Oct 15, 2026
I'll pick this up. Diffed lpad_impl against rpad_impl and yeah, the only real difference is rpad does write_str(string) before the fill and then append_value(""), while lpad writes the fill and then append_value(string). So a const generic for the side should cover it without touching the hot loop.
I'll fold the no-fill branch into the fill path with " " and pull the i32::MAX / negative length check into a small helper, keeping the error message the same.
Getting a baseline from the pad benches on main now, will put before/after numbers in the PR.
refactor: share a single pad kernel between lpad and rpad (apache#26367)