narwhals icon indicating copy to clipboard operation
narwhals copied to clipboard

fix: `ArrowSeries.fill_null(strategy=..., limit=...)` and `None` at the "edge"

Open FBruzzesi opened this issue 3 weeks ago • 2 comments

Description

Hotfix for ArrowSeries.fill_null with strategy and limit specified and None at start/end of the array

What type of PR is this? (check all applicable)

  • [ ] 💾 Refactor
  • [ ] ✨ Feature
  • [x] 🐛 Bug Fix
  • [ ] 🔧 Optimization
  • [ ] 📝 Documentation
  • [ ] ✅ Test
  • [ ] 🐳 Other

Related issues

  • Closes #3327

Checklist

  • [x] Code follows style guide (ruff)
  • [x] Tests added
  • [ ] Documented the changes

FBruzzesi avatar Nov 29 '25 14:11 FBruzzesi

Thanks for the ping @FBruzzesi

Did you see this part of the issue?

[!NOTE] I stumbled into this while trying to avoid the numpy dependency and expensive reverse copies The version I have in (#3325) is already fixed + avoids numpy when pyarrow.arange is available (https://github.com/apache/arrow/pull/46778)

I'm asking because the fix I had added 2 lines vs:

Show this PR (#3335)

https://github.com/narwhals-dev/narwhals/blob/b1d4d0c1dc4208585f4f70ac20f9cead23ec04c4/narwhals/_arrow/series.py#L681-L713

It was just the not_oob and overwriting index_not_null bits here 🙂:

Show (#3325)

https://github.com/narwhals-dev/narwhals/blob/7be2d290eec77f4143752c0d6f99c4741b72d411/narwhals/_plan/arrow/functions.py#L630-L640

If any of the functions seem odd, they're just very thin wrappers/aliases. I have a really tough time remembering the pyarrow names 😅

Show wrappers/aliases

https://github.com/narwhals-dev/narwhals/blob/7be2d290eec77f4143752c0d6f99c4741b72d411/narwhals/_plan/arrow/functions.py#L549-L550

https://github.com/narwhals-dev/narwhals/blob/7be2d290eec77f4143752c0d6f99c4741b72d411/narwhals/_plan/arrow/functions.py#L422-L427

https://github.com/narwhals-dev/narwhals/blob/7be2d290eec77f4143752c0d6f99c4741b72d411/narwhals/_plan/arrow/functions.py#L132

https://github.com/narwhals-dev/narwhals/blob/7be2d290eec77f4143752c0d6f99c4741b72d411/narwhals/_plan/arrow/functions.py#L144

https://github.com/narwhals-dev/narwhals/blob/7be2d290eec77f4143752c0d6f99c4741b72d411/narwhals/_plan/arrow/functions.py#L136

https://github.com/narwhals-dev/narwhals/blob/7be2d290eec77f4143752c0d6f99c4741b72d411/narwhals/_plan/arrow/functions.py#L138

dangotbanned avatar Nov 29 '25 17:11 dangotbanned

Thanks @dangotbanned (https://github.com/narwhals-dev/narwhals/pull/3335#issuecomment-3591831415) - I certainly missed that. I assumed that if you had a solution already, this would have been pushed to main rather than in a branch 😂

I just added the solution from your branch in https://github.com/narwhals-dev/narwhals/pull/3335/commits/a74c00de8ffe8d0a800dbfacda32734232698337

FBruzzesi avatar Nov 30 '25 09:11 FBruzzesi

Thanks @dangotbanned (#3335 (comment)) - I certainly missed that.

No worries 😅

I assumed that if you had a solution already, this would have been pushed to main rather than in a branch 😂

Yeah doing it that way probably would make more sense

I've got a lot of other things in (#3325) that are ready to be upstreamed, but I'm just reporting bugs as I find them for now Trying to stay focused and see this one through 🤞

dangotbanned avatar Dec 01 '25 11:12 dangotbanned