Skip to content

Plan coalesce() of a Stata numeric column in grouped mutate() - #468

Merged
jbearak merged 6 commits into
mainfrom
claude/coalesce-plan
Oct 27, 2026
Merged

jbearak merged 6 commits into
mainfrom
claude/coalesce-plan

Conversation

@jbearak

@jbearak jbearak commented Oct 27, 2026

Copy link
Copy Markdown
Owner

Summary

coalesce(b11, 0) in a grouped mutate() ran dplyr 1.2.1's coalesce() for each group, which casts its arguments to their common type, finds the missing values of each but the last with vec_detect_missing() and builds the result with vec_case_when(). Over the 42,739 women in the first 100,000 rows of the India 2021 DHS births file, mutate(b, k = coalesce(b11, 0), .by = caseid) took about 5.4 seconds, where a tibble takes about 0.51.

ADR 0015 makes . and .a through .z ordinary values in the package's equality proxy, so vec_detect_missing() finds no missing value in a Stata numeric, and each group's call gives what if_else(x == x, x, y) gives. This PR adds a plan for coalesce(x, y) and dplyr::coalesce(x, y), where x names a Stata numeric column and y is a double or integer number, or a negated one. It runs ADR 0075's if_else() plan with x == x as the condition. ADR 0129 records the details.

Behavior

Values, attributes, ALTREP forms, warnings and errors are unchanged. coalesce() still keeps a Stata numeric's missing values, which dta_coalesce() fills. y joins x's type and can widen its storage, as an if_else() literal does. A y the cast rejects for that storage, a column of double storage and another function named coalesce keep the loop.

The native call admits coalesce() as ADR 0082 admits na_if(): the functions it reaches in dplyr, vctrs, rlang and base must match records made when the package was built, their primitives must be base's, and the routines they pass to .Call() and .External2() must be those the package captured at load. R checks that none of those closures is being debugged, that vctrs dispatches a Stata numeric and y's type to the package's methods, that vec_detect_missing() takes the package's equality proxy, and that no vec_proxy_equal() method for a list sits in the global environment or vctrs' table, since dplyr checks the argument list for missing values. The condition's == is admitted as ADR 0067 admits it, so a == R finds before base's also keeps the loop, though coalesce() calls none.

As in ADR 0075 and ADR 0082, the plan does not prove what it skips, so a traced or replaced mask$evaluate(), rlang::eval_tidy() or package helper of the loop no longer sees a planned expression's later groups.

Timing

Best of three runs in fresh R sessions, in seconds, with the builds interleaved. Before is main at cd976e8.

Call Before After Tibble
mutate(b, k = coalesce(b11, 0), .by = caseid) 5.389 0.053 0.510
mutate(b, k = coalesce(b11, 0L), .by = caseid) 5.120 0.053 0.516
mutate(b, k = coalesce(b3, -1), .by = caseid) 4.639 0.041 0.510

Tests

  • test-summary-plan.R compares coalesce() with the group loop for each storage the if_else() plan takes, with double and integer numbers, value labels, missing and tagged missing values, numbers that widen the storage, a negated number, dplyr::coalesce(), a result that replaces its column, and groups at and past ADR 0072's size gate.
  • It checks the declines: another function of the name, a column named for a function the lookup reads, a y that is not a number, an omitted y, a column of double storage, a y the cast rejects, an old file's long values, an attribute the casts drop with a warning, global methods for the equality proxy and y's cast, and a list equality proxy that makes dplyr find x missing. Each global binding a test sets is put back afterwards.
  • It checks that tracing vec_case_when() or a primitive base's match.fun() calls, replacing a routine the functions pass to .Call(), or debugging one of the admitted functions keeps the loop, and that a one-shot debugger stops in the first group, in a child R process listed in the manifest.
  • The full suite passes: 170 files, 2,311 tests. The native manifest and repository guard tests pass.
  • Codex review ran four rounds. The first found that a vec_proxy_equal.list method changes dplyr's check of the argument list. The second found that the debug check missed vctrs' vec_ptype_finalise.default() and the other functions vctrs' native code calls, that the child process loaded the installed package, and that the tests removed global bindings that existed before them. The third found that the primitives base's lapply() and match.fun() call went unchecked, and that one test overwrote a session's own list equality proxy. This PR fixes each, and the fourth review is clean, as is a fifth after the merge of main.

Record vctrs' finaliser, restore dispatch and ALTREP slice and base's
sys.frame() for the coalesce() plan, so debugging one of them keeps the
loop. Load the package under test in the one-shot debugger child, and
restore global methods the tests replace.
Record is.function(), is.object() and `||` and look every recorded
primitive up from base's namespace too, so a traced primitive that an
interpreted base helper would find keeps the loop. Restore the session's
own vec_proxy_equal.list binding around the test that checks it.
# Conflicts:
#	r-package/dtatools/NEWS.md
#	r-package/dtatools/tools/native-test-manifest.json
@coderabbitai

coderabbitai Bot commented Oct 27, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@jbearak
jbearak merged commit b236e73 into main Oct 27, 2026
4 checks passed
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.

1 participant