diff --git a/R/suggestions.R b/R/suggestions.R index f089040..2d554ae 100644 --- a/R/suggestions.R +++ b/R/suggestions.R @@ -84,6 +84,21 @@ #' @return List of `list(recipe_id, label, available_years, hint, #' ig_recipe_id, trigger, suppressed_amount, suppressed_years, #' suppressed_codes)`, possibly empty. +#' +#' Decomposed (Issue #33) into three extracted helpers to stay within the +#' project's "functions under 50 lines" convention: +#' \itemize{ +#' \item `.query_candidate_recipes()` -- candidate recipe lookup by +#' category/subtype scope + `category_type` filter (#34) + M/L exclusion. +#' \item `.query_recipe_meta()` -- metadata (label, year spans). +#' \item `.query_covered_years()` -- Path 1 gap-year coverage via the +#' recipe's own generic join. +#' } +#' The for-loop that merges covered-years + suppressed-components into +#' suggestion objects stays inline here because it interleaves +#' empty_hit/supp_hit precedence with field assembly. Likewise kept inline: +#' the M/L-exclusion design-comment block and the final +#' `.attach_ig_counterparts()` call. #' @noRd .build_suggestions <- function(con, cohort, years, category, result, basis, flow_prefixes, long_view, @@ -111,28 +126,16 @@ # by `category` (`.ALL_CATEGORIES` is never a row in # `summary_categories.category`, so a category-keyed sub-select always # came back empty here). The M/L exclusion below is unchanged either way. - candidate_scope_sql <- if (isTRUE(all_categories)) { - sprintf( - "SELECT DISTINCT item_code FROM summary_categories WHERE %s IN (%s)", - subtype_col, .sql_lit_chr(subtype_scope) - ) - } else { - sprintf( - "SELECT DISTINCT item_code FROM summary_categories WHERE category IN (%s)", - .sql_lit_chr(category) - ) - } - candidates <- DBI::dbGetQuery(con, sprintf( - "SELECT DISTINCT recipe_id FROM harmonization_recipes - WHERE component_code IN ( - %s - ) - AND recipe_id NOT IN ( - SELECT DISTINCT recipe_id FROM harmonization_recipes - WHERE LEFT(component_code, 1) IN ('M', 'L') - )", - candidate_scope_sql - ))$recipe_id + # + # Issue #34: scope the candidate query by `category_type` ('expenditure' + # vs 'revenue') to prevent cross-flow-family leakage -- e.g. + # `cog_revenue(category = "Corrections")` must not surface + # expenditure-only recipes (E04/E05) merely because they share the same + # category name in summary_categories. The type is derived from + # flow_prefixes: E/F/G -> 'expenditure', anything else -> 'revenue'. + candidates <- .query_candidate_recipes(con, category, flow_prefixes, + all_categories, subtype_col, + subtype_scope) if (length(candidates) == 0L) return(list()) result_years <- if (is.null(result) || nrow(result) == 0L) { @@ -164,36 +167,11 @@ if (length(gap_years) == 0L && nrow(supp) == 0L) return(list()) - meta <- tibble::as_tibble(DBI::dbGetQuery(con, sprintf( - "SELECT recipe_id, any_value(label) AS label, - MIN(year_min) AS year_min, MAX(year_max) AS year_max - FROM harmonization_recipes - WHERE recipe_id IN (%s) - GROUP BY recipe_id", - .sql_lit_chr(candidates) - ))) + meta <- .query_recipe_meta(con, candidates) # Path 1 (unchanged): (recipe, year) pairs the recipe's own generic join # covers for this government, restricted to the gap years. - covered <- if (length(gap_years) == 0L) { - data.frame(recipe_id = character(0), year = integer(0)) - } else { - DBI::dbGetQuery(con, sprintf( - "SELECT DISTINCT r.recipe_id, l.year - FROM long l - JOIN harmonization_recipes r - ON l.item_code = r.component_code - AND l.year BETWEEN r.year_min AND r.year_max - AND (r.gov_type_scope = 'all' - OR (r.gov_type_scope = 'state' AND l.type = 0) - OR (r.gov_type_scope = 'local' AND l.type BETWEEN 1 AND 3)) - WHERE r.recipe_id IN (%s) - AND %s - AND l.year IN (%s)", - .sql_lit_chr(candidates), .cohort_sql(cohort, "l.canonical_govid"), - paste(gap_years, collapse = ",") - )) - } + covered <- .query_covered_years(con, candidates, cohort, gap_years) suggestions <- list() for (rid in candidates) { @@ -284,6 +262,171 @@ #' `ig_federal_b47_wide`'s own `"B"` IS inside revenue's own #' `flow_prefixes`, so only this second, family-specific check blocks #' the search. +#' Query candidate harmonization recipe IDs for a coverage-gap suggestion. +#' +#' Selects recipes whose component codes fall within the requested scope +#'(category or subtype allowlist), excluding any recipe that is ITSELF an +#'intergovernmental (M/L) recipe -- i.e. every one of its own component codes +#'is M/L-prefixed. Without this exclusion, a category whose summary_categories +#'rows span both a Direct family (e.g. E04/E05, "Corrections") and its M/L +#'counterpart (M04/M05) makes the M/L recipe itself a raw top-level candidate +#'for a plain `cog_spending()` call -- following that hint would silently return +#'intergovernmental dollars under `expenditure_concept = "direct"` provenance. +#' +#' Scope is by `category_type` ('expenditure' vs 'revenue', Issue #34) to prevent +#'cross-flow-family leakage: `cog_revenue(category = "Corrections")` must not +#'surface expenditure-only recipes (E04/E05) merely because they share the same +#'category name in summary_categories. The type is derived from flow_prefixes: +#'E/F/G -> 'expenditure', anything else -> 'revenue'. +#' +#' In all-categories mode (`all_categories = TRUE`) the inner sub-select is scoped +#'by `subtype_col`/`subtype_scope` -- the same allowlist `.build_verb_sql()` applies +#'as a WHERE predicate to make the summed result a *concept* (see R/spending.R), +#'not by `category`. `.ALL_CATEGORIES` ("All Categories") is never itself a row in +#'`summary_categories.category`, so a category-keyed sub-select always returns zero +#'candidates and silently disables signposting. +#' +#' @param con Active DuckDB connection. +#' @param category Category name, or `NULL`. +#' @param flow_prefixes The calling verb's own flow-type prefixes (see +#'.build_suggestions()). Used to derive `category_type` (#34). +#' @param all_categories `TRUE` when the caller used `.ALL_CATEGORIES`. +#' @param subtype_col Name of the summary_categories subtype column to scope by +#'when `all_categories = TRUE`; ignored otherwise. +#' @param subtype_scope Character vector of subtype values to scope by when +#'`all_categories = TRUE`; ignored otherwise. +#' @return Character vector of recipe IDs (possibly empty). +#' @noRd +.query_candidate_recipes <- function(con, category, flow_prefixes, + all_categories = FALSE, + subtype_col = NULL, subtype_scope = NULL) { + # Issue #34: derive category_type from flow_prefixes to prevent + # cross-flow-family leakage -- e.g. cog_revenue(category = "Corrections") + # must not surface expenditure-only recipes merely because they share the + # same category name in summary_categories. + category_type <- if (all(flow_prefixes %in% c("E", "F", "G"))) { + "expenditure" + } else { + "revenue" + } + + candidate_scope_sql <- if (isTRUE(all_categories)) { + sprintf( + "SELECT DISTINCT item_code FROM summary_categories WHERE %s IN (%s) AND category_type = '%s'", + subtype_col, .sql_lit_chr(subtype_scope), category_type + ) + } else { + sprintf( + "SELECT DISTINCT item_code FROM summary_categories WHERE category IN (%s) AND category_type = '%s'", + .sql_lit_chr(category), category_type + ) + } + + DBI::dbGetQuery(con, sprintf( + "SELECT DISTINCT recipe_id FROM harmonization_recipes\n WHERE component_code IN (\n %s\n )\n AND recipe_id NOT IN (\n SELECT DISTINCT recipe_id FROM harmonization_recipes\n WHERE LEFT(component_code, 1) IN ('M', 'L')\n )", + candidate_scope_sql + ))$recipe_id +} + +#' Query gap-year coverage: which (recipe, year) pairs the recipe's own generic +#join covers for this government, restricted to `gap_years`. +# +#This is Path 1 of a suggestion (unchanged): it finds recipes whose component +#codes' generic join produces at least one row for this government in each gap +#year -- i.e. the category returned nothing in that year but a recipe would fill +#it. +# +#@param con Active DuckDB connection. +#@param candidates Character vector of recipe IDs to check coverage for. +#@param cohort The verb's cohort object (see `.make_cohort()`), rendered into the +#`govid predicate on the joined `long` scan via `.cohort_sql()`. +#@param gap_years Integer vector of requested years absent from the result. +#@return Data frame with columns `recipe_id` (character) and `year` (integer). +# Returns an empty data frame (`recipe_id = character(0)`, `year = integer(0)`) +#when `gap_years` is empty, so callers can safely reference `$recipe_id`. +#@noRd +.query_covered_years <- function(con, candidates, cohort, gap_years) { + if (length(gap_years) == 0L) { + return(data.frame(recipe_id = character(0), year = integer(0))) + } + DBI::dbGetQuery(con, sprintf( + "SELECT DISTINCT r.recipe_id, l.year\n FROM long l\n JOIN harmonization_recipes r\n ON l.item_code = r.component_code\n AND l.year BETWEEN r.year_min AND r.year_max\n AND (r.gov_type_scope = 'all'\n OR (r.gov_type_scope = 'state' AND l.type = 0)\n OR (r.gov_type_scope = 'local' AND l.type BETWEEN 1 AND 3))\n WHERE r.recipe_id IN (%s)\n AND %s\n AND l.year IN (%s)", + .sql_lit_chr(candidates), .cohort_sql(cohort, "l.canonical_govid"), + paste(gap_years, collapse = ",") + )) +} + +#' Query recipe metadata: labels and year spans for a set of candidate recipes. +#' +#' @param con Active DuckDB connection. +#' @param candidates Character vector of recipe IDs to look up. +#' @return Tibble with columns `recipe_id`, `label`, `year_min` (int), and +#' `year_max` (int). +#' @noRd +.query_recipe_meta <- function(con, candidates) { + tibble::as_tibble(DBI::dbGetQuery(con, sprintf( + "SELECT recipe_id, any_value(label) AS label, + MIN(year_min) AS year_min, MAX(year_max) AS year_max + FROM harmonization_recipes + WHERE recipe_id IN (%s) + GROUP BY recipe_id", + .sql_lit_chr(candidates) + ))) +} + +#' Attach `ig_recipe_id` to each suggestion: the intergovernmental-expenditure +#' recipe (an M-to-local or L-to-state recipe) whose component codes cover +#' exactly the same set of function suffixes as the firing recipe's own +#' components, e.g. `corrections_combined`'s {E04, E05} -> suffixes {"04", +#' "05"} matches `corrections_ig_local_combined`'s {M04, M05} -> the same +#' {"04", "05"}. `NULL` when no such recipe exists, which also covers the +#' case where the firing recipe already IS the IG recipe (self-matches are +#' excluded, so an IG recipe never names itself as its own counterpart). +#' +#' Matching is deliberately an exact set match, not "any suffix in common": +#' the two-digit suffix only means the same "function" across recipes that +#' share the underlying Census functional-classification scheme (E/F/G/L/M +#' all use "04"/"05" for corrections). M/L "combined other" codes (47/89/ +#' 91-94) reuse digits for an unrelated catch-all construct, so e.g. +#' `general_gov_e89_wide`'s {E85, E89} -> {"85", "89"} must NOT match +#' `ige_local_m89_wide`'s {"89", "91", "92", "93"} on the shared "89" alone. +#' Checked by hand against the full harmonization_recipes catalog: only the +#' corrections family (E/F/G/M, suffixes 04/05) has an exact-set match in +#' this corpus. +#' +#' Exact-set suffix matching is NOT enough on its own, though: the same +#' reused-digit problem exists ACROSS the revenue-side IG families too. +#' `ig_local_d47_wide` (D47/D94, suffixes {"47","94"}) is an exact-set match +#' for `ige_local_m47_wide` (M47/M94, same suffixes) even though one is +#' intergovernmental REVENUE received from local governments and the other is +#' intergovernmental EXPENDITURE paid to local governments -- unrelated flows +#' that happen to reuse "47"/"94" for their own "transit/utilities" and +#' "other/combined" catch-alls. `ig_federal_b47_wide`, `ig_state_c47_wide`, +#' and their `*_89` siblings all collide the same way. None of this is +#' reachable via `cog_revenue()` in the bundled fixture today (its B/C/D +#' recipes never happen to have a covered gap year for any fixture govid), +#' but it IS reachable via a mis-scoped `cog_spending()` call on a +#' revenue-only category, e.g. `cog_spending(gov, category = "IG Federal")` +#' fires `ig_federal_b47_wide`/`ig_federal_b89_wide` for real in the fixture +#' -- so this is a live, not merely theoretical, gap. +#' +#' Two flow-family checks close this, both required (see +#' `tests/testthat/test-expenditure-concept.R`, "revenue-flavored ... never +#' receives an M/L counterpart" tests, for the pairwise verification): +#' 1. `own_prefix %in% flow_prefixes`: the firing recipe's own component +#' codes must belong to the calling verb's own flow family (the same +#' `flow_prefixes` `.build_harmonization_block()` uses, see +#' `R/basis.R`). This blocks a recipe surfaced through a mis-scoped +#' category from ever reaching the M/L search, e.g. `cog_spending()`'s +#' flow_prefixes are `c("E","F","G")`, which `ig_federal_b47_wide`'s own +#' "B" is not part of. +#' 2. `own_prefix %in% c("E","F","G")`: M/L only ever pairs with the +#' DIRECT-expenditure family, never with revenue (`cog_revenue()`'s +#' flow_prefixes already fold B/C/D in as ordinary revenue -- there is +#' no separate "Total" bolt-on for revenue the way `expenditure_concept` +#' adds one for spending) and never with ANOTHER M/L recipe (without +#' this check, `ige_local_m47_wide` would wrongly match sibling +#' `ige_state_l47_wide` on their shared {"47","94"} suffix set). #' @noRd .attach_ig_counterparts <- function(con, suggestions, flow_prefixes) { if (length(suggestions) == 0L) return(suggestions)