Skip to content

fix(polars): exclude nulls from nunique and approx_nunique - #12038

Open
mkzung wants to merge 1 commit into
ibis-project:mainfrom
mkzung:fix/polars-nunique-null
Open

fix(polars): exclude nulls from nunique and approx_nunique#12038
mkzung wants to merge 1 commit into
ibis-project:mainfrom
mkzung:fix/polars-nunique-null

Conversation

@mkzung

@mkzung mkzung commented Jul 19, 2026

Copy link
Copy Markdown

Description of changes

nunique and approx_nunique count nulls on the polars backend but not on the SQL backends:

t = ibis.memtable({"x": ["a", "b", "a", None]}, schema={"x": "string"})
t.x.nunique()         # duckdb 2, polars 3
t.x.approx_nunique()  # duckdb 2, polars 3

An all-null column shows the same split: 0 on the SQL backends, 1 on polars.

The polars compiler maps CountDistinct to polars' n_unique and ApproxCountDistinct to approx_n_unique, and both treat null as a distinct value. The SQL backends compile to COUNT(DISTINCT ...), which ignores nulls. The shared backend tests already assume nulls are excluded, using .dropna().nunique() and .nunique() as the expected values, so polars is the odd one out. The existing tests do not catch it because the column they aggregate has no nulls.

This gives both ops their own translation that drops nulls before counting, following the CountDistinctStar handler next to them, and adds a test with a null and an all-null column. approx_nunique is not in that test because its result is approximate and already xfailed for exactness on some backends.

polars is the only backend with its own compiler, so nothing else changes here: the rest go through the SQL compiler and already ignore nulls.

I checked that duckdb and polars agree afterwards for null, all-null and no-null columns, for where= (including a null in the predicate), for group_by(...).agg(...), and across int, float, bool, string, date, timestamp, decimal, array and struct columns. Those are the two backends I can run locally, so if another one needs a mark here, happy to add it

@github-actions github-actions Bot added tests Issues or PRs related to tests polars The polars backend labels Jul 19, 2026
@mkzung
mkzung force-pushed the fix/polars-nunique-null branch from 8984441 to d844635 Compare August 7, 2026 11:20
The polars backend mapped CountDistinct and ApproxCountDistinct straight to
polars' n_unique and approx_n_unique, both of which count null as a distinct
value. The SQL backends emit COUNT(DISTINCT ...), which ignores nulls, so the
answer differed by backend:

    t = ibis.memtable({"x": ["a", "b", "a", None]}, schema={"x": "string"})
    t.x.nunique()         # duckdb 2, polars 3
    t.x.approx_nunique()  # duckdb 2, polars 3

An all-null column shows the same split: 0 on SQL backends, 1 on polars.

The shared backend tests already treat nulls as excluded, using
.dropna().nunique() and .nunique() as the expected values, so polars was the
odd one out. Give both ops their own translation that drops nulls first,
following the CountDistinctStar handler next to them.

Adds a test for the exact case. approx_nunique is left untested there because
its result is approximate and already xfailed for exactness on some backends.
@mkzung
mkzung force-pushed the fix/polars-nunique-null branch from d844635 to 4a0f5da Compare August 14, 2026 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

polars The polars backend tests Issues or PRs related to tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant