perf: verdict lazy par agregats SQL, plus de collect integral des booleens by VincentGuyader · Pull Request #50 · ThinkR-open/datadiff

VincentGuyader · 2026-07-03T09:33:49Z

Closes #22

Probleme

Le commentaire du bloc lazy promettait « without loading data into R memory (~62 MB for 4M rows) » mais le collect() de la table slim chargeait N x (T+E) logicals en RAM R : ~2 Go pour 4M lignes x 125 colonnes (les logicals R pesent 4 octets), 32x le chiffre annonce - sur le cas d'usage phare du package (Parquet plus gros que la RAM). Toute l'info du cas vert tient dans (T+E) paires (n, n_failed).

Correctif

lazy_boolean_counts() : un scan d'agregats SQL sur la temp table (SUM(CASE WHEN ok THEN 0 ELSE 1 END) compte FALSE et NULL, semantique exacte des reducteurs locaux, verifiee sur DuckDB ET SQLite y compris la truthiness 0/1 SQLite)
build_coverage(counts =) : comptes precalcules sur le chemin lazy, chemin local byte-identique
Cas vert : plus aucun collect - nrow(res$reponse$tbl) passe de N (50 000 dans le test) a 1 (placeholder constant)
Cas rouge : seules les colonnes booleennes EN ECHEC sont collectees pour l'agent (+ row_count_ok si actif) ; extraits inchanges
Helpers datadiff_ok_col/eq_col length-guarded : le fantome recycle0 a mordu une 2e fois via les agregats - la garde centrale immunise la classe
Vignette : caveat get_sundered_data sur cas vert lazy (placeholder, pas les lignes source)

Tests (TDD red-first)

