Repository navigation
Plan coalesce() of a Stata numeric column in grouped mutate() - #468
Merged
Merged
Conversation
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
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
coalesce(b11, 0)in a groupedmutate()ran dplyr 1.2.1'scoalesce()for each group, which casts its arguments to their common type, finds the missing values of each but the last withvec_detect_missing()and builds the result withvec_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.athrough.zordinary values in the package's equality proxy, sovec_detect_missing()finds no missing value in a Stata numeric, and each group's call gives whatif_else(x == x, x, y)gives. This PR adds a plan forcoalesce(x, y)anddplyr::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'sif_else()plan withx == xas 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, whichdta_coalesce()fills. y joins x's type and can widen its storage, as anif_else()literal does. A y the cast rejects for that storage, a column of double storage and another function namedcoalescekeep the loop.The native call admits
coalesce()as ADR 0082 admitsna_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, thatvec_detect_missing()takes the package's equality proxy, and that novec_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, thoughcoalesce()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.
mutate(b, k = coalesce(b11, 0), .by = caseid)mutate(b, k = coalesce(b11, 0L), .by = caseid)mutate(b, k = coalesce(b3, -1), .by = caseid)Tests
test-summary-plan.Rcomparescoalesce()with the group loop for each storage theif_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.vec_case_when()or a primitive base'smatch.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.vec_proxy_equal.listmethod 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'slapply()andmatch.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.