Skip to content

Gather Stata strings in the owned discrete gather - #465

Merged
jbearak merged 4 commits into
mainfrom
claude/split-strings
Oct 27, 2026
Merged

jbearak merged 4 commits into
mainfrom
claude/split-strings

Conversation

@jbearak

@jbearak jbearak commented Oct 27, 2026

Copy link
Copy Markdown
Owner

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's vec_proxy() and vec_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 what vec_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() or vec_restore(), no longer sees a gathered string column. A one-shot debugger that debugonce() sets on either method, which isdebugged() 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.

Call Before After Tibble
mutate(b, k = seq_along(caseid), .by = caseid) 1.21 0.46 0.056
mutate(b, k = paste(caseid, bidx), .by = caseid) 1.37 0.65 0.13
group_split(group_by(b, caseid)) 5.59 5.42 0.51
nest_by(b, caseid) 3.60 3.41 0.61
summarise(b, k = first(caseid), .by = v008) 0.006 0.005 0.001

group_split() and nest_by() still spend most of their time building each group's tibble.

Tests

  • test-split-column.R compares 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.
  • It checks the declines: an unowned vector, names, an unknown attribute, an extra class, an S4 object, a materialized compact string, missing and out-of-range locations, a debugged method, one in the global environment, one replaced in vctrs' table, and a debugged table method after trace() replaced the namespace's copy. Every change to shared state is undone on exit.
  • test-owned-atoms.R passes the new records to the gather.
  • The full suite passes: 170 files, 2,301 tests. The native manifest and repository guard tests pass.
  • Codex review ran two rounds. The first found that isdebugged() misses a one-shot debugonce(), 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.

@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 c91ca74 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