Repository navigation
Capture nested chunks of plain tibbles in one native call - #470
Merged
Merged
Conversation
# Conflicts: # r-package/dtatools/NEWS.md # r-package/dtatools/R/dibble-rows.R # r-package/dtatools/src/dtatools-internal.h # r-package/dtatools/src/init.c # 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
|
# Conflicts: # r-package/dtatools/NEWS.md
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
nest_by()andgroup_nest()gather each group's rows into a plain tibble, nest the tibbles in a list_of and capture that list when they publish the result. On main,nest_by(b, caseid)over the 42,739 women in the first 100,000 rows of the India 2021 DHS births file takes about 3.6 seconds. Most of it went to three R steps that ran once for each group: the verdict of which columns to gather natively, a second selection of the nested columns, and the capture's per-tibble work in a hash table,attributes(),.row_names_info()and.metadata_copy()for each column.This PR moves the verdict into
C_dtatools_gather_native(), selects the nested columns once, and addsC_dtatools_capture_frame_list(), which captures a list of plain tibbles in one call as the R code does. ADR 0126 records the details.Behavior
Results are unchanged. The native calls return
NULL, and the R code runs, wherever the R code could run a provider's code or coerce a value: ALTREP lists, names, declarations, row names and columns other than the package's own, classed lists other than a list_of, other attributes, attributed or S4 names, classes and row names. The capture copies attributes in the order the R code sets them..dibble_nest()selects the nested columns once instead of once for each group. Before each group's chunk,C_dtatools_nested_unchanged()checks what the selection reads:keep, the grouping names, the list of columns, its names and the selected columns' positions. Once that check fails, or a gather reaches R code, each later group selects the columns again, as the R code does. Every group selects them when R code could run as the chunks are built: a foreign ALTREP column, list of names or group's rows, rows with a class, or akeep, list or grouping names that are objects, ALTREP or carry attributes.isTRUE()andsetdiff()see at most two calls, for the selection and the prototype, until a group selects again.vctrs::new_list_of()gives R's ALTREP wrapper for a list of 64 or more chunks, which the capture cannot see through with R's C API, so.dibble_nest()sets the list_of's attributes on the new chunks' list in place. Chunks with a missing name still go throughvctrs::new_list_of().A traced or replaced
.declared_dta_storage(),.dta_merge_has_compact_storage(),.data_columns(),.row_names_info(),.metadata_copy(),utils::hashtab()orrlang::obj_address()no longer sees these calls. The R capture moves to.capture_dibble_nested_r(), so the test that edits a string literal of each profiled function now skips.capture_dibble_nested(), whose body holds none.Timing
Best of three runs in fresh R sessions, in seconds, with the builds interleaved. Before is main at cd976e8.
nest_by(b, caseid)group_nest(b, caseid)group_split(group_by(b, caseid))tidyr::nest(b, data = -caseid)takes about 4.6 seconds with either build. Over five sessions each, interleaved, its fastest run took 4.605 seconds on main and 4.611 with this PR.group_split()still spends most of its time building each group's dibble.Tests
test-group-capture.Rcompares the native capture with the R capture for lists of tibbles, a tibble that appears twice and as the prototype, reversed attribute orders and a named list_of. It checks that the capture declines other lists and tibbles, ALTREP columns, names and row names, names with attributes, and S4 names, classes and row names.test-split-column.Rcompares the native verdict with the R verdict for every column of three files, materialized compact columns, broken declarations, data frames and ALTREP lists, declarations and names, and checks that ALTREP names' callbacks do not run in the native call.test-group-capture.Rchecks, forgroup_nest()andnest_by(), that the second group's chunk takes a column renamed by reference after the first group's: by a vctrs proxy method, by the provider of a foreign ALTREP column, rows orkeep, by alength()method of classed rows orkeep, and by a tracedtibble::new_tibble(). It also checks that a class a tracednew_tibble()giveskeepchanges the second group's columns.keep, a tracedtibble::new_tibble(), and a class added tokeepby reference. The eighth also found a stale manifest. This PR fixes each, and the thirteenth review is clean.