fix/258-getcommonChunk code in C++

[Nishita-shah1]

Summary

Fixes #258 by adding a small C++ fast path for common-column detection inside getCommonChunk().

New file: src/get_common_chunk.cpp (~154 lines)

Exported function: common_value_for_group_subset_cpp(value_lists)

For one (column, group), given values split by chunk subset (e.g. each showSelected level), it checks whether those values are identical across chunks. This mirrors R's common_value_for_group_subset() (matrix / NA / scalar logic from PR #242).

Architecture: R still groups columns and groups via detect_common_value_dt(). C++ only accelerates the inner compare. No change to TSV format, public API, geom-.r, or animint.js.

What this PR adds

Item	Detail
src/get_common_chunk.cpp	C++ inner compare (~154 lines)
src/RcppExports.cpp, R/RcppExports.R	Rcpp registration
R/z_animintHelpers.R	common_value_for_group_subset() calls C++ when compiled; R fallback via options(animint2.use.cpp)
tests/testthat/test-compiler-getCommonChunk.R	11 unit tests (38 expectations)
vignettes/get-common-chunk-cpp.Rmd	File-wise explanation and test guide
DESCRIPTION, NAMESPACE, NEWS.md, .ci/atime/tests.R	Rcpp wiring, changelog, benchmark

C++ design (review-friendly)

Layer	Where
Grouping (per column, per group, per chunk)	R: detect_common_value_dt()
Inner compare (matrix + NA + scalar)	C++: common_value_for_group_subset_cpp()
Assembly + TSV write	R: getCommonChunk(), split_recursive(), saveChunks()

Internal C++ helpers: is_na_at, eq_at, scalar_at, wrap_common (correct R list shape for common column).

Documentation

Vignette: vignettes/get-common-chunk-cpp.Rmd

After install: vignette("get-common-chunk-cpp", package = "animint2")

---

Motivation

After PRs #242 and #255, getCommonChunk() is correct but the inner compare (matrix + NA handling) runs in R for every (column, group, chunk subset). This PR moves that hot inner loop to C++ while keeping grouping in R for a small, reviewable diff.

---

Design highlights

1. R groups, C++ compares - ~154 lines of C++ vs ~320 line full-scan version
2. SEXP element compare - correct NA handling across numeric / integer / logical / character
3. Rf_Scalar* - Windows / R 4.5+ compatible C API
4. wrap_common() - returns list(common = list(vector), is.common = ...) matching R shape
5. No file I/O in C++ - R still runs dcast, split_recursive, saveChunks

---

Quick test

Requires Rtools on Windows for first load_all().

library(devtools)
load_all()
library(testthat)
library(data.table)

built <- data.table(
  group = rep(1:2, each = 4),
  showSelected = rep(c(1, 1, 2, 2), 2),
  x = rep(c(10, 20), each = 4),
  y = rep(c(1, 2), each = 4),
  fill = c("a", "a", "b", "b", "c", "c", "d", "d")
)
setkeyv(built, c("group", "showSelected"))

# Detection table (C++ inner compare when compiled)
dt <- animint2:::detect_common_value_dt(built, c("x", "y", "fill"), "showSelected")
print(dt[, .(col.name, group, is.common)])
# x, y -> TRUE; fill -> FALSE

# Full common / varied split
result <- animint2:::getCommonChunk(built, "showSelected", list(group = "group"))
names(result$common)   # "x", "y", "group"
result$varied          # nested list with fill per chunk

# Unit tests (no browser / servr needed)
testthat::test_file("tests/testthat/test-compiler-getCommonChunk.R")
# Expected: FAIL 0 | PASS 38

# Verify C++ compiled
"common_value_for_group_subset_cpp" %in% ls(asNamespace("animint2"), all = TRUE)

[tdhock]

thanks, but this seems overly complex (300+ lines of C++)
is it possible to simplify? (I was expecting <100 lines of C++ code to review, maybe I was being too optimistic though?)

[Nishita-shah1]

Okay, I'll do it

…

On Mon, 29 Jun 2026, 20:45 Toby Dylan Hocking, ***@***.***> wrote: *tdhock* left a comment (animint/animint2#342) <#342 (comment)> thanks, but this seems overly complex (300+ lines of C++) is it possible to simplify? (I was expecting <100 lines of C++ code to review, maybe I was being too optimistic though?) — Reply to this email directly, view it on GitHub <#342?email_source=notifications&email_token=BCA4SQILXJKASH2I2JBP5FL5CKBX7A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOBTGQYTANBZHA22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4834104985>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BCA4SQLDGN6B4VWARQFUMZD5CKBX7AVCNFSNUABEKJSXA33TNF2G64TZHMZDANZQHE3TQNZ3JFZXG5LFHM2DONRUGA2DOMJSGGQXMAQ> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/BCA4SQOW353TAQA6MTQYNR35CKBX7A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOBTGQYTANBZHA22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android <https://github.com/notifications/mobile/android/BCA4SQMPCXWIOP74WOGKTPT5CKBX7A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOBTGQYTANBZHA22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today! You are receiving this because you authored the thread.Message ID: ***@***.***>

[codecov]

Codecov Report

❌ Patch coverage is 72.65193% with 99 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.81%. Comparing base (f265a03) to head (0d903de).

Files with missing lines	Patch %	Lines
src/get_common_chunk.cpp	72.92%	85 Missing ⚠️
R/z_animintHelpers.R	70.83%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
- Coverage   72.88%   72.81%   -0.07%     
==========================================
  Files         164      165       +1     
  Lines        8868     9211     +343     
==========================================
+ Hits         6463     6707     +244     
- Misses       2405     2504      +99     

Flag	Coverage Δ
javascript	80.70% <ø> (ø)
r	69.25% <72.65%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Faye-yufan]

Hi @Nishita-shah1 , could you confirm that with() block is not changing the animint2.use.cpp value globally? If it changed on global scope, later cpp_dt is still using R implementation.

[Nishita-shah1]

Hi @Faye-yufan , sure I'll confirm it. Sorry for late reply, I wasn't well since few days.

[Nishita-shah1]

Confirmed, that was a bug. with(options(...)) set animint2.use.cpp = FALSE globally and never restored it, so both r_dt and cpp_dt used R.

Fixed by using options() + on.exit() to restore, then setting animint2.use.cpp = TRUE before cpp_dt. Thanks for catching this!

[Faye-yufan]

Is Rf_ScalarReal() returning an R object? If it returns R object, is it necessary to add these objects to R's protection stack to keep those away from R's garbage collector?
Since the loop in common_value_for_group_subset_cpp keeps allocating more scalars using scalar_at(), which can trigger R's GC, it would collect the unprotected scalars. Later scalars_to_vector can read freed memory.

[Nishita-shah1]

Yes. Rf_ScalarReal() (and the other Rf_Scalar*) allocate R objects, so unprotected pointers stored across later allocations can be collected by GC.

That was a real risk in the old code (scalar_at() into std::vector<SEXP>, then scalars_to_vector).

Fixed by storing (chunk, row) index refs instead of temporary SEXPs, and only allocating a scalar when immediately wrapping it into the return List (so Rcpp protects it right away).

[tdhock]

hi, what is the status here?

[tdhock]

we should get atime ci working, and add a test, before merging this PR (so we can see the speed improvement).

[tdhock]

• HEAD=fix/258-getCommonChunk()-to-C++ much faster for getCommonChunk C++ #258
  

Generated via commit 0d903de

Download link for the artifact containing the test results: ↓ atime-results.zip

Task	Duration
R setup and installing dependencies	2 minutes and 23 seconds
Installing different package versions	21 seconds
Running and plotting the test cases	6 minutes and 46 seconds

[Nishita-shah1]

hi, what is the status here?

Hi Toby,

Status: ready for review/merge from my side.

• C++ simplified , unit tests passing
• Review fixes done given by @Faye-yufan
• atime #258 test is up; results look clean (no timing issues vs master)

Happy to adjust anything else you want.

[Nishita-shah1]

Hi @tdhock , @Faye-yufan , @suhaani-agarwal could you please review this PR, and if everything is right can we merge it?

[suhaani-agarwal]

thanks for this, @Nishita-shah1,i pulled the branch and ran the tests, all pass and cover real cases, nice work there.

1 thing I wanted to check before merging: the atime plot shows Fast/Slow basically overlapping (p~=0.84), which reads like no measurable speedup. looking at issue #258 again, it specifically calls out the by= grouping loop over columns/grps as the slow part to move to C++, but this PR keeps that loop in R and only moves the inner compare step that runs after it. that might explain the flat benchmar- the part thats slow is still R?
(could be I'm misreading the issue's scope, but wanted to check before we take on a compiled dependency for a change that may not be hitting the actual bottleneck)
curious what you think on this!

[Nishita-shah1]

Hi @suhaani-agarwal , thanks for the review, yes the old C++ only did the inner compare, so the by= grouping loop stayed in R and atime looked flat (p≈0.84).
Toby had asked for ~150 lines of C++ to keep the PR easy to review. I still moved the column/group scan into C++ as you suggested (detect_common_value_dt_cpp), and I fixed the atime workload so it actually builds a common chunk instead of returning NULL early.

The new atime plot is attached: HEAD is much faster (pred.Nx ≈ 65x, p=0). Locally, 20k rows / 5k groups was ~23s in R vs ~0.5s in C++.

The PR is larger again than the simplified version. Happy to keep this if you and @tdhock are okay with the extra C++, or split the grouping scan into a follow-up.

#342 (comment)

[tdhock]

thanks for sharing the atime result, but why is the Fast curve almost as large as Slow? (and both are much larger than HEAD)
I did not have time to review the C++ code, but it is encouraging that tests still pass.