Skip to content

fix: setup_pointblank_agent - cols_reference deprecie, steps egalite sur booleen derive #52

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
VincentGuyader merged 1 commit into from
Jul 3, 2026
Merged

fix: setup_pointblank_agent - cols_reference deprecie, steps egalite sur booleen derive #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,17 @@

## Bug fixes

* `setup_pointblank_agent()` cleanup: the `cols_reference` argument was never
read (verified by grep since its introduction) and is now deprecated
(warning when supplied, removal planned); the local equality steps validate
a derived `<col>__eq` boolean with the shared NA semantics instead of
embedding the whole reference vector in each step (O(rows) per step,
serialised with the report, and unable to express the one-sided/two-sided
NA distinction); the roxygen example is now executable and representative
(it used to target a `__ok` column that did not exist); `get_col_names()`
is hoisted out of the per-column loop; the dead `cols_reference`/
`cols_candidate` locals of the caller are gone (issue #25).

* `equal_mode: normalized` now does what the vignette always said: it implies
`case_insensitive = TRUE` and `trim = TRUE` for character columns unless
those flags are set explicitly (an explicit value wins over the mode). It
Expand Down
23 changes: 10 additions & 13 deletions R/compare_datasets_from_yaml.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,8 +540,6 @@ compare_datasets_from_yaml <- function(data_reference,

# Analyze columns
col_analysis <- analyze_columns(data_reference, data_candidate, ignore_columns = ignore_columns)
cols_reference <- col_analysis$cols_reference
cols_candidate <- col_analysis$cols_candidate
missing_in_candidate <- col_analysis$missing_in_candidate
extra_in_candidate <- col_analysis$extra_in_candidate
common_cols <- col_analysis$common_cols
Expand Down Expand Up @@ -864,17 +862,16 @@ compare_datasets_from_yaml <- function(data_reference,
}
agent <- setup_pointblank_agent(
cmp_for_agent,
cols_reference,
fail$eq,
fail$tol,
row_validation_info,
ref_suffix,
warn_at,
stop_at,
label,
na_equal,
lang,
locale,
common_cols = fail$eq,
tol_cols = fail$tol,
row_validation_info = row_validation_info,
ref_suffix = ref_suffix,
warn_at = warn_at,
stop_at = stop_at,
label = label,
na_equal = na_equal,
lang = lang,
locale = locale,
missing_in_candidate = missing_in_candidate,
type_mismatch_cols = type_mismatch_cols,
add_col_exists_steps = FALSE
Expand Down
85 changes: 63 additions & 22 deletions R/pointblank_setup.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,19 @@
#' Creates and configures a pointblank validation agent with all necessary validation steps
#' including column existence checks, exact value comparisons, and tolerance validations.
#'
#' The equality steps validate a `<col>__eq` boolean (one-sided NA fails,
#' two-sided NA follows `na_equal`, matching the tolerance kernel and the lazy
#' SQL): when `cmp` does not already carry it, it is derived on the fly for a
#' local data.frame. Tolerance steps expect the `<col>__ok` booleans to be
#' precomputed (see [add_tolerance_columns()]). A lazy `cmp` must carry every
#' boolean column already.
#'
#' @param cmp Comparison dataframe with candidate and reference data
#' @param cols_reference Character vector of reference column names
#' @param common_cols Character vector of columns present in both datasets
#' @param cols_reference Deprecated and unused; supplying any non-NULL value
#' raises a warning (an explicit NULL is silent) and the argument will be
#' removed in a future release
#' @param common_cols Character vector of equality columns to validate (the
#' internal pipeline passes only the failing ones on its failure path)
#' @param tol_cols Character vector of columns with tolerance rules
#' @param row_validation_info List with row validation information from validate_row_counts
#' @param ref_suffix Suffix for reference columns
Expand All @@ -29,21 +39,36 @@
#' data columns.
#' @return Configured pointblank agent ready for interrogation
#' @examples
#' cmp <- data.frame(a = 1:3, a__reference = 1:3, b = c(1.1, 2.2, 3.3),
#' b__reference = c(1.0, 2.0, 3.0))
#' cmp <- data.frame(
#' a = 1:3, a__reference = 1:3,
#' b = c(1.1, 2.2, 3.3), b__reference = c(1.0, 2.0, 3.0),
#' b__ok = c(FALSE, FALSE, FALSE)
#' )
#' row_info <- list(check_count = FALSE)
#' setup_pointblank_agent(cmp, c("a", "b"), c("a", "b"), "b", row_info,
#' "__reference", 0.1, 0.1, "Test", TRUE)
#' agent <- setup_pointblank_agent(
#' cmp,
#' common_cols = "a", tol_cols = "b", row_validation_info = row_info,
#' ref_suffix = "__reference", warn_at = 0.1, stop_at = 0.1,
#' label = "Test", na_equal = TRUE
#' )
#' pointblank::all_passed(pointblank::interrogate(agent))
#' @importFrom pointblank create_agent col_exists col_vals_equal action_levels
#' @importFrom tidyselect all_of
#' @importFrom rlang :=
#' @export
setup_pointblank_agent <- function(cmp, cols_reference, common_cols, tol_cols,
setup_pointblank_agent <- function(cmp, cols_reference = NULL, common_cols, tol_cols,
row_validation_info = NULL, ref_suffix, warn_at, stop_at, label,
na_equal, lang = "fr", locale = "fr_FR",
missing_in_candidate = character(0),
type_mismatch_cols = character(0),
add_col_exists_steps = TRUE) {
if (!is.null(cols_reference)) {
warning(
"The 'cols_reference' argument of setup_pointblank_agent() is deprecated and unused; it will be removed in a future release.",
call. = FALSE
)
}

# 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) {
Expand All @@ -66,6 +91,25 @@ setup_pointblank_agent <- function(cmp, cols_reference, common_cols, tol_cols,
}
}

# Derive the missing <col>__eq booleans BEFORE creating the agent, so every
# equality step validates the same boolean the verdict logic uses (shared NA
# semantics) instead of embedding the whole reference vector in the step
# (O(n_rows) per step, serialised with the report, and na_pass cannot
# express the one-sided/two-sided NA distinction).
eq_cols <- setdiff(x = common_cols, y = tol_cols)
if (!is_non_local(cmp)) {
for (c in eq_cols) {
eq_col <- datadiff_eq_col(c)
if (!(eq_col %in% get_col_names(cmp))) {
cmp[[eq_col]] <- eq_col_bool(
cmp, col = c, ref_suffix = ref_suffix, na_equal = na_equal
)
}
}
}
# Hoisted once, AFTER every cmp mutation above
col_names <- get_col_names(cmp)

agent <- create_agent(tbl = cmp, label = label,
actions = action_levels(warn_at = warn_at, stop_at = stop_at),
lang = lang,
Expand Down Expand Up @@ -105,22 +149,19 @@ 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 <- datadiff_eq_col(c)
if (eq_precomputed %in% get_col_names(cmp)) {
# Lazy path: use pre-computed boolean equality column
agent <- agent %>%
col_vals_equal(columns = all_of(eq_precomputed), value = TRUE, na_pass = FALSE)
} else {
# Local path: compare directly against reference column values
agent <- agent %>%
col_vals_equal(
columns = all_of(c),
value = cmp[[paste0(c, ref_suffix)]],
na_pass = na_equal
)
eq_col <- datadiff_eq_col(c)
if (!(eq_col %in% col_names)) {
# Only reachable with a lazy cmp missing its precomputed boolean:
# the documented contract requires it (deriving it here would need
# the raw columns, absent from the slim lazy table)
stop(sprintf(
"equality boolean column '%s' missing from cmp (a lazy cmp must carry the precomputed __eq columns)",
eq_col
), call. = FALSE)
}
agent <- agent %>%
col_vals_equal(columns = all_of(eq_col), value = TRUE, na_pass = FALSE)
}

for (c in tol_cols) {
Expand All @@ -130,7 +171,7 @@ setup_pointblank_agent <- function(cmp, cols_reference, common_cols, tol_cols,

# Add row count validation if needed
# Use colnames() instead of names() to work correctly with lazy tables
if (!is.null(row_validation_info) && isTRUE(row_validation_info$check_count) && "row_count_ok" %in% get_col_names(cmp)) {
if (!is.null(row_validation_info) && isTRUE(row_validation_info$check_count) && "row_count_ok" %in% col_names) {
agent <- agent %>% col_vals_equal(columns = all_of("row_count_ok"), value = TRUE, na_pass = FALSE)
}

Expand Down
33 changes: 26 additions & 7 deletions man/setup_pointblank_agent.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

73 changes: 61 additions & 12 deletions tests/testthat/test-pointblank-setup.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@ test_that("setup_pointblank_agent creates valid agent", {
text__reference = c("a", "b", "c")
)

cols_reference <- c("id", "value", "text")
common_cols <- c("id", "value", "text")
tol_cols <- c("value")
rules <- list()
ref_suffix <- "__reference"

# Test agent creation
agent <- setup_pointblank_agent(cmp, cols_reference, common_cols, tol_cols, rules, ref_suffix,
agent <- setup_pointblank_agent(cmp, common_cols = common_cols, tol_cols = tol_cols,
row_validation_info = rules, ref_suffix = ref_suffix,
warn_at = 0.1, stop_at = 0.1, label = "Test", na_equal = TRUE)

# Check that agent was created
Expand All @@ -40,13 +40,13 @@ test_that("setup_pointblank_agent handles different column types", {
text__reference = c("a", "b")
)

cols_reference <- c("id", "text")
common_cols <- c("id", "text")
tol_cols <- character(0) # No tolerance columns
rules <- list()
ref_suffix <- "__reference"

agent <- setup_pointblank_agent(cmp, cols_reference, common_cols, tol_cols, rules, ref_suffix,
agent <- setup_pointblank_agent(cmp, common_cols = common_cols, tol_cols = tol_cols,
row_validation_info = rules, ref_suffix = ref_suffix,
warn_at = 0.5, stop_at = 0.5, label = "Test Exact", na_equal = FALSE)

expect_s3_class(agent, "ptblank_agent")
Expand All @@ -57,7 +57,8 @@ test_that("setup_pointblank_agent handles empty inputs", {
# Test with minimal data
cmp <- data.frame(id = 1, id__reference = 1)

agent <- setup_pointblank_agent(cmp, c("id"), c("id"), character(0), list(), "__reference",
agent <- setup_pointblank_agent(cmp, common_cols = c("id"), tol_cols = character(0),
row_validation_info = list(), ref_suffix = "__reference",
warn_at = 1.0, stop_at = 1.0, label = "Minimal", na_equal = TRUE)

expect_s3_class(agent, "ptblank_agent")
Expand All @@ -67,7 +68,8 @@ test_that("setup_pointblank_agent handles empty inputs", {
test_that("setup_pointblank_agent uses French language by default", {
cmp <- data.frame(id = 1, id__reference = 1)

agent <- setup_pointblank_agent(cmp, c("id"), c("id"), character(0), list(), "__reference",
agent <- setup_pointblank_agent(cmp, common_cols = c("id"), tol_cols = character(0),
row_validation_info = list(), ref_suffix = "__reference",
warn_at = 0.1, stop_at = 0.1, label = "Test", na_equal = TRUE)

expect_s3_class(agent, "ptblank_agent")
Expand All @@ -79,7 +81,8 @@ test_that("setup_pointblank_agent accepts custom language parameters", {
cmp <- data.frame(id = 1, id__reference = 1)

# Test English
agent_en <- setup_pointblank_agent(cmp, c("id"), c("id"), character(0), list(), "__reference",
agent_en <- setup_pointblank_agent(cmp, common_cols = c("id"), tol_cols = character(0),
row_validation_info = list(), ref_suffix = "__reference",
warn_at = 0.1, stop_at = 0.1, label = "Test EN",
na_equal = TRUE, lang = "en", locale = "en_US")

Expand All @@ -88,7 +91,8 @@ test_that("setup_pointblank_agent accepts custom language parameters", {
expect_equal(agent_en$locale, "en_US")

# Test German
agent_de <- setup_pointblank_agent(cmp, c("id"), c("id"), character(0), list(), "__reference",
agent_de <- setup_pointblank_agent(cmp, common_cols = c("id"), tol_cols = character(0),
row_validation_info = list(), ref_suffix = "__reference",
warn_at = 0.1, stop_at = 0.1, label = "Test DE",
na_equal = TRUE, lang = "de", locale = "de_DE")

Expand All @@ -101,21 +105,24 @@ test_that("setup_pointblank_agent accepts various supported languages", {
cmp <- data.frame(id = 1, id__reference = 1)

# Test Spanish
agent_es <- setup_pointblank_agent(cmp, c("id"), c("id"), character(0), list(), "__reference",
agent_es <- setup_pointblank_agent(cmp, common_cols = c("id"), tol_cols = character(0),
row_validation_info = list(), ref_suffix = "__reference",
warn_at = 0.1, stop_at = 0.1, label = "Test ES",
na_equal = TRUE, lang = "es", locale = "es_ES")
expect_equal(agent_es$lang, "es")
expect_equal(agent_es$locale, "es_ES")

# Test Portuguese
agent_pt <- setup_pointblank_agent(cmp, c("id"), c("id"), character(0), list(), "__reference",
agent_pt <- setup_pointblank_agent(cmp, common_cols = c("id"), tol_cols = character(0),
row_validation_info = list(), ref_suffix = "__reference",
warn_at = 0.1, stop_at = 0.1, label = "Test PT",
na_equal = TRUE, lang = "pt", locale = "pt_BR")
expect_equal(agent_pt$lang, "pt")
expect_equal(agent_pt$locale, "pt_BR")

# Test Italian
agent_it <- setup_pointblank_agent(cmp, c("id"), c("id"), character(0), list(), "__reference",
agent_it <- setup_pointblank_agent(cmp, common_cols = c("id"), tol_cols = character(0),
row_validation_info = list(), ref_suffix = "__reference",
warn_at = 0.1, stop_at = 0.1, label = "Test IT",
na_equal = TRUE, lang = "it", locale = "it_IT")
expect_equal(agent_it$lang, "it")
Expand All @@ -135,7 +142,6 @@ test_that("setup_pointblank_agent handles lazy table with missing_in_candidate a
# Passing a lazy table exercises the dplyr::mutate() path (lines 52 and 63)
agent <- setup_pointblank_agent(
lazy_tbl,
cols_reference = character(0),
common_cols = character(0),
tol_cols = c("val"),
row_validation_info = list(check_count = FALSE),
Expand All @@ -150,3 +156,46 @@ test_that("setup_pointblank_agent handles lazy table with missing_in_candidate a

expect_s3_class(agent, "ptblank_agent")
})

test_that("cols_reference is deprecated and unused", {
cmp <- data.frame(a = 1:3, a__reference = 1:3)
row_info <- list(check_count = FALSE)
expect_warning(
setup_pointblank_agent(cmp, cols_reference = c("a"), common_cols = "a",
tol_cols = character(0), row_validation_info = row_info,
ref_suffix = "__reference", warn_at = 0.1, stop_at = 0.1,
label = "x", na_equal = TRUE),
regexp = "deprecated"
)
# Omitting it works, produces the same agent shape and stays silent
expect_no_warning(
agent <- setup_pointblank_agent(cmp, common_cols = "a",
tol_cols = character(0), row_validation_info = row_info,
ref_suffix = "__reference", warn_at = 0.1, stop_at = 0.1,
label = "x", na_equal = TRUE)
)
expect_s3_class(agent, "ptblank_agent")
})

test_that("direct equality steps use the shared NA semantics via a derived boolean", {
# One-sided NA with na_equal = TRUE: must FAIL (kernel/lazy semantics),
# not pass through col_vals_equal's na_pass
cmp <- data.frame(
s = c("a", NA, "c"),
s__reference = c("a", "b", "c"),
stringsAsFactors = FALSE
)
row_info <- list(check_count = FALSE)
agent <- setup_pointblank_agent(cmp, common_cols = "s",
tol_cols = character(0), row_validation_info = row_info,
ref_suffix = "__reference", warn_at = 0.1, stop_at = 0.1,
label = "na semantics", na_equal = TRUE)
reponse <- pointblank::interrogate(agent)
expect_false(pointblank::all_passed(reponse))

# The step no longer embeds the reference VECTOR in the agent: the equality
# is validated through a derived __eq boolean column
vs <- reponse$validation_set
step_cols <- vapply(vs$column, function(cc) cc[1], character(1))
expect_true("s__eq" %in% step_cols)
})
Loading