[ddsjoberg]

Follow-up to pharmaverse#610 — targets that PR's branch, so merging here folds these commits into it.

Reviewing pharmaverse#610 I found the group-partitioning rewrite semantically equivalent to the code it replaces on essentially every input I tried (NA in by/strata, unobserved factor levels, sparse by × strata grids, zero-row and zero-column data, Date/logical/haven_labelled keys, data.table, key-name collisions, and the include_data/include_by_and_strata/rename_columns combinations). Three things did change behavior, none covered by the test suite.

1. The nested data lost its attributes (base data frame input)

data[cols_to_keep] drops the data frame's own attributes, and data_to_slice[idx, , drop = FALSE] drops every column's attributes — including label, which clinical ADaM data leans on heavily. Non-default row names were also discarded by rownames(sub) <- NULL.

input	attr(df, "my_attr")	attr(df$v, "label")
data.frame, main	keepme	V label
data.frame, pharmaverse#610	NULL	NULL
tibble, either	keepme	V label

Tibble input was fine, because [.tbl_df delegates to vctrs::vec_slice(). So ard_summary() output was unaffected — but nest_for_ard() is exported, and ard_strata() hands the nested frames straight to a user-supplied .f, where stripped labels are visible.

Fixed by dropping the by/strata columns in place and slicing with vctrs::vec_slice(), which preserves both and normalizes row names the way dplyr::filter() did. It's also a little shorter than what it replaces.

2. Hard error on classed by/strata columns

Error in `vctrs::vec_match()`: Can't combine `g` <my_class> and `g` <character>.

.unique_and_sorted() can return a bare vector where data holds a classed one — unique() drops the class of vectors without a method for it — and vec_match() then aborts for any class vctrs cannot coerce. The old %in% comparison never needed a common type, so it coerced happily. Now the underlying values are compared when the classes disagree.

The blast radius is narrower than it looks: well-behaved classes are unaffected, either because unique() keeps the class (factor, Date, difftime, haven_labelled) or because vctrs can coerce what is left (glue, AsIs). It bites classes with neither — noquote, utils::as.roman, and anything hand-rolled.

3. Dead coercion branch in .process_nested_list_as_df()

} else if (!is.list(x[[new_column]])) {
  x[[new_column]] <- as.list(x[[new_column]])
}

No call site reaches this — instrumenting the function across the full suite records zero calls with a pre-existing new_column, and all six internal call sites build fmt_fun/stat_label fresh. Where it would run it silently overwrote an atomic column that dplyr::rows_update() previously rejected outright. Removed.

Also here

Tests for all three, plus one pinning the grouped-data-frame behavior. On a grouped_df, main kept the grouping column in the nested frames despite include_by_and_strata = FALSE (dplyr's "Adding missing grouping variables"); pharmaverse#610 correctly drops it, so the test locks that in as intended rather than incidental.

All five new assertions fail on pharmaverse#610 as it stands and pass here. testthat::test_local(): 875 passing, 0 failures, 1 skip. styler reports no changes to the touched files.

One note, no change made

The repo's own benchmark job on pharmaverse#610 puts ard_summary at −11.1%, CI [−24.6%, +2.5%] — not distinguishable from noise, versus the 6.5× in the description (which measures the internal helpers in isolation). The same run's +9.6% / +31% / +21% on ard_tabulate high-card, sort_ard_hierarchical, and filter_ard_hierarchical are noise too: none of them call nest_for_ard() or .process_nested_list_as_df(). Worth a re-run before reading much into either direction. (The red X on that job is only the fork-token failure posting the comment, not the benchmark itself.)

🤖 Generated with Claude Code

[ddsjoberg]

hey @kpagacz , I didn't have time for a full review, but Claude found these differences from the last version to your PR. I closed this PR bc it was pushed before I read the code (and I don't have the time to read the code tonight unfortunately).

[ddsjoberg]

Hi @kpagacz ,

I had a chance to review. It's probably not a big issue if the attributes are lost for data frames. But if it's not too much overhead, let's add the code to keep it.

Can you take a look at the code, do we need any other changes?

Thanks!