feat: Add n_distinct(..., na.rm = TRUE) support for multiple passed columns

[lschneiderbauer]

My previous contribution only supported n_distinct(..., na.rm = TRUE) with for a single column, because duckdb's FILTER clause was not supported in a windowed context. This changed with duckdb v1.2.

This PR adds na.rm = TRUE support for multiple columns by using duckdb's struct and using FILTER to filter for non-null values before counting distinct elements.

I also adjusted the tests to simply compare against the dplyr result of n_distinct() instead of testing against explicit values to make sure the semantics is identical.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.41%. Comparing base (5c88c6f) to head (88a8d1c).
⚠️ Report is 2631 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1588       +/-   ##
===========================================
- Coverage   63.61%   53.41%   -10.21%     
===========================================
  Files         123      151       +28     
  Lines        5266    10325     +5059     
===========================================
+ Hits         3350     5515     +2165     
- Misses       1916     4810     +2894     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[krlmlr]

Thanks, great!

[krlmlr]

I wonder if we can keep this SQL unchanged in the very common case of just one column.

[lschneiderbauer]

I changed it, so that in the single-colum case we fall back to the unchanged SQL.

[krlmlr]

Is paste0() the correct idiom here? The other SQL generators use sql_expr() (for simple expressions) or build_sql() (for nonstandard constructs).

[lschneiderbauer]

I couldn't make build_sql() work with column lists (unless I would manually paste those together again). I noticed that build_sql() is superseeded by glue_sql2() (tidyverse/dbplyr#1249) which I now mostly adapted.

There is an exception though:

      str_null_check <-
        sql(paste0(paste0(list(...), " IS NOT NULL"), collapse = " AND "))

I couldn't figure out how to use glue_sql2() or the other helpers to construct this SQL statement without using paste0() explicitely, and I suspect it's not possible.

[krlmlr]

Thanks for the heads up. My concern is mainly about ... containing complex expressions, how is that handled?

[lschneiderbauer]

That's currently handled the same way n_distinct() is handled in (base) dbplyr:
translate(n_distinct(inject(something), y, na.rm = TRUE), window = FALSE) in itself will translate anything:

<SQL> COUNT(DISTINCT row(inject(something), y)) FILTER (inject(something) IS NOT NULL AND y IS NOT NULL)

(This is true despite the usage of glue_sql2().)

However, mutate() itself checks if the expressions given map to existing columns, and only then passes them on to the SQL translator:

df |> 
  mutate(
    z = n_distinct(inject(something), na.rm = TRUE)
  )

Error in `mutate()`:
ℹ In argument: `z = n_distinct(inject(something), na.rm = TRUE)`
Caused by error:
! Object `something` not found.

I cannot comment on whether it was a design-decision to not let glue_sql2() perform more checks or whether this is an oversight.

[krlmlr]

Thanks!