revert: drop the I3(b) suppression pre-check gate (#9)
Scoped re-review measured .needs_suppression_query() against the fixture
and found it doesn't pay for itself: it skips the round trip on ~3% of
healthy candidate-bearing calls, ~0% of the multi-govid batch shape
(cog_geographic_rollup()/cog_peer_compare()) it was meant to help, and
reaching the gate costs an unconditional metadata query that on its own
roughly cancels the expected saving -- net slower on the fixture. The
gate was also correct (0 unsound skips) but left an untested exactness
invariant (result$codes_included and the anti-join sharing the harmonized
item_code space) whose silent violation would kill signposting, which is
the exact failure class uscogdata#9 exists to prevent.
Owner's call: revert it and keep the code simple. A batch-aware
optimization, if warranted, is a separate issue.
Removes .needs_suppression_query() entirely (function, roxygen, call
site, comp_rows/flow_components), restoring .build_suggestions() to call
.suppressed_components() directly -- unchanged from f77adb6 except that
it still threads flow_prefixes through (I1, kept). Also removes the two
tests that existed solely to exercise the gate (the five-branch synthetic
test and the local_mocked_bindings call-counter test); no test asserting
real signposting behavior was touched.
I1 (flow_prefixes filter), I2 (schema wording + ig_recipe_id required),
I3(a) (restated NOT EXISTS literals for partition pruning), and M4
(reworded scope claims) are all untouched by this revert.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+15
-90
@@ -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 (<candidates>)`, 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.
|
||||
#'
|
||||
|
||||
Reference in New Issue
Block a user