shazilkhan opened a new pull request, #26367:
URL: https://github.com/apache/datafusion/pull/26367
## Which issue does this PR close?
- Closes #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.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
