From 06cbb84dd044e1c2cf76fd15a1f8d661d7525c1c Mon Sep 17 00:00:00 2001 From: Carson Date: Tue, 13 Oct 2026 11:03:39 -0500 Subject: [PATCH 1/2] Add caching_reader() Expose the ggsql crate's CachingReader to R. Source reads hit the primary reader and are memoized (TTL + LRU byte budget); ggsql-generated derived tables run on a private in-memory DuckDB cache, so queries with layer-level sources (DRAW ... FROM ) work end to end. The primary can be a connection string (duckdb/odbc/snowflake, same dispatch as Reader$new()) or the execute_sql/register/unregister callbacks custom_reader() takes. Cache behavior follows the GGSQL_CACHE_TTL / GGSQL_CACHE_MAX_BYTES / GGSQL_CACHE_DISABLED env vars, overridable per reader via ttl_secs, max_bytes, and enabled. Also: - Track names registered via ggsql_register() on the Reader and merge them into ggsql_table_names(), since registrations on a caching reader land in the private cache, which the primary's SHOW TABLES never sees. - Surface the R condition message from failing reader hooks (via geterrmessage()) instead of extendr's opaque call dump; affects custom_reader() too. --- .gitignore | 2 + NAMESPACE | 1 + NEWS.md | 10 ++ R/extendr-wrappers.R | 4 + R/reader.R | 179 +++++++++++++++++++++++++-- man/caching_reader.Rd | 88 +++++++++++++ man/custom_reader.Rd | 1 + man/duckdb_reader.Rd | 1 + man/odbc_reader.Rd | 1 + man/snowflake_reader.Rd | 1 + src/rust/src/lib.rs | 136 +++++++++++++++++++- tests/testthat/test-caching-reader.R | 143 +++++++++++++++++++++ 12 files changed, 555 insertions(+), 12 deletions(-) create mode 100644 man/caching_reader.Rd create mode 100644 tests/testthat/test-caching-reader.R diff --git a/.gitignore b/.gitignore index 5a9ed2c..c2012f5 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,5 @@ node_modules src/vendor src/rust/vendor.tar.xz src/rust/vendor/ +sandbox/ +notes-*.md diff --git a/NAMESPACE b/NAMESPACE index afc4260..a2f9135 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -13,6 +13,7 @@ S3method(names,ggsql_tables) S3method(print,ggsql_tables) S3method(print,ggsql_validated) S3method(str,Spec) +export(caching_reader) export(custom_reader) export(duckdb_reader) export(ggsqlOutput) diff --git a/NEWS.md b/NEWS.md index c1d0bbd..05036b3 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,15 @@ # ggsql (development version) +* New `caching_reader()` wraps a primary reader (a connection string or R + callbacks, as in `custom_reader()`) with an in-memory DuckDB cache. Source + reads are memoized with a TTL and LRU byte budget, and ggsql-generated + derived tables run on the private cache, so queries with layer-level + sources (`VISUALISE ... DRAW ... FROM
`) work end to end. Cache + behavior is configured with the `GGSQL_CACHE_TTL`, + `GGSQL_CACHE_MAX_BYTES`, and `GGSQL_CACHE_DISABLED` environment + variables, overridable per reader via the `ttl_secs`, `max_bytes`, and + `enabled` arguments. + # ggsql 0.5.2 This release updates the bundled ggsql Rust library to 0.5.2 and moves display diff --git a/R/extendr-wrappers.R b/R/extendr-wrappers.R index 8038d5d..020ab74 100644 --- a/R/extendr-wrappers.R +++ b/R/extendr-wrappers.R @@ -18,6 +18,10 @@ GgsqlReader <- new.env(parent = emptyenv()) GgsqlReader$new <- function(connection) .Call(wrap__GgsqlReader__new, connection) +GgsqlReader$new_caching <- function(connection, ttl_secs, max_bytes, enabled) .Call(wrap__GgsqlReader__new_caching, connection, ttl_secs, max_bytes, enabled) + +GgsqlReader$new_caching_custom <- function(execute_sql, register, unregister, ttl_secs, max_bytes, enabled) .Call(wrap__GgsqlReader__new_caching_custom, execute_sql, register, unregister, ttl_secs, max_bytes, enabled) + GgsqlReader$new_custom <- function(execute_sql, register, unregister) .Call(wrap__GgsqlReader__new_custom, execute_sql, register, unregister) GgsqlReader$register_ipc <- function(name, ipc_bytes, replace) invisible(.Call(wrap__GgsqlReader__register_ipc, self, name, ipc_bytes, replace)) diff --git a/R/reader.R b/R/reader.R index e0346c0..41270ad 100644 --- a/R/reader.R +++ b/R/reader.R @@ -356,24 +356,177 @@ custom_reader <- function(execute_sql, register = NULL, unregister = NULL) { check_function(register, allow_null = TRUE) check_function(unregister, allow_null = TRUE) - # The Rust side always exchanges tables as Arrow IPC bytes. We wrap the - # user's hooks so Rust never sees an R data.frame. NULL maps to an empty - # frame — matches the public `ggsql_execute_sql()` contract for value-less - # statements (e.g. CREATE TABLE) that ggsql issues internally. + wrapped <- wrap_reader_callbacks(execute_sql, register, unregister) + reader <- Reader$new(NULL) + reader$.ptr <- GgsqlReader$new_custom( + wrapped$execute_sql, + wrapped$register, + wrapped$unregister + ) + reader +} + +# The Rust side always exchanges tables as Arrow IPC bytes. We wrap the +# user's hooks so Rust never sees an R data.frame. NULL maps to an empty +# frame — matches the public `ggsql_execute_sql()` contract for value-less +# statements (e.g. CREATE TABLE) that ggsql issues internally. +wrap_reader_callbacks <- function(execute_sql, register, unregister) { + # Re-raise hook errors without call info so the Rust side surfaces the + # bare condition message (via geterrmessage()) rather than a deparsed + # call — e.g. DuckDB's "Catalog Error: ..." reaches the caller intact. + raise_clean <- function(e) { + stop(conditionMessage(e), call. = FALSE) + } exec_wrapped <- function(sql) { - out <- execute_sql(sql) - if (is.null(out)) out <- data.frame() + out <- tryCatch(execute_sql(sql), error = raise_clean) + if (is.null(out)) { + out <- data.frame() + } if (is.raw(out)) out else df_to_ipc(out) } reg_wrapped <- if (!is.null(register)) { function(name, ipc_bytes, replace) { - register(name, ipc_to_df(ipc_bytes), isTRUE(replace)) + tryCatch( + register(name, ipc_to_df(ipc_bytes), isTRUE(replace)), + error = raise_clean + ) invisible(NULL) } } + unreg_wrapped <- if (!is.null(unregister)) { + function(name) { + tryCatch(unregister(name), error = raise_clean) + } + } + list( + execute_sql = exec_wrapped, + register = reg_wrapped, + unregister = unreg_wrapped + ) +} + +#' Create a caching reader +#' +#' Wraps a primary reader with an in-memory DuckDB cache. Source reads hit +#' the primary and are memoized (a time-to-live plus an LRU byte budget); +#' all ggsql-generated derived tables run on the private in-memory cache. +#' Because every layer source is staged through that cache, queries with +#' layer-level sources (`VISUALISE ... DRAW ... FROM
`) work through +#' a caching reader. +#' +#' @param primary Either a connection string (same schemes as +#' [duckdb_reader()] / [odbc_reader()] / [snowflake_reader()], e.g. +#' `"duckdb://memory"`) or a function `function(sql)` that executes `sql` +#' and returns a data frame (or raw Arrow IPC bytes), as in +#' [custom_reader()]. To wrap an existing reader, pass its connection +#' string instead — a `Reader` object cannot be wrapped directly. +#' @param register,unregister Optional callbacks used only when `primary` is +#' a function, with the same meaning as in [custom_reader()]. +#' @param ttl_secs Seconds before a memoized read is treated as stale. +#' `NULL` (the default) uses the `GGSQL_CACHE_TTL` environment variable, +#' falling back to 300. +#' @param max_bytes Cumulative byte budget across memoized results before +#' LRU eviction. `NULL` (the default) uses the `GGSQL_CACHE_MAX_BYTES` +#' environment variable, falling back to 512 MiB. +#' @param enabled When `FALSE`, reads always hit the primary. `NULL` (the +#' default) uses the `GGSQL_CACHE_DISABLED` environment variable, falling +#' back to `TRUE`. +#' +#' @section Registered tables and `ggsql_table_names()`: +#' Tables registered via [ggsql_register()] on a caching reader live in the +#' private cache, not the primary database, so the primary's `SHOW TABLES` +#' does not list them. [ggsql_table_names()] and [ggsql_table()] still see +#' them: registrations are tracked and merged into the backend's listing. +#' Note that `SHOW TABLES` is itself a memoizable read, so tables created +#' on the primary by other means (e.g. a `CREATE TABLE` statement) can take +#' up to `ttl_secs` to appear in the listing. +#' +#' @return A `Reader` object, usable anywhere the other `*_reader()` +#' constructors are accepted. +#' +#' @export +#' +#' @family readers +#' +#' @examples +#' reader <- caching_reader("duckdb://memory") +#' ggsql_register(reader, mtcars, "cars") +#' ggsql_execute_sql(reader, "SELECT mpg, disp FROM cars LIMIT 3") +#' +#' # Wrapping R callbacks (the custom-reader path): identical reads are +#' # served from the cache without hitting the primary. +#' backend <- duckdb_reader() +#' reader <- caching_reader( +#' primary = function(sql) ggsql_execute_sql(backend, sql), +#' register = function(name, df, replace) { +#' ggsql_register(backend, df, name, replace = replace) +#' }, +#' unregister = function(name) ggsql_unregister(backend, name) +#' ) +#' ggsql_register(reader, mtcars, "cars") +#' ggsql_execute_sql(reader, "SELECT mpg, disp FROM cars LIMIT 3") +#' +caching_reader <- function( + primary, + register = NULL, + unregister = NULL, + ttl_secs = NULL, + max_bytes = NULL, + enabled = NULL +) { + if (inherits(primary, "Reader")) { + cli::cli_abort( + paste( + "{.fn caching_reader} cannot wrap an existing {.cls Reader};", + "pass its connection string instead", + "(e.g. {.code caching_reader(\"duckdb://memory\")})." + ) + ) + } + check_function(register, allow_null = TRUE) + check_function(unregister, allow_null = TRUE) + check_number_decimal(ttl_secs, min = 0, allow_null = TRUE) + check_number_decimal(max_bytes, min = 0, allow_null = TRUE) + check_logical(enabled, allow_null = TRUE) + + ttl_secs <- if (!is.null(ttl_secs)) as.numeric(ttl_secs) + max_bytes <- if (!is.null(max_bytes)) as.numeric(max_bytes) reader <- Reader$new(NULL) - reader$.ptr <- GgsqlReader$new_custom(exec_wrapped, reg_wrapped, unregister) + if (is.function(primary)) { + wrapped <- wrap_reader_callbacks(primary, register, unregister) + reader$.ptr <- GgsqlReader$new_caching_custom( + wrapped$execute_sql, + wrapped$register, + wrapped$unregister, + ttl_secs, + max_bytes, + enabled + ) + } else { + check_string(primary, allow_empty = FALSE) + if (!is.null(register) || !is.null(unregister)) { + cli::cli_abort( + "{.arg register} and {.arg unregister} are only used when {.arg primary} is a function." + ) + } + schemes <- c("duckdb", "odbc", "snowflake") + scheme <- tolower(sub("://.*$", "", primary)) + if (!scheme %in% schemes) { + cli::cli_abort( + "Unsupported connection scheme {.val {scheme}}; expected one of {.or {.val {schemes}}}." + ) + } + reader$.ptr <- GgsqlReader$new_caching( + primary, + ttl_secs, + max_bytes, + enabled + ) + if (startsWith(tolower(primary), "duckdb://")) { + configure_duckdb_paths(reader$.ptr) + } + } reader } @@ -409,6 +562,8 @@ Reader <- R6::R6Class( cloneable = FALSE, public = list( .ptr = NULL, + # Names registered via `ggsql_register()`; see `ggsql_table_names()`. + .registered = character(0), initialize = function(connection) { # `NULL` lets subclasses / factory constructors install a custom @@ -465,6 +620,10 @@ ggsql_register <- function(reader, df, name, replace = FALSE) { check_string(name, allow_empty = FALSE) ipc_bytes <- df_to_ipc(df) reader$.ptr$register_ipc(name, ipc_bytes, replace) + # Track registrations so `ggsql_table_names()` can merge them into the + # backend's listing — on a caching reader they land in the private cache, + # which `SHOW TABLES` (routed to the primary) never sees. + reader$.registered <- union(reader$.registered, name) invisible(reader) } @@ -474,6 +633,7 @@ ggsql_unregister <- function(reader, name) { check_r6(reader, "Reader") check_string(name, allow_empty = FALSE) reader$.ptr$unregister(name) + reader$.registered <- setdiff(reader$.registered, name) invisible(reader) } @@ -491,7 +651,8 @@ ggsql_table <- function(reader, name) { #' @rdname ggsql_register #' @export ggsql_table_names <- function(reader) { - ggsql_execute_sql(reader, "SHOW TABLES")$name + check_r6(reader, "Reader") + union(ggsql_execute_sql(reader, "SHOW TABLES")$name, reader$.registered) } #' Execute a ggsql query diff --git a/man/caching_reader.Rd b/man/caching_reader.Rd new file mode 100644 index 0000000..cd126b1 --- /dev/null +++ b/man/caching_reader.Rd @@ -0,0 +1,88 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/reader.R +\name{caching_reader} +\alias{caching_reader} +\title{Create a caching reader} +\usage{ +caching_reader( + primary, + register = NULL, + unregister = NULL, + ttl_secs = NULL, + max_bytes = NULL, + enabled = NULL +) +} +\arguments{ +\item{primary}{Either a connection string (same schemes as +\code{\link{duckdb_reader}()} / \code{\link{odbc_reader}()} / \code{\link{snowflake_reader}()}, e.g. +\code{"duckdb://memory"}) or a function \verb{function(sql)} that executes \code{sql} +and returns a data frame (or raw Arrow IPC bytes), as in +\code{\link{custom_reader}()}. To wrap an existing reader, pass its connection +string instead — a \code{Reader} object cannot be wrapped directly.} + +\item{register, unregister}{Optional callbacks used only when \code{primary} is +a function, with the same meaning as in \code{\link{custom_reader}()}.} + +\item{ttl_secs}{Seconds before a memoized read is treated as stale. +\code{NULL} (the default) uses the \code{GGSQL_CACHE_TTL} environment variable, +falling back to 300.} + +\item{max_bytes}{Cumulative byte budget across memoized results before +LRU eviction. \code{NULL} (the default) uses the \code{GGSQL_CACHE_MAX_BYTES} +environment variable, falling back to 512 MiB.} + +\item{enabled}{When \code{FALSE}, reads always hit the primary. \code{NULL} (the +default) uses the \code{GGSQL_CACHE_DISABLED} environment variable, falling +back to \code{TRUE}.} +} +\value{ +A \code{Reader} object, usable anywhere the other \verb{*_reader()} +constructors are accepted. +} +\description{ +Wraps a primary reader with an in-memory DuckDB cache. Source reads hit +the primary and are memoized (a time-to-live plus an LRU byte budget); +all ggsql-generated derived tables run on the private in-memory cache. +Because every layer source is staged through that cache, queries with +layer-level sources (\verb{VISUALISE ... DRAW ... FROM
}) work through +a caching reader. +} +\section{Registered tables and \code{ggsql_table_names()}}{ + +Tables registered via \code{\link{ggsql_register}()} on a caching reader live in the +private cache, not the primary database, so the primary's \verb{SHOW TABLES} +does not list them. \code{\link{ggsql_table_names}()} and \code{\link{ggsql_table}()} still see +them: registrations are tracked and merged into the backend's listing. +Note that \verb{SHOW TABLES} is itself a memoizable read, so tables created +on the primary by other means (e.g. a \verb{CREATE TABLE} statement) can take +up to \code{ttl_secs} to appear in the listing. +} + +\examples{ +reader <- caching_reader("duckdb://memory") +ggsql_register(reader, mtcars, "cars") +ggsql_execute_sql(reader, "SELECT mpg, disp FROM cars LIMIT 3") + +# Wrapping R callbacks (the custom-reader path): identical reads are +# served from the cache without hitting the primary. +backend <- duckdb_reader() +reader <- caching_reader( + primary = function(sql) ggsql_execute_sql(backend, sql), + register = function(name, df, replace) { + ggsql_register(backend, df, name, replace = replace) + }, + unregister = function(name) ggsql_unregister(backend, name) +) +ggsql_register(reader, mtcars, "cars") +ggsql_execute_sql(reader, "SELECT mpg, disp FROM cars LIMIT 3") + +} +\seealso{ +Other readers: +\code{\link{custom_reader}()}, +\code{\link{duckdb_reader}()}, +\code{\link{odbc_reader}()}, +\code{\link{snowflake_reader}()} +} +\concept{readers} diff --git a/man/custom_reader.Rd b/man/custom_reader.Rd index c2e32b0..b57d017 100644 --- a/man/custom_reader.Rd +++ b/man/custom_reader.Rd @@ -56,6 +56,7 @@ ggsql_execute_sql(reader, "SELECT mpg, disp FROM cars LIMIT 3") } \seealso{ Other readers: +\code{\link{caching_reader}()}, \code{\link{duckdb_reader}()}, \code{\link{odbc_reader}()}, \code{\link{snowflake_reader}()} diff --git a/man/duckdb_reader.Rd b/man/duckdb_reader.Rd index 7176ba3..69166a1 100644 --- a/man/duckdb_reader.Rd +++ b/man/duckdb_reader.Rd @@ -27,6 +27,7 @@ df <- ggsql_execute_sql(reader, "SELECT mpg, disp FROM cars LIMIT 5") } \seealso{ Other readers: +\code{\link{caching_reader}()}, \code{\link{custom_reader}()}, \code{\link{odbc_reader}()}, \code{\link{snowflake_reader}()} diff --git a/man/odbc_reader.Rd b/man/odbc_reader.Rd index afaa353..0f158d3 100644 --- a/man/odbc_reader.Rd +++ b/man/odbc_reader.Rd @@ -76,6 +76,7 @@ reader <- odbc_reader("Driver={SQLite3};Database=:memory:") } \seealso{ Other readers: +\code{\link{caching_reader}()}, \code{\link{custom_reader}()}, \code{\link{duckdb_reader}()}, \code{\link{snowflake_reader}()} diff --git a/man/snowflake_reader.Rd b/man/snowflake_reader.Rd index 28d63d9..bef06f8 100644 --- a/man/snowflake_reader.Rd +++ b/man/snowflake_reader.Rd @@ -84,6 +84,7 @@ reader <- snowflake_reader( } \seealso{ Other readers: +\code{\link{caching_reader}()}, \code{\link{custom_reader}()}, \code{\link{duckdb_reader}()}, \code{\link{odbc_reader}()} diff --git a/src/rust/src/lib.rs b/src/rust/src/lib.rs index fb3f733..a804c93 100644 --- a/src/rust/src/lib.rs +++ b/src/rust/src/lib.rs @@ -3,7 +3,8 @@ use extendr_api::prelude::*; use arrow::ipc::reader::StreamReader; use arrow::ipc::writer::StreamWriter; use arrow::record_batch::RecordBatch; -use ggsql::reader::{execute_with_reader, DuckDBReader, OdbcReader, Reader, Spec}; +use ggsql::reader::cache::{CacheConfig, CacheConfigOverride}; +use ggsql::reader::{execute_with_reader, CachingReader, DuckDBReader, OdbcReader, Reader, Spec}; use ggsql::validate::validate as rust_validate; use ggsql::writer::{ HepWriter, PdfWriter, SvgWriter, VegaLiteWriter as RustVegaLiteWriter, Writer, @@ -58,14 +59,31 @@ struct RCallbackReader { unregister_fn: Option, } +// SAFETY: `Robj` is not `Send` because R's C API is single-threaded, but an +// `RCallbackReader` never actually crosses threads: it is created, invoked, +// and dropped entirely on the R main thread (via `.Call` into `GgsqlReader` +// methods and R's GC finalizer). The `Send` bound on `CachingReader`'s +// primary reader is therefore honored by construction. +unsafe impl Send for RCallbackReader {} + impl RCallbackReader { fn call_fn(fun: &Robj, args: Pairlist, hook: &str) -> std::result::Result { let f: Function = fun .clone() .try_into() .map_err(|_| GgsqlError::ReaderError(format!("{hook} hook is not an R function")))?; - f.call(args) - .map_err(|e| GgsqlError::ReaderError(format!("{hook} hook failed: {e}"))) + f.call(args).map_err(|e| { + // extendr's error Display shows the failed call, not the R + // condition message; surface the message so consumers (e.g. an + // LLM tool loop) see the underlying "Catalog Error: ..." rather + // than an opaque wrapper. + let detail = R!("geterrmessage()") + .ok() + .and_then(|r| r.as_str().map(|s| s.trim().to_string())) + .filter(|s| !s.is_empty()) + .unwrap_or_else(|| e.to_string()); + GgsqlError::ReaderError(format!("{hook} hook failed: {detail}")) + }) } } @@ -119,6 +137,60 @@ enum InnerReader { DuckDB(DuckDBReader), Odbc(OdbcReader), Custom(RCallbackReader), + Caching(CachingReader), +} + +/// Build a native reader from a connection string, dispatching on the URI +/// scheme (same rules as `GgsqlReader::new`). +fn native_reader_from_connection( + connection: &str, +) -> std::result::Result, String> { + let scheme = connection.split("://").next().unwrap_or("").to_lowercase(); + match scheme.as_str() { + "duckdb" => DuckDBReader::from_connection_string(connection) + .map(|r| Box::new(r) as Box) + .map_err(|e| e.to_string()), + "odbc" => OdbcReader::from_connection_string(connection) + .map(|r| Box::new(r) as Box) + .map_err(|e| e.to_string()), + "snowflake" => { + // See the snowflake arm of `GgsqlReader::new` for why the URI is + // rewritten to odbc:// with Driver=Snowflake. + let params = connection.split_once("://").map(|x| x.1).unwrap_or(""); + let odbc_uri = if params.to_lowercase().contains("driver=") { + format!("odbc://{params}") + } else { + format!("odbc://Driver=Snowflake;{params}") + }; + OdbcReader::from_connection_string(&odbc_uri) + .map(|r| Box::new(r) as Box) + .map_err(|e| e.to_string()) + } + other => Err(format!("Unsupported connection scheme: {other}")), + } +} + +/// Merge optional per-field overrides (from R `NULL`s) onto env-derived +/// cache configuration. +fn cache_config( + ttl_secs: Nullable, + max_bytes: Nullable, + enabled: Nullable, +) -> CacheConfig { + CacheConfig::from_env().merge(CacheConfigOverride { + ttl_secs: match ttl_secs { + Nullable::NotNull(v) if v >= 0.0 => Some(v as u64), + _ => None, + }, + max_bytes: match max_bytes { + Nullable::NotNull(v) if v >= 0.0 => Some(v as u64), + _ => None, + }, + enabled: match enabled { + Nullable::NotNull(v) => Some(v), + _ => None, + }, + }) } #[extendr] @@ -162,6 +234,60 @@ impl GgsqlReader { Self { inner } } + /// Create a caching reader wrapping a native primary (given as a + /// connection string). Source reads hit the primary and are memoized + /// (TTL + LRU byte budget); derived tables live in a private in-memory + /// DuckDB cache. `NULL` config values fall back to the GGSQL_CACHE_* + /// environment variables, then defaults. + fn new_caching( + connection: &str, + ttl_secs: Nullable, + max_bytes: Nullable, + enabled: Nullable, + ) -> Self { + let cache = DuckDBReader::from_connection_string("duckdb://memory") + .expect("Failed to create the in-memory cache backend"); + let primary = native_reader_from_connection(connection) + .expect("Failed to create the caching reader's primary"); + Self { + inner: InnerReader::Caching(CachingReader::with_config( + primary, + Box::new(cache), + connection, + "duckdb", + cache_config(ttl_secs, max_bytes, enabled), + )), + } + } + + /// Create a caching reader wrapping a callback (custom) primary. The URI + /// only feeds cache-key derivation, which needs a stable-per-instance + /// string; the cache is private to each reader, so a constant suffices. + fn new_caching_custom( + execute_sql: Robj, + register: Robj, + unregister: Robj, + ttl_secs: Nullable, + max_bytes: Nullable, + enabled: Nullable, + ) -> Self { + let cache = DuckDBReader::from_connection_string("duckdb://memory") + .expect("Failed to create the in-memory cache backend"); + let custom = Self::new_custom(execute_sql, register, unregister); + let InnerReader::Custom(primary) = custom.inner else { + unreachable!("new_custom always builds a Custom reader") + }; + Self { + inner: InnerReader::Caching(CachingReader::with_config( + Box::new(primary), + Box::new(cache), + "r-callback", + "duckdb", + cache_config(ttl_secs, max_bytes, enabled), + )), + } + } + fn new_custom(execute_sql: Robj, register: Robj, unregister: Robj) -> Self { fn opt_fn(r: Robj, name: &str) -> Option { if r.is_null() { @@ -190,6 +316,7 @@ impl GgsqlReader { InnerReader::DuckDB(r) => r.register(name, df, replace), InnerReader::Odbc(r) => r.register(name, df, replace), InnerReader::Custom(r) => r.register(name, df, replace), + InnerReader::Caching(r) => r.register(name, df, replace), }; result.expect("Failed to register table"); } @@ -199,6 +326,7 @@ impl GgsqlReader { InnerReader::DuckDB(r) => r.unregister(name), InnerReader::Odbc(r) => r.unregister(name), InnerReader::Custom(r) => r.unregister(name), + InnerReader::Caching(r) => r.unregister(name), }; result.expect("Failed to unregister table"); } @@ -208,6 +336,7 @@ impl GgsqlReader { InnerReader::DuckDB(r) => r.execute_sql(sql), InnerReader::Odbc(r) => r.execute_sql(sql), InnerReader::Custom(r) => r.execute_sql(sql), + InnerReader::Caching(r) => r.execute_sql(sql), } .expect("SQL execution failed"); let bytes = df_to_ipc_stream(&df).expect("Failed to serialize DataFrame"); @@ -219,6 +348,7 @@ impl GgsqlReader { InnerReader::DuckDB(r) => r.execute(query), InnerReader::Odbc(r) => r.execute(query), InnerReader::Custom(r) => r.execute(query), + InnerReader::Caching(r) => r.execute(query), } .expect("Query execution failed"); GgsqlSpec { inner: spec } diff --git a/tests/testthat/test-caching-reader.R b/tests/testthat/test-caching-reader.R new file mode 100644 index 0000000..1ca6398 --- /dev/null +++ b/tests/testthat/test-caching-reader.R @@ -0,0 +1,143 @@ +# A bridged primary that counts source reads, backed by an in-memory DuckDB. +counting_primary <- function() { + env <- new.env(parent = emptyenv()) + env$calls <- 0L + backend <- duckdb_reader() + ggsql_execute_sql( + backend, + "CREATE TABLE t AS SELECT * FROM (VALUES (1), (2), (3)) AS t(v)" + ) + env$execute_sql <- function(sql) { + env$calls <- env$calls + 1L + ggsql_execute_sql(backend, sql) + } + env +} + +test_that("caching_reader creates a reader from a connection string", { + reader <- caching_reader("duckdb://memory") + expect_s3_class(reader, "Reader") +}) + +test_that("caching_reader rejects a Reader instance with guidance", { + inner <- duckdb_reader() + expect_error(caching_reader(inner), "connection string") +}) + +test_that("caching_reader rejects an unsupported scheme", { + expect_error(caching_reader("sqlite://memory"), "scheme") +}) + +test_that("caching_reader accepts a callback primary without register", { + # register/unregister are optional, mirroring custom_reader() + reader <- caching_reader(primary = function(sql) data.frame()) + expect_s3_class(reader, "Reader") +}) + +test_that("caching_reader rejects register/unregister with a string primary", { + expect_error( + caching_reader("duckdb://memory", register = function(...) NULL), + "only used when" + ) + expect_error( + caching_reader("duckdb://memory", unregister = function(...) NULL), + "only used when" + ) +}) + +test_that("caching_reader memoizes identical reads", { + primary <- counting_primary() + reader <- caching_reader(primary$execute_sql) + ggsql_execute_sql(reader, "SELECT * FROM t") + ggsql_execute_sql(reader, "SELECT * FROM t") + expect_equal(primary$calls, 1L) +}) + +test_that("caching_reader sends distinct reads to the primary", { + primary <- counting_primary() + reader <- caching_reader(primary$execute_sql) + ggsql_execute_sql(reader, "SELECT * FROM t") + ggsql_execute_sql(reader, "SELECT v + 1 AS v FROM t") + expect_equal(primary$calls, 2L) +}) + +test_that("caching_reader honors ttl_secs", { + primary <- counting_primary() + reader <- caching_reader(primary$execute_sql, ttl_secs = 1) + ggsql_execute_sql(reader, "SELECT * FROM t") + Sys.sleep(1.1) + ggsql_execute_sql(reader, "SELECT * FROM t") + expect_equal(primary$calls, 2L) +}) + +test_that("caching_reader honors enabled = FALSE", { + primary <- counting_primary() + reader <- caching_reader(primary$execute_sql, enabled = FALSE) + ggsql_execute_sql(reader, "SELECT * FROM t") + ggsql_execute_sql(reader, "SELECT * FROM t") + expect_equal(primary$calls, 2L) +}) + +test_that("caching_reader honors GGSQL_CACHE_DISABLED", { + withr::local_envvar(GGSQL_CACHE_DISABLED = "1") + primary <- counting_primary() + reader <- caching_reader(primary$execute_sql) + ggsql_execute_sql(reader, "SELECT * FROM t") + ggsql_execute_sql(reader, "SELECT * FROM t") + expect_equal(primary$calls, 2L) +}) + +test_that("keyword config overrides the environment", { + withr::local_envvar(GGSQL_CACHE_DISABLED = "1") + primary <- counting_primary() + reader <- caching_reader(primary$execute_sql, enabled = TRUE) + ggsql_execute_sql(reader, "SELECT * FROM t") + ggsql_execute_sql(reader, "SELECT * FROM t") + expect_equal(primary$calls, 1L) +}) + +test_that("layer-level sources work through a bridged primary", { + # VISUALISE ... FROM
stages the layer source through the cache, + # so the query runs end to end instead of being rejected. + primary <- counting_primary() + reader <- caching_reader(primary$execute_sql) + spec <- ggsql_execute( + reader, + "VISUALISE x DRAW histogram MAPPING v AS x FROM t" + ) + expect_s3_class(spec, "Spec") +}) + +test_that("registered tables are cache-resident and skip the primary", { + primary <- counting_primary() + reader <- caching_reader(primary$execute_sql) + ggsql_register(reader, data.frame(v = 1:3), "only_in_cache") + spec <- ggsql_execute( + reader, + "VISUALISE x DRAW histogram MAPPING v AS x FROM only_in_cache" + ) + expect_s3_class(spec, "Spec") + expect_equal(primary$calls, 0L) +}) + +test_that("ggsql_table_names and ggsql_table see cache-registered tables", { + # Registrations land in the private cache, which the primary's + # SHOW TABLES never sees; R-side tracking merges them into the listing. + reader <- caching_reader("duckdb://memory") + ggsql_register(reader, mtcars, "cars") + expect_true("cars" %in% ggsql_table_names(reader)) + expect_s3_class(ggsql_table(reader, "cars"), "data.frame") + ggsql_unregister(reader, "cars") + expect_false("cars" %in% ggsql_table_names(reader)) + expect_null(ggsql_table(reader, "cars")) +}) + +test_that("caching reader executes a full ggsql query", { + reader <- caching_reader("duckdb://memory") + spec <- ggsql_execute( + reader, + "SELECT 1 AS x, 2 AS y VISUALISE x, y DRAW point" + ) + expect_s3_class(spec, "Spec") + expect_equal(ggsql_metadata(spec)$rows, 1) +}) From 6cceabc97f3f19fe58598f1a5f7889f59e9b68ba Mon Sep 17 00:00:00 2001 From: Carson Date: Tue, 13 Oct 2026 11:07:36 -0500 Subject: [PATCH 2/2] ci: opt in to fork PR checkout in format-suggest actions/checkout v4 now refuses to check out fork PR code under pull_request_target without allow-unsafe-pr-checkout. Safe here: the checked-out code is treated as passive data (air format + reviewdog suggestions), never executed. --- .github/workflows/format-suggest.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/format-suggest.yaml b/.github/workflows/format-suggest.yaml index 8c4f117..5a0a0ac 100644 --- a/.github/workflows/format-suggest.yaml +++ b/.github/workflows/format-suggest.yaml @@ -31,6 +31,11 @@ jobs: - uses: actions/checkout@v4 with: ref: ${{ github.event.pull_request.head.sha }} + # Opt in to checking out fork PR code under pull_request_target. + # Safe here: the checked-out code is only ever treated as passive + # data (air reformats it; reviewdog posts suggestions) — nothing + # from the fork is executed. See the comment on the trigger above. + allow-unsafe-pr-checkout: true - name: Install uses: posit-dev/setup-air@v1