Skip to content

Commit 7cba453

Browse files
Merge pull request #62 from ThinkR-open/fix/keys-empty-list
fix: keys: [] dans un fichier de regles vaut absence de cle
2 parents 1a6d32b + 96bd082 commit 7cba453

6 files changed

Lines changed: 180 additions & 19 deletions

File tree

‎NEWS.md‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,19 @@
1+
# datadiff (development version)
2+
3+
## Bug fixes
4+
5+
* `keys: []` in a rules file was read as an empty key and rejected before the
6+
data was even looked at ("Parameter 'key' must be a non-empty character
7+
vector"), although `[]` is the natural spelling of "no key" in a hand-written
8+
YAML. `keys: []` and the legacy `key: []` now mean "no key", exactly like an
9+
absent field or `keys: ~`: the comparison is positional. A present `keys`
10+
field now wins over a legacy `key` field whatever its value: `keys: ~` next
11+
to `key: [id]` used to select `id`, it means "no key" like `keys: []` does.
12+
The `key` argument
13+
of `compare_datasets_from_yaml()` and `write_rules_template()` is unchanged:
14+
an empty vector there is still an error, `NULL` is the way to ask for a
15+
positional comparison from R code (issue #59).
16+
117
# datadiff 0.6.0
218

319
## New features

‎R/compare_datasets_from_yaml.R‎

Lines changed: 45 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@
66
#'
77
#' @param data_reference A dataframe or tibble used as reference for rule generation
88
#' @param key Character vector specifying column name(s) to use as join key(s) for data
9-
#' comparison. If NULL, comparison is positional (row by row).
9+
#' comparison. `NULL` (the default) writes `keys: ~` and means a positional
10+
#' comparison (row by row); an empty vector is an error.
1011
#' @param label Descriptive label for the validation report
1112
#' @param path Character string specifying the output YAML file path (default: "rules.yaml")
1213
#' @param version Numeric version of the rules format (default: 1)
@@ -226,6 +227,27 @@ validate_duckdb_memory_limit <- function(duckdb_memory_limit) {
226227
invisible(NULL)
227228
}
228229

230+
#' Read the join key declared in a rules file
231+
#'
232+
#' In a YAML file, "no key" has several spellings: an absent field, `keys: ~`,
233+
#' or `keys: []`, which `yaml::read_yaml()` reads as `list()` and not as
234+
#' `NULL`. All of them resolve to `NULL` here. Values are coerced to character,
235+
#' YAML being stringly typed (`keys: [2024]` designates the column "2024").
236+
#'
237+
#' @param value The `keys` (or legacy `key`) field as read from the YAML.
238+
#' @return A non-empty character vector, or `NULL` for "no key".
239+
#' @noRd
240+
yaml_key <- function(value) {
241+
if (is.null(value)) {
242+
return(NULL)
243+
}
244+
key <- as.character(unlist(value, use.names = FALSE))
245+
if (length(key) == 0L) {
246+
return(NULL)
247+
}
248+
key
249+
}
250+
229251
#' Validate a comparison key against both datasets
230252
#'
231253
#' Shared guard for every code path that consumes a key: checks the type and
@@ -286,11 +308,15 @@ validate_comparison_key <- function(key, ref_cols, cand_cols) {
286308
#' | report label | `label` | `label` | "Comparing candidate vs reference" |
287309
#'
288310
#' When the YAML contains both `keys` and the legacy singular `key` field,
289-
#' `keys` (the canonical field written by [write_rules_template()]) wins.
311+
#' `keys` (the canonical field written by [write_rules_template()]) wins,
312+
#' whatever its value: `keys: ~` or `keys: []` next to `key: [id]` means "no
313+
#' key", not `id`.
290314
#'
291-
#' The `key` argument must be a character vector (a non-character value is an
292-
#' error), while YAML-sourced key values are coerced to character (YAML being
293-
#' stringly typed, `keys: [2024]` designates the column named "2024").
315+
#' The `key` argument must be a non-empty character vector (a non-character
316+
#' or empty value is an error: use `NULL` for a positional comparison), while
317+
#' YAML-sourced key values are coerced to character (YAML being stringly
318+
#' typed, `keys: [2024]` designates the column named "2024"). In the YAML, an
319+
#' absent `keys` field, `keys: ~` and `keys: []` all mean "no key".
294320
#'
295321
#' Note for `path = NULL`: the auto-generated rules template itself carries a
296322
#' label (`"Comparison with default rules"`, or the explicit `label` argument),
@@ -301,7 +327,9 @@ validate_comparison_key <- function(key, ref_cols, cand_cols) {
301327
#' @param data_reference Reference dataframe, tibble, or lazy table (tbl_lazy)
302328
#' @param data_candidate Candidate dataframe to validate against reference
303329
#' @param key Optional character vector of column names to use as join keys for
304-
#' ordered comparison. Every key column must exist in both datasets; otherwise
330+
#' ordered comparison; `NULL` (the default) defers to the YAML `keys` field,
331+
#' and a positional comparison follows when neither gives a key. An empty
332+
#' vector is an error. Every key column must exist in both datasets; otherwise
305333
#' an error is raised naming the missing column(s) and the dataset(s) concerned.
306334
#' See the "Argument vs YAML precedence" section.
307335
#' @param path Path to YAML file containing validation rules. If NULL, default rules are
@@ -540,12 +568,18 @@ compare_datasets_from_yaml <- function(data_reference,
540568
}
541569

542570
# Precedence: explicit argument > YAML defaults > none. The canonical YAML
543-
# field is "keys" (what write_rules_template() writes); a legacy singular
544-
# "key" field is honored as fallback. [[ avoids $ partial matching.
571+
# field is "keys" (what write_rules_template() writes); the legacy singular
572+
# "key" field is honored only when "keys" is absent from the file. A present
573+
# "keys" wins whatever its value, including the no-key spellings ~ and [],
574+
# hence a presence test rather than %||%, which reads ~ as an absence.
575+
# [[ avoids $ partial matching; NULL[["x"]] is NULL, so a file without a
576+
# defaults section is safe.
545577
if (is.null(key)) {
546-
key <- rules$defaults[["keys"]] %||% rules$defaults[["key"]]
547-
if (!is.null(key)) {
548-
key <- as.character(unlist(key, use.names = FALSE))
578+
defaults <- rules[["defaults"]]
579+
if ("keys" %in% names(defaults)) {
580+
key <- yaml_key(defaults[["keys"]])
581+
} else {
582+
key <- yaml_key(defaults[["key"]])
549583
}
550584
}
551585

‎man/compare_datasets_from_yaml.Rd‎

Lines changed: 12 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎man/write_rules_template.Rd‎

Lines changed: 2 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
# In a rules file, `keys: []` means "no key", exactly like an absent field or
2+
# `keys: ~`: yaml::read_yaml() reads [] as list(), which is not NULL, and must
3+
# not be mistaken for an empty key. The `key` argument, R code, stays strict.
4+
5+
# legacy = TRUE adds a singular `key: [f2]` field, which a present `keys`
6+
# field must beat whatever its value: without it, the no-key spellings could
7+
# not be told apart from one another.
8+
write_rules_with <- function(key_line, legacy = FALSE) {
9+
path <- tempfile(fileext = ".yaml")
10+
writeLines(c(
11+
"version: 1",
12+
"defaults:",
13+
key_line,
14+
if (legacy) " key: [f2]",
15+
" label: essai",
16+
"by_type:",
17+
" character:",
18+
" equal_mode: exact"
19+
), con = path)
20+
path
21+
}
22+
23+
tbl <- data.frame(f2 = "OUI", f3 = "NON", stringsAsFactors = FALSE)
24+
25+
test_that("keys: [] in the rules is read as no key: identical tables pass", {
26+
path <- write_rules_with(" keys: []")
27+
on.exit(unlink(path), add = TRUE)
28+
expect_no_error(res <- compare_datasets_from_yaml(tbl, tbl, path = path))
29+
expect_true(res$all_passed)
30+
})
31+
32+
test_that("keys: [] behaves like keys: ~ and like an absent keys field", {
33+
paths <- c(write_rules_with(" keys: []"), write_rules_with(" keys: ~"),
34+
write_rules_with(" na_equal: yes"))
35+
on.exit(unlink(paths), add = TRUE)
36+
cand <- data.frame(f2 = "OUI", f3 = "non", stringsAsFactors = FALSE)
37+
covs <- lapply(paths, FUN = function(p) compare_datasets_from_yaml(tbl, cand, path = p)$coverage)
38+
expect_false(all(covs[[1]]$n_failed == 0))
39+
expect_identical(covs[[1]], covs[[2]])
40+
expect_identical(covs[[1]], covs[[3]])
41+
})
42+
43+
test_that("keys: [] is positional: every column is compared, unequal counts error", {
44+
path <- write_rules_with(" keys: []")
45+
on.exit(unlink(path), add = TRUE)
46+
res <- compare_datasets_from_yaml(tbl, tbl, path = path)
47+
expect_setequal(res$coverage$column[res$coverage$check == "equality"], c("f2", "f3"))
48+
expect_error(
49+
compare_datasets_from_yaml(tbl, rbind(tbl, tbl), path = path),
50+
regexp = "same number of rows"
51+
)
52+
})
53+
54+
test_that("legacy singular key: [] is read as no key as well", {
55+
path <- write_rules_with(" key: []")
56+
on.exit(unlink(path), add = TRUE)
57+
expect_true(compare_datasets_from_yaml(tbl, tbl, path = path)$all_passed)
58+
})
59+
60+
test_that("a present keys field wins over a legacy key field, whatever its no-key spelling", {
61+
paths <- c(write_rules_with(" keys: []", legacy = TRUE), write_rules_with(" keys: ~", legacy = TRUE))
62+
on.exit(unlink(paths), add = TRUE)
63+
covs <- lapply(paths, FUN = function(p) compare_datasets_from_yaml(tbl, tbl, path = p)$coverage)
64+
for (cov in covs) {
65+
expect_setequal(cov$column[cov$check == "equality"], c("f2", "f3"))
66+
}
67+
expect_identical(covs[[1]], covs[[2]])
68+
})
69+
70+
test_that("an absent keys field still falls back to the legacy key field", {
71+
path <- write_rules_with(" na_equal: yes", legacy = TRUE)
72+
on.exit(unlink(path), add = TRUE)
73+
res <- compare_datasets_from_yaml(tbl, tbl, path = path)
74+
expect_identical(res$coverage$column[res$coverage$check == "equality"], "f3")
75+
})
76+
77+
test_that("lazy path: keys: [] is positional as well", {
78+
skip_if_not_installed("duckdb")
79+
skip_if_not_installed("DBI")
80+
con <- DBI::dbConnect(duckdb::duckdb())
81+
on.exit(DBI::dbDisconnect(con, shutdown = TRUE), add = TRUE)
82+
duckdb::duckdb_register(con, name = "ref_kel", df = tbl)
83+
duckdb::duckdb_register(con, name = "cand_kel", df = tbl)
84+
path <- write_rules_with(" keys: []")
85+
on.exit(unlink(path), add = TRUE)
86+
res <- suppressMessages(
87+
compare_datasets_from_yaml(dplyr::tbl(con, "ref_kel"), dplyr::tbl(con, "cand_kel"), path = path)
88+
)
89+
expect_true(res$all_passed)
90+
expect_setequal(res$coverage$column[res$coverage$check == "equality"], c("f2", "f3"))
91+
})
92+
93+
test_that("the key argument stays strict: an empty vector is an error, not a positional comparison", {
94+
path <- write_rules_with(" keys: [f2]")
95+
on.exit(unlink(path), add = TRUE)
96+
expect_error(
97+
compare_datasets_from_yaml(tbl, tbl, key = character(0), path = path),
98+
regexp = "non-empty character vector"
99+
)
100+
expect_error(
101+
write_rules_template(tbl, key = character(0), path = tempfile(fileext = ".yaml")),
102+
regexp = "non-empty character vector"
103+
)
104+
})

‎vignettes/datadiff.Rmd‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,7 @@ defaults:
255255
ignore_columns: # columns excluded from comparison entirely
256256
- documentation
257257
- updated_at
258-
keys: id # join key (single or composite)
258+
keys: id # join key (single or composite); ~ or [] for none
259259
label: ref vs cand # label shown in the pointblank report
260260

261261
# Note: when a setting exists both as a function argument and in the YAML,

0 commit comments

Comments
 (0)