Repository navigation
Conversation
cpsievert
changed the title
Port of the Python approach in #321 to pkg-r: replace the hand-rolled SQL/VISUALISE split with whole-query execution through a caching reader wrapping the querychat executor. - Base reads go to the executor (database pushdown preserved) and are memoized for the cache TTL: a session of chart refinements over one shared base read costs 1 source read instead of N. - Derived tables live in the reader's private in-memory cache, so layer-level sources (DRAW ... FROM <table>) now run end to end; previously rejected outright. The ggsql-syntax.md prompt update on the base branch now holds for R as well. - execute_ggsql() takes a reader and the raw query string; validation feedback still runs through ggsql::ggsql_validate() first. - One caching reader per chat client (created lazily on first viz call), and one per restore_viz_widgets() pass so bookmarked widgets share memoized base reads — the regression the Python side hit. - tbl_sql results are materialized with dplyr::collect() in the bridge. Requires ggsql >= 0.5.2.9000 (enforced lazily via check_installed()). Docs: new "Caching" section in vignette("visualize") covering memoization, the staleness caveat, and the GGSQL_CACHE_* knobs.
cpsievert
force-pushed
the
ggsql-caching-reader-r
branch
from
October 13, 2026 16:42
1749e64 to
aa71e16
Compare
CI installs ggsql 0.5.2 (CRAN), which lacks caching_reader(): - check_viz_deps() and the bookmark-restore gate now require ggsql >= 0.5.2.9000, so older ggsql drops the visualize tool / skips restores with a warning (the established UX) instead of erroring at call time. - viz_caching_reader() looks up caching_reader via getExportedValue() so R CMD check doesn't warn about a missing export on older ggsql. - Viz tool tests skip below the floor, mirroring test-querychat-viz.R.
cpsievert
force-pushed
the
ggsql-caching-reader-r
branch
2 times, most recently
from
October 13, 2026 17:21
01c5ba6 to
aa17058
Compare
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
force-pushed
the
ggsql-caching-reader-r
branch
from
October 13, 2026 17:26
aa17058 to
e014071
Compare
Mirrors the pkg-py cleanup: execute_ggsql() was a one-line wrapper around ggsql::ggsql_execute() whose roxygen block mostly restated CachingReader behavior, and its version gate was redundant with the gates already at the tool entry points (visualize_result(), restore_viz_widgets()). Inline ggsql::ggsql_execute() at both call sites and drop man/execute_ggsql.Rd. viz_caching_reader() stays: unlike the deleted wrapper it carries real logic (version gate, R CMD check-safe lookup of caching_reader, and tbl_sql materialization). Tests: test-querychat-viz.R calls ggsql::ggsql_execute() directly, and the test-viz-tool.R mocks move from the querychat namespace to the ggsql one (ggsql_execute), alongside the existing ggsql mocks.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The pkg-r counterpart of #321. Replaces querychat-R's hand-rolled SQL/VISUALISE split (
pkg-r/R/querychat_viz.R) with whole-query execution throughggsql::caching_reader()wrapping the querychat executor:DRAW ... FROM <table>) now run end-to-end — previously rejected outright byhas_layer_level_source(). Note 3 of the Rggsql-syntax.mdprompt is updated accordingly: it now documents attachingFROM <source>to a layer'sMAPPING, with an example combining raw points from the final SELECT and a summary line from a CTE.execute_ggsql()takes a reader and the raw query string; callers wanting validation feedback runggsql::ggsql_validate()first (aValidatedsplits layer-level sources into overlapping SQL/visual parts that can't be concatenated back).restore_viz_widgets()creates one reader per restore pass.tbl_sqlresults are materialized withdplyr::collect()in the bridge.Dependency
Requires
ggsql::caching_reader(), available in ggsql-r >= 0.5.2.9000 — see posit-dev/ggsql-r#18 (still draft, which is why this PR is too). The floor is enforced at the tool gates: with an older ggsql,check_viz_deps()drops the"visualize"tool with a warning (the established UX for missing ggsql) rather than erroring at call time, the package remains installable against ggsql 0.5.2, and viz-related tests skip below the floor. A temporaryRemotes: posit-dev/ggsql-r#18inpkg-r/DESCRIPTIONmakes CI install the dev ggsql so the floor-gated tests actually run there; it must be removed before release (Remotesis not CRAN-compliant) once a ggsql release withcaching_reader()is on CRAN/r-universe.Docs
New "Caching" section in
vignette("visualize"): memoization behavior, the staleness caveat (charts can lag data changes by up to the TTL when update queries are enabled or the database is written externally), and theGGSQL_CACHE_TTL/GGSQL_CACHE_MAX_BYTES/GGSQL_CACHE_DISABLEDknobs. The "How it works" steps now describe whole-query execution through the caching reader.Benchmark
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 in R (counting executor around a
DataFrameSource): 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, the multi-source query ran, and bookmark restore of two widgets sharing a base read cost 1 source read. The benefit is smaller for cheap local sources and largest for remote databases, where every eliminated read is a network round trip.Test plan
test-querychat-viz.Rrewritten for the reader-first API: basic/uppercase-column queries,FROMbefore VISUALISE, layer-levelFROMsources (previously rejected), a raw + summary overlay via CTE, plus a "viz caching behavior" block proving identical queries share memoized reads through one reader and independent readers don't share.test-QueryChat.Rskip below the ggsql floor, matching the tool-gate behavior.