diff --git a/R/suggestions.R b/R/suggestions.R index 3f6eb9b..0a148c0 100644 --- a/R/suggestions.R +++ b/R/suggestions.R @@ -102,43 +102,22 @@ # Path 2 (uscogdata#9): component dollars this government holds that the # verb's own view structurally excludes. Measured across ALL requested # years, not just gap years -- the whole point is that a year with rows can - # still be missing dollars. + # still be missing dollars. Scoped to the calling verb's own flow_prefixes + # (I1) -- see `.suppressed_components()`'s own roxygen for why. # - # I3(b): the anti-join inside `.suppressed_components()` is real DuckDB - # work, and unconditionally running it here regressed the common healthy - # path -- pre-#9, a fully-covered category query returned right after the - # cheap `candidates` query above. `.needs_suppression_query()` is a free - # (in-memory), EXACT (not heuristic) pre-check built from `result$ - # codes_included`, which the verb's own basis query already computed: it - # never skips a call that could have found something (see its own - # roxygen), so a recipe still qualifies on suppression alone with zero gap - # years -- it just avoids the round trip when that already-in-memory - # evidence rules it out. - # - # The pre-check needs a candidate's FULL component set, not just the - # component(s) that got it INTO `candidates` above -- a recipe with a - # modern leaf component (e.g. `welfare_cash_e67_wide`'s J67, a - # summary_categories member) also carries a wide-era aggregate component - # (E67) that is absent from summary_categories entirely and so would never - # surface via that query, yet is exactly the component this feature exists - # to catch. This is a second query, but a cheap one: metadata only, - # `recipe_id IN ()`, no `long`/govid/year involvement -- the - # same class of query as `meta` below. - comp_rows <- DBI::dbGetQuery(con, sprintf( - "SELECT DISTINCT recipe_id, component_code FROM harmonization_recipes - WHERE recipe_id IN (%s)", - .sql_lit_chr(candidates) - )) - flow_components <- unique(comp_rows$component_code[ - substr(comp_rows$component_code, 1L, 1L) %in% flow_prefixes]) - supp <- if (.needs_suppression_query(flow_components, result, govid, years)) { - .suppressed_components(con, candidates, govid, years, long_view, flow_prefixes) - } else { - tibble::tibble( - recipe_id = character(0), year = numeric(0), - suppressed_amount = numeric(0), suppressed_codes = character(0) - ) - } + # This runs unconditionally whenever there are candidates -- an earlier + # revision of this fix wave tried a free, in-memory pre-check + # (`.needs_suppression_query()`) to skip the round trip on an already- + # covered path, but a scoped re-review measured it against the fixture and + # found it didn't pay for itself (it skipped ~3% of healthy calls, ~0% of + # the multi-govid batch shape it was meant to help, at a net cost increase + # once its own always-run metadata query was counted) while adding an + # untested exactness invariant -- that `result$codes_included` and this + # anti-join share the harmonized `item_code` space -- whose silent + # violation would kill signposting, the exact failure class uscogdata#9 + # exists to prevent. Owner's call: keep this simple; a batch-aware + # optimization, if one is worth building, is a separate issue. + supp <- .suppressed_components(con, candidates, govid, years, long_view, flow_prefixes) if (length(gap_years) == 0L && nrow(supp) == 0L) return(list()) @@ -205,60 +184,6 @@ .attach_ig_counterparts(con, suggestions, flow_prefixes) } -#' Cheap (no SQL), exact pre-check gating the `.suppressed_components()` -#' round trip (uscogdata#9 review, finding I3(b)). -#' -#' Reuses `result`, which the verb's own basis query already computed and -#' which carries `codes_included` -- the DISTINCT item codes the verb's view -#' actually returned -- grouped by exactly `(year, canonical_govid, -#' category)`. A component code appearing there for a given (govid, year) -#' can only have come from the view, so it is -- by construction -- NOT -#' excluded for that (govid, year, item_code) key, which is exactly -#' `.suppressed_components()`'s own anti-join key. So: if every requested -#' (govid, year) pair already accounts for every one of the candidates' -#' flow-scoped component codes this way, the real measurement is guaranteed -#' to return zero rows for every one of them, and can be skipped outright. -#' -#' This is exact, not a heuristic approximation: it only ever returns `FALSE` -#' (skip) when the answer is provably "nothing to find", so it never -#' silences a genuine suppression fire. Any (govid, year) pair this cheaply -#' available evidence does not positively cover -- including a pair with -#' zero rows at all (a gap year), or one government of many in a large -#' batch call whose result happens to omit that year -- is conservatively -#' treated as "might be suppressed", so the real query still runs whenever -#' there is genuine doubt. In particular this does NOT special-case -#' `gap_years`: a recipe with zero gap years can still need the real query, -#' and one with every requested year a gap still gets `TRUE` here (the -#' `is.null(result) || nrow(result) == 0L` branch) rather than being -#' skipped. -#' -#' @param component_codes Character vector of candidate component codes, -#' already restricted to the calling verb's own `flow_prefixes` (I1) -- -#' see `.build_suggestions()`'s `flow_components`. -#' @param result Same `result` `.build_suggestions()` was passed. -#' @param govid Character vector of canonical_govid values. -#' @param years Integer vector of requested years. -#' @return `TRUE` if `.suppressed_components()` must actually run; `FALSE` -#' if it is already provably going to return zero rows. -#' @noRd -.needs_suppression_query <- function(component_codes, result, govid, years) { - if (length(component_codes) == 0L) return(FALSE) - if (is.null(result) || nrow(result) == 0L) return(TRUE) - - req_key <- paste(rep(govid, times = length(years)), - rep(as.integer(years), each = length(govid))) - res_key <- paste(result$canonical_govid, as.integer(result$year)) - codes_by_key <- split(result$codes_included, res_key) - - for (k in unique(req_key)) { - codes_here <- codes_by_key[[k]] - if (is.null(codes_here)) return(TRUE) - present <- unique(unlist(strsplit(codes_here, ",", fixed = TRUE))) - if (!all(component_codes %in% present)) return(TRUE) - } - FALSE -} - #' Measure, per (recipe, year), the component dollars this government holds #' that the calling verb's own long view structurally excludes. #' diff --git a/tests/testthat/test-recipes.R b/tests/testthat/test-recipes.R index 0db8562..a251119 100644 --- a/tests/testthat/test-recipes.R +++ b/tests/testthat/test-recipes.R @@ -245,68 +245,6 @@ test_that(".select_long_view maps annotated view bases to their long views", { "spending_long") }) -# --- I3(b): the free pre-check gating .suppressed_components() ------------- - -test_that(".needs_suppression_query skips only when the evidence rules out suppression", { - # Every requested (govid, year) already accounts for every component code: - # .suppressed_components() is guaranteed to find nothing, so it is safe to - # skip the round trip. - result_full <- tibble::tibble( - year = c(2019L, 2019L, 2020L, 2020L), - canonical_govid = c("A", "B", "A", "B"), - codes_included = c("E01,E02", "E01,E02,E03", "E01,E02", "E01,E02") - ) - expect_false(uscogdata:::.needs_suppression_query( - c("E01", "E02"), result_full, govid = c("A", "B"), years = c(2019L, 2020L))) - - # One (govid, year) is missing a component -- cannot rule out suppression, - # so the real measurement must still run. - result_gap <- result_full - result_gap$codes_included[result_gap$canonical_govid == "B" & result_gap$year == 2020L] <- "E01" - expect_true(uscogdata:::.needs_suppression_query( - c("E01", "E02"), result_gap, govid = c("A", "B"), years = c(2019L, 2020L))) - - # A requested (govid, year) is entirely absent from `result` (e.g. a gap - # year, or one government of many in a batch call) -- conservatively TRUE. - result_absent <- result_full[!(result_full$canonical_govid == "B" & result_full$year == 2020L), ] - expect_true(uscogdata:::.needs_suppression_query( - c("E01", "E02"), result_absent, govid = c("A", "B"), years = c(2019L, 2020L))) - - # No candidate component belongs to the calling verb's own flow family (the - # I1 cross-flow-family case) -- nothing could ever be measured, so skip. - expect_false(uscogdata:::.needs_suppression_query( - character(0), result_full, govid = c("A", "B"), years = c(2019L, 2020L))) - - # An empty result (e.g. every requested year is a gap) can never positively - # rule out suppression -- conservatively TRUE. - expect_true(uscogdata:::.needs_suppression_query( - c("E01"), result_full[0, ], govid = "A", years = 2019L)) -}) - -test_that("I3(b): a suppression-only fire (zero gap years) still runs the real measurement", { - # Public Welfare FY2011 for LA County has rows in every requested year (no - # gap_years), so this exercises exactly the path I3(b) must not break: the - # pre-check must return TRUE here, and the real .suppressed_components() - # round trip must actually execute, or the whole uscogdata#9 feature would - # go dark on its own motivating case. - skip_if_no_corpus() - called <- FALSE - orig <- uscogdata:::.suppressed_components - testthat::local_mocked_bindings( - .suppressed_components = function(...) { - called <<- TRUE - orig(...) - }, - .package = "uscogdata" - ) - r <- suppressMessages( - cog_spending("061037123085", years = 2011L, category = "Public Welfare")) - expect_true(called) - sugg <- attr(r, "provenance")$suggestions - triggers <- vapply(sugg, function(s) s$trigger, character(1)) - expect_true(all(triggers == "suppressed_component")) -}) - test_that(".suppressed_components measures the E67/E68 dollars Public Welfare drops", { skip_if_no_corpus() con <- uscogdata:::.ensure_session()