Skip to content

chore: review follow-ups for list.contains - #1

Merged
jonasdedden merged 3 commits into
jonasdedden:upstream/list-contains-backendsfrom
narwhals-dev:review/list-contains-backends
Oct 3, 2026
Merged

jonasdedden merged 3 commits into
jonasdedden:upstream/list-contains-backendsfrom
narwhals-dev:review/list-contains-backends

Conversation

@FBruzzesi

@FBruzzesi FBruzzesi commented Oct 2, 2026 •

Copy link
Copy Markdown

Follow-ups from reviewing narwhals-dev#4001, opened against your branch so you can keep, change or drop each part.

Kernel (_arrow/utils.py)

  • Consecutive small chunks are now combined into runs of at least 2^17 values before counting. Each block costs ~35 us of pyarrow calls, so inputs with many small chunks spent most of their time on that overhead. Large chunks are still sliced without copying, as before.
  • Split into _list_blocks (chunking) and _list_block_contains (the count). Empty chunks are skipped, and or 0 is gone, since pc.max of a non-empty block is never null.

Your benchmark script, with two added variants and two many-chunk cases. All variants gave identical results. The script as posted had a few truncated lines, which I filled in.

Case PR head 608d8a6 this PR
3 elements, rare (1e-5) 123 ms, 10 MB 123 ms, 10 MB
3 elements, common (30%) 130 ms, 10 MB 130 ms, 10 MB
1,000 elements, rare 108 ms, 9 MB 108 ms, 9 MB
1,000 elements, common 117 ms, 9 MB 115 ms, 9 MB
3 elements, common, chunks of 1,000 lists 627 ms, 8 MB 171 ms, 8 MB
1,000 elements, common, chunks of 10 lists 262 ms, 2 MB 138 ms, 3 MB

Dask

  • With List casts supported, nw.lit(None, nw.List(...)) stopped raising and returned a String column instead, because Dask scalars drop the dtype (dask scalars don't preserve dtype dask/dask#11637, the same cause as the xfailed test_date_lit). It raises NotImplementedError again.
  • The "Only pyarrow-backed lists" check is reachable (object-dtype lists with convert-string off), so it is now tested instead of marked # pragma: no cover.

Tests

  • test_contains_chunked replaces the pyarrow-only chunk test. It runs on every eager backend, with tiny block sizes so that both slicing and combining run.
  • test_contains_256_matches covers the uint8 to uint16 boundary, where 256 matches would wrap to 0.
  • Polars 1.44 gets list.contains wrong on chunks sliced from the same array. fix: Fix list.contains on multi-chunk sliced lists pola-rs/polars#29635 fixes it but is unreleased, so the test xfails Polars>=1.44 for now.
  • Simplified the Dask test constructor and the nested-dtype skips in assert_frame_equal_test.py.

AI assistance: reviewed and drafted with Claude Code. I have read the whole diff.

@jonasdedden
jonasdedden merged commit b93bfe9 into jonasdedden:upstream/list-contains-backends Oct 3, 2026
@FBruzzesi
FBruzzesi deleted the review/list-contains-backends branch October 3, 2026 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants