Repository navigation
Gather Stata strings in the owned discrete gather - #465
Merged
Merged
Conversation
|
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
.gather_dta_columns()gathered Stata numerics natively and the package's owned logical vectors and factors with the owned discrete gather, but sent every string column to vctrs..split_dta_column(), which splits a column into its groups in one native call (ADR 0051), declined for strings, so a grouped verb that reads a string column sliced each group's rows through vctrs and the package'svec_proxy()andvec_restore()methods for a Stata string.This PR teaches the owned discrete gather three kinds of string column: a compact Stata string from a reader that no read has materialized, an owned Stata string, and an owned character vector without a class, as
gen()makes one. For a Stata string it does whatvec_restore.dta_string()and.new_dta_string()do, step by step..split_dta_column()now splits a compact Stata string as it splits the other owned columns. ADR 0128 records the details.Behavior
Values, attributes, ALTREP forms, the dictionaries' caches and the owned vectors' sharing marks are unchanged. The gather goes to the R code for names, attributes outside
.dta_variable_attribute_names, other classes, S4 objects, a materialized compact string, locations vctrs would convert or reject, and a setter that declines.The gather checks that vctrs' dispatch would reach the package's methods: no binding of either name in the global environment, and in vctrs' table the methods as defined when dtatools was built, already forced. Until vctrs first dispatches on a Stata string in a session, the methods are unforced promises, so the first grouped read of a string column goes through vctrs. R checks that neither table method is being debugged.
A traced or replaced
.dta_string_data(),.metadata_view(),.declared_string_storage(),.new_dta_string(),.restore_dta_variable_metadata()or.set_dta_string_attribute(), or vctrs'vec_slice(),vec_proxy()orvec_restore(), no longer sees a gathered string column. A one-shot debugger thatdebugonce()sets on either method, whichisdebugged()does not report, no longer stops in a gather the native call takes.Timing
Best of three runs in fresh R sessions, in seconds, with the builds interleaved, over the 42,739 women in the first 100,000 rows of the India 2021 DHS births file. Before is main.
mutate(b, k = seq_along(caseid), .by = caseid)mutate(b, k = paste(caseid, bidx), .by = caseid)group_split(group_by(b, caseid))nest_by(b, caseid)summarise(b, k = first(caseid), .by = v008)group_split()andnest_by()still spend most of their time building each group's tibble.Tests
test-split-column.Rcompares the split and each group's gather with vctrs' slice of a fresh copy for a compact string whose chunks are ordinary and compact, a generated string, a Stata string and owned character vectors with and without a label. It compares the chunks' values, attributes, ALTREP forms, dictionary caches and sharing marks, and the source's.trace()replaced the namespace's copy. Every change to shared state is undone on exit.test-owned-atoms.Rpasses the new records to the gather.isdebugged()misses a one-shotdebugonce(), which ADR 0128 now records, that the debug check read the namespace's methods rather than the ones in vctrs' table, that the new test left shared state behind on failure, and that a grown test's manifest threshold needed raising. This PR fixes each, and the second review is clean.