Extract three functions from the ~140-line .build_suggestions()
orchestrator to comply with the 'functions under 50 lines' convention:
- .query_candidate_recipes(): candidate recipe lookup by category/subtype
scope, plus M/L self-exclusion
- .query_recipe_meta(): metadata lookup for labels and year spans
- .query_covered_years(): Path 1 gap-year coverage query via the recipe's
own generic join; returns empty data frame when gap_years is empty
Kept inline per design: the for-loop that merges covered-years +
suppressed-components into suggestion objects, the M/L-exclusion comment
block as call-site rationale, and .attach_ig_counterparts() at the end.
Pure extraction, no behavior change -- SQL text is unchanged apart from
whitespace. Restored real multi-line SQL string literals in the two new
helpers (the original candidate/covered-years queries were written that
way; keep it consistent with .query_recipe_meta()) and normal roxygen
'#'' comment-marker spacing throughout, both of which drifted during
extraction in an earlier pass.
All 1084 tests pass (2 skipped live-corpus), measured devtools::test()
against this commit in a clean worktree.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cog_spending(), cog_revenue() and cog_balances() gain optional state/type
arguments. Both default to NULL, so every existing govid-based call is
unchanged.
The verbs took a cohort only as a govid vector, which .sql_lit_chr()
rendered into a quoted IN list and .verb_spendrev() embedded into 5-8
separate statements per call: the scope check, the main aggregate, the
per-capita join, the harmonization block, and the suggestion and
suppression queries. For type = "city" that list is 301,589 characters,
parsed and planned from scratch every time it appears.
Passing state/type instead expresses the cohort as a subquery against
canonical_fips_xwalk, so its size never enters the SQL string at all.
Measured on the production corpus, same FY2022 aggregate over the
20,106-government city cohort, DUCKDB_THREADS=2, median of 5:
IN (20,106 literals) -- 0.3.0 432 ms
join against a temp cohort table 132 ms
predicate on canonical_fips_xwalk 102 ms
no cohort filter at all (the floor) 105 ms
The predicate reaches the no-filter floor: the cohort restriction is
now free. End to end through cog_spending(category = "Police"),
1080 ms -> 271 ms, 3.99x -- larger than the single-query saving,
because the repetition across statements is what actually cost.
Design decisions, both made explicitly rather than left implicit:
- govid AND state/type INTERSECT. "These ids, narrowed to that
state/type" is a real query, and an error here could never be
relaxed later without breaking callers.
- A predicate cohort has no id list to report, so
provenance$scope$govids_found/govids_missing stay empty and a new
scope$cohort block carries state, type and n_governments. Resolving
the ids just to report them would put 20,000 govids in every
fleet-scale response body -- the cost this change removes. A
govid-named cohort's provenance is untouched.
state/type are coerced with .coerce_state_to_fips()/.coerce_type(), the
same helpers cog_gov_search() uses. That is load-bearing: the argument
is a postal abbreviation ("WI") while fips_state holds a FIPS code
("55"), and a predicate on the raw parameter matches nothing and returns
an empty result indistinguishable from "reported nothing". cog-api hit
exactly this trap optimizing the same path.
.attach_per_capita() now keys its population lookup on the govids present
in the result rather than the requested cohort. Those are the only ones
its LEFT JOIN can match, so the output is identical -- but it needs no id
list, and on a paginated call it looks up one page instead of the fleet.
Fixes uscogdata#58.
.build_suggestions()'s recipe-candidate sub-select was keyed on
`WHERE category IN (<category>)`. The reserved pseudo-category
"All Categories" is never itself a row in summary_categories.category, so
in all-categories mode `candidates` always came back empty and coverage
signposting (uscogdata#9) was structurally impossible for the one mode
whose entire premise is "you cannot sum the wrong scope" -- measured on
Los Angeles County FY2011: category = "Public Welfare" reports 2
suggestions (incl. $271,589,000 excluded E68), category = "All Categories"
reported 0, silently losing that same signal.
Apply the branch's own design principle: the concept boundary is subtype,
not category. .build_suggestions() now accepts all_categories/subtype_col/
subtype_scope (all optional, default off, so no other caller's behaviour
changes) and, when all-categories mode is active, scopes the candidate
sub-select by `<subtype_col> IN (<subtype_scope>)` instead -- symmetric
with .build_verb_sql()'s own WHERE predicate. The M/L recipe exclusion and
the is.null(category) early return are unchanged.
After the fix, LA County FY2011 "All Categories" reports 5 suggestions,
including welfare_cash_e68_wide for the exact $271,589,000 gap.
Adds two covering tests to test-all-categories.R using the bundled fixture
(AL state gov, FY2011, "Corrections"): one end-to-end (per-category and
all-categories both signpost the same recipe) and one direct on
.build_suggestions() proving the subtype-vs-category branch is what
changes the query. Updates the 0.2.0 NEWS entry.
R/suggestions.R crossed the project's 400-line limit (424 lines).
Pure move of .suppressed_components() and its roxygen block per the
plan's Task 5 Step 2 remedy; no logic, SQL, or wording changed.
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>
Final whole-branch review fix wave for the partial-coverage signposting
feature:
- I1: .suppressed_components() now filters measured recipe components to
the calling verb's own flow_prefixes. Without this, a candidate recipe
from the OTHER flow family was always absent from the verb's own view by
construction and so was always reported as "suppressed" -- fabricating a
dollar claim across flow families (cog_revenue(category = "Corrections")
claimed $3.63B excluded that cog_spending() actually reports in full).
- I2: reworded provenance-v1.json's trigger/suppressed_amount descriptions
to describe what the code actually measures (the verb's underlying long
view, not "the result"), and to note suppressed_amount can be negative.
Added ig_recipe_id to the suggestions items' required list, matching the
key's always-set/nullable runtime behavior.
- I3(a): restated the year/govid literals inside .suppressed_components()'s
NOT EXISTS subquery so DuckDB can partition-prune that side too (verified
via EXPLAIN: Scanning Files 1/4 instead of an unfiltered full scan;
all.equal(old, new) results confirmed unchanged).
- I3(b): added .needs_suppression_query(), a free, exact pre-check reusing
the verb's own already-computed result$codes_included to skip the anti-
join round trip on the common fully-covered path, without weakening the
"suppression can fire with zero gap years" guarantee.
- M4: corrected the overbroad "confines every fire to 2011" scope claim in
R/suggestions.R and NEWS.md -- the suppressed-dollar measurement is now
flow-scoped (post-I1), but the empty_year trigger itself is not, and can
still fire in modern years for a mis-scoped cross-flow-family category.
Added a regression test for I1 plus direct unit-test coverage for the new
flow-family filter and the I3(b) pre-check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
.inform_suggestions() and cog_explain() now render suppressed_amount /
suppressed_years / suppressed_codes as a continuation line on each
suggestion bullet whenever suppressed_amount > 0 (an empty_year fire can
carry them too, so this keys off the amount, not trigger). Also renames
the cli header from "Coverage gap detected" to "Incomplete coverage" --
a partial-coverage fire is not a gap, the year has rows, they're just
short.
.build_suggestions()'s candidate query picks recipes by component_code
matching the requested category's summary_categories rows, with no
flow-prefix filter. Task 1's M04/M05 category rows share the
"Corrections" category with the Direct-flavored E04/E05, so
corrections_ig_local_combined (entirely M-prefixed) became a raw
top-level candidate for a plain (Direct) cog_spending() call.
Following that hint would silently return intergovernmental dollars
under provenance$expenditure_concept = "direct".
Task 6's flow-family gate in .attach_ig_counterparts() already protects
the *counterpart* lookup (deciding whether a firing suggestion gets an
ig_recipe_id attached) but never touched the candidate list itself.
Exclude any recipe with an M/L-prefixed component from candidates
unconditionally -- an M/L recipe should never be a coverage-gap filler
for either verb, which is a stronger guarantee than the counterpart
gate's flow_prefixes check.
Confirmed via the full suite: before this fix, a Direct cog_spending()
call for category = "Corrections" printed "corrections_ig_local_combined
... re-run with recipe = 'corrections_ig_local_combined'" as its own
suggestion; after, it appears only as the "intergovernmental
counterpart" annotation on corrections_combined and its capital-outlay
siblings. The pre-existing "IG Federal" mis-scoped test (revenue-side
B-prefixed recipes) is unaffected -- those aren't M/L, so they remain
valid candidates with ig_recipe_id still gated to NULL.
Review found the suffix-set match alone is unsafe: revenue-side recipes
(ig_federal_b47_wide, ig_state_c47_wide, ig_local_d47_wide, and their *_89
siblings) coincidentally share exact suffix sets with M/L expenditure
recipes despite representing a different flow direction. Reachable today via
a mis-scoped cog_spending(category = "IG Federal") call, not just
cog_revenue(). Thread flow_prefixes (same parameter .build_harmonization_block
already uses) through .build_suggestions()/.attach_ig_counterparts() and
require a firing recipe's own prefixes to be both in the calling verb's flow
family and within {E,F,G} before searching the M/L catalog.
Adds cog_recipes() to list the curated harmonization_recipes catalog (24
recipes / schema_version >= 5), and a recipe= argument on cog_spending()/
cog_revenue() that runs a recipe's generic multi-code join instead of the
category view: SUM(amt * weight) across whichever component codes are
present for a (year, canonical_govid), scoped by gov_type_scope. The join
deliberately does not filter is_aggregate -- the wide era (<= 2011) exposes
these split families (corrections 04+05, IG *89/*47, U4- rents, etc.) ONLY
as aggregate rows, with leaf codes first appearing in 2012, so excluding
aggregates would zero out the wide-era half of every recipe. This is safe
by corpus construction: wide-era rows are aggregate-only, modern rows are
leaf-only, and every component is year-scoped, so there is no
double-counting. recipe= is mutually exclusive with category=; the result's
subtype column reads "recipe" and category reads the recipe's label.
Adds recipe-component-driven signposting: when a basis="harmonized" +
category query comes back with zero rows in a requested year, and a
harmonization recipe covering that category would actually produce rows
for this government in that year (via the same join .run_recipe() uses),
the recipe is surfaced in provenance$suggestions plus one
cli::cli_inform() message. This is deliberately keyed off recipe
components rather than harmonization_map's suggested_recipe_id column
(which is empty on every live row -- the wide era's split families are
NA-by-construction via aggregate exclusion, not an NA ruling to hang a
suggestion off of).
Also populates the previously-always-empty provenance$series_break_refs
(schema v5 only: series_breaks_pq rows whose fin_code is among the
observed codes and whose break_year falls in the requested span), and
extends cog_explain() with Basis/Harmonization/Recipe/Suggestions/Series
breaks sections.
jaredandClaude Sonnet 5