refactor: share a single pad kernel between lpad and rpad by shazilkhan · Pull Request #26367 · apache/datafusion

shazilkhan · 2026-10-15T11:29:51Z

Which issue does this PR close?

Closes Share a single pad kernel between lpad and rpad #26303.

Rationale for this change

lpad and rpad had four copies of the same per-row padding loop, plus two copies of the scalar fast path. The only real difference between them was whether the fill gets written before or after the string.

What changes are included in this PR?

New unicode/pad.rs with a single kernel, generic over a PadSide trait (LeftPad / RightPad, same idea as LeftRightSlicer in common.rs). lpad.rs and rpad.rs are now thin wrappers with just the docs and tests.
The no-fill case goes through the same loop with " " as the fill, so the second loop is gone.
parse_target_len replaces the six copies of the i32::MAX / negative length check. Error text is unchanged.

Two things came out of benchmarking:

Once the shared length check had its error branch inline, it stopped getting inlined into the hot loop and the no-fill path got noticeably slower. Moving the error into a #[cold] function fixed that.
The non-ASCII path now writes the fill in whole copies instead of one char at a time with a % per char. That's where the speedup below comes from.

I also added no fill cases to the pad benchmarks (first commit). None of the existing cases covered a 2-arg call with a length column, which is the path this changes the most.

What is the testing strategy for this PR?

Existing lpad/rpad unit tests and sqllogictests pass unchanged. I added tests in pad.rs that pass array arguments to both functions, since most of the existing ones go through the scalar fast path: no fill, fill, nulls, zero length, truncation, string view input, a partial multi-char unicode fill (rpad('josé', 6, 'ñéx')), and the too-large length error.

Benchmarks

cargo bench -p datafusion-functions --features unicode_expressions --bench unicode_expressions -- "pad size=", main vs this branch, run back to back on the same machine:

non-ASCII string with a fill (8 cases): 31–48% faster
everything else (40 cases): somewhere between −16% and +14% in both directions, which is about the noise on my machine. The scalar cases, whose logic didn't change, moved by the same amount.

Happy to re-run anything if someone wants different numbers.

Are there any user-facing changes?

No.

Separately, I found that a negative length coming from a column panics on main (pad_data_capacity does len as usize). That's not part of this PR, and I'll open its own issue.

…e chunks

neilconway

Thanks @shazilkhan! Overall this looks very reasonable. Can you move the new tests to SLT where possible?

It would be easier to review this PR if you had sent the refactor as a separate PR from optimizations, benchmark changes, or test enhancements. I don't think it's worth redoing the work now but please consider that for the future.

neilconway · 2026-10-20T15:11:13Z

+}
+
+#[cfg(test)]
+mod tests {


Most (or all?) of these tests can be done in SLT, which should be preferred when possible.

neilconway · 2026-10-20T15:16:08Z

run benchmark unicode_expressions
env:
BENCH_FILTER: pad
baseline:
ref: c113e29

adriangbot · 2026-10-20T15:18:43Z

🤖 Benchmark running (GKE) | trigger
Instance: c4a-highmem-16 (12 vCPU / 65 GiB) | Linux bench-c6294268555-3646-wf76r 6.12.94+ #1 SMP Fri Aug 21 08:00:16 UTC 2026 aarch64 GNU/Linux

CPU Details (lscpu)

Architecture:                            aarch64
CPU op-mode(s):                          64-bit
Byte Order:                              Little Endian
CPU(s):                                  16
On-line CPU(s) list:                     0-15
Vendor ID:                               ARM
Model name:                              Neoverse-V2
Model:                                   1
Thread(s) per core:                      1
Core(s) per cluster:                     16
Socket(s):                               -
Cluster(s):                              1
Stepping:                                r0p1
BogoMIPS:                                2000.00
Flags:                                   fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 sm3 sm4 asimddp sha512 sve asimdfhm dit uscat ilrcpc flagm sb paca pacg dcpodp sve2 sveaes svepmull svebitperm svesha3 svesm4 flagm2 frint svei8mm svebf16 i8mm bf16 dgh rng bti
L1d cache:                               1 MiB (16 instances)
L1i cache:                               1 MiB (16 instances)
L2 cache:                                32 MiB (16 instances)
L3 cache:                                80 MiB (1 instance)
NUMA node(s):                            1
NUMA node0 CPU(s):                       0-15
Vulnerability Gather data sampling:      Not affected
Vulnerability Indirect target selection: Not affected
Vulnerability Itlb multihit:             Not affected
Vulnerability L1tf:                      Not affected
Vulnerability Mds:                       Not affected
Vulnerability Meltdown:                  Not affected
Vulnerability Mmio stale data:           Not affected
Vulnerability Reg file data sampling:    Not affected
Vulnerability Retbleed:                  Not affected
Vulnerability Spec rstack overflow:      Not affected
Vulnerability Spec store bypass:         Mitigation; Speculative Store Bypass disabled via prctl
Vulnerability Spectre v1:                Mitigation; __user pointer sanitization
Vulnerability Spectre v2:                Mitigation; CSV2, BHB
Vulnerability Srbds:                     Not affected
Vulnerability Tsa:                       Not affected
Vulnerability Tsx async abort:           Not affected
Vulnerability Vmscape:                   Not affected

Comparing shared-pad-kernel (7c5c7d7) to c113e29 diff

Run configuration

run benchmark unicode_expressions
env:
  BENCH_FILTER: "pad"
baseline:
  ref: "c113e2989fee2355afcfca69a3173fdefe7fee79"

Results will be posted here when complete

File an issue against this benchmark runner

adriangbot · 2026-10-20T15:46:01Z

🤖 Benchmark completed (GKE) | trigger

Instance: c4a-highmem-16 (12 vCPU / 65 GiB)

Comparing shared-pad-kernel (7c5c7d7) to c113e29 diff

Run configuration

run benchmark unicode_expressions
env:
  BENCH_FILTER: "pad"
baseline:
  ref: "c113e2989fee2355afcfca69a3173fdefe7fee79"

CPU Details (lscpu)

Architecture:                            aarch64
CPU op-mode(s):                          64-bit
Byte Order:                              Little Endian
CPU(s):                                  16
On-line CPU(s) list:                     0-15
Vendor ID:                               ARM
Model name:                              Neoverse-V2
Model:                                   1
Thread(s) per core:                      1
Core(s) per cluster:                     16
Socket(s):                               -
Cluster(s):                              1
Stepping:                                r0p1
BogoMIPS:                                2000.00
Flags:                                   fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 sm3 sm4 asimddp sha512 sve asimdfhm dit uscat ilrcpc flagm sb paca pacg dcpodp sve2 sveaes svepmull svebitperm svesha3 svesm4 flagm2 frint svei8mm svebf16 i8mm bf16 dgh rng bti
L1d cache:                               1 MiB (16 instances)
L1i cache:                               1 MiB (16 instances)
L2 cache:                                32 MiB (16 instances)
L3 cache:                                80 MiB (1 instance)
NUMA node(s):                            1
NUMA node0 CPU(s):                       0-15
Vulnerability Gather data sampling:      Not affected
Vulnerability Indirect target selection: Not affected
Vulnerability Itlb multihit:             Not affected
Vulnerability L1tf:                      Not affected
Vulnerability Mds:                       Not affected
Vulnerability Meltdown:                  Not affected
Vulnerability Mmio stale data:           Not affected
Vulnerability Reg file data sampling:    Not affected
Vulnerability Retbleed:                  Not affected
Vulnerability Spec rstack overflow:      Not affected
Vulnerability Spec store bypass:         Mitigation; Speculative Store Bypass disabled via prctl
Vulnerability Spectre v1:                Mitigation; __user pointer sanitization
Vulnerability Spectre v2:                Mitigation; CSV2, BHB
Vulnerability Srbds:                     Not affected
Vulnerability Tsa:                       Not affected
Vulnerability Tsx async abort:           Not affected
Vulnerability Vmscape:                   Not affected

Details

group                                                                                   HEAD                                   shared-pad-kernel
-----                                                                                   ----                                   -----------------
lpad size=1024/lpad stringview [size=1024, str_len=20, target=50]                       1.06     14.7±0.01µs        ? ?/sec    1.00     13.9±0.01µs        ? ?/sec
lpad size=1024/lpad stringview [size=1024, str_len=5, target=20]                        1.07     15.3±0.02µs        ? ?/sec    1.00     14.3±0.01µs        ? ?/sec
lpad size=1024/lpad stringview scalar [size=1024, str_len=5, target=20, fill='x']       1.02     10.2±0.10µs        ? ?/sec    1.00     10.0±0.09µs        ? ?/sec
lpad size=1024/lpad stringview unicode [size=1024, target=20]                           1.74     36.8±0.39µs        ? ?/sec    1.00     21.1±0.02µs        ? ?/sec
lpad size=1024/lpad utf8 [size=1024, str_len=20, target=50]                             1.00     13.4±0.01µs        ? ?/sec    1.03     13.7±0.03µs        ? ?/sec
lpad size=1024/lpad utf8 [size=1024, str_len=5, target=20]                              1.01     15.6±0.01µs        ? ?/sec    1.00     15.4±0.03µs        ? ?/sec
lpad size=1024/lpad utf8 no fill [size=1024, str_len=5, target=20]                      1.00     11.3±0.01µs        ? ?/sec    1.03     11.6±0.01µs        ? ?/sec
lpad size=1024/lpad utf8 scalar [size=1024, str_len=5, target=20, fill='x']             1.00      7.6±0.14µs        ? ?/sec    1.00      7.6±0.05µs        ? ?/sec
lpad size=1024/lpad utf8 scalar truncate [size=1024, str_len=20, target=5, fill='é']    1.00      9.9±0.01µs        ? ?/sec    1.00      9.9±0.00µs        ? ?/sec
lpad size=1024/lpad utf8 scalar unicode [size=1024, str_len=5, target=20, fill='é']     1.02     10.9±0.02µs        ? ?/sec    1.00     10.7±0.00µs        ? ?/sec
lpad size=1024/lpad utf8 unicode [size=1024, target=20]                                 1.66     35.7±0.03µs        ? ?/sec    1.00     21.5±0.02µs        ? ?/sec
lpad size=1024/lpad utf8 unicode no fill [size=1024, target=20]                         1.00     16.2±0.01µs        ? ?/sec    1.05     17.1±0.00µs        ? ?/sec
lpad size=4096/lpad stringview [size=4096, str_len=20, target=50]                       1.00     57.0±0.09µs        ? ?/sec    1.07     60.9±0.10µs        ? ?/sec
lpad size=4096/lpad stringview [size=4096, str_len=5, target=20]                        1.14     67.6±0.24µs        ? ?/sec    1.00     59.4±0.39µs        ? ?/sec
lpad size=4096/lpad stringview scalar [size=4096, str_len=5, target=20, fill='x']       1.01     38.7±0.44µs        ? ?/sec    1.00     38.4±0.27µs        ? ?/sec
lpad size=4096/lpad stringview unicode [size=4096, target=20]                           1.67    155.9±1.48µs        ? ?/sec    1.00     93.1±0.55µs        ? ?/sec
lpad size=4096/lpad utf8 [size=4096, str_len=20, target=50]                             1.03     56.1±0.21µs        ? ?/sec    1.00     54.3±0.13µs        ? ?/sec
lpad size=4096/lpad utf8 [size=4096, str_len=5, target=20]                              1.00     62.8±0.57µs        ? ?/sec    1.02     64.3±0.12µs        ? ?/sec
lpad size=4096/lpad utf8 no fill [size=4096, str_len=5, target=20]                      1.00     47.3±0.24µs        ? ?/sec    1.04     49.3±0.21µs        ? ?/sec
lpad size=4096/lpad utf8 scalar [size=4096, str_len=5, target=20, fill='x']             1.00     28.6±0.46µs        ? ?/sec    1.01     28.7±0.17µs        ? ?/sec
lpad size=4096/lpad utf8 scalar truncate [size=4096, str_len=20, target=5, fill='é']    1.01     37.5±0.08µs        ? ?/sec    1.00     37.2±0.01µs        ? ?/sec
lpad size=4096/lpad utf8 scalar unicode [size=4096, str_len=5, target=20, fill='é']     1.02     40.9±0.10µs        ? ?/sec    1.00     40.2±0.02µs        ? ?/sec
lpad size=4096/lpad utf8 unicode [size=4096, target=20]                                 1.59    150.4±0.29µs        ? ?/sec    1.00     94.8±0.41µs        ? ?/sec
lpad size=4096/lpad utf8 unicode no fill [size=4096, target=20]                         1.02     78.9±0.05µs        ? ?/sec    1.00     77.3±0.23µs        ? ?/sec
rpad size=1024/rpad stringview [size=1024, str_len=20, target=50]                       1.05     15.0±0.01µs        ? ?/sec    1.00     14.4±0.00µs        ? ?/sec
rpad size=1024/rpad stringview [size=1024, str_len=5, target=20]                        1.02     15.1±0.01µs        ? ?/sec    1.00     14.8±0.00µs        ? ?/sec
rpad size=1024/rpad stringview scalar [size=1024, str_len=5, target=20, fill='x']       1.00     10.6±0.01µs        ? ?/sec    1.00     10.6±0.00µs        ? ?/sec
rpad size=1024/rpad stringview unicode [size=1024, target=20]                           1.75     38.0±0.34µs        ? ?/sec    1.00     21.7±0.01µs        ? ?/sec
rpad size=1024/rpad utf8 [size=1024, str_len=20, target=50]                             1.00     13.6±0.01µs        ? ?/sec    1.02     13.9±0.12µs        ? ?/sec
rpad size=1024/rpad utf8 [size=1024, str_len=5, target=20]                              1.00     15.6±0.02µs        ? ?/sec    1.04     16.1±0.14µs        ? ?/sec
rpad size=1024/rpad utf8 no fill [size=1024, str_len=5, target=20]                      1.00     11.8±0.00µs        ? ?/sec    1.05     12.4±0.01µs        ? ?/sec
rpad size=1024/rpad utf8 scalar [size=1024, str_len=5, target=20, fill='x']             1.00      8.3±0.08µs        ? ?/sec    1.01      8.4±0.01µs        ? ?/sec
rpad size=1024/rpad utf8 scalar truncate [size=1024, str_len=20, target=5, fill='é']    1.00      9.9±0.05µs        ? ?/sec    1.09     10.8±0.01µs        ? ?/sec
rpad size=1024/rpad utf8 scalar unicode [size=1024, str_len=5, target=20, fill='é']     1.00     11.6±0.07µs        ? ?/sec    1.00     11.6±0.00µs        ? ?/sec
rpad size=1024/rpad utf8 unicode [size=1024, target=20]                                 1.72     36.5±0.02µs        ? ?/sec    1.00     21.3±0.09µs        ? ?/sec
rpad size=1024/rpad utf8 unicode no fill [size=1024, target=20]                         1.00     16.5±0.01µs        ? ?/sec    1.02     16.9±0.01µs        ? ?/sec
rpad size=4096/rpad stringview [size=4096, str_len=20, target=50]                       1.11     64.0±0.33µs        ? ?/sec    1.00     57.7±0.09µs        ? ?/sec
rpad size=4096/rpad stringview [size=4096, str_len=5, target=20]                        1.01     64.4±0.37µs        ? ?/sec    1.00     63.6±0.49µs        ? ?/sec
rpad size=4096/rpad stringview scalar [size=4096, str_len=5, target=20, fill='x']       1.00     40.1±0.10µs        ? ?/sec    1.00     40.1±0.01µs        ? ?/sec
rpad size=4096/rpad stringview unicode [size=4096, target=20]                           1.68    159.4±1.17µs        ? ?/sec    1.00     95.1±0.50µs        ? ?/sec
rpad size=4096/rpad utf8 [size=4096, str_len=20, target=50]                             1.00     55.9±0.21µs        ? ?/sec    1.08     60.3±0.51µs        ? ?/sec
rpad size=4096/rpad utf8 [size=4096, str_len=5, target=20]                              1.00     65.3±0.24µs        ? ?/sec    1.05     68.6±0.71µs        ? ?/sec
rpad size=4096/rpad utf8 no fill [size=4096, str_len=5, target=20]                      1.11     59.2±0.22µs        ? ?/sec    1.00     53.3±0.62µs        ? ?/sec
rpad size=4096/rpad utf8 scalar [size=4096, str_len=5, target=20, fill='x']             1.00     30.5±0.20µs        ? ?/sec    1.02     31.2±0.01µs        ? ?/sec
rpad size=4096/rpad utf8 scalar truncate [size=4096, str_len=20, target=5, fill='é']    1.00     38.1±0.24µs        ? ?/sec    1.08     41.0±0.02µs        ? ?/sec
rpad size=4096/rpad utf8 scalar unicode [size=4096, str_len=5, target=20, fill='é']     1.00     43.4±0.29µs        ? ?/sec    1.01     43.7±0.01µs        ? ?/sec
rpad size=4096/rpad utf8 unicode [size=4096, target=20]                                 1.65    154.3±0.24µs        ? ?/sec    1.00     93.8±0.69µs        ? ?/sec
rpad size=4096/rpad utf8 unicode no fill [size=4096, target=20]                         1.00     71.8±0.38µs        ? ?/sec    1.20     85.9±0.11µs        ? ?/sec

Resource Usage

unicode_expressions — base (merge-base)

Metric	Value
Wall time	820.2s
Peak memory	235.4 MiB
Avg memory	162.5 MiB
CPU user	788.2s
CPU sys	0.7s
Peak spill	0 B

unicode_expressions — branch

Metric	Value
Wall time	805.2s
Peak memory	244.2 MiB
Avg memory	163.8 MiB
CPU user	787.0s
CPU sys	0.6s
Peak spill	0 B

File an issue against this benchmark runner

Per review feedback, the pad.rs unit tests are all expressible in SQL, so they now live in string_literal.slt next to the other lpad/rpad tests.

shazilkhan · 2026-10-20T18:11:35Z

Thanks for the review and for running the benchmarks @neilconway!

Tests: moved them all to SLT in string_literal.slt, next to the existing lpad/rpad tests, and removed the unit tests from pad.rs. Pushed in f41e6ea.
Splitting PRs: fair point. Next time I'll send the refactor on its own and keep perf and test changes separate.
rpad utf8 unicode no fill [size=4096] (1.20x): I couldn't find a cause in the code. That path writes the spaces the same way as before, and lpad shares it and is flat in your run, as is the 1024-row case. Locally both lpad and rpad move about ±10% on this case between runs, so I think it's mostly noise. Happy to look closer if you think it's real.

shazilkhan · 2026-10-20T21:20:39Z

Updated the branch with main, which also clears the semver warning (the timezone items were added to main after this branch was cut). Could a maintainer approve the CI workflows when you get a chance?

codecov-commenter · 2026-10-21T01:14:28Z

Codecov Report

❌ Patch coverage is 89.21933% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.94%. Comparing base (79650ec) to head (3aed5bf).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/functions/src/unicode/pad.rs	89.05%	16 Missing and 13 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #26367      +/-   ##
==========================================
- Coverage   82.94%   82.94%   -0.01%     
==========================================
  Files        1156     1157       +1     
  Lines      458581   458276     -305     
  Branches   458581   458276     -305     
==========================================
- Hits       380382   380125     -257     
+ Misses      55145    55103      -42     
+ Partials    23054    23048       -6

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

shazilkhan added 3 commits October 15, 2026 08:10

bench: add lpad/rpad benchmarks without a fill argument

c113e29

refactor: share a single pad kernel between lpad and rpad

313c25f

perf: keep the per-row pad length check small and write fills in whol…

7c5c7d7

…e chunks

github-actions Bot added the functions Changes to functions implementation label Oct 15, 2026

shazilkhan force-pushed the shared-pad-kernel branch from ea011db to 7c5c7d7 Compare October 15, 2026 11:32

neilconway approved these changes Oct 20, 2026

View reviewed changes

test: move lpad/rpad array-path tests to sqllogictest

f41e6ea

Per review feedback, the pad.rs unit tests are all expressible in SQL, so they now live in string_literal.slt next to the other lpad/rpad tests.

github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Oct 20, 2026

github-actions Bot added the auto detected api change Auto detected API change label Oct 20, 2026

Merge remote-tracking branch 'origin/main' into shared-pad-kernel

3aed5bf

github-actions Bot removed the auto detected api change Auto detected API change label Oct 21, 2026

Navigation Menu

Repository navigation

refactor: share a single pad kernel between lpad and rpad - #26367

refactor: share a single pad kernel between lpad and rpad#26367
shazilkhan wants to merge 5 commits into
apache:mainfrom
shazilkhan:shared-pad-kernel

shazilkhan commented Oct 15, 2026

Uh oh!

neilconway left a comment

Uh oh!

neilconway Oct 20, 2026

Uh oh!

neilconway commented Oct 20, 2026

Uh oh!

adriangbot commented Oct 20, 2026

Uh oh!

adriangbot commented Oct 20, 2026

Uh oh!

shazilkhan commented Oct 20, 2026

Uh oh!

shazilkhan commented Oct 20, 2026

Uh oh!

codecov-commenter commented Oct 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

shazilkhan commented Oct 15, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

What is the testing strategy for this PR?

Benchmarks

Are there any user-facing changes?

Uh oh!

neilconway left a comment

Choose a reason for hiding this comment

Uh oh!

neilconway Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

neilconway commented Oct 20, 2026

Uh oh!

adriangbot commented Oct 20, 2026

Uh oh!

adriangbot commented Oct 20, 2026

Uh oh!

shazilkhan commented Oct 20, 2026

Uh oh!

shazilkhan commented Oct 20, 2026

Uh oh!

codecov-commenter commented Oct 21, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants