diff --git a/NEWS.md b/NEWS.md index 31492fc..cd93936 100644 --- a/NEWS.md +++ b/NEWS.md @@ -5,8 +5,9 @@ * `cog_spending()` and `cog_revenue()` accept the reserved category `"All Categories"`, returning one summed row per `(year, canonical_govid, subtype)` across every category inside the - requested concept's subtype scope. Combine with `subtype = "operations"` - for an operating-expenditure total. `cog_geographic_rollup()` inherits it, + requested concept's subtype scope. Filtering the result to + `spend_subtype == "operations"` gives an operating-expenditure total. + `cog_geographic_rollup()` inherits it, which is the efficient way to build a geographic total — previously a caller had to issue one rollup per category and sum the results (cog-api#37). diff --git a/R/balances.R b/R/balances.R index 4a9567d..994e0b2 100644 --- a/R/balances.R +++ b/R/balances.R @@ -28,7 +28,12 @@ #' every combination would be either redundant or empty. #' `category = "Fund Balances"` is exactly the `general` family #' (`W01`/`W31`/`W61`). `balance_subtype` is returned, so a finer split is -#' one `dplyr::filter()` away. +#' one `dplyr::filter()` away. The reserved pseudo-category +#' `"All Categories"` (see [cog_spending()]) is **not** supported here and +#' errors with class `uscogdata_all_categories_unsupported`: it sums a +#' concept's subtype scope, and holdings are a stock with no concept +#' vocabulary to sum across. Omit `category` to get every category broken +#' out instead. #' @param per_capita Divide holdings by population. Note this is a **stock per #' resident** (reserves per person), which is *not* comparable to #' [cog_spending()]'s per-capita figures -- those are a flow per person. @@ -74,6 +79,13 @@ cog_balances <- function(govid, years, category = NULL, # 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. + # + # allow_all_categories is left at its FALSE default (contrast + # .verb_spendrev(), which passes TRUE): the all-categories mode's "sum" + # only means something in terms of a concept's subtype scope, and holdings + # have no concept vocabulary. The reuse above is exactly why this can be a + # one-line default rather than a second bespoke check -- see the + # validator's own doc comment for the incident that made that matter. .validate_verb_inputs(govid, years, category, per_capita, adjust_to_year, recipe) years <- as.integer(years) diff --git a/R/provenance.R b/R/provenance.R index 77cf2d1..55126fc 100644 --- a/R/provenance.R +++ b/R/provenance.R @@ -67,7 +67,15 @@ basis_note = basis_note, expenditure_concept = expenditure_concept, expenditure_concept_note = expenditure_concept_note, - expenditure_concept_direct_suppressed = isTRUE(expenditure_concept_direct_suppressed), + # isTRUE() alone would collapse a deliberate NA (all-categories mode, + # where suppression detection cannot run -- see .verb_spendrev()) down to + # FALSE, turning "we don't know" back into the false claim this field + # exists to avoid. Preserve NA; otherwise normalize to a strict logical. + expenditure_concept_direct_suppressed = if (isTRUE(is.na(expenditure_concept_direct_suppressed))) { + NA + } else { + isTRUE(expenditure_concept_direct_suppressed) + }, revenue_concept = revenue_concept, harmonization = harmonization %||% list( applied = FALSE, na_rows_excluded = 0L, na_amount_excluded = 0, diff --git a/R/spending.R b/R/spending.R index c30fa26..6a692d3 100644 --- a/R/spending.R +++ b/R/spending.R @@ -150,7 +150,12 @@ #' component (when one exists), and #' `provenance$expenditure_concept_direct_suppressed` is `TRUE` -- the #' figure in those rows is the intergovernmental leg alone, not Direct + -#' IG. +#' IG. When `category = "All Categories"` is combined with +#' `expenditure_concept = "total"`, this detection cannot run (it keys on +#' per-category rows, which all-categories mode collapses to one literal +#' value), so `expenditure_concept_direct_suppressed` is `NA` rather than a +#' possibly-false `FALSE`; query an explicit `category` to get a real +#' answer. #' @param complete If `TRUE`, fill the requested grid so that a cell the #' corpus does not carry still appears, labelled with **why** it is #' missing, and add a `value_source` column to every row: @@ -274,8 +279,13 @@ cog_spending <- function(govid, years, category = NULL, } govid <- .coerce_govid_input(govid, arg = "govid") + # allow_all_categories = TRUE: cog_spending()/cog_revenue() are the two + # verbs the reserved pseudo-category is defined for. cog_balances() shares + # this validator but leaves the argument at its FALSE default, so it + # rejects "All Categories" instead of silently returning zero rows + # (finding 3, all-categories review). .validate_verb_inputs(govid, years, category, per_capita, adjust_to_year, - recipe) + recipe, allow_all_categories = TRUE) # Recognize the reserved pseudo-category. Detected after type validation so a # non-character `category` still fails with the ordinary type error. @@ -327,6 +337,12 @@ cog_spending <- function(govid, years, category = NULL, "Use `expenditure_concept = \"direct\"` with `complete = TRUE`, or drop `complete`." ) } + if (complete && all_categories) { + .abort_complete_unsupported( + "`category = \"All Categories\"` collapses the category dimension that `code_set` grids over (see `.completion_grid_sql()`), so there is no per-category grid left to fill -- filling a summed row has no defined semantics.", + "Drop `complete`, or use `complete = TRUE` with an explicit `category` (or `category = NULL` for every category)." + ) + } years <- as.integer(years) if (!is.null(adjust_to_year)) adjust_to_year <- as.integer(adjust_to_year) @@ -438,13 +454,32 @@ cog_spending <- function(govid, years, category = NULL, # direct spending in that category, which is correct, ordinary data). When # a covering recipe is found, both the row-level notes and the provenance # say so rather than pass silently as a plausible Total. - direct_suppressed_info <- if (identical(expenditure_concept, "total")) { + # + # In all-categories mode this cannot run at all: .detect_direct_suppressed() + # keys on (year, canonical_govid, category), and every row shares the same + # literal "All Categories" value, so the key collides across every real + # category for that (year, govid) -- an IG-only row for a suppressed + # category becomes indistinguishable from one sharing a key with an + # unrelated category's ordinary Direct row. `has_direct` would then read + # TRUE whenever the government has ANY direct spending at all, and the + # detector could never fire. Rather than run it and report a false FALSE, + # skip it and record NA -- the provenance must stop making a claim it + # cannot support (finding 1, all-categories review). + suppression_unavailable <- all_categories && + identical(expenditure_concept, "total") + direct_suppressed_info <- if (suppression_unavailable) { + list(flag = rep(NA, nrow(result)), notes = rep(NA_character_, nrow(result))) + } else if (identical(expenditure_concept, "total")) { .detect_direct_suppressed(con, result, subtype_col) } else { list(flag = rep(FALSE, nrow(result)), notes = rep(NA_character_, nrow(result))) } direct_suppressed <- direct_suppressed_info$flag - direct_suppressed_flag <- isTRUE(any(direct_suppressed)) + direct_suppressed_flag <- if (suppression_unavailable) { + NA + } else { + isTRUE(any(direct_suppressed)) + } result$notes <- .notes_column(result, direct_suppressed_info$notes) @@ -453,9 +488,20 @@ cog_spending <- function(govid, years, category = NULL, # leg is suppressed for at least one requested (year, category), append an # explicit warning rather than let the base note's "Total = Direct + IG" # framing stand unqualified for rows where that arithmetic didn't happen. + # When suppression detection itself is unavailable (all-categories mode), + # say so instead of silently reusing the unqualified base note. expenditure_concept_note_for_prov <- if (identical(expenditure_concept, "total")) { base_note <- "Total = Direct + intergovernmental (M to local govts + L to state govts). Legacy-era IG is assembled from aggregate-flagged rows, which are year-disjoint from their modern leaf components; the L-- family total is excluded." - if (direct_suppressed_flag) { + if (suppression_unavailable) { + paste0( + base_note, + " NOTE: direct-leg-suppression detection is unavailable when ", + "`category = \"All Categories\"` -- it keys on per-category rows, ", + "which this mode collapses. `expenditure_concept_direct_suppressed` ", + "is NA here rather than a possibly-false FALSE; query an explicit ", + "`category` (or `category = NULL`) to get a real answer." + ) + } else if (isTRUE(direct_suppressed_flag)) { paste0( base_note, " NOTE: for at least one requested (year, category) the Direct leg ", @@ -503,9 +549,22 @@ cog_spending <- function(govid, years, category = NULL, result } +#' Shared input validation for the money/holdings verbs. +#' +#' `allow_all_categories` gates the reserved pseudo-category +#' `.ALL_CATEGORIES` ("All Categories"). It is meaningful only where a +#' concept's subtype scope defines what "all" sums over -- +#' `cog_spending()`/`cog_revenue()`, via `.verb_spendrev()`, pass `TRUE`. +#' `cog_balances()` leaves it at the `FALSE` default: holdings are a stock +#' with no concept vocabulary to sum across (see R/balances.R), and before +#' this guard existed `cog_balances(category = "All Categories")` silently +#' matched zero crosswalk rows and returned an empty result with no error +#' (finding 3, all-categories review). This validator is shared specifically +#' so the three verbs cannot drift apart on this again. #' @noRd .validate_verb_inputs <- function(govid, years, category, - per_capita, adjust_to_year, recipe = NULL) { + per_capita, adjust_to_year, recipe = NULL, + allow_all_categories = FALSE) { if (!is.character(govid) || length(govid) == 0L) { cli::cli_abort("`govid` must be a non-empty character vector.") } @@ -515,6 +574,14 @@ cog_spending <- function(govid, years, category = NULL, if (!is.null(category) && !is.character(category)) { cli::cli_abort("`category` must be character or NULL.") } + if (!allow_all_categories && !is.null(category) && + .ALL_CATEGORIES %in% category) { + cli::cli_abort(c( + "{.val {(.ALL_CATEGORIES)}} is not supported here.", + i = "It sums a spending or revenue concept's subtype scope; this verb has no concept vocabulary to sum across.", + i = "Use {.fn cog_spending} or {.fn cog_revenue} for an all-categories total." + ), class = "uscogdata_all_categories_unsupported") + } if (!is.logical(per_capita) || length(per_capita) != 1L) { cli::cli_abort("`per_capita` must be a length-1 logical.") } @@ -650,8 +717,10 @@ cog_spending <- function(govid, years, category = NULL, # this feature exists to surface. # Collapse the category dimension. subtype is deliberately KEPT: it is what - # makes `subtype = "operations"` + all-categories mean "operating - # expenditure", the measure a fiscal comparison actually wants. + # lets a caller filter the result to `spend_subtype == "operations"` and + # get an operating-expenditure total, the measure a fiscal comparison + # actually wants. (There is no `subtype` argument -- this is a post-hoc + # filter on the returned column, not a query parameter.) category_select <- if (all_categories) { sprintf("%s AS category", .sql_lit_chr(.ALL_CATEGORIES)) } else { diff --git a/inst/schemas/provenance-v1.json b/inst/schemas/provenance-v1.json index a47945d..597fcd4 100644 --- a/inst/schemas/provenance-v1.json +++ b/inst/schemas/provenance-v1.json @@ -22,8 +22,8 @@ "description": "How the intergovernmental leg was assembled; null for 'primary' and 'direct'." }, "expenditure_concept_direct_suppressed": { - "type": "boolean", - "description": "TRUE when expenditure_concept = 'total' and at least one requested (year, category) has intergovernmental rows but NO Direct rows in this corpus (typically a legacy aggregate-only family) -- those result rows report the intergovernmental leg alone, not Direct + IG. Always FALSE for expenditure_concept = 'primary' or 'direct'. See the affected rows' `notes` for the recovering recipe, if any." + "type": ["boolean", "null"], + "description": "TRUE when expenditure_concept = 'total' and at least one requested (year, category) has intergovernmental rows but NO Direct rows in this corpus (typically a legacy aggregate-only family) -- those result rows report the intergovernmental leg alone, not Direct + IG. Always FALSE for expenditure_concept = 'primary' or 'direct'. null (NA) when expenditure_concept = 'total' AND category = 'All Categories': the detector keys on per-category rows, which that mode collapses, so suppression cannot be computed -- see `expenditure_concept_note`. See the affected rows' `notes` for the recovering recipe, if any." }, "revenue_concept": { "type": "string", diff --git a/man/cog_balances.Rd b/man/cog_balances.Rd index f594eba..4dab4a7 100644 --- a/man/cog_balances.Rd +++ b/man/cog_balances.Rd @@ -28,7 +28,12 @@ argument: for holdings, `category` is a strict coarsening of every combination would be either redundant or empty. `category = "Fund Balances"` is exactly the `general` family (`W01`/`W31`/`W61`). `balance_subtype` is returned, so a finer split is -one `dplyr::filter()` away.} +one `dplyr::filter()` away. The reserved pseudo-category +`"All Categories"` (see [cog_spending()]) is **not** supported here and +errors with class `uscogdata_all_categories_unsupported`: it sums a +concept's subtype scope, and holdings are a stock with no concept +vocabulary to sum across. Omit `category` to get every category broken +out instead.} \item{per_capita}{Divide holdings by population. Note this is a **stock per resident** (reserves per person), which is *not* comparable to diff --git a/man/cog_spending.Rd b/man/cog_spending.Rd index 1fee7d1..4a9a0e4 100644 --- a/man/cog_spending.Rd +++ b/man/cog_spending.Rd @@ -106,7 +106,12 @@ possibly-misleading `"harmonized"`/`"raw"` value.} component (when one exists), and `provenance$expenditure_concept_direct_suppressed` is `TRUE` -- the figure in those rows is the intergovernmental leg alone, not Direct + - IG.} + IG. When `category = "All Categories"` is combined with + `expenditure_concept = "total"`, this detection cannot run (it keys on + per-category rows, which all-categories mode collapses to one literal + value), so `expenditure_concept_direct_suppressed` is `NA` rather than a + possibly-false `FALSE`; query an explicit `category` to get a real + answer.} \item{complete}{If `TRUE`, fill the requested grid so that a cell the corpus does not carry still appears, labelled with **why** it is diff --git a/tests/testthat/test-all-categories.R b/tests/testthat/test-all-categories.R index c181fad..641fea6 100644 --- a/tests/testthat/test-all-categories.R +++ b/tests/testthat/test-all-categories.R @@ -123,3 +123,69 @@ test_that('cog_categories(pattern=) matches the pseudo-category', { hit <- cog_categories(pattern = "^All Categories$") expect_equal(nrow(hit), 2L) }) + +# --- final whole-branch review fixes --------------------------------------- + +test_that('complete = TRUE is refused when combined with "All Categories"', { + # .completion_grid_sql() would emit `AND c.category IN ('All Categories')`, + # match zero crosswalk rows, and the early return in .complete_result() + # would stamp completion$applied = TRUE, rows_filled = 0 -- reading as "the + # grid was checked and nothing was missing" when nothing was actually + # checked. Filling a summed row has no defined semantics, so the verb must + # refuse the combination outright (finding 2). + expect_error( + cog_spending("552025209777", 2019L, category = "All Categories", + complete = TRUE), + class = "uscogdata_complete_unsupported" + ) + expect_error( + cog_revenue("552025209777", 2019L, category = "All Categories", + complete = TRUE), + class = "uscogdata_complete_unsupported" + ) +}) + +test_that('cog_balances() rejects "All Categories" instead of silently returning zero rows', { + # cog_balances() reuses .validate_verb_inputs() but did not pass + # allow_all_categories = TRUE, so "All Categories" used to become + # `AND category IN ('All Categories')` against balance_annotated -- 0 + # matching crosswalk rows, 0 rows back, no error (finding 3). Holdings are + # a stock with no concept vocabulary to sum across, so the honest answer is + # to refuse, the same way cog_spending()/cog_revenue() refuse other + # nonsensical combinations. + expect_error( + cog_balances("552025209777", 2019L, category = "All Categories"), + class = "uscogdata_all_categories_unsupported" + ) + # An ordinary category still works -- this is not a blanket regression. + r <- suppressMessages( + cog_balances("552025209777", 2019L, category = "Fund Balances") + ) + expect_gt(nrow(r), 0L) +}) + +test_that('expenditure_concept_direct_suppressed is NA, not FALSE, when categories are collapsed', { + # .detect_direct_suppressed() keys on + # paste(year, canonical_govid, category, sep = "\r"). In all-categories + # mode every row carries the literal "All Categories" value, so an IG-only + # row's key collides with any ordinary Direct row for the same + # (year, govid) -- has_direct reads TRUE whenever the government has ANY + # direct spending at all, candidate is always empty, and the detector can + # never fire. Before the fix this silently reported FALSE, an affirmative + # claim the code did not actually compute (finding 1). NA is the honest + # answer: cog_explain(x, format = "list") is required here, since without + # format = "list" it returns the result tibble, not the provenance list. + gov <- "552025209777" + t <- cog_spending(gov, 2019L, category = "All Categories", + expenditure_concept = "total") + prov <- cog_explain(t, format = "list") + expect_true(is.na(prov$expenditure_concept_direct_suppressed)) + expect_false(isTRUE(prov$expenditure_concept_direct_suppressed)) + expect_match(prov$expenditure_concept_note, "unavailable", fixed = TRUE) + + # A per-category "total" query on the same government/year is unaffected -- + # the detector can still key correctly and reports a strict logical. + t_by_cat <- cog_spending(gov, 2019L, expenditure_concept = "total") + prov_by_cat <- cog_explain(t_by_cat, format = "list") + expect_false(is.na(prov_by_cat$expenditure_concept_direct_suppressed)) +})