You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Share a single pad kernel between lpad and rpad #26303
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 the i32::MAX check, 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 the i32::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 under datafusion/functions/src/unicode, leaving lpad.rs/rpad.rs as 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.
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.
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)