From 38be24f541751030be8a0cff960f541018b7247b Mon Sep 17 00:00:00 2001 From: infra-bot Date: Fri, 3 Jul 2026 11:41:16 +0200 Subject: [PATCH] docs+fix: ecarts doc/code resolus, read_rules parle a l'editeur de YAML (issue #32) - @return complet (all_passed en tete, agent decrit fidelement : configure mais non interroge) - README : Quick Start zero-config d'abord, cle explicite partout ; la section Dev n'embarque plus de sorties check/covr perissables (figees a la 0.4.2 depuis mars) - vignette : restriction locale des diagnostics __absdiff/__thresh dans les extraits ; caveat NaN DuckDB-only du chemin lazy (releve au triage de la PR #41) - read_rules : erreur de version explicite (fichier + version declaree + version supportee, fini le stopifnot brut) ; warning sur les champs inconnus racine/defaults (typos by_nmae, no_equal) en nommant les champs connus - la friction principale d'un YAML edite a la main - write_rules_template refuse version != 1 en amont (il ecrivait des templates que read_rules refusait ensuite) --- NEWS.md | 20 ++++++ R/compare_datasets_from_yaml.R | 61 ++++++++++++++++- README.Rmd | 37 ++++------- README.md | 105 ++++++++++++------------------ man/compare_datasets_from_yaml.Rd | 6 +- tests/testthat/test-main.R | 65 ++++++++++++++++++ vignettes/datadiff.Rmd | 11 ++++ 7 files changed, 216 insertions(+), 89 deletions(-) diff --git a/NEWS.md b/NEWS.md index cbb9a2c..f2085af 100644 --- a/NEWS.md +++ b/NEWS.md @@ -86,6 +86,26 @@ projection (`paste0(character(0), "__ok")` yields `"__ok"`) that `any_of()` had been eating since 0.4.8 (issue #17). +## Documentation and input friction + +* Doc-vs-code drift resolved (issue #32): `@return` of + `compare_datasets_from_yaml()` now lists all 8 elements including + `all_passed` (the first one) and describes `agent` truthfully (configured, + NOT interrogated - `reponse` is); the README Quick Start shows the + zero-configuration mode first and passes `key` explicitly everywhere; the + README "Dev part" no longer embeds check/coverage outputs that go stale + (it froze results from 0.4.2); the vignette documents the local-only + restriction of the `__absdiff`/`__thresh` extract diagnostics and the + DuckDB-only NaN caveat of the lazy booleans. + +* `read_rules()` speaks to the person who edits the YAML by hand: an + unsupported `version` gets an explicit error naming the file, the declared + and the supported versions (was a raw `stopifnot` output), and unknown + top-level or `defaults` fields (typos like `by_nmae:` or `no_equal:`) get a + warning naming them and the known fields, instead of being silently + ignored. `write_rules_template()` rejects a `version` other than 1 upfront + (it happily wrote version 2 templates that `read_rules()` then refused). + ## Bug fixes * Three NEWS-vs-code contradictions introduced by the "Perf (#1)" commit after diff --git a/R/compare_datasets_from_yaml.R b/R/compare_datasets_from_yaml.R index 6f2d58d..14727d6 100644 --- a/R/compare_datasets_from_yaml.R +++ b/R/compare_datasets_from_yaml.R @@ -50,6 +50,13 @@ write_rules_template <- function(data_reference, datetime_equal_mode = "exact", logical_equal_mode = "exact") { + if (!identical(suppressWarnings(as.numeric(version)), 1)) { + stop(sprintf( + "Parameter 'version' must be 1 (the only rules version read_rules() accepts), got %s.", + version + ), call. = FALSE) + } + # Validate key if provided # Use a 0-row collect to retrieve column names and types: works for both local # data.frames and lazy tables (tbl_lazy) without loading all rows. @@ -122,7 +129,53 @@ write_rules_template <- function(data_reference, #' @export read_rules <- function(path) { r <- read_yaml(path) - stopifnot(is.list(r), !is.null(r$version), r$version == 1) + if (!is.list(r)) { + stop(sprintf("'%s' does not contain a YAML mapping of rules.", path), + call. = FALSE) + } + version_ok <- is.atomic(r$version) && + length(r$version) == 1 && + identical(suppressWarnings(as.numeric(r$version)), 1) + if (!version_ok) { + shown <- if (is.atomic(r$version) && length(r$version) == 1) { + as.character(r$version) + } else if (is.null(r$version)) { + "" + } else { + # lists / vectors from YAML like [1, 2] or {v: 1}: printable summary + paste(class(r$version)[1], "of length", length(r$version)) + } + stop(sprintf( + "'%s' declares rules version %s; this version of {datadiff} supports version 1 only.", + path, shown + ), call. = FALSE) + } + # Hand-edited YAML again: a scalar where a mapping is expected + # (defaults: yes) would crash far away on a $ access; refuse it here + for (section in c("defaults", "by_type", "by_name", "row_validation")) { + if (!is.null(r[[section]]) && !is.list(r[[section]])) { + stop(sprintf( + "'%s': the '%s' section must be a YAML mapping (key: value lines), not a single value.", + path, section + ), call. = FALSE) + } + } + # A hand-edited YAML is the main input of this package: flag the fields the + # comparison will silently ignore (typos like by_nmae or no_equal) + known_top <- c("version", "defaults", "by_type", "by_name", "row_validation") + unknown_top <- setdiff(names(r), known_top) + known_defaults <- c("na_equal", "ignore_columns", "keys", "key", "label") + unknown_defaults <- setdiff(names(r$defaults %||% list()), known_defaults) + unknown <- c(unknown_top, unknown_defaults) + if (length(unknown) > 0) { + warning(sprintf( + "'%s' contains field(s) the comparison will ignore: %s. Check for typos (known top-level fields: %s; known defaults fields: %s).", + path, + paste(sprintf("'%s'", unknown), collapse = ", "), + paste(known_top, collapse = ", "), + paste(known_defaults, collapse = ", ") + ), call. = FALSE) + } r$defaults <- r$defaults %||% list() r$by_type <- r$by_type %||% list() r$by_name <- r$by_name %||% list() @@ -292,7 +345,11 @@ validate_comparison_key <- function(key, ref_cols, cand_cols) { #' effect when Arrow datasets are used: plain `data.frame`s or `tbl_lazy` #' inputs ignore a valid value. #' @return A list containing: -#' \item{agent}{Configured pointblank agent with validation results} +#' \item{all_passed}{Logical; \code{TRUE} when every check passed. The +#' first element of the returned list and the single verdict consumers +#' should read.} +#' \item{agent}{The configured pointblank agent as built (NOT interrogated: +#' the verdict lives in \code{reponse}, which is the interrogated one)} #' \item{reponse}{Interrogated pointblank agent (class \code{datadiff_report}): #' usable by \code{pointblank::all_passed()} / \code{get_data_extracts()}; #' printing it lazily renders a full pointblank-style report from diff --git a/README.Rmd b/README.Rmd index f0f9712..8a77056 100644 --- a/README.Rmd +++ b/README.Rmd @@ -46,16 +46,20 @@ candidate <- data.frame( category = c("a", "b", "c") # Different case ) -# Generate a rules template +# Zero configuration: near-exact rules generated from the reference structure +result <- compare_datasets_from_yaml(reference, candidate, key = "id") +result$all_passed # FALSE here: exact rules reject the differences above + +# To tune tolerances, generate a rules template and edit it +rules_path <- tempfile(fileext = ".yaml") write_rules_template(reference, key = "id", - path = "validation_rules.yaml", + path = rules_path, numeric_abs = 0.01, character_case_insensitive = TRUE) -# Edit the rules file to configure validation (e.g., set tolerances, case sensitivity) - -# Compare datasets -result <- compare_datasets_from_yaml(reference, candidate, path = "validation_rules.yaml") +# Compare datasets with the tuned rules (key stays explicit) +result <- compare_datasets_from_yaml(reference, candidate, + key = "id", path = rules_path) result$all_passed # overall verdict (TRUE/FALSE) result$coverage # one row per check performed: column, type, n, n_failed, status @@ -401,24 +405,11 @@ MIT License ## Dev part -This `README` has been compiled on the - -```{r} -Sys.time() -``` - -Here are the test & coverage results: +Run the checks and the coverage locally (embedded outputs went stale as the +package evolved, so this README no longer freezes them at compile time): -```{r error = TRUE} -devtools::check(quiet = TRUE) -``` - -```{r echo = FALSE} -unloadNamespace("datadiff") -``` - -```{r error = TRUE} -Sys.setenv("NOT_CRAN" = TRUE) +```r +devtools::check() covr::package_coverage() ``` diff --git a/README.md b/README.md index 3c3cee6..814bffd 100644 --- a/README.md +++ b/README.md @@ -37,16 +37,20 @@ candidate <- data.frame( category = c("a", "b", "c") # Different case ) -# Generate a rules template +# Zero configuration: near-exact rules generated from the reference structure +result <- compare_datasets_from_yaml(reference, candidate, key = "id") +result$all_passed # FALSE here: exact rules reject the differences above + +# To tune tolerances, generate a rules template and edit it +rules_path <- tempfile(fileext = ".yaml") write_rules_template(reference, key = "id", - path = "validation_rules.yaml", + path = rules_path, numeric_abs = 0.01, character_case_insensitive = TRUE) -# Edit the rules file to configure validation (e.g., set tolerances, case sensitivity) - -# Compare datasets -result <- compare_datasets_from_yaml(reference, candidate, path = "validation_rules.yaml") +# Compare datasets with the tuned rules (key stays explicit) +result <- compare_datasets_from_yaml(reference, candidate, + key = "id", path = rules_path) result$all_passed # overall verdict (TRUE/FALSE) result$coverage # one row per check performed: column, type, n, n_failed, status @@ -142,13 +146,13 @@ by_name: `by_name` taking precedence. A column not listed in `by_name` uses its `by_type` defaults unchanged. -| Column | Effective rule | Source | -|----|----|----| -| `id` | `abs: 0` | `by_type.integer` | -| `amount` | `abs: 0.01, rel: 0` | `by_name.amount` overrides `by_type.numeric` | -| `unit_price` | `abs: 0.001, rel: 0.0001` | `by_name.unit_price` overrides `by_type.numeric` | -| `category` | `case_insensitive: yes, trim: yes` | `by_name.category` overrides `by_type.character` | -| `created_at` | `equal_mode: exact` | `by_type.date` (by_name is redundant here) | +| Column | Effective rule | Source | +|--------------|------------------------------------|--------------------------------------------------| +| `id` | `abs: 0` | `by_type.integer` | +| `amount` | `abs: 0.01, rel: 0` | `by_name.amount` overrides `by_type.numeric` | +| `unit_price` | `abs: 0.001, rel: 0.0001` | `by_name.unit_price` overrides `by_type.numeric` | +| `category` | `case_insensitive: yes, trim: yes` | `by_name.category` overrides `by_type.character` | +| `created_at` | `equal_mode: exact` | `by_type.date` (by_name is redundant here) | ### Numeric tolerance: formula, edge effects, and best practices @@ -162,10 +166,10 @@ threshold**: The two parameters **add up**: they are not two independent guards. -| Parameter | Role | Default value | -|----|----|----| -| `abs` | Absolute tolerance: fixed floor, independent of the magnitude of values | `1e-9` | -| `rel` | Relative tolerance: fraction of the reference value added to the threshold | `0` | +| Parameter | Role | Default value | +|-----------|----------------------------------------------------------------------------|---------------| +| `abs` | Absolute tolerance: fixed floor, independent of the magnitude of values | `1e-9` | +| `rel` | Relative tolerance: fraction of the reference value added to the threshold | `0` | #### Pure absolute mode (recommended by default) @@ -201,11 +205,11 @@ by_type: rel: 0.01 # tolerance of 1% of the reference value ``` -| Reference | Candidate | Difference | Threshold (1%) | Result | -|----|---:|---:|---:|----| -| 100.00 | 100.50 | 0.50 | 1.00 | OK | -| 1 000 000.00 | 1 005 000.00 | 5 000 | 10 000 | OK | -| 0.00 | 0.001 | 0.001 | **0** | **ERROR** (implicit division by zero) | +| Reference | Candidate | Difference | Threshold (1%) | Result | +|--------------|-------------:|-----------:|---------------:|---------------------------------------| +| 100.00 | 100.50 | 0.50 | 1.00 | OK | +| 1 000 000.00 | 1 005 000.00 | 5 000 | 10 000 | OK | +| 0.00 | 0.001 | 0.001 | **0** | **ERROR** (implicit division by zero) | > **Warning**: if the reference value is `0`, the relative threshold is > `0`, so any difference, even tiny, will be detected as an error. @@ -286,7 +290,7 @@ row_validation: This validates that the candidate dataset has between 950 and 1050 rows (1000 +/- 50). -If `expected_count` is not specified, the reference dataset's row count +If `expected_count` is not specified, the reference dataset’s row count is used as the expected value. ## Comparing Parquet files (large datasets) @@ -294,7 +298,7 @@ is used as the expected value. `datadiff` can compare Parquet files that are too large to fit in RAM. The recommended approach uses `arrow::open_dataset()` - **do not call `arrow::to_duckdb()` yourself** before passing to `datadiff`; the -package handles the Arrow -> DuckDB conversion internally with a single +package handles the Arrow -\> DuckDB conversion internally with a single connection. ### Recommended strategy: Arrow Dataset (lazy, out-of-core) @@ -322,12 +326,12 @@ Internally, `compare_datasets_from_yaml()`: 1. Opens a private DuckDB connection (`fresh_con`). 2. Materialises each Parquet dataset as a DuckDB physical temp table - via `read_parquet()` - all memory is managed by DuckDB's buffer + via `read_parquet()` - all memory is managed by DuckDB’s buffer pool, so disk spilling works correctly. 3. Runs the full join + 125 boolean expressions as a single lazy SQL query. 4. `dplyr::compute()` materialises only the slim boolean result table - (~125 logical columns * N rows) - the wide source data is never + (~125 logical columns \* N rows) - the wide source data is never loaded into R. 5. `dplyr::collect()` brings the slim boolean table (~few GB) into R and passes a plain `data.frame` to pointblank - no live DuckDB @@ -336,7 +340,7 @@ Internally, `compare_datasets_from_yaml()`: ### Memory tuning: `duckdb_memory_limit` -DuckDB's default memory cap (80 % of total RAM) can leave insufficient +DuckDB’s default memory cap (80 % of total RAM) can leave insufficient headroom when R, Arrow, and the OS are already using significant memory. The `duckdb_memory_limit` parameter controls how much RAM DuckDB may use before spilling intermediate results to `tempdir()`: @@ -365,12 +369,12 @@ limit only applies when Arrow datasets are used - it has no effect for ### Strategy comparison -| Strategy | Input type | RAM usage | Requires | -|----|----|----|----| -| **Arrow Dataset** ✅ recommended | `arrow::open_dataset()` | Slim boolean table only (~few GB) | arrow, duckdb | -| Arrow Table | `arrow::read_parquet(as_data_frame=FALSE)` | Same as Arrow Dataset | arrow, duckdb | -| Lazy table (dbplyr) | `tbl(con, "table_name")` | Slim boolean table only | DBI, dbplyr | -| `data.frame` | `read.csv()`, `readr::read_csv()`, etc. | Full data in RAM | - | +| Strategy | Input type | RAM usage | Requires | +|----------------------------------|--------------------------------------------|-----------------------------------|---------------| +| **Arrow Dataset** ✅ recommended | `arrow::open_dataset()` | Slim boolean table only (~few GB) | arrow, duckdb | +| Arrow Table | `arrow::read_parquet(as_data_frame=FALSE)` | Same as Arrow Dataset | arrow, duckdb | +| Lazy table (dbplyr) | `tbl(con, "table_name")` | Slim boolean table only | DBI, dbplyr | +| `data.frame` | `read.csv()`, `readr::read_csv()`, etc. | Full data in RAM | \- | ### What NOT to do @@ -389,7 +393,7 @@ ref_df <- dplyr::collect(ds_ref) # loads full 4 GB into R RAM DuckDB spills to `tempdir()` when the memory limit is reached. On Windows this is typically `C:\Users\\AppData\Local\Temp`. Ensure -that directory has sufficient free disk space (up to ~2-3* the size of +that directory has sufficient free disk space (up to ~2-3\* the size of your Parquet files in the worst case). ## Main Functions @@ -421,36 +425,11 @@ MIT License ## Dev part -This `README` has been compiled on the - -``` r -Sys.time() -#> [1] "2026-03-11 18:07:04 CET" -``` - -Here are the test & coverage results: - -``` r -devtools::check(quiet = TRUE) -#> ℹ Loading datadiff -#> ── R CMD check results ───────────────────────────────────── datadiff 0.4.2 ──── -#> Duration: 4m 47.4s -#> -#> ❯ checking for future file timestamps ... NOTE -#> unable to verify current time -#> -#> 0 errors ✔ | 0 warnings ✔ | 1 note ✖ -``` +Run the checks and the coverage locally (embedded outputs went stale as +the package evolved, so this README no longer freezes them at compile +time): ``` r -Sys.setenv("NOT_CRAN" = TRUE) +devtools::check() covr::package_coverage() -#> datadiff Coverage: 98.98% -#> R/preprocessing.R: 97.22% -#> R/compare_datasets_from_yaml.R: 98.58% -#> R/data_types.R: 100.00% -#> R/pointblank_setup.R: 100.00% -#> R/tolerance.R: 100.00% -#> R/utils.R: 100.00% -#> R/validation.R: 100.00% ``` diff --git a/man/compare_datasets_from_yaml.Rd b/man/compare_datasets_from_yaml.Rd index 91bf198..e550b56 100644 --- a/man/compare_datasets_from_yaml.Rd +++ b/man/compare_datasets_from_yaml.Rd @@ -89,7 +89,11 @@ inputs ignore a valid value.} } \value{ A list containing: -\item{agent}{Configured pointblank agent with validation results} +\item{all_passed}{Logical; \code{TRUE} when every check passed. The +first element of the returned list and the single verdict consumers +should read.} +\item{agent}{The configured pointblank agent as built (NOT interrogated: +the verdict lives in \code{reponse}, which is the interrogated one)} \item{reponse}{Interrogated pointblank agent (class \code{datadiff_report}): usable by \code{pointblank::all_passed()} / \code{get_data_extracts()}; printing it lazily renders a full pointblank-style report from diff --git a/tests/testthat/test-main.R b/tests/testthat/test-main.R index e505206..2471942 100644 --- a/tests/testthat/test-main.R +++ b/tests/testthat/test-main.R @@ -60,3 +60,68 @@ test_that("read_rules validates YAML structure", { expect_error(object = read_rules(invalid_path)) }) + +test_that("read_rules gives an actionable error for unsupported versions", { + p <- tempfile(fileext = ".yaml") + on.exit(unlink(p), add = TRUE) + writeLines('version: 2\ndefaults: {na_equal: yes}', con = p) + err <- tryCatch(read_rules(p), error = function(e) { + conditionMessage(e) + }) + expect_match(err, "version", ignore.case = TRUE) + expect_match(err, "2", fixed = TRUE) # the offending version + expect_match(err, "1", fixed = TRUE) # the supported version + expect_no_match(err, "is not TRUE", fixed = TRUE) # no raw stopifnot output +}) + +test_that("read_rules warns on unknown top-level and defaults fields", { + p <- tempfile(fileext = ".yaml") + on.exit(unlink(p), add = TRUE) + writeLines(' +version: 1 +defaults: {keys: [id], na_equal: yes, no_equal: yes} +by_nmae: + numeric: {abs: 0.1} +', con = p) + warns <- character(0) + withCallingHandlers( + rules <- read_rules(p), + warning = function(w) { + warns <<- c(warns, conditionMessage(w)) + invokeRestart("muffleWarning") + } + ) + expect_true(any(grepl("by_nmae", warns))) # typo of by_name flagged + expect_true(any(grepl("no_equal", warns))) # typo in defaults flagged +}) + +test_that("write_rules_template rejects an unsupported version upfront", { + df <- data.frame(id = 1:2, x = c(1, 2)) + expect_error( + write_rules_template(df, key = "id", path = tempfile(fileext = ".yaml"), + version = 2), + regexp = "version" + ) +}) + +test_that("read_rules stays actionable on non-scalar versions", { + p <- tempfile(fileext = ".yaml") + on.exit(unlink(p), add = TRUE) + writeLines('version: {v: 1}\ndefaults: {na_equal: yes}', con = p) + err <- tryCatch(read_rules(p), error = function(e) { + conditionMessage(e) + }) + expect_match(err, "version", ignore.case = TRUE) + expect_no_match(err, "invalid format", fixed = TRUE) # no raw sprintf crash +}) + +test_that("read_rules rejects non-mapping sections explicitly", { + p <- tempfile(fileext = ".yaml") + on.exit(unlink(p), add = TRUE) + writeLines('version: 1\ndefaults: yes', con = p) + err <- tryCatch(read_rules(p), error = function(e) { + conditionMessage(e) + }) + expect_match(err, "defaults") + expect_match(err, "mapping", ignore.case = TRUE) +}) diff --git a/vignettes/datadiff.Rmd b/vignettes/datadiff.Rmd index 6003f36..d30a3f8 100644 --- a/vignettes/datadiff.Rmd +++ b/vignettes/datadiff.Rmd @@ -182,6 +182,14 @@ extracts <- pointblank::get_data_extracts(result_fail$reponse) extracts[[1]] ``` +### Measured deviations in the extracts + +On the in-memory (data.frame) path, the extracts of a failing tolerance +column also carry the measured deviation and the applied threshold +(`__absdiff`, `__thresh`). The lazy path never had these +diagnostics: its agent works on a slim boolean table and its extracts only +show the boolean columns. + ### Controlling how many failing rows are extracted For large datasets, extracting all failing rows can consume significant memory. @@ -912,6 +920,9 @@ DBI::dbDisconnect(con) ``` This works with any DBI-compatible backend: SQLite, PostgreSQL, Snowflake, etc. +Caveat: the NaN-aware semantics of the lazy booleans are wired for DuckDB +(`isnan()`); a non-DuckDB backend that stores NaN keeps NULL-only rules for +those values (see the `add_bool_cols_sql()` notes). ### Arrow / Parquet (out-of-core)