feat: limit/offset on cog_gov_search() and cog_balances() (#57)
verbs were left materializing everything and slicing in R -- the pattern behind the 2026-08-06 production incident. cog_gov_search() had no LIMIT at all, so an unfiltered call returns the entire 40,336-row crosswalk. Extracted the #39 machinery into R/pagination.R first (.validate_pagination(), .paginate_sql(), .take_pagination_total()) rather than growing a third inline copy: three definitions of what total_rows means is three places for it to drift. Conflict refusals stay at the call sites because each verb's conflict set differs. .verb_spendrev() now uses the shared helpers and is unchanged in behaviour. The empty-page fallback query is now passed as a thunk, so the unpaginated SQL is only BUILT when an offset actually lands past the end instead of on every paged call. Two things #57 did not anticipate: - cog_gov_search()'s ORDER BY was not a total order. population_acs DESC NULLS LAST leaves ties -- and the whole NULL block -- in scan order, so two requests can order them differently and a paged sweep duplicates one row while dropping another. Added canonical_govid as tiebreaker. Unpaginated output changes only in the relative order of already-tied rows. - Basket mode returns one resolved row per requested name plus a sidecar covering all of them, so a page of it is not a page of anything the caller asked for. Refused with uscogdata_basket_pagination_conflict rather than silently ignoring the arguments. Both default to NULL, so cog-api adopts them behind its existing formals() probe with no lockstep deploy. Suite: 1067 passed, 0 failed, 0 warnings (2 pre-existing live-corpus skips).
This commit is contained in:
@@ -0,0 +1,81 @@
|
||||
# R/pagination.R
|
||||
#
|
||||
# Shared limit/offset machinery. #39 established the semantics inside
|
||||
# .verb_spendrev(); #57 extends them to cog_gov_search() and cog_balances(),
|
||||
# which is what made a single definition worth having: three inline copies of
|
||||
# "coerce, refuse, unwrap the count" would be three places for the meaning of
|
||||
# `total_rows` to drift.
|
||||
#
|
||||
# The SQL side stays in .build_verb_sql() (R/spending.R) -- it already wraps
|
||||
# the aggregate in an outer SELECT so COUNT(*) OVER() sees the post-GROUP-BY
|
||||
# row count rather than the pre-aggregation one, and that is the subtle part
|
||||
# worth not duplicating either.
|
||||
|
||||
#' Coerce and check a limit/offset pair.
|
||||
#'
|
||||
#' Returns the coerced pair, or NULL for `limit` when no page was requested.
|
||||
#' `offset` defaults to 0 whenever `limit` is set, so a caller can supply just
|
||||
#' `limit` and get the first page.
|
||||
#'
|
||||
#' Conflicts with other arguments are deliberately NOT checked here: they
|
||||
#' differ per verb (`complete`/`recipe` for the money verbs, basket mode for
|
||||
#' `cog_gov_search()`, `recipe` alone for `cog_balances()`), and a shared
|
||||
#' function taking a list of conflict flags would be harder to read than the
|
||||
#' three explicit refusals at the call sites.
|
||||
#' @noRd
|
||||
.validate_pagination <- function(limit, offset) {
|
||||
if (is.null(limit)) {
|
||||
return(list(limit = NULL, offset = NULL))
|
||||
}
|
||||
limit <- as.integer(limit)
|
||||
if (length(limit) != 1L || is.na(limit) || limit < 0L) {
|
||||
cli::cli_abort("`limit` must be a single non-negative integer.",
|
||||
class = "uscogdata_invalid_pagination")
|
||||
}
|
||||
offset <- if (is.null(offset)) 0L else as.integer(offset)
|
||||
if (length(offset) != 1L || is.na(offset) || offset < 0L) {
|
||||
cli::cli_abort("`offset` must be a single non-negative integer.",
|
||||
class = "uscogdata_invalid_pagination")
|
||||
}
|
||||
list(limit = limit, offset = offset)
|
||||
}
|
||||
|
||||
#' Wrap a query so one page comes back carrying the unpaginated total.
|
||||
#'
|
||||
#' `COUNT(*) OVER()` rides along as an ordinary column, so the caller gets the
|
||||
#' true total from the SAME scan instead of a second round trip. The outer
|
||||
#' `SELECT *` matters: appending LIMIT/OFFSET directly to a grouped query would
|
||||
#' have the window function count pre-aggregation rows.
|
||||
#' @noRd
|
||||
.paginate_sql <- function(base_sql, limit, offset) {
|
||||
if (is.null(limit)) return(base_sql)
|
||||
sprintf(
|
||||
"SELECT *, COUNT(*) OVER() AS pagination_total_rows
|
||||
FROM (%s) AS _paged
|
||||
LIMIT %d OFFSET %d",
|
||||
base_sql, limit, offset
|
||||
)
|
||||
}
|
||||
|
||||
#' Strip the count column back out and report the unpaginated total.
|
||||
#'
|
||||
#' Returns `list(result = , total_rows = )`.
|
||||
#'
|
||||
#' An empty page -- an offset past the end -- carries no row to read the window
|
||||
#' function off, so that one case falls back to a second, unpaginated
|
||||
#' `COUNT(*)` rather than reporting a wrong zero. `unpaged_sql` is passed as a
|
||||
#' function so the fallback query is only BUILT when it is actually needed;
|
||||
#' every caller's unpaginated SQL is otherwise constructed on every paged call
|
||||
#' and thrown away.
|
||||
#' @noRd
|
||||
.take_pagination_total <- function(result, con, unpaged_sql) {
|
||||
if (nrow(result) > 0L) {
|
||||
total <- result$pagination_total_rows[[1]]
|
||||
result$pagination_total_rows <- NULL
|
||||
return(list(result = result, total_rows = as.integer(total)))
|
||||
}
|
||||
count_sql <- sprintf("SELECT COUNT(*) AS n FROM (%s) AS _uncounted",
|
||||
if (is.function(unpaged_sql)) unpaged_sql() else unpaged_sql)
|
||||
list(result = result,
|
||||
total_rows = as.integer(DBI::dbGetQuery(con, count_sql)$n[[1]]))
|
||||
}
|
||||
Reference in New Issue
Block a user