fix: decouple visual styling from tbl_hierarchical_rate_by_grade to prevent Cartesian join explosion by Melkiades · Pull Request #228 · insightsengineering/crane

Melkiades · 2026-05-06T09:52:33Z

Fixes #226

Description

tbl_hierarchical_rate_by_grade() was blanking the label column for grade rows (label = "") before returning. When tbl_with_pools() called tbl_merge(), the join on (variable, row_type, var_label, label) lost row uniqueness — every blank-label grade row matched every other, producing a Cartesian cross-join with massive row duplication and column suffixing.

Fix: Decouple data generation from visual presentation:

tbl_hierarchical_rate_by_grade() now returns a structurally pristine table where label retains unique grade text. Injects custom_info metadata for downstream use.
add_grade_column() (new exported function) applies all visual formatting (label_grade column, label blanking, headers, indentation) as a post-processing step.

New workflow

# Standalone
tbl_hierarchical_rate_by_grade(...) |> add_grade_column()

# With pooled columns (the fix)
tbl_with_pools(..., .tbl_fun = tbl_hierarchical_rate_by_grade) |> add_grade_column()

How to test

# This previously caused a Cartesian explosion — now completes instantly
tbl_with_pools(
  data = adae, pools = my_pools, by = "TRTA",
  denominator = adsl,
  .tbl_fun = tbl_hierarchical_rate_by_grade,
  variables = c(AEBODSYS, AEDECOD, AETOXGR),
  grade_groups = list("Grade 1-2" = c("1", "2"))
) |> add_grade_column()

Run devtools::document() then devtools::test(). Accept new snapshots with testthat::snapshot_accept().

Files changed

File	Change
`R/tbl_hierarchical_rate_by_grade.R`	Removed visual styling, added `custom_info` metadata injection
`R/add_grade_column.R`	New exported post-processing function
`tests/testthat/test-add_grade_column.R`	8 test cases
`tests/testthat/test-tbl_hierarchical_rate_by_grade.R`	Updated to pipe through `add_grade_column()`
`tests/testthat/test-tbl_with_pools.R`	Added end-to-end pipeline regression test

Melkiades · 2026-05-06T10:17:41Z

@jszczypinski sorry for adding you, I just asked for last committer ahah

…revent Cartesian join explosion tbl_hierarchical_rate_by_grade() was blanking the label column for grade rows before returning, causing tbl_merge() inside tbl_with_pools() to lose row uniqueness and produce a Cartesian cross-join. Split into two functions: - tbl_hierarchical_rate_by_grade(): returns structurally pristine table with unique label values, injects custom_info metadata - add_grade_column(): post-processing function that applies all visual formatting (label_grade column, label blanking, headers, indentation) Co-authored-by: Ona <no-reply@ona.com>

…revent Cartesian join explosion tbl_hierarchical_rate_by_grade() was blanking the label column for grade rows before returning, causing tbl_merge() inside tbl_with_pools() to lose row uniqueness and produce a Cartesian cross-join. Split into two functions: - tbl_hierarchical_rate_by_grade(): returns structurally pristine table with unique label values, injects custom_info metadata - add_grade_column(): post-processing function that applies all visual formatting (label_grade column, label blanking, headers, indentation)

