Skip to content

Add caching_reader() - #18

Draft
cpsievert wants to merge 2 commits into
posit-dev:mainfrom
cpsievert:caching-reader
Draft

cpsievert wants to merge 2 commits into
posit-dev:mainfrom
cpsievert:caching-reader

Conversation

@cpsievert

@cpsievert cpsievert commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Note: this PR was drafted with AI assistance and has not had rigorous human review. It's intended to start a discussion on whether this is an acceptable direction — please treat it accordingly.

Summary

Two things are impossible or wasteful with a callback-backed reader (custom_reader()) today. Every read re-executes the source SQL, even when identical to the read before it. And queries with layer-level sources (DRAW ... FROM <table>) fail outright, because a callback reader gives ggsql nowhere to stage derived tables.

The Rust crate has handled both since 0.5.2 with CachingReader: source reads go to the primary and are memoized (TTL plus an LRU byte budget), and all ggsql-generated derived SQL runs on a private in-memory DuckDB cache. This PR exposes it to R as caching_reader(), mirroring the Python binding in posit-dev/ggsql-python#10.

reader <- caching_reader("duckdb://memory")
ggsql_register(reader, mtcars, "cars")
spec <- ggsql_execute(reader, "SELECT * FROM cars VISUALISE mpg AS x DRAW histogram")

primary can be a connection string (duckdb://, odbc://, snowflake:// — the same dispatch as Reader$new(), one scheme more than the Python binding accepts) or the execute_sql/register/unregister callbacks custom_reader() takes. Passing an existing Reader errors with guidance. Cache behavior follows the same GGSQL_CACHE_TTL / GGSQL_CACHE_MAX_BYTES / GGSQL_CACHE_DISABLED env vars as the Python binding, overridable per reader.

Design decisions worth a look

  • Registered tables vs. introspection. Registrations on a caching reader land in the private cache, but SHOW TABLES routes to the primary, so ggsql_table_names()/ggsql_table() couldn't see them. Fixed R-side by tracking registrations on the Reader object and merging them into the listing. An upstream fix was considered and set aside: the crate documents introspection as deliberately describing only the primary, and it wouldn't help R anyway (ggsql_table_names() issues SHOW TABLES as raw SQL).
  • Behavior change to custom_reader(): hook errors now surface the R condition message (e.g. DuckDB's "Catalog Error: ...") instead of extendr's opaque call dump. Small and self-contained; happy to split into its own PR.
  • unsafe impl Send for RCallbackReader — required by CachingReader's Box<dyn Reader + Send>; safe because the R FFI only runs on R's main thread. Reasoning is in a SAFETY comment; worth a second pair of eyes.

Known upstream issues (ggsql crate, not yet filed)

Both cross-binding; neither affects the querychat path. Issue drafts exist and can be filed once this direction is agreed.

  1. SHOW TABLES results are memoized, so tables created on the primary take up to ttl_secs to appear in listings. Documented on ?caching_reader.
  2. CREATE TABLE ... FROM <registered_table> routes to the cache, but the new name never enters the resident set, so subsequent reads route to the primary and fail permanently. No binding-side workaround is possible.

Downstream

Motivating use case is querychat's R viz tool — see posit-dev/querychat#327 for the integration (the pkg-r port of posit-dev/querychat#321).

Where caching pays off, and how we verified it. Querychat's viz loop is iterative refinement: the LLM reissues the same data query with a different chart encoding again and again. Before this change, every chart call re-executed that query against the data source from scratch — the dominant cost of the loop. With a caching reader, one reader lives for the chat session and reuses the first result whenever the SQL text repeats, so a refinement costs no source read at all. As evidence, we simulated a typical session end to end: nine chart refinements over one shared base read, one exact repeat, and one multi-source query of a kind the old code rejected outright. The data source was read once instead of ten times, and the multi-source query ran. In the Python benchmark (3M-row data frame) this also cut session wall time by ~35%; the benefit is smaller for cheap local sources and largest for remote databases, where every eliminated read is a network round trip.

Test plan

16 new tests covering dispatch, memoization, TTL, env-var config, layer-level sources, and the table-listing fix. Full suite: 191 expectations, 0 failures; air / cargo fmt clean. (The format-suggest failure on this PR is a fork-PR infrastructure issue — actions/checkout now refuses fork checkouts under pull_request_target; a fix is included here, though it only takes effect for future PRs once merged, since that workflow runs from the base branch.)

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 <table>) 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.
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.
cpsievert added a commit to posit-dev/querychat that referenced this pull request Oct 13, 2026
Adds a Remotes entry so CI installs ggsql with caching_reader()
(0.5.2.9000), letting the floor-gated viz tests actually run instead
of skip. Temporary: remove once a ggsql release with caching_reader()
is on CRAN/r-universe (Remotes is not CRAN-compliant anyway).
cpsievert added a commit to posit-dev/querychat that referenced this pull request Oct 13, 2026
Adds a Remotes entry so CI installs ggsql with caching_reader()
(0.5.2.9000), letting the floor-gated viz tests actually run instead
of skip. The ggsql= prefix is required: the repo (ggsql-r) and the
package (ggsql) have different names, and pak only applies Remotes
entries whose package name matches the dependency.

Temporary: remove once a ggsql release with caching_reader() is on
CRAN/r-universe (Remotes is not CRAN-compliant anyway).
cpsievert added a commit to posit-dev/querychat that referenced this pull request Oct 13, 2026
Adds a Remotes entry so CI installs ggsql with caching_reader()
(0.5.2.9000), letting the floor-gated viz tests actually run instead
of skip. The ggsql= prefix is required: the repo (ggsql-r) and the
package (ggsql) have different names, and pak only applies Remotes
entries whose package name matches the dependency.

Temporary: remove once a ggsql release with caching_reader() is on
CRAN/r-universe (Remotes is not CRAN-compliant anyway).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant