From 22c2478634902092dd082d4d944f78e35e2dccee Mon Sep 17 00:00:00 2001 From: Jared Knowles Date: Mon, 3 Aug 2026 11:36:18 -0400 Subject: [PATCH] fix(balances): validate the full signature, surface caveats in cog_explain, memoise coverage windows (#25) Final-review findings F-1, F-2, F-6, F-8 (plus the F-9 @return reword, which shares R/balances.R). F-2: .validate_balance_inputs() checked 2 of cog_balances()' 7 arguments. years = integer(0) leaked a raw DuckDB 'Parser Error ... AND year IN ()' with the generated SQL echoed back; govid = character(0) and a non-character category returned 0 rows with no error at all; recipe = c("a","b") threw 'the condition has length > 1' from inside .validate_recipe_id(). Replaced with a call to the money verbs' own .validate_verb_inputs() (R/spending.R), which validates the exact superset needed. Deleted the local copy rather than extending it -- two validators is how they drift. Placed AFTER .coerce_govid_input(), because .validate_verb_inputs() asserts is.character(govid) and a data-frame govid is not unwrapped before that. This is helper reuse of the same kind as .build_verb_sql()/.attach_per_capita(); the verb still does NOT route through .verb_spendrev(). F-1: falls out of F-2 for free -- the recipe/category mutual-exclusivity guard lives inside .validate_verb_inputs(). Previously recipe silently discarded category AND overwrote provenance$category with the recipe label, so a caller asking for Fund Balances got X40/Z77 insurance-trust holdings with no trace of the dropped filter. F-6: cog_explain() rendered every provenance caveat block except balance_caveats. Since .emit_balance_caveats() fires at most once per session -- and is routinely consumed by a suppressMessages() call or an unread knitr chunk -- cog_explain() is the only surface left for a caller who deliberately audits the result. Added a 'Holdings caveats' section guarded on !is.null(prov$balance_caveats). Also relabels the cosmetic 'Concept: NA' line on balance results as 'not applicable (holdings are a stock, not a flow)'. F-8: the coverage-window query has no govid and no year predicate -- its answer depends only on the mounted corpus -- yet it scanned all of balance_long on every call (35% of verb runtime on the fixture, and a per-request throughput ceiling for cog-api#26). Memoised in .uscogdata_env$balance_coverage_windows, invalidated by cog_close(), the same pattern as .uscogdata_env$manifest. --- R/balance_caveats.R | 55 ++++++++++++++++++++++++++++++++------------- R/balances.R | 44 +++++++++++++++++------------------- R/explain.R | 30 +++++++++++++++++++++++++ R/session.R | 2 ++ man/cog_balances.Rd | 13 ++++++----- 5 files changed, 99 insertions(+), 45 deletions(-) diff --git a/R/balance_caveats.R b/R/balance_caveats.R index 711f66f..dc25401 100644 --- a/R/balance_caveats.R +++ b/R/balance_caveats.R @@ -17,16 +17,7 @@ #' truncated relative to the requested span. #' @noRd .balance_caveats <- function(con, codes_observed, years) { - windows <- DBI::dbGetQuery(con, - "SELECT c.balance_subtype AS subtype, - MIN(l.year) AS year_min, - MAX(l.year) AS year_max - FROM balance_long l - JOIN summary_categories c USING (item_code) - WHERE c.balance_subtype IS NOT NULL - GROUP BY 1 - ORDER BY 1" - ) + cw <- .balance_coverage_windows(con) observed_subtypes <- if (length(codes_observed) == 0L) { character(0) @@ -38,12 +29,6 @@ ))$balance_subtype } - cw <- stats::setNames( - lapply(seq_len(nrow(windows)), - function(i) as.integer(c(windows$year_min[i], windows$year_max[i]))), - windows$subtype - ) - # A family is "truncated" when the caller asked for years outside the span # that family actually covers -- the FY2016 employee-retirement termination # and the FY2021 end of the W family are both this shape. @@ -68,6 +53,44 @@ ) } +#' Per-subtype [min year, max year] extents for EVERY balance subtype in the +#' mounted corpus, memoised for the session. +#' +#' The query carries no govid and no year predicate -- its answer is a property +#' of the mounted corpus alone and cannot change between calls -- but it scans +#' the whole of `balance_long`, which measured 35% of `cog_balances()` runtime +#' on the bundled fixture and would be a per-request throughput ceiling once +#' cog-api#26 serves this verb over HTTP. Memoised in `.uscogdata_env` and +#' invalidated by `cog_close()`, the same pattern as `.uscogdata_env$manifest`. +#' +#' Scope is deliberately corpus-wide rather than query-scoped: a caller asking +#' "is there a family I missed?" needs every window. The observed-scoped field +#' is `truncated`. Documented as such in inst/schemas/provenance-v1.json. +#' @noRd +.balance_coverage_windows <- function(con) { + cached <- .uscogdata_env$balance_coverage_windows + if (!is.null(cached)) return(cached) + + windows <- DBI::dbGetQuery(con, + "SELECT c.balance_subtype AS subtype, + MIN(l.year) AS year_min, + MAX(l.year) AS year_max + FROM balance_long l + JOIN summary_categories c USING (item_code) + WHERE c.balance_subtype IS NOT NULL + GROUP BY 1 + ORDER BY 1" + ) + + cw <- stats::setNames( + lapply(seq_len(nrow(windows)), + function(i) as.integer(c(windows$year_min[i], windows$year_max[i]))), + windows$subtype + ) + .uscogdata_env$balance_coverage_windows <- cw + cw +} + #' TRUE the first time `key` is seen this session, FALSE thereafter. #' Reset by cog_close(). #' @noRd diff --git a/R/balances.R b/R/balances.R index 5f755f3..4a9567d 100644 --- a/R/balances.R +++ b/R/balances.R @@ -44,14 +44,17 @@ #' @return Tibble with columns `year`, `canonical_govid`, `gov_name`, #' `balance_subtype`, `category`, `amt_nominal`, `codes_included`, #' `aggregate_fallback`, plus optional `amt_per_capita_nominal` and -#' `pop_source` (when `per_capita = TRUE`), and optional `amt_real` and -#' `amt_per_capita_real` (when `adjust_to_year` is set). Amounts are full -#' US dollars. +#' `pop_source` (when `per_capita = TRUE`), optional `amt_real` (when +#' `adjust_to_year` is set), and optional `amt_per_capita_real` (only when +#' **both** `per_capita = TRUE` and `adjust_to_year` are set -- there is no +#' nominal per-capita column to deflate otherwise). Amounts are full US +#' dollars. #' #' Carries a `provenance` attribute matching #' `inst/schemas/provenance-v1.json`, whose `balance_caveats` block reports -#' `not_gaap`, `not_gaap_note`, `coverage_window` (measured per-subtype year -#' extents) and `truncated` (subtypes whose coverage falls short of the +#' `not_gaap`, `not_gaap_note`, `coverage_window` (measured year extents for +#' every balance subtype in the mounted corpus, not only the observed ones) +#' and `truncated` (the observed subtypes whose coverage falls short of the #' requested years). `expenditure_concept`/`revenue_concept` are `NA` -- #' holdings are a stock, not a flow, so neither concept vocabulary applies. #' @export @@ -60,8 +63,19 @@ cog_balances <- function(govid, years, category = NULL, basis = c("harmonized", "raw"), recipe = NULL) { call <- match.call() basis <- match.arg(basis, c("harmonized", "raw")) - .validate_balance_inputs(per_capita, adjust_to_year) + # Coerce FIRST, validate second: .validate_verb_inputs() asserts + # is.character(govid), and a data-frame govid (cog_gov_search() output) has + # not been unwrapped yet at this point. govid <- .coerce_govid_input(govid) + # The money verbs' validator, reused rather than re-implemented (R/spending.R). + # It covers the exact superset cog_balances() needs -- including the + # recipe/category mutual-exclusivity guard -- so a second local copy would + # only be a place for the two to drift apart. This is the same kind of + # helper reuse as .build_verb_sql()/.attach_per_capita() below; it does NOT + # route the verb through .verb_spendrev(), which stays deliberately unused + # here because its flow vocabulary is meaningless for a stock. + .validate_verb_inputs(govid, years, category, per_capita, adjust_to_year, + recipe) years <- as.integer(years) if (!is.null(adjust_to_year)) adjust_to_year <- as.integer(adjust_to_year) @@ -129,24 +143,6 @@ cog_balances <- function(govid, years, category = NULL, result } -#' Cheap type validation for the two arguments cog_balances() shares with the -#' money verbs. Mirrors the per_capita/adjust_to_year checks in -#' .validate_verb_inputs() (R/spending.R) -- category/recipe validation is -#' deliberately out of scope here (uscogdata#25 Task 3 review note). -#' @noRd -.validate_balance_inputs <- function(per_capita, adjust_to_year) { - if (!is.logical(per_capita) || length(per_capita) != 1L) { - cli::cli_abort("`per_capita` must be a length-1 logical.") - } - if (!is.null(adjust_to_year)) { - if (!(is.integer(adjust_to_year) || is.numeric(adjust_to_year)) || - length(adjust_to_year) != 1L) { - cli::cli_abort("`adjust_to_year` must be NULL or a length-1 integer.") - } - } - invisible(TRUE) -} - #' Abort unless the mounted corpus classifies balance codes. #' #' `balance_subtype` arrived with cog_pipeline #76/#77 without a diff --git a/R/explain.R b/R/explain.R index 5309e97..37380c4 100644 --- a/R/explain.R +++ b/R/explain.R @@ -68,6 +68,11 @@ cog_explain <- function(result, format = c("print", "list")) { if (!is.null(prov$revenue_concept)) { cli::cli_text("Concept: {prov$revenue_concept} revenue") } + } else if (identical(prov$verb, "cog_balances")) { + # Both concept fields are deliberately NA here (a stock has no flow + # concept). Printing the raw NA reads as a missing value rather than an + # intentional one, so say what it means instead. + cli::cli_text("Concept: not applicable (holdings are a stock, not a flow)") } else if (!is.null(prov$expenditure_concept)) { concept_note <- if (!is.null(prov$expenditure_concept_note) && !is.na(prov$expenditure_concept_note)) { @@ -174,6 +179,31 @@ cog_explain <- function(result, format = c("print", "list")) { cli::cli_ul(.series_break_story_lines(prov$corpus_break_refs)) } + # Balance results only (NULL on money-verb provenance, so they are + # unaffected). This is the ONLY on-demand surface for the GAAP disclosure: + # .emit_balance_caveats() fires at most once per session, and is routinely + # consumed by a suppressMessages() call or by a knitted chunk nobody reads, + # so a caller who deliberately audits a result with cog_explain() must still + # be told. + bc <- prov$balance_caveats + if (!is.null(bc)) { + cli::cli_h2("Holdings caveats") + if (!is.null(bc$not_gaap_note)) cli::cli_alert_warning(bc$not_gaap_note) + if (length(bc$truncated) > 0L) { + cli::cli_text( + "Requested years extend beyond what these families actually cover:" + ) + cli::cli_ul(vapply(bc$truncated, function(s) { + w <- bc$coverage_window[[s]] + if (length(w) == 2L) { + sprintf("%s: covered %s-%s in this corpus", s, w[1], w[2]) + } else { + s + } + }, character(1))) + } + } + cli::cli_h2("Transformations") uc <- prov$transformations$units_conversion if (isTRUE(uc$applied)) { diff --git a/R/session.R b/R/session.R index 8f55887..ef1c28c 100644 --- a/R/session.R +++ b/R/session.R @@ -96,4 +96,6 @@ cog_close <- function() { .uscogdata_env$con <- NULL .uscogdata_env$manifest <- NULL .uscogdata_env$balance_caveats_shown <- NULL + # Memoised corpus-constant; a different corpus may be mounted next. + .uscogdata_env$balance_coverage_windows <- NULL } diff --git a/man/cog_balances.Rd b/man/cog_balances.Rd index 9b96b97..f594eba 100644 --- a/man/cog_balances.Rd +++ b/man/cog_balances.Rd @@ -49,14 +49,17 @@ wide era to the modern one.} Tibble with columns `year`, `canonical_govid`, `gov_name`, `balance_subtype`, `category`, `amt_nominal`, `codes_included`, `aggregate_fallback`, plus optional `amt_per_capita_nominal` and - `pop_source` (when `per_capita = TRUE`), and optional `amt_real` and - `amt_per_capita_real` (when `adjust_to_year` is set). Amounts are full - US dollars. + `pop_source` (when `per_capita = TRUE`), optional `amt_real` (when + `adjust_to_year` is set), and optional `amt_per_capita_real` (only when + **both** `per_capita = TRUE` and `adjust_to_year` are set -- there is no + nominal per-capita column to deflate otherwise). Amounts are full US + dollars. Carries a `provenance` attribute matching `inst/schemas/provenance-v1.json`, whose `balance_caveats` block reports - `not_gaap`, `not_gaap_note`, `coverage_window` (measured per-subtype year - extents) and `truncated` (subtypes whose coverage falls short of the + `not_gaap`, `not_gaap_note`, `coverage_window` (measured year extents for + every balance subtype in the mounted corpus, not only the observed ones) + and `truncated` (the observed subtypes whose coverage falls short of the requested years). `expenditure_concept`/`revenue_concept` are `NA` -- holdings are a stock, not a flow, so neither concept vocabulary applies. }