diff --git a/R/views.R b/R/views.R index 28bf42c..5d1c4dc 100644 --- a/R/views.R +++ b/R/views.R @@ -78,6 +78,55 @@ file %in% basename(paths) } +#' Build the SQL path expression for the partitioned `long` table. +#' +#' DuckDB cannot expand a glob over generic HTTP: there is no directory +#' listing to expand against, and `allow_asterisks_in_http_paths` only +#' forwards the literal `**/*` as a filename, which 404s. Measured against +#' the published corpus on 2026-08-08, an explicit file list returns the +#' same 46,148,034 rows the (working) `hf://` glob does, and +#' `hive_partitioning = true` still recovers `year` from the paths. +#' +#' The manifest already enumerates every partition, so we build the list +#' from it. This is host-agnostic -- Nextcloud, HuggingFace and a local +#' fixture take the same path -- where an `hf://` URL would tie the reader +#' to one vendor's protocol and still need special-casing, since manifest +#' fetching goes through httr2, which cannot speak `hf://`. +#' +#' Falls back to the glob when the manifest carries no partition list: a +#' hand-built manifest in a test (see test-views.R) or a corpus predating +#' the field. Both are local, where globbing works. +#' @noRd +.long_files_sql <- function(url, manifest) { + parts <- manifest$files$long_partitions %||% list() + if (length(parts) == 0L) { + return(.sql_lit_chr(paste0(url, "data/long/**/*.parquet"))) + } + paths <- vapply(parts, function(p) as.character(p$path), character(1)) + paste0("[", .sql_lit_chr(paste0(url, paths)), "]") +} + +#' Substitute the corpus-location tokens in a view's SQL text. +#' +#' One place knows the token vocabulary. `.register_views()` and the tests +#' that execute a view file directly both route through here. This exists +#' because four test sites had hand-rolled the `{url}` substitution -- one +#' of them commented as doing it "exactly as .register_views() does" -- and +#' every one of them broke the moment a second token was introduced. +#' +#' `{long_files}` must be substituted BEFORE `{url}`: it expands to a string +#' that itself contains the url, so the reverse order leaves the token in +#' place and DuckDB's parser fails on the brace. +#' +#' `manifest` defaults to empty, which routes `.long_files_sql()` to its glob +#' fallback -- correct for the local temp corpora the direct-execution tests +#' build. +#' @noRd +.render_view_sql <- function(sql, url, manifest = list()) { + sql <- gsub("\\{long_files\\}", .long_files_sql(url, manifest), sql, fixed = FALSE) + gsub("\\{url\\}", url, sql, fixed = FALSE) +} + #' Register DuckDB views from inst/sql/ SQL files #' @noRd .register_views <- function(con, url, manifest) { @@ -91,7 +140,7 @@ !.corpus_has_table(manifest, .representation_view_files[[base]])) next if (base %in% .balance_view_files && !.corpus_has_balance_subtype(con)) next sql <- paste(readLines(f, warn = FALSE), collapse = "\n") - sql <- gsub("\\{url\\}", url, sql, fixed = FALSE) + sql <- .render_view_sql(sql, url, manifest) DBI::dbExecute(con, sql) } } diff --git a/inst/sql/10-long.sql b/inst/sql/10-long.sql index 5cf5fcc..d33f0c4 100644 --- a/inst/sql/10-long.sql +++ b/inst/sql/10-long.sql @@ -1,3 +1,7 @@ CREATE OR REPLACE VIEW long AS SELECT * -FROM read_parquet('{url}data/long/**/*.parquet', hive_partitioning = true); +-- {long_files} carries its own quoting: a bracketed list of every partition +-- the manifest enumerates, or a single quoted glob on fallback. Do NOT wrap +-- it in quotes. See .long_files_sql() in R/views.R for why a glob alone +-- cannot work over HTTP. +FROM read_parquet({long_files}, hive_partitioning = true); diff --git a/tests/testthat/test-balances.R b/tests/testthat/test-balances.R index 86d5ba4..c278a73 100644 --- a/tests/testthat/test-balances.R +++ b/tests/testthat/test-balances.R @@ -66,7 +66,7 @@ test_that("inst/sql/26-balance_long.sql enforces NOT is_aggregate (real SQL text 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) + uscogdata:::.render_view_sql(txt, paste0(tmp, "/")) } con <- DBI::dbConnect(duckdb::duckdb()) diff --git a/tests/testthat/test-long-files.R b/tests/testthat/test-long-files.R new file mode 100644 index 0000000..af84007 --- /dev/null +++ b/tests/testthat/test-long-files.R @@ -0,0 +1,64 @@ +test_that(".long_files_sql enumerates every partition the manifest lists", { + manifest <- list(files = list(long_partitions = list( + list(year = 2011L, path = "data/long/year=2011/part-0.parquet"), + list(year = 2012L, path = "data/long/year=2012/part-0.parquet") + ))) + expect_equal( + uscogdata:::.long_files_sql("https://example.org/corpus/", manifest), + paste0( + "['https://example.org/corpus/data/long/year=2011/part-0.parquet',", + "'https://example.org/corpus/data/long/year=2012/part-0.parquet']" + ) + ) +}) + +test_that(".long_files_sql falls back to the glob when no partition list is present", { + # test-views.R registers views with a hand-built manifest that has no + # `files` element. That must keep working: the glob is valid for the + # local paths such a manifest is used with. + expect_equal( + uscogdata:::.long_files_sql("/tmp/corpus/", list(schema_version = 4L)), + "'/tmp/corpus/data/long/**/*.parquet'" + ) + expect_equal( + uscogdata:::.long_files_sql("/tmp/corpus/", list(files = list(long_partitions = list()))), + "'/tmp/corpus/data/long/**/*.parquet'" + ) +}) + +test_that("the enumerated list matches the bundled fixture's partition count", { + skip_if_no_corpus() + m <- jsonlite::fromJSON( + file.path(fixture_corpus_path(), "manifest.json"), simplifyVector = FALSE + ) + out <- uscogdata:::.long_files_sql(fixture_corpus_path(), m) + expect_equal( + lengths(regmatches(out, gregexpr("part-0\\.parquet", out))), + length(m$files$long_partitions) + ) +}) + +test_that("no view SQL survives rendering with an unsubstituted token", { + # Introducing {long_files} broke four test sites that had hand-rolled the + # {url} substitution -- each failed with a DuckDB parser error on the + # surviving brace. This asserts the whole SQL directory renders clean, so + # a future token cannot reintroduce that silently. + sql_dir <- system.file("sql", package = "uscogdata") + for (f in list.files(sql_dir, pattern = "\\.sql$", full.names = TRUE)) { + rendered <- uscogdata:::.render_view_sql( + paste(readLines(f, warn = FALSE), collapse = "\n"), "/tmp/corpus/" + ) + expect_false(grepl("\\{[a-z_]+\\}", rendered), label = basename(f)) + } +}) + +test_that("registered `long` view reads through the enumerated list", { + skip_if_no_corpus() + with_fixture_corpus({ + con <- uscogdata:::.ensure_session() + n <- DBI::dbGetQuery(con, "SELECT count(*) AS n FROM long")$n + expect_gt(n, 0) + yrs <- DBI::dbGetQuery(con, "SELECT DISTINCT year FROM long ORDER BY year")$year + expect_true(all(c(2011, 2012, 2019, 2020) %in% yrs)) + }) +}) diff --git a/tests/testthat/test-views.R b/tests/testthat/test-views.R index 654c744..08c548c 100644 --- a/tests/testthat/test-views.R +++ b/tests/testthat/test-views.R @@ -103,7 +103,7 @@ test_that("inst/sql/22- and 23- harmonized views enforce every WHERE predicate ( 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) + uscogdata:::.render_view_sql(txt, paste0(tmp, "/")) } con <- DBI::dbConnect(duckdb::duckdb()) @@ -183,7 +183,7 @@ test_that("inst/sql/24- and 25- IG views retain aggregates, COALESCE NULL harmon 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) + uscogdata:::.render_view_sql(txt, paste0(tmp, "/")) } con <- DBI::dbConnect(duckdb::duckdb()) @@ -335,7 +335,7 @@ test_that(".harmonization_view_files guard is necessary: registration against a 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\\}", url, txt, fixed = FALSE) + uscogdata:::.render_view_sql(txt, url) } con2 <- DBI::dbConnect(duckdb::duckdb()) on.exit(DBI::dbDisconnect(con2, shutdown = TRUE), add = TRUE)