- add_grade_column() now returns early if label_grade already exists - metadata lookup iterates over x$tbls instead of hardcoding tbls[[1]] - added idempotency test (test #8) - fixed broken comment block in tbl_with_pools test Co-authored-by: Ona <no-reply@ona.com>

- add_grade_column() now returns early if label_grade already exists - metadata lookup iterates over x$tbls instead of hardcoding tbls[[1]] - added idempotency test (test #8) - fixed broken comment block in tbl_with_pools test

Co-authored-by: Ona <no-reply@ona.com>

Melkiades · 2026-05-06T10:20:36Z

@jszczypinski I did revert them ahah crazy stuff

Melkiades

Self-review after fixes. All previously identified issues are resolved:

Idempotency guard added (line 75): early return if label_grade already exists, prevents silent corruption on double-call.
Robust metadata lookup (line 78): Find() iterates over x$tbls instead of hardcoding tbls[[1]].
Idempotency test added (test #8): verifies table_body is identical after second call and grade labels are not corrupted.
Comment block fixed in test-tbl_with_pools.R.
Docs updated to match implementation.

Remaining notes:

Output snapshots for tbl_hierarchical_rate_by_grade were cleared and need regeneration on first devtools::test() run (testthat::snapshot_accept()).
devtools::document() must be run to export add_grade_column in NAMESPACE.

…elect deprecation Three tidyselect contexts in tbl_mmrm() were passing string variables (arm, visit) as bare names instead of using all_of(). This triggered deprecation warnings from tidyselect 1.1.0+. Co-authored-by: Ona <no-reply@ona.com>

…elect deprecation Three tidyselect contexts in tbl_mmrm() were passing string variables (arm, visit) as bare names instead of using all_of(). This triggered deprecation warnings from tidyselect 1.1.0+.

…elect deprecation Three tidyselect contexts in tbl_mmrm() were passing string variables (arm, visit) as bare names instead of using all_of(). This triggered deprecation warnings from tidyselect 1.1.0+. Also added AGENTS.md to .gitignore. Co-authored-by: Ona <no-reply@ona.com>

…elect deprecation Three tidyselect contexts in tbl_mmrm() were passing string variables (arm, visit) as bare names instead of using all_of(). This triggered deprecation warnings from tidyselect 1.1.0+. Also added AGENTS.md to .gitignore.

github-actions · 2026-05-06T13:00:14Z

Unit Tests Summary

1 files 233 suites 3m 18s ⏱️
233 tests 233 ✅ 0 💤 0 ❌
682 runs 682 ✅ 0 💤 0 ❌

Results for commit 56339db.

♻️ This comment has been updated with latest results.

github-actions · 2026-05-06T13:00:18Z

Unit Test Performance Difference

Test Suite	$Status$	Time on `main`	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
add_grade_column	👶		$+0.00$	$+3$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	Time on `main`	$±Time$	Test Case
add_forest	💚	$5.99$	$-4.60$	add_forest_table_engine_flextable_works
add_grade_column	👶		$+0.00$	add_grade_column_blanks_stats_for_SOC_label_rows
add_grade_column	👶		$+0.00$	add_grade_column_errors_on_non_gtsummary_input
add_grade_column	👶		$+0.00$	add_grade_column_errors_when_custom_info_is_missing
add_grade_column	👶		$+0.00$	add_grade_column_extracts_custom_info_from_merged_tables
add_grade_column	👶		$+0.00$	add_grade_column_is_idempotent_when_called_twice
add_grade_column	👶		$+0.00$	add_grade_column_recodes_zero_statistics
add_grade_column	👶		$+0.00$	add_grade_column_sets_correct_header_labels
add_grade_column	👶		$+21.68$	add_grade_column_works_on_a_standalone_tbl_hierarchical_rate_by_grade
add_grade_column	👶		$+0.00$	add_grade_column_works_without_grade_groups
add_hierarchical_count_row	💚	$3.96$	$-3.17$	add_hierarchical_count_row_works
annotate_gg	💚	$9.49$	$-3.88$	annotate_lineplot_df_warns_if_plot_is_not_generated_by_gg_lineplot
annotate_gg_pkc	💔	$1.35$	$+1.33$	annotate_pkc_df_warns_when_given_a_standard_ggplot_object
tbl_hierarchical_rate_by_grade	👶		$+0.00$	tbl_hierarchical_rate_by_grade_appends_missing_grade_group_levels_to_character_grade
tbl_rmpt	💔	$16.57$	$+1.36$	tbl_rmpt_works_with_default_parameters_Example_1_
tbl_with_pools	👶		$+0.01$	tbl_with_pools_add_grade_column_pipeline_does_not_duplicate_rows_226_
tbl_with_pools	💔	$2.24$	$+8.97$	tbl_with_pools_validates_inputs_correctly

Results for commit fadcafb

♻️ This comment has been updated with latest results.

github-actions · 2026-05-06T13:00:37Z

Code Coverage Summary

Filename                                 Stmts    Miss  Cover    Missing
-------------------------------------  -------  ------  -------  ------------------------------------------------------------------------------------------------
R/add_blank_rows.R                          63       0  100.00%
R/add_difference_row.R                     101       0  100.00%
R/add_forest_utils.R                        97      10  89.69%   76-79, 94-100
R/add_forest.R                             139       0  100.00%
R/add_hierarchical_count_row.R              33       0  100.00%
R/adjust_stat_columns_wrap.R                29       1  96.55%   53
R/annotate_gg_km.R                         135       0  100.00%
R/annotate_gg_pkc.R                         92       0  100.00%
R/annotate_gg.R                             81       0  100.00%
R/ard_tabulate_abnormal_by_baseline.R       65       0  100.00%
R/crane-package.R                            2       2  0.00%    26-27
R/deprecated.R                              21      21  0.00%    15-51
R/df_add_poolings.R                         41       0  100.00%
R/get_cox_pairwise_df.R                     96       6  93.75%   226-231
R/gg_km_utils.R                             35      14  60.00%   18-35
R/gg_km.R                                  143      37  74.13%   54-57, 74, 101, 175-180, 183-186, 196-198, 203-204, 238-240, 247-250, 254, 265-269, 282, 284-286
R/gg_lineplot.R                             94       0  100.00%
R/gg_mmrm_lineplot.R                       102       1  99.02%   106
R/gg_pkc_lineplot.R                         98       0  100.00%
R/gg_utils.R                               221       0  100.00%
R/label_roche.R                             72       0  100.00%
R/modify_header_rm_md.R                     18       2  88.89%   35-36
R/modify_zero_recode.R                      20       1  95.00%   64
R/reverse_difference_ci.R                   33       0  100.00%
R/tbl_baseline_chg.R                       188       0  100.00%
R/tbl_coxph.R                               90       1  98.89%   219
R/tbl_hierarchical_incidence_rate.R        209       0  100.00%
R/tbl_hierarchical_rate_and_count.R        339      13  96.17%   343, 425, 446-456
R/tbl_hierarchical_rate_by_grade.R         317       3  99.05%   169-171
R/tbl_listing.R                             35       0  100.00%
R/tbl_mmrm.R                               254       1  99.61%   393
R/tbl_null_report.R                          9       0  100.00%
R/tbl_rmpt.R                               157      12  92.36%   297-302, 314-319
R/tbl_roche_subgroups.R                    155       0  100.00%
R/tbl_roche_summary.R                       64       0  100.00%
R/tbl_shift.R                              116       0  100.00%
R/tbl_survfit_quantiles.R                  132       1  99.24%   295
R/tbl_survfit_times.R                       92       0  100.00%
R/tbl_with_pools.R                          64       0  100.00%
R/theme_gtsummary_roche.R                   84       1  98.81%   61
R/utils.R                                   36       0  100.00%
TOTAL                                     4172     127  96.96%

Diff against main

Filename                              Stmts    Miss  Cover
----------------------------------  -------  ------  -------
R/tbl_hierarchical_rate_by_grade.R      +43       0  +0.15%
TOTAL                                   +43       0  +0.03%

Results for commit: 56339db

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@Rdname

Tightly coupled function now shares the same file and help page via @Rdname. Removed standalone R/add_grade_column.R and its man page. Co-authored-by: Ona <no-reply@ona.com>

@Rdname

Tightly coupled function now shares the same file and help page via @Rdname. Removed standalone R/add_grade_column.R and its man page.

Co-authored-by: Ona <no-reply@ona.com>

…engineering/crane into js/fix-cartesian-join-explosion

Bare `label` inside modify_table_body() lambda is not visible to R CMD check. Use .data$label for all references.

- Fix test #2: assert no label_grade indent rows exist (was always-true) - Add comment to test #7: explain post_fmt_fun is a proxy check - Use slice_head(n = 30) for deterministic row selection - Add local_wide_snapshot() helper for consistent snapshot width Co-authored-by: Ona <no-reply@ona.com>

- Fix test #2: assert no label_grade indent rows exist (was always-true) - Add comment to test #7: explain post_fmt_fun is a proxy check - Use slice_head(n = 30) for deterministic row selection - Add local_wide_snapshot() helper for consistent snapshot width

Signed-off-by: Davide Garolini <dgarolini@gmail.com>

Melkiades · 2026-05-20T17:32:20Z

ready for review @shajoezhu @llrs-roche (Jan is out till monday I believe)

Signed-off-by: Davide Garolini <dgarolini@gmail.com>

llrs-roche

I have yet to check the PR locally or verified that the issue is indeed fixed but I have reviewed the code.

See comments below for some concerns

llrs-roche · 2026-05-21T13:47:42Z

+#' @export
+add_grade_column <- function(x) {


Why is added with tbl_hierarchical_rate_by_grade() if it must be called also after any merging?

Fair question! They share the help page because add_grade_column() only works on output from tbl_hierarchical_rate_by_grade() — it reads the custom_info metadata that function stores. So they are tightly coupled, just called at different points in the pipeline. Added a paragraph to the docs explaining this relationship.

If this graph describes the situation I think it should be a stand alone function:

flowchart LR %% Data Nodes and Functions A["1. Build Table<br>(tbl_hierarchical_rate_by_grade())"] B["2. Merge Tables (Optional)<br>(tbl_merge() / tbl_with_pools())"] C["3. Finalize Table<br>(add_grade_column())"] %% Data Flow A --> B B --> C A -->C

Loading

In any case please add a link/@seealso from tbl_with_pools() to the help page of add_grade_column().

llrs-roche · 2026-05-21T13:48:38Z

Do we need a special version of cards or anything?
When running this PR locally I see many failures:

devtools::test(filter = 'add_grade_column')
[ FAIL 7 | WARN 0 | SKIP 0 | PASS 2 ]

- remove helper-snapshot_width.R, inline withr::local_options() - condense test title, add comment explaining custom data subsets - add @details to add_grade_column() docs explaining coupling - add TODO for anonymous function extraction (#251)

llrs-roche

Just one more comment about the documentation. Keep in mind to address the spelling CI output.
Approving as all the major points are addressed now

Melkiades and others added 6 commits May 6, 2026 10:19

docs: update @details to match Find() lookup

8becdc8

Co-authored-by: Ona <no-reply@ona.com>

docs: update @details to match Find() lookup

9c30ff7

Melkiades force-pushed the js/fix-cartesian-join-explosion branch from 1afacd6 to 8becdc8 Compare May 6, 2026 10:19

Melkiades commented May 6, 2026

View reviewed changes

Melkiades and others added 6 commits May 6, 2026 12:52

docs and md

fc7795e

docs and md

02b4a8e

snapshot

b23ab6d

snapshot

ed63de6

fix: wrap bare string vars with all_of() in tbl_mmrm to silence tidys…

c2d8a7e

…elect deprecation Three tidyselect contexts in tbl_mmrm() were passing string variables (arm, visit) as bare names instead of using all_of(). This triggered deprecation warnings from tidyselect 1.1.0+.

Melkiades force-pushed the js/fix-cartesian-join-explosion branch from 51ffb39 to 74c8419 Compare May 6, 2026 11:50

jszczypinski and others added 2 commits May 6, 2026 12:37

Melkiades force-pushed the js/fix-cartesian-join-explosion branch from 803c081 to 76f4c0b Compare May 6, 2026 12:37

Melkiades marked this pull request as ready for review May 6, 2026 12:53

jszczypinski and others added 6 commits May 6, 2026 13:20

refactor: move add_grade_column() into tbl_hierarchical_rate_by_grade.R

1bd231c

Tightly coupled function now shares the same file and help page via @Rdname. Removed standalone R/add_grade_column.R and its man page. Co-authored-by: Ona <no-reply@ona.com>

refactor: move add_grade_column() into tbl_hierarchical_rate_by_grade.R

3b65517

Tightly coupled function now shares the same file and help page via @Rdname. Removed standalone R/add_grade_column.R and its man page.

style: place @Rdname directly above @export for add_grade_column

cc636c2

Co-authored-by: Ona <no-reply@ona.com>

style: place @Rdname directly above @export for add_grade_column

a8ca796

Merge branch 'js/fix-cartesian-join-explosion' of github.com:insights…

25d76ee

…engineering/crane into js/fix-cartesian-join-explosion

Merge branch 'js/fix-cartesian-join-explosion' of github.com:insights…

a594068

…engineering/crane into js/fix-cartesian-join-explosion

fix: use .data$label to resolve R CMD check NOTE

1e233a4

Bare `label` inside modify_table_body() lambda is not visible to R CMD check. Use .data$label for all references.

Melkiades commented May 8, 2026

View reviewed changes

Comment thread tests/testthat/_snaps/tbl_hierarchical_rate_by_grade.md Outdated

Melkiades and others added 13 commits May 8, 2026 13:29

fix snaps on local

0a33d5f

fix snaps on local

f5cc741

fix tests

359feaa

fix tests

649ca75

Merge branch 'main' into js/fix-cartesian-join-explosion

f98eccb

Merge branch 'main' into js/fix-cartesian-join-explosion

d05454a

update tbl_hierarchical_rate_by_grade snapshots after main merge

f42c7e2

update tbl_hierarchical_rate_by_grade snapshots after main merge

92af30f

add NEWS entry for add_grade_column() (#226)

da0f144

add NEWS entry for add_grade_column() (#226)

8c7f6de

Merge branch 'main' into js/fix-cartesian-join-explosion

ecd2958

Signed-off-by: Davide Garolini <dgarolini@gmail.com>

Melkiades commented May 20, 2026

View reviewed changes

Comment thread tests/testthat/helper-snapshot_width.R Outdated

Melkiades requested a review from jszczypinski May 20, 2026 15:19

snaps

a6da1a5

Melkiades force-pushed the js/fix-cartesian-join-explosion branch from a6da1a5 to 8c7f6de Compare May 20, 2026 17:34

Melkiades and others added 3 commits May 20, 2026 19:38

Merge branch 'main' into js/fix-cartesian-join-explosion

f45e9b3

Signed-off-by: Davide Garolini <dgarolini@gmail.com>

Merge branch 'main' into js/fix-cartesian-join-explosion

939e500

Signed-off-by: Davide Garolini <dgarolini@gmail.com>

merge

76ef8f4

llrs-roche reviewed May 21, 2026

View reviewed changes

Comment thread R/tbl_hierarchical_rate_by_grade.R

Comment thread tests/testthat/helper-snapshot_width.R Outdated

Comment thread tests/testthat/test-tbl_with_pools.R

Comment thread tests/testthat/test-tbl_with_pools.R Outdated

llrs-roche reviewed May 21, 2026

View reviewed changes

llrs-roche self-assigned this May 21, 2026

Melkiades mentioned this pull request May 22, 2026

Extract anonymous formatting functions from add_grade_column() #251

Open

address reviewer comments on PR #228

56339db

- remove helper-snapshot_width.R, inline withr::local_options() - condense test title, add comment explaining custom data subsets - add @details to add_grade_column() docs explaining coupling - add TODO for anonymous function extraction (#251)

llrs-roche approved these changes May 22, 2026

View reviewed changes

Uh oh!

Conversation

Melkiades commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

New workflow

How to test

Files changed

Uh oh!

Melkiades commented May 6, 2026

Uh oh!

Melkiades commented May 6, 2026

Uh oh!

Melkiades left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Tests Summary

Uh oh!

github-actions Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Performance Difference

Uh oh!

github-actions Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Summary

Diff against main

Uh oh!

Uh oh!

Uh oh!

Melkiades commented May 20, 2026

Uh oh!

llrs-roche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llrs-roche May 21, 2026

Choose a reason for hiding this comment

Uh oh!

Melkiades May 22, 2026

Choose a reason for hiding this comment

Uh oh!

llrs-roche May 22, 2026

Choose a reason for hiding this comment

Uh oh!

llrs-roche commented May 21, 2026

Uh oh!

llrs-roche left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Melkiades commented May 6, 2026 •

edited

Loading

github-actions Bot commented May 6, 2026 •

edited

Loading

github-actions Bot commented May 6, 2026 •

edited

Loading

github-actions Bot commented May 6, 2026 •

edited

Loading

llrs-roche left a comment •

edited

Loading