fix: address Task 3 code review (bool_or, invariant tests, guards, docs)
Nine review items on the expenditure_concept = direct|total feature: - bool_and(is_aggregate) -> bool_or(is_aggregate) for aggregate_fallback: bool_and silently misreported $5,740,775,000 of aggregate-sourced IG dollars (AL state 2011) as aggregate_fallback = FALSE, because the dense wide-era data puts a $0 leaf row in the same group as the real aggregate row. bool_or is a no-op for Direct/Revenue (verified: 0 mismatched groups across both tables) and correct for the IG leg. - Added a year-disjointness invariant test for the four legacy aggregate/leaf IG pairs (M47/M94, M89/M91-93, L47/L94, L89/L91-93), scoped to the aggregate flag rather than bare code presence (M89/L89 continue past 2011 as independent, non-aggregate leaves). - Extended the real-SQL-text/synthetic-parquet harness in test-views.R to pin ig_long/ig_long_harmonized's predicates directly (aggregate rows retained, NULL harmonized_code coalesced, L-- excluded), rather than relying on one fixture row's incidental shape. - Added a test proving the .harmonization_view_files schema-v5 guard is necessary (not just incidental) against a corpus whose `long` genuinely lacks a harmonized_code column, and rewrote the misleading "v5-only parquet files" comment to name both real reasons a file is gated. - Fixed an NA-fragile subtype filter, extended the expected-view-list test, guarded .verb_spendrev() against total on a non-spending view_base, added a roxygen caveat against summing total across levels of government, and replaced an uncheckable corpus-wide SQL comment figure with a fixture-verifiable one. Full suite: 485/0/0 -> 503/0/0 (18 new expectations, zero pre-existing value changed).
This commit is contained in:
+35
-2
@@ -49,7 +49,12 @@
|
||||
#' to the state government (`L` codes, excluding the `L--` family-total
|
||||
#' rollup) -- so results gain rows with `spend_subtype ==
|
||||
#' "intergovernmental"`. Mutually exclusive with `recipe` (a recipe
|
||||
#' already defines its own component codes).
|
||||
#' already defines its own component codes). **Do not sum `"total"`
|
||||
#' results across levels of government** (e.g. state + county + city):
|
||||
#' a state's `M12` payment to a school district is the same dollar the
|
||||
#' district reports as its own direct `E12`, so summing both double-counts
|
||||
#' it. This matters in particular with [cog_geographic_rollup()], which
|
||||
#' sums across exactly that kind of multi-layer government set.
|
||||
#' @return Tibble with columns `year`, `canonical_govid`, `gov_name`,
|
||||
#' `spend_subtype`, `category`, `amt_nominal`, optional `amt_real`,
|
||||
#' optional `amt_per_capita_nominal`, optional `amt_per_capita_real`,
|
||||
@@ -111,6 +116,24 @@ cog_spending <- function(govid, years, category = NULL,
|
||||
), class = "uscogdata_recipe_concept_conflict")
|
||||
}
|
||||
|
||||
# .verb_spendrev() is shared with cog_revenue(), which never exposes
|
||||
# expenditure_concept and always resolves it to "direct" -- so nothing on
|
||||
# the public API can reach this today. But it's a cheap guard against a
|
||||
# future call (direct or via a modified cog_revenue()) that would UNION
|
||||
# the IG leg's expenditure M/L rows into a revenue result, which has no
|
||||
# matching IG view and no sensible meaning.
|
||||
if (identical(expenditure_concept, "total") &&
|
||||
!identical(view_base, "spending_annotated")) {
|
||||
cli::cli_abort(
|
||||
paste0(
|
||||
"`expenditure_concept = \"total\"` is only supported for spending ",
|
||||
"(view_base = \"spending_annotated\"); got view_base = ",
|
||||
"{.val {view_base}}."
|
||||
),
|
||||
class = "uscogdata_expenditure_concept_unsupported"
|
||||
)
|
||||
}
|
||||
|
||||
years <- as.integer(years)
|
||||
if (!is.null(adjust_to_year)) adjust_to_year <- as.integer(adjust_to_year)
|
||||
|
||||
@@ -280,6 +303,16 @@ cog_spending <- function(govid, years, category = NULL,
|
||||
sprintf("(SELECT * FROM %s UNION ALL SELECT * FROM %s)", view, ig_view)
|
||||
}
|
||||
|
||||
# bool_or(), not bool_and(): a no-op for the Direct/revenue legs (those
|
||||
# views filter NOT is_aggregate, so no row in any group is ever aggregate),
|
||||
# but load-bearing for the IG leg, which deliberately keeps aggregate rows
|
||||
# (see inst/sql/24-ig_long.sql). The wide era is dense -- every government
|
||||
# has a row for every code in a family, most of them $0 -- so a $0 leaf
|
||||
# commonly lands in the same (year, gov, subtype, category) group as the
|
||||
# real aggregate row. bool_and() would then read FALSE for that group even
|
||||
# though its dollars came entirely from an aggregate row, silently
|
||||
# suppressing the "Aggregate fallback applied" note on exactly the rows
|
||||
# this feature exists to surface.
|
||||
sprintf(
|
||||
"SELECT
|
||||
year,
|
||||
@@ -289,7 +322,7 @@ cog_spending <- function(govid, years, category = NULL,
|
||||
category,
|
||||
SUM(amt) * 1000.0 AS amt_nominal,
|
||||
string_agg(DISTINCT item_code, ',' ORDER BY item_code) AS codes_included,
|
||||
bool_and(is_aggregate) AS aggregate_fallback
|
||||
bool_or(is_aggregate) AS aggregate_fallback
|
||||
FROM %2$s
|
||||
WHERE canonical_govid IN (%3$s)
|
||||
AND year IN (%4$s)
|
||||
|
||||
@@ -1,15 +1,25 @@
|
||||
# R/views.R
|
||||
|
||||
# SQL files whose view definitions read schema-v5-only parquet tables
|
||||
# (harmonization_map.parquet, harmonization_recipes.parquet,
|
||||
# series_breaks.parquet) or select from views built on top of them. DuckDB's
|
||||
# read_parquet() resolves the file at CREATE VIEW time (even for a view, it
|
||||
# still needs the source schema) and errors immediately -- "IO Error: No
|
||||
# files found" -- if the path doesn't exist, so these cannot be registered
|
||||
# unconditionally against a v4 corpus the way the rest of inst/sql/ is.
|
||||
# Registration is therefore gated on manifest$schema_version >= 5; verb-level
|
||||
# *usage* of the resulting views is separately gated by .resolve_basis() /
|
||||
# .require_schema_v5().
|
||||
# SQL files that cannot be registered unconditionally against a v4 corpus,
|
||||
# for one of two distinct reasons -- both fail at CREATE VIEW time (DuckDB
|
||||
# resolves a view's source schema eagerly, even though it defers execution),
|
||||
# so a v4 corpus can't tolerate either unconditionally:
|
||||
#
|
||||
# (a) Missing FILE. 33-/34-/35- read_parquet() a v5-only parquet table
|
||||
# (harmonization_map.parquet, harmonization_recipes.parquet,
|
||||
# series_breaks.parquet) that doesn't exist at all on a v4 corpus --
|
||||
# "IO Error: No files found".
|
||||
#
|
||||
# (b) Missing COLUMN. 22-/23-/25- reference `long.harmonized_code`, a
|
||||
# column that does not exist on a v4 corpus's `long` table (harmonized
|
||||
# space was introduced in schema v5) -- "Binder Error: Referenced
|
||||
# column harmonized_code not found". 42-/43-/45- are on this list only
|
||||
# because they SELECT s.* FROM the (a)/(b) views above, so they'd fail
|
||||
# to resolve their own source view if it weren't already skipped.
|
||||
#
|
||||
# Registration is therefore gated on manifest$schema_version >= 5 for all of
|
||||
# them; verb-level *usage* of the resulting views is separately gated by
|
||||
# .resolve_basis() / .require_schema_v5().
|
||||
.harmonization_view_files <- c(
|
||||
"22-spending_long_harmonized.sql",
|
||||
"23-revenue_long_harmonized.sql",
|
||||
|
||||
Reference in New Issue
Block a user