Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,19 @@

## Robustness

* Report construction hardening (all internal to `R/report.R`):
`datadiff_report_html()` now shares the `print()` memoization (it reads the
cached report and feeds the cache, instead of rebuilding agent + report on
every call); a genuine evaluation error on the real agent (`eval_error`,
where `n_failed` is NA) is propagated to the report instead of being
silently replaced by the synthetic all-pass branch and forced to FALSE; a
dead per-row count-injection block was removed (its fields were entirely
overwritten by the vectorised coverage block); and the `__ok`/`__eq`/
`__missing_col_`/`__type_mismatch_` naming conventions plus the default
warn/stop levels are now defined once in `R/constants.R` and shared by the
producers, the verdict consumers and the report mapping, so a one-sided
rename can no longer silently break the step mapping (issue #29).

* `arrow_dataset_to_duckdb()` builds its SQL safely: Parquet file paths are
quoted with `DBI::dbQuoteString()` (a path containing a quote, common in
French like `l'export/`, broke the query with a raw SQL syntax error), the
Expand Down
8 changes: 4 additions & 4 deletions R/compare_datasets_from_yaml.R
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ compare_datasets_from_yaml <- function(data_reference,
data_candidate,
key = NULL,
path = NULL,
warn_at = 0.00000000000001, stop_at = 0.00000000000001,
warn_at = 1e-14, stop_at = 1e-14,
ref_suffix = "__reference",
label = NULL,
error_msg_no_key = "Without keys, both tables must have the same number of rows.",
Expand Down Expand Up @@ -671,8 +671,8 @@ compare_datasets_from_yaml <- function(data_reference,
paste0(cols, suffix)
}
val_cols <- c(
suffix_all(tol_cols, suffix = "__ok"),
suffix_all(eq_cols, suffix = "__eq"),
suffix_all(tol_cols, suffix = datadiff_suffix_ok),
suffix_all(eq_cols, suffix = datadiff_suffix_eq),
if (isTRUE(row_validation_info$check_count)) {
"row_count_ok"
} else {
Expand Down Expand Up @@ -758,7 +758,7 @@ compare_datasets_from_yaml <- function(data_reference,
# express these semantics.
if (!is_lazy && length(fail$eq) > 0) {
for (c in fail$eq) {
cmp_for_agent[[paste0(c, "__eq")]] <- eq_col_bool(
cmp_for_agent[[datadiff_eq_col(c)]] <- eq_col_bool(
cmp_for_agent, col = c, ref_suffix = ref_suffix, na_equal = na_equal
)
}
Expand Down
31 changes: 31 additions & 0 deletions R/constants.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# Internal naming conventions shared by the boolean producers (tolerance.R,
# compare_datasets_from_yaml.R), the verdict consumers (fast_path.R,
# coverage.R) and the report step mapping (report.R, pointblank_setup.R).
# They live in one place because a drift on one side breaks the mapping
# SILENTLY: the report step is simply not found and its extract is lost.

# Suffix of the per-column boolean produced for a tolerance column.
datadiff_suffix_ok <- "__ok"

# Suffix of the per-column boolean produced for an equality column.
datadiff_suffix_eq <- "__eq"

# Prefix of the dummy column carrying a missing-column failure step.
datadiff_prefix_missing_col <- "__missing_col_"

# Prefix of the dummy column carrying a type-mismatch failure step.
datadiff_prefix_type_mismatch <- "__type_mismatch_"

# Column name helpers: the only supported way to build these names.
datadiff_ok_col <- function(col) {
paste0(col, datadiff_suffix_ok)
}
datadiff_eq_col <- function(col) {
paste0(col, datadiff_suffix_eq)
}

# Default pointblank action levels (fractions of failing rows). Used as the
# internal fallback everywhere a threshold is reconstructed from attributes;
# the public compare_datasets_from_yaml() signature documents the same value.
datadiff_default_warn_at <- 1e-14
datadiff_default_stop_at <- 1e-14
11 changes: 6 additions & 5 deletions R/fast_path.R
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@
# error: all(NULL) is TRUE, so returning NULL would silently turn a dropped
# column into a false all-pass.
tol_col_bool <- function(tbl, col) {
b <- tbl[[paste0(col, "__ok")]]
b <- tbl[[datadiff_ok_col(col)]]
if (is.null(b)) {
stop(sprintf("internal error: boolean column '%s__ok' missing", col),
stop(sprintf("internal error: boolean column '%s' missing",
datadiff_ok_col(col)),
call. = FALSE)
}
b
Expand All @@ -26,7 +27,7 @@ tol_col_bool <- function(tbl, col) {
# the pre-computed <col>__eq vector (lazy path) or, when absent (local path),
# the raw comparison with one-sided NA = FALSE and two-sided NA = na_equal.
eq_col_bool <- function(tbl, col, ref_suffix, na_equal) {
eq_precomputed <- tbl[[paste0(col, "__eq")]]
eq_precomputed <- tbl[[datadiff_eq_col(col)]]
if (!is.null(eq_precomputed)) {
return(eq_precomputed)
}
Expand All @@ -44,8 +45,8 @@ eq_col_bool <- function(tbl, col, ref_suffix, na_equal) {
}
)
stop(sprintf(
"internal error: equality column(s) %s missing (no precomputed '%s__eq' either)",
paste(missing_cols, collapse = " and "), col
"internal error: equality column(s) %s missing (no precomputed '%s' either)",
paste(missing_cols, collapse = " and "), datadiff_eq_col(col)
), call. = FALSE)
}
cand_na <- is.na(cand_vals)
Expand Down
12 changes: 6 additions & 6 deletions R/pointblank_setup.R
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ setup_pointblank_agent <- function(cmp, cols_reference, common_cols, tol_cols,
# Add dummy columns for missing columns BEFORE creating the agent
# These columns are set to FALSE and we'll check they equal TRUE (will fail)
for (c in missing_in_candidate) {
dummy_col <- paste0("__missing_col_", c)
dummy_col <- paste0(datadiff_prefix_missing_col, c)
if (is_non_local(cmp)) {
cmp <- dplyr::mutate(cmp, !!dummy_col := FALSE)
} else {
Expand All @@ -58,7 +58,7 @@ setup_pointblank_agent <- function(cmp, cols_reference, common_cols, tol_cols,
# Add dummy FALSE columns for type-mismatched columns.
# These will generate a dedicated failing validation step per column.
for (c in type_mismatch_cols) {
dummy_col <- paste0("__type_mismatch_", c)
dummy_col <- paste0(datadiff_prefix_type_mismatch, c)
if (is_non_local(cmp)) {
cmp <- dplyr::mutate(cmp, !!dummy_col := FALSE)
} else {
Expand All @@ -83,7 +83,7 @@ setup_pointblank_agent <- function(cmp, cols_reference, common_cols, tol_cols,

# For missing columns, add a validation that will fail
for (c in missing_in_candidate) {
dummy_col <- paste0("__missing_col_", c)
dummy_col <- paste0(datadiff_prefix_missing_col, c)
agent <- agent %>%
col_vals_equal(
columns = all_of(dummy_col),
Expand All @@ -95,7 +95,7 @@ setup_pointblank_agent <- function(cmp, cols_reference, common_cols, tol_cols,

# For type-mismatched columns, add a validation that will always fail.
for (c in type_mismatch_cols) {
dummy_col <- paste0("__type_mismatch_", c)
dummy_col <- paste0(datadiff_prefix_type_mismatch, c)
agent <- agent %>%
col_vals_equal(
columns = all_of(dummy_col),
Expand All @@ -107,7 +107,7 @@ setup_pointblank_agent <- function(cmp, cols_reference, common_cols, tol_cols,

eq_cols <- setdiff(x = common_cols, y = tol_cols)
for (c in eq_cols) {
eq_precomputed <- paste0(c, "__eq")
eq_precomputed <- datadiff_eq_col(c)
if (eq_precomputed %in% get_col_names(cmp)) {
# Lazy path: use pre-computed boolean equality column
agent <- agent %>%
Expand All @@ -124,7 +124,7 @@ setup_pointblank_agent <- function(cmp, cols_reference, common_cols, tol_cols,
}

for (c in tol_cols) {
ok_col <- paste0(c, "__ok")
ok_col <- datadiff_ok_col(c)
agent <- agent %>% col_vals_equal(columns = all_of(ok_col), value = TRUE, na_pass = FALSE)
}

Expand Down
77 changes: 40 additions & 37 deletions R/report.R
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ report_underlying_col <- function(col) {
if (identical(col, "row_count_ok")) {
return("<row_count>")
}
if (startsWith(col, "__missing_col_")) {
return(sub("^__missing_col_", "", col))
if (startsWith(col, datadiff_prefix_missing_col)) {
return(sub(paste0("^", datadiff_prefix_missing_col), "", x = col))
}
if (startsWith(col, "__type_mismatch_")) {
return(sub("^__type_mismatch_", "", col))
if (startsWith(col, datadiff_prefix_type_mismatch)) {
return(sub(paste0("^", datadiff_prefix_type_mismatch), "", x = col))
}
sub("__(ok|eq)$", "", col)
sub(sprintf("(%s|%s)$", datadiff_suffix_ok, datadiff_suffix_eq), "", x = col)
}

# Build a pointblank agent whose report mirrors the coverage table.
Expand All @@ -33,17 +33,20 @@ report_underlying_col <- function(col) {
# synthetic passing row is added for the rest. Without `real_agent`, a purely
# synthetic count-only agent is produced (no extracts).
build_report_agent <- function(coverage, label, lang = "fr", locale = "fr_FR",
warn_at = 1e-14, stop_at = 1e-14,
warn_at = datadiff_default_warn_at, stop_at = datadiff_default_stop_at,
real_agent = NULL) {
n <- nrow(coverage)

# Augment the real interrogated agent only when it has genuine failures
# (real extracts to preserve). On an all-pass comparison the real agent is the
# (real extracts to preserve) or evaluation errors (n_failed is NA there, so
# the count test alone would silently route a broken interrogation to the
# synthetic all-pass branch). On an all-pass comparison the real agent is the
# minimal placeholder (a single col_exists step); cloning its template would
# mislabel every value check as col_exists, so fall through to the synthetic
# col_vals_equal build instead.
if (!is.null(real_agent) &&
any(real_agent$validation_set$n_failed > 0, na.rm = TRUE)) {
(any(real_agent$validation_set$n_failed > 0, na.rm = TRUE) ||
any(real_agent$validation_set$eval_error, na.rm = TRUE))) {
rvs <- real_agent$validation_set
real_cols <- vapply(seq_len(nrow(rvs)), function(j) {
report_underlying_col(rvs$column[[j]][1])
Expand All @@ -55,41 +58,39 @@ build_report_agent <- function(coverage, label, lang = "fr", locale = "fr_FR",
template <- rvs[1, , drop = FALSE]
rows <- vector("list", n)
new_extracts <- list()
real_eval_error <- logical(n)
real_eval_warning <- logical(n)
for (i in seq_len(n)) {
col <- coverage$column[i]
# Only value checks (tolerance / equality) map to a real step: that is
# where the genuine row-level extract matters. col_exists and the
# structural checks (missing_column, type_mismatch, row_count) are
# synthesized from coverage - mapping them to a real step would pull in
# per-row dummy results (e.g. n_failed = nrow) that contradict coverage.
# Consequence: an eval_error on a structural step is intentionally not
# surfaced per-column (only value-step evaluation errors are).
j <- if (coverage$check[i] %in% c("tolerance", "equality")) {
match(col, real_cols)
} else {
NA_integer_
}
if (!is.na(j)) {
# Genuine interrogated step: keep it, and carry its real extract over to
# the new step index.
# Genuine interrogated step: keep it, carry its real extract over to
# the new step index, and remember its evaluation outcome (the
# vectorised coverage overwrite below must not mask a real
# eval_error/eval_warning behind FALSE).
row <- rvs[j, , drop = FALSE]
real_eval_error[i] <- isTRUE(rvs$eval_error[j])
real_eval_warning[i] <- isTRUE(rvs$eval_warning[j])
old_key <- as.character(rvs$i[j])
if (!is.null(old_extracts[[old_key]])) {
new_extracts[[as.character(i)]] <- old_extracts[[old_key]]
}
} else {
# Column the targeted agent did not validate (it passed): synthesise a
# passing row from the real-row template.
# passing row from the real-row template. Every count/verdict field is
# set authoritatively from coverage in the vectorised block below.
row <- template
row$eval_error <- FALSE
row$eval_warning <- FALSE
row$n <- as.numeric(coverage$n[i])
row$n_passed <- as.numeric(coverage$n[i] - coverage$n_failed[i])
row$n_failed <- as.numeric(coverage$n_failed[i])
row$f_passed <- if (coverage$n[i] > 0) row$n_passed / coverage$n[i] else 1
row$f_failed <- if (coverage$n[i] > 0) coverage$n_failed[i] / coverage$n[i] else 0
row$all_passed <- coverage$n_failed[i] == 0L
row$warn <- FALSE
row$stop <- FALSE
row$notify <- FALSE
}
row$column <- list(coverage$column[i])
row$label <- coverage$check[i]
Expand All @@ -113,8 +114,8 @@ build_report_agent <- function(coverage, label, lang = "fr", locale = "fr_FR",
new_vs$warn <- coverage$n_failed > 0L & new_vs$f_failed >= warn_at
new_vs$stop <- coverage$n_failed > 0L & new_vs$f_failed >= stop_at
new_vs$notify <- rep(FALSE, nrow(new_vs))
new_vs$eval_error <- rep(FALSE, nrow(new_vs))
new_vs$eval_warning <- rep(FALSE, nrow(new_vs))
new_vs$eval_error <- real_eval_error
new_vs$eval_warning <- real_eval_warning
real_agent$validation_set <- new_vs
real_agent$extracts <- new_extracts
return(real_agent)
Expand Down Expand Up @@ -180,7 +181,7 @@ mark_agent_interrogated <- function(agent) {
# class (print dispatches to print.datadiff_report) and stash what is needed to
# render the full report, plus a (reference-semantics) environment for caching.
as_datadiff_report <- function(reponse, coverage, label, lang, locale,
warn_at = 1e-14, stop_at = 1e-14) {
warn_at = datadiff_default_warn_at, stop_at = datadiff_default_stop_at) {
attr(reponse, "datadiff_coverage") <- coverage
attr(reponse, "datadiff_label") <- label
attr(reponse, "datadiff_lang") <- lang
Expand All @@ -205,8 +206,8 @@ datadiff_render_report <- function(x) {
label = attr(x, "datadiff_label") %||% "datadiff report",
lang = attr(x, "datadiff_lang") %||% "fr",
locale = attr(x, "datadiff_locale") %||% "fr_FR",
warn_at = attr(x, "datadiff_warn_at") %||% 1e-14,
stop_at = attr(x, "datadiff_stop_at") %||% 1e-14,
warn_at = attr(x, "datadiff_warn_at") %||% datadiff_default_warn_at,
stop_at = attr(x, "datadiff_stop_at") %||% datadiff_default_stop_at,
real_agent = x
)
)
Expand Down Expand Up @@ -251,16 +252,18 @@ datadiff_report_html <- function(res, file = NULL) {
stop("`res` has no `coverage`; was it produced by compare_datasets_from_yaml()?")
}
reponse <- res$reponse
agent <- build_report_agent(
coverage = coverage,
label = attr(reponse, "datadiff_label") %||% "datadiff report",
lang = attr(reponse, "datadiff_lang") %||% "fr",
locale = attr(reponse, "datadiff_locale") %||% "fr_FR",
warn_at = attr(reponse, "datadiff_warn_at") %||% 1e-14,
stop_at = attr(reponse, "datadiff_stop_at") %||% 1e-14,
real_agent = reponse
)
report <- pointblank::get_agent_report(agent)
report <- if (inherits(reponse, "datadiff_report")) {
# Share the print() memoization: read the cached report when a print
# already built it, and feed the cache otherwise
datadiff_render_report(reponse)
} else {
# No lazy-report wrapper (hand-assembled res): one-shot synthetic build
pointblank::get_agent_report(build_report_agent(
coverage = coverage,
label = "datadiff report",
real_agent = reponse
))
}
if (!is.null(file)) {
pointblank::export_report(report, filename = file, quiet = TRUE)
}
Expand Down
10 changes: 5 additions & 5 deletions R/tolerance.R
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ add_ok_columns <- function(cmp, tol_cols, col_rules, ref_suffix, na_equal) {
na_equal = na_equal
)
}
names(ok_cols) <- paste0(tol_cols, "__ok")
names(ok_cols) <- datadiff_ok_col(tol_cols)
cbind(cmp, list2DF(ok_cols))
}

Expand Down Expand Up @@ -204,15 +204,15 @@ add_bool_cols_sql <- function(cmp, tol_cols, eq_cols, col_rules, ref_suffix,
rt <- col_rules[[c]][["rel"]] %||% 0
within <- sprintf("ABS(%s - %s) <= (%s + %s * ABS(%s)) + %s * ABS(%s)",
cc, rc, num(at), num(rt), rc, num(fp_eps), rc)
sprintf("%s AS %s", case_tol(cc, rc, cond = within), q(paste0(c, "__ok")))
sprintf("%s AS %s", case_tol(cc, rc, cond = within), q(datadiff_ok_col(c)))
}, FUN.VALUE = character(1), USE.NAMES = FALSE)

eq_exprs <- vapply(X = eq_cols, FUN = function(c) {
cc <- q(c)
rc <- q(paste0(c, ref_suffix))
sprintf("%s AS %s",
case_eq(cc, rc, nan_aware = c %in% eq_num_cols),
q(paste0(c, "__eq")))
q(datadiff_eq_col(c)))
}, FUN.VALUE = character(1), USE.NAMES = FALSE)

exprs <- c(tol_exprs, eq_exprs)
Expand Down Expand Up @@ -258,7 +258,7 @@ add_tolerance_columns <- function(cmp, tol_cols, col_rules, ref_suffix, na_equal
rc_sym <- dplyr::sym(reference_c)
absdiff_col <- paste0(c, "__absdiff")
thresh_col <- paste0(c, "__thresh")
ok_col <- paste0(c, "__ok")
ok_col <- datadiff_ok_col(c)
absdiff_sym <- dplyr::sym(absdiff_col)
thresh_sym <- dplyr::sym(thresh_col)

Expand Down Expand Up @@ -323,7 +323,7 @@ add_tolerance_columns <- function(cmp, tol_cols, col_rules, ref_suffix, na_equal

names(absdiff_cols) <- paste0(tol_cols, "__absdiff")
names(thresh_cols) <- paste0(tol_cols, "__thresh")
names(ok_cols) <- paste0(tol_cols, "__ok")
names(ok_cols) <- datadiff_ok_col(tol_cols)

cbind(cmp, list2DF(c(absdiff_cols, thresh_cols, ok_cols)))
}
Loading
Loading