test-lazy-aggregates.R : vert 50k lignes (comptes intacts + placeholder), rouge (colonnes en echec seules dans l'agent + comptes exacts + extraits presents), SQLite. Rouges constates sur placeholder et colonnes.

Suite complete : 992 PASS / 0 FAIL sur la pile (rebasee sur audit-0.4.10 courant, qui integre le travail #11 de main - conflit resolu en conservant les deux blocs).

Review interne

pr-reviewer : APPROVE (equivalence agregats/reducteurs verifiee sur les deux backends, cas degenere zero-colonne trace, ordre on.exit confirme). Nits appliques : commentaire sans narration, if/else braces, arguments DBI nommes, caveat vignette.

Copilot

Pull request overview

Cette PR corrige un problème de performance/mémoire sur le chemin lazy : le verdict et la coverage ne nécessitent plus de collect() intégral de la table slim de booléens en RAM R, grâce à un calcul de comptages via agrégats SQL, et ne collecte en local (pour pointblank) que les colonnes en échec.

Changes:

Ajout de lazy_boolean_counts() et branchement de build_coverage(counts=) pour dériver (n, n_failed) via un unique scan d’agrégats SQL côté DB.
Sur le chemin lazy, suppression du collect() systématique : placeholder constant-size en cas vert, et collect() limité aux seules colonnes booléennes en échec (± row_count_ok) en cas rouge.
Ajout de tests d’intégration (DuckDB + SQLite) et mise à jour doc (vignette + NEWS) pour refléter le nouveau comportement.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`R/coverage.R`	Ajoute le calcul des comptes via agrégats SQL (`lazy_boolean_counts`) et permet d’injecter des comptes pré-calculés dans `build_coverage()`.
`R/compare_datasets_from_yaml.R`	Utilise les comptes SQL sur le chemin lazy et ne collecte plus la table slim entière (collecte ciblée en cas d’échec).
`R/constants.R`	Rend les helpers de noms `datadiff_ok_col` / `datadiff_eq_col` robustes au cas `character(0)` (garde anti “recycle0 phantom”).
`tests/testthat/test-lazy-aggregates.R`	Couvre cas vert/rouge lazy (DuckDB) + validation des comptes sur SQLite.
`vignettes/datadiff.Rmd`	Ajoute un caveat sur `get_sundered_data()` pour un all-pass lazy (placeholder).
`NEWS.md`	Documente le changement de perf/mémoire sur le verdict lazy (issue #22).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

VincentGuyader · 2026-07-03T09:41:57Z

@@ -717,34 +712,45 @@ compare_datasets_from_yaml <- function(data_reference,
    # result into R so that pointblank receives a plain data.frame - avoiding
    # DuckDB connection-state issues (is_tbl_mssql crash) during interrogation.


Valide et applique : le commentaire du compute() decrit desormais le scan d'agregats + collect des seules colonnes en echec (meme classe que la PR #46 : commentaire de la v1 ayant survecu au raffinement - la regle #11 item 12 extraite alors aurait du me le faire attraper, c'est note).

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

VincentGuyader · 2026-07-03T09:59:39Z

@@ -713,38 +708,51 @@
    tmp_tbl_name  <- datadiff_tmp_table_name()


Valides tous les deux, et le second cachait un vrai bug au-dela de l'aller-retour inutile : sur un echec purement structurel (check_count off), l'agent recevait une table vide, les steps dummy interrogeaient 0 unite et passaient - pointblank::all_passed() aurait contredit la coverage (en pratique le cas crashait avant : Query contains no columns). Corrige : garde val_cols vide (aucun compute), garde red_cols vide avec seed d'UNE ligne pour porter les unites en echec des steps structurels, 2 tests de regression (echec structurel seul, comparaison cle-seule).

+    if (is_lazy) {
+      red_cols <- c(
+        datadiff_ok_col(fail$tol),
+        datadiff_eq_col(fail$eq),
+        if (isTRUE(row_validation_info$check_count)) {
+          "row_count_ok"
+        } else {
+          character(0)
+        }
+      )
+      cmp_for_agent <- dplyr::collect(
+        dplyr::select(cmp_slim_computed, dplyr::all_of(red_cols))
+      )
+    }


Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

VincentGuyader · 2026-07-03T10:06:20Z

+    cnt <- counts[[c]] %||% tol_col_counts(tbl, col = c)
    add(c, "tolerance", cnt$n, cnt$n_failed)
  }
  for (c in eq_cols) {
-    cnt <- eq_col_counts(tbl, col = c, ref_suffix = ref_suffix, na_equal = na_equal)
+    cnt <- counts[[c]] %||%
+      eq_col_counts(tbl, col = c, ref_suffix = ref_suffix, na_equal = na_equal)
    add(c, "equality", cnt$n, cnt$n_failed)


Valide et applique : les compteurs de coverage sont stockes en numeric (as.numeric) - as.integer debordait a NA au-dela de 2^31-1, precisement atteignable maintenant que les comptes viennent d'agregats SQL sur des tables que R n'aurait pas pu collecter. Les consommateurs (all(n_failed == 0L), rapport, summary) fonctionnent a l'identique sur des doubles.

…leens (issue #22) Le collect() de la table slim chargeait N x (T+E) logicals en RAM R (~2 Go pour 4M lignes x 125 colonnes sur le cas vert), contredisant la promesse without loading data into R memory du commentaire - alors que le verdict tient dans (T+E) paires (n, n_failed). - lazy_boolean_counts() : un scan d'agregats SQL sur la temp table (SUM(CASE WHEN ok THEN 0 ELSE 1 END) compte FALSE et NULL, semantique exacte des reducteurs locaux ; SUM sur table vide -> NULL -> 0) - build_coverage(counts =) : les comptes precalcules remplacent les accesseurs locaux sur le chemin lazy - cas vert : plus aucun collect, l'agent placeholder est de taille constante (nrow(res$reponse$tbl) <= 1 au lieu de N) - cas rouge : seules les colonnes booleennes EN ECHEC sont collectees pour l'agent (+ row_count_ok si check actif), extraits inchanges - les helpers datadiff_ok_col/eq_col deviennent length-guarded : le fantome recycle0 (paste0(character(0), suffixe) == suffixe) a mordu une 2e fois via lazy_boolean_counts ; la garde centrale immunise la classe et le suffix_all local devient inutile

VincentGuyader requested a review from Copilot July 3, 2026 09:33

Copilot started reviewing on behalf of VincentGuyader July 3, 2026 09:34 View session

Copilot AI reviewed Jul 3, 2026

View reviewed changes

VincentGuyader force-pushed the perf-issue-22-lazy-aggregates branch from 40b984b to 5c0f45f Compare July 3, 2026 09:41

VincentGuyader requested a review from Copilot July 3, 2026 09:41

Copilot started reviewing on behalf of VincentGuyader July 3, 2026 09:42 View session

Copilot AI reviewed Jul 3, 2026

View reviewed changes

VincentGuyader force-pushed the perf-issue-22-lazy-aggregates branch from 5c0f45f to 0bb7857 Compare July 3, 2026 09:59

VincentGuyader requested a review from Copilot July 3, 2026 09:59

Copilot started reviewing on behalf of VincentGuyader July 3, 2026 10:00 View session

Copilot AI reviewed Jul 3, 2026

View reviewed changes

VincentGuyader force-pushed the perf-issue-22-lazy-aggregates branch from 0bb7857 to b94b345 Compare July 3, 2026 10:06

VincentGuyader merged commit 301361e into audit-0.4.10 Jul 3, 2026
6 checks passed

VincentGuyader mentioned this pull request Jul 3, 2026

datadiff 0.6.0 : traitement des 24 issues ouvertes de l'audit #58

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

perf: verdict lazy par agregats SQL, plus de collect integral des booleens#50

perf: verdict lazy par agregats SQL, plus de collect integral des booleens#50
VincentGuyader merged 1 commit into
audit-0.4.10from
perf-issue-22-lazy-aggregates

VincentGuyader commented Jul 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

VincentGuyader Jul 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

VincentGuyader Jul 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

VincentGuyader Jul 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -717,34 +712,45 @@ compare_datasets_from_yaml <- function(data_reference,
		# result into R so that pointblank receives a plain data.frame - avoiding
		# DuckDB connection-state issues (is_tbl_mssql crash) during interrogation.

		@@ -713,38 +708,51 @@
		tmp_tbl_name <- datadiff_tmp_table_name()

Uh oh!

Conversation

VincentGuyader commented Jul 3, 2026

Probleme

Correctif

Tests (TDD red-first)

Review interne

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

VincentGuyader Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

VincentGuyader Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

VincentGuyader Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants