fix(pyarrow): handle non-positive width in str.zfill#3770
Conversation
|
thanks @mkzung !
i think i'd prefer that, could we do that instead please? thanks 🙏 |
ce7c603 to
0363a10
Compare
|
Done. The Arrow backend now raises InvalidOperationError on a negative width, matching Polars, instead of returning unchanged, and width 0 stays a no-op. I also moved the regression tests to pyarrow only, since Polars' own zfill(0) raises on the older CI versions. |
| # Match that here rather than letting pyarrow's utf8_lpad(width - 1) | ||
| # crash later with a negative pad width. | ||
| msg = f"zfill does not support a negative width, got {width}." | ||
| raise InvalidOperationError(msg) |
There was a problem hiding this comment.
@MarcoGorelli should we push this check up to narwhals/expr_str.py. If we do that then
- The error can be raised as early as possible for all backends
- All implementations will have a uniform error message
If not, then I'm happy with the implementation here.
|
Worth adding that the inconsistency is wider than zfill. With a negative width today, pandas returns the string unchanged, Polars raises So moving the check up to mkzung/narwhals@main...mkzung:narwhals:fix/zfill-validate-in-public-layer Happy to fold it into this PR, or to leave it until you and @MarcoGorelli have settled where the check belongs. |
Description
On the PyArrow backend,
Series.str.zfill(width)andExpr.str.zfill(width)crash withpyarrow.lib.ArrowInvalid: Negative buffer resizewheneverwidth <= 0.The result is built with
pc.case_when(...), and one of the branches ispc.utf8_lpad(remaining_chars, width - 1, padding="0").pc.case_whenevaluates every branch eagerly, so forwidth <= 0thewidth - 1pad length is negative and PyArrow raises before any row is selected.This handles the two non-positive cases the way the other backends do. A width of 0 is a no-op, since every string is already at least 0 long, so it returns the input unchanged. A negative width raises
InvalidOperationError, matching Polars, whose zfill argument is unsigned. PyArrow was the only backend that crashed on either one.Reproduction (before this change):
The regression tests are scoped to PyArrow, since that is the backend that crashed.
What type of PR is this? (check all applicable)
Related issues
AI assistance
I used an AI coding assistant to help locate the root cause and draft the regression test. I have read, run, and verified every line, and I take full responsibility for the change.
Checklist
str.zfill)