From 77f48047b17d3b73a0eaa1889145bf60b65326fc Mon Sep 17 00:00:00 2001 From: Jared Knowles Date: Sat, 18 Jul 2026 23:55:48 -0400 Subject: [PATCH] fix: exercise real harmonized-view SQL in tests; unambiguous recipe provenance test-views.R's harmonized-view test previously ran a hand-rolled REPLACE query with no WHERE clause, so a regression in any of inst/sql/22-spending_long_harmonized.sql / 23-revenue_long_harmonized.sql's three predicates (NOT is_aggregate, harmonized_code IS NOT NULL, the E/F/G/K or T/A/U/B/C/D prefix filter) would go uncaught. Replaced it with a test that reads the real SQL files off disk, substitutes {url} exactly as .register_views() does, and executes them (plus their 10-long.sql dependency) against a synthetic hive-partitioned parquet tree written via DuckDB's own COPY ... TO (FORMAT PARQUET) (no arrow dependency, matching this package's existing convention). Ten rows are crafted so each predicate is independently falsifiable by a specific row; manually broke each predicate in turn to confirm the test fails exactly as expected, then restored the SQL files (see the task report for the RED-phase transcript). Also fixes a provenance ambiguity: a recipe= query bypasses spending_annotated(_harmonized)/revenue_annotated(_harmonized) entirely (.run_recipe() joins `long` directly), so basis= has no effect on it, but provenance was still reporting basis = "harmonized"/"raw" (whatever the argument resolved to) with harmonization$applied = FALSE alongside it -- misleading, since it looks like harmonization was evaluated and found nothing to exclude rather than "not applicable here." Recipe results now report basis = "recipe" with an inert harmonization block carrying an explicit note, regardless of what basis= was passed. --- R/spending.R | 47 ++++++++++---- man/cog_revenue.Rd | 11 +++- man/cog_spending.Rd | 11 +++- tests/testthat/test-recipes.R | 30 ++++++++- tests/testthat/test-views.R | 113 ++++++++++++++++++++++++++-------- 5 files changed, 169 insertions(+), 43 deletions(-) diff --git a/R/spending.R b/R/spending.R index 70582b6..bb4f641 100644 --- a/R/spending.R +++ b/R/spending.R @@ -28,13 +28,20 @@ #' folding). On a corpus with `schema_version < 5` (no harmonization #' tables), `basis` silently resolves to `"raw"` when left at its default #' and the resolution is recorded in the provenance; explicitly passing -#' `basis = "harmonized"` on such a corpus aborts. +#' `basis = "harmonized"` on such a corpus aborts. Ignored when `recipe` +#' is set (see below). #' @param recipe Optional harmonization recipe id (see [cog_recipes()]) for #' multi-code cross-vintage series that a 1:1 harmonized_code mapping #' can't express (e.g. a wide-era aggregate that only splits into leaf #' codes in the modern era). Mutually exclusive with `category`. The #' result's subtype column reads `"recipe"` and `category` reads the -#' recipe's label. Requires `schema_version >= 5`. +#' recipe's label. Requires `schema_version >= 5`. A recipe query bypasses +#' `basis` entirely (it joins `long` directly rather than going through +#' the `*_annotated`/`*_annotated_harmonized` views), so the `basis` +#' argument is ignored and the result's provenance reports +#' `basis = "recipe"` with an inert `harmonization` block (`applied = +#' FALSE`, pointing at the `recipe` block instead) rather than a +#' possibly-misleading `"harmonized"`/`"raw"` value. #' @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`, @@ -109,14 +116,32 @@ cog_spending <- function(govid, years, category = NULL, result$notes <- .notes_column(result) - harmonization <- .build_harmonization_block( - con, govid, years, resolved, flow_prefixes - ) - - suggestions <- if (is.null(recipe)) { - .build_suggestions(con, govid, years, category, result, resolved$basis) + # A recipe result doesn't go through spending_annotated(_harmonized) / + # revenue_annotated(_harmonized) at all -- .run_recipe()'s generic join + # reads `long` directly -- so `basis` and the `harmonization` exclusion + # count (which is itself computed from `long`, independent of which view + # a non-recipe query used) would describe a code path this result never + # took. Rather than report a technically-still-computed but misleading + # basis = "harmonized"/"raw" + harmonization$applied combo, recipe + # results report basis = "recipe" and an explicit, inert harmonization + # block pointing at the `recipe` block instead. Task 12 (cog-api) passes + # provenance through verbatim, so this needs to be unambiguous rather + # than technically-defensible-but-confusing. + if (!is.null(recipe)) { + basis_for_prov <- "recipe" + basis_note_for_prov <- NA_character_ + harmonization <- list( + applied = FALSE, na_rows_excluded = 0L, na_amount_excluded = 0, + note = "basis/harmonization not applicable to recipe results; see the recipe block instead" + ) + suggestions <- list() } else { - list() + basis_for_prov <- resolved$basis + basis_note_for_prov <- resolved$note + harmonization <- .build_harmonization_block( + con, govid, years, resolved, flow_prefixes + ) + suggestions <- .build_suggestions(con, govid, years, category, result, resolved$basis) } prov <- .build_provenance( @@ -130,8 +155,8 @@ cog_spending <- function(govid, years, category = NULL, result = result, sql = sql, subtype_col = subtype_col, - basis = resolved$basis, - basis_note = resolved$note, + basis = basis_for_prov, + basis_note = basis_note_for_prov, harmonization = harmonization, recipe = recipe_block, suggestions = suggestions diff --git a/man/cog_revenue.Rd b/man/cog_revenue.Rd index 18b77df..843d48e 100644 --- a/man/cog_revenue.Rd +++ b/man/cog_revenue.Rd @@ -40,14 +40,21 @@ reproduces the pre-Phase-R2 behavior (published item codes, no folding). On a corpus with `schema_version < 5` (no harmonization tables), `basis` silently resolves to `"raw"` when left at its default and the resolution is recorded in the provenance; explicitly passing -`basis = "harmonized"` on such a corpus aborts.} +`basis = "harmonized"` on such a corpus aborts. Ignored when `recipe` +is set (see below).} \item{recipe}{Optional harmonization recipe id (see [cog_recipes()]) for multi-code cross-vintage series that a 1:1 harmonized_code mapping can't express (e.g. a wide-era aggregate that only splits into leaf codes in the modern era). Mutually exclusive with `category`. The result's subtype column reads `"recipe"` and `category` reads the -recipe's label. Requires `schema_version >= 5`.} +recipe's label. Requires `schema_version >= 5`. A recipe query bypasses +`basis` entirely (it joins `long` directly rather than going through +the `*_annotated`/`*_annotated_harmonized` views), so the `basis` +argument is ignored and the result's provenance reports +`basis = "recipe"` with an inert `harmonization` block (`applied = +FALSE`, pointing at the `recipe` block instead) rather than a +possibly-misleading `"harmonized"`/`"raw"` value.} } \value{ Tibble with columns `year`, `canonical_govid`, `gov_name`, diff --git a/man/cog_spending.Rd b/man/cog_spending.Rd index 8e4c52a..f1016ec 100644 --- a/man/cog_spending.Rd +++ b/man/cog_spending.Rd @@ -40,14 +40,21 @@ reproduces the pre-Phase-R2 behavior (published item codes, no folding). On a corpus with `schema_version < 5` (no harmonization tables), `basis` silently resolves to `"raw"` when left at its default and the resolution is recorded in the provenance; explicitly passing -`basis = "harmonized"` on such a corpus aborts.} +`basis = "harmonized"` on such a corpus aborts. Ignored when `recipe` +is set (see below).} \item{recipe}{Optional harmonization recipe id (see [cog_recipes()]) for multi-code cross-vintage series that a 1:1 harmonized_code mapping can't express (e.g. a wide-era aggregate that only splits into leaf codes in the modern era). Mutually exclusive with `category`. The result's subtype column reads `"recipe"` and `category` reads the -recipe's label. Requires `schema_version >= 5`.} +recipe's label. Requires `schema_version >= 5`. A recipe query bypasses +`basis` entirely (it joins `long` directly rather than going through +the `*_annotated`/`*_annotated_harmonized` views), so the `basis` +argument is ignored and the result's provenance reports +`basis = "recipe"` with an inert `harmonization` block (`applied = +FALSE`, pointing at the `recipe` block instead) rather than a +possibly-misleading `"harmonized"`/`"raw"` value.} } \value{ Tibble with columns `year`, `canonical_govid`, `gov_name`, diff --git a/tests/testthat/test-recipes.R b/tests/testthat/test-recipes.R index 44a1bf3..0bf9518 100644 --- a/tests/testthat/test-recipes.R +++ b/tests/testthat/test-recipes.R @@ -69,7 +69,7 @@ test_that("recipe result carries a recipe provenance block with component rows", r <- cog_spending("121011212191", years = c(2011L, 2012L), recipe = "corrections_combined") prov <- attr(r, "provenance") - expect_equal(prov$basis, "harmonized") + expect_equal(prov$basis, "recipe") expect_equal(prov$category, "Corrections (functions 04+05 combined)") expect_type(prov$recipe, "list") expect_equal(prov$recipe$recipe_id, "corrections_combined") @@ -82,6 +82,34 @@ test_that("recipe result carries a recipe provenance block with component rows", expect_length(prov$suggestions, 0L) }) +test_that("recipe results report an unambiguous basis/harmonization, ignoring basis=", { + skip_if_no_corpus() + # A recipe query bypasses spending_annotated(_harmonized) entirely -- + # .run_recipe() joins `long` directly -- so `basis` must never read + # "harmonized"/"raw" (which would describe a code path this query never + # took) regardless of what the caller passed for `basis`. Task 12 + # consumes provenance verbatim, so this needs to be unambiguous. + r_default <- cog_spending("121011212191", years = c(2011L, 2012L), + recipe = "corrections_combined") + r_raw <- cog_spending("121011212191", years = c(2011L, 2012L), + recipe = "corrections_combined", basis = "raw") + r_harm <- cog_spending("121011212191", years = c(2011L, 2012L), + recipe = "corrections_combined", basis = "harmonized") + + for (r in list(r_default, r_raw, r_harm)) { + prov <- attr(r, "provenance") + expect_equal(prov$basis, "recipe") + expect_true(is.na(prov$basis_note)) + expect_false(prov$harmonization$applied) + expect_equal(prov$harmonization$na_rows_excluded, 0L) + expect_match(prov$harmonization$note, "recipe", ignore.case = TRUE) + } + + # basis= truly has zero effect on a recipe query's actual numbers. + expect_equal(r_raw$amt_nominal, r_harm$amt_nominal) + expect_equal(r_default$amt_nominal, r_raw$amt_nominal) +}) + test_that("recipe = 't19_selective_sales_wide' sums the local T11/T14 legs when present", { skip_if_no_corpus() # Westminster City, CA (canonical_govid 082001211654): T11 = 0 in 2011, diff --git a/tests/testthat/test-views.R b/tests/testthat/test-views.R index fe30aad..75eaa5b 100644 --- a/tests/testthat/test-views.R +++ b/tests/testthat/test-views.R @@ -14,41 +14,100 @@ test_that("all expected views register on session open", { expect_true(all(expected %in% views$table_name)) }) -test_that("the REPLACE(harmonized_code AS item_code) pattern folds a collapsed code", { - # spending_long_harmonized / revenue_long_harmonized (inst/sql/22-, 23-) - # are defined as: - # SELECT * REPLACE (harmonized_code AS item_code) FROM long WHERE ... +test_that("inst/sql/22- and 23- harmonized views enforce every WHERE predicate (real SQL text, synthetic parquet)", { + # spending_long_harmonized / revenue_long_harmonized are three-predicate + # views: + # SELECT * REPLACE (harmonized_code AS item_code) + # FROM long + # WHERE NOT is_aggregate + # AND harmonized_code IS NOT NULL + # AND LEFT(harmonized_code, 1) IN () # None of the curated harmonization_map's `collapse` rulings land inside # the bundled fixture's 2011-2020 window for spending/revenue-prefixed # codes (see the "basis = 'harmonized' (default) matches 'raw'" test in # test-spending.R and docs/phase_r_harmonization_review.md § 0.2/§ 2), so - # there is no real fixture row that exercises a nonzero fold. This test - # proves the REPLACE mechanism itself is correct against a synthetic - # long-shaped table with a deliberate E38 -> E36 collapse, independent of - # whether the bundled data happens to contain one right now. + # there is no real fixture row that exercises a nonzero fold or a + # predicate-excluded row. Rather than re-implement the WHERE clause by + # hand against an in-memory VALUES table (which would only prove the SQL + # *pattern* works, not that the deployed inst/sql/22-/23- text actually + # applies it), this test reads the real SQL files off disk, substitutes + # {url} exactly as .register_views() does, and executes them -- plus + # their 10-long.sql dependency -- against a synthetic hive-partitioned + # parquet tree written to a temp dir. A regression in any predicate (e.g. + # `NOT is_aggregate` dropped, the prefix list changed, the NULL guard + # removed) would change which of the rows below survive. + # + # The synthetic parquet is written with DuckDB's own COPY ... TO (FORMAT + # PARQUET) rather than the arrow package: this package has no arrow + # dependency (CLAUDE.md "No arrow dependency -- DuckDB reads parquet + # natively"), and DuckDB can round-trip its own parquet writer/reader + # without adding one for tests either. skip_if_no_corpus() - con <- cog_open() - on.exit(cog_close()) - DBI::dbExecute(con, " - CREATE OR REPLACE TEMP TABLE synthetic_long AS - SELECT * FROM (VALUES - ('121011212191', 2004, 'E36', 70942668, false, 'E36'), - ('121011212191', 2004, 'E38', 837485, false, 'E36'), - ('121011212191', 2004, 'E62', 100000, false, 'E62') - ) AS t(canonical_govid, year, item_code, amt, is_aggregate, harmonized_code) - ") + tmp <- withr::local_tempdir() + part_dir <- file.path(tmp, "data", "long", "year=2004") + dir.create(part_dir, recursive = TRUE) + part_path <- file.path(part_dir, "part-0.parquet") - folded <- DBI::dbGetQuery(con, " - SELECT item_code, SUM(amt) AS amt - FROM (SELECT * REPLACE (harmonized_code AS item_code) FROM synthetic_long) - GROUP BY item_code ORDER BY item_code - ") - expect_setequal(folded$item_code, c("E36", "E62")) - expect_equal(folded$amt[folded$item_code == "E36"], 70942668 + 837485) - expect_equal(folded$amt[folded$item_code == "E62"], 100000) + write_con <- DBI::dbConnect(duckdb::duckdb()) + on.exit(DBI::dbDisconnect(write_con, shutdown = TRUE), add = TRUE) + DBI::dbExecute(write_con, sprintf(" + COPY ( + SELECT * FROM (VALUES + -- Spending (E/F/G/K) rows, exercised against spending_long_harmonized: + ('spend-A', 'E36', 100, false, 'E36'), -- control: passes every predicate as-is + ('spend-B', 'E38', 50, false, 'E36'), -- collapse-fold: passes every predicate, renamed to E36 + ('spend-C', 'E05', 999999, true, 'E05'), -- excluded ONLY by `NOT is_aggregate` + ('spend-D', 'E99', 888888, false, NULL), -- excluded by `harmonized_code IS NOT NULL` + -- 'S74' is outside BOTH flow families (E/F/G/K spending and + -- T/A/U/B/C/D revenue -- it mirrors the real corpus's own + -- non-flow-type codes like S74/Z61), so it can only leak into + -- EITHER view via the E/F/G/K or T/A/U/B/C/D prefix filter, never + -- both at once -- a prefix drawn from the other view's own family + -- (e.g. a real T-code for the spending row) would incorrectly + -- leak into the other view's assertion below and not discriminate + -- the predicate under test. + ('spend-E', 'S74', 777777, false, 'S74'), -- excluded ONLY by the E/F/G/K prefix filter + -- Revenue (T/A/U/B/C/D) rows, exercised against revenue_long_harmonized: + ('rev-A', 'U11', 200, false, 'U11'), -- control: passes every predicate as-is + ('rev-B', 'U10', 25, false, 'U11'), -- collapse-fold: passes every predicate, renamed to U11 + ('rev-C', 'T29', 555555, true, 'T29'), -- excluded ONLY by `NOT is_aggregate` + ('rev-D', 'T88', 444444, false, NULL), -- excluded by `harmonized_code IS NOT NULL` + ('rev-E', 'Z61', 333333, false, 'Z61') -- excluded ONLY by the T/A/U/B/C/D prefix filter + ) AS t(canonical_govid, item_code, amt, is_aggregate, harmonized_code) + ) TO %s (FORMAT PARQUET) + ", uscogdata:::.sql_lit_chr(part_path))) - DBI::dbExecute(con, "DROP TABLE synthetic_long") + sql_dir <- system.file("sql", package = "uscogdata") + .read_view_sql <- function(filename) { + txt <- paste(readLines(file.path(sql_dir, filename), warn = FALSE), collapse = "\n") + gsub("\\{url\\}", paste0(tmp, "/"), txt, fixed = FALSE) + } + + con <- DBI::dbConnect(duckdb::duckdb()) + on.exit(DBI::dbDisconnect(con, shutdown = TRUE), add = TRUE) + DBI::dbExecute(con, .read_view_sql("10-long.sql")) + DBI::dbExecute(con, .read_view_sql("22-spending_long_harmonized.sql")) + DBI::dbExecute(con, .read_view_sql("23-revenue_long_harmonized.sql")) + + spend <- DBI::dbGetQuery(con, + "SELECT item_code, SUM(amt) AS amt FROM spending_long_harmonized + GROUP BY item_code ORDER BY item_code" + ) + # Exactly one surviving row: spend-C (aggregate), spend-D (NULL + # harmonized_code), and spend-E (wrong prefix family) must all be gone, + # and spend-A + spend-B must be folded together under E36. + expect_equal(nrow(spend), 1L) + expect_equal(spend$item_code, "E36") + expect_equal(spend$amt, 150) + + rev <- DBI::dbGetQuery(con, + "SELECT item_code, SUM(amt) AS amt FROM revenue_long_harmonized + GROUP BY item_code ORDER BY item_code" + ) + expect_equal(nrow(rev), 1L) + expect_equal(rev$item_code, "U11") + expect_equal(rev$amt, 225) }) test_that(".build_series_break_refs matches fin_code + break_year window", {