Skip to content

Share a single pad kernel between lpad and rpad #26303

Closed

Description

@andygrove

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.

Additional context

Line references are pinned to commit 0d79252.

Activity

  1. added
    enhancementNew feature or request
    help wantedExtra attention is needed
    functionsChanges to functions implementation
    tech debtCode complexity, duplication or structure that makes the codebase harder to change
    on Oct 14, 2026
  2. shazilkhan commented on Oct 15, 2026

    @shazilkhan
    Contributor

    take

  3. shazilkhan commented on Oct 15, 2026

    @shazilkhan
    Contributor

    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.

  4. added a commit that references this issue on Oct 21, 2026
    29580fa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

enhancementNew feature or requestfunctionsChanges to functions implementationhelp wantedExtra attention is neededtech debtCode complexity, duplication or structure that makes the codebase harder to change

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions