Repository navigation
feat(pkg-py): route ggsql viz execution through ggsql.CachingReader - #321
Merged
Merged
Conversation
This was referenced Oct 12, 2026
cpsievert
added a commit
that referenced
this pull request
Oct 12, 2026
cpsievert
added a commit
that referenced
this pull request
Oct 12, 2026
The prompt ships inside a specific querychat version, so the hedged 'works where querychat executes through a ggsql caching reader' wording only confused the model — and this version's execution path (R and Python) rejects layer-level FROM outright. State the rule plainly and stop advertising MAPPING ... FROM source in the grammar blocks. The caching-reader PRs (#321 + R equivalent) can relax this when they land.
cpsievert
added a commit
that referenced
this pull request
Oct 12, 2026
The caching-reader execution path (#321 + R equivalent) lands in the same release, so document the end state instead of the intermediate one: DRAW ... MAPPING ... FROM <source> works, shown with an example plotting raw points from the final SELECT alongside a summary line from a CTE (verified end-to-end through ggsql.CachingReader 0.5.2). Also notes that FROM attaches to MAPPING — a bare 'DRAW line FROM t' is a parse error.
cpsievert
force-pushed
the
ggsql-caching-reader
branch
from
October 13, 2026 15:10
9b765ba to
65d885f
Compare
cpsievert
added a commit
that referenced
this pull request
Oct 13, 2026
cpsievert
force-pushed
the
ggsql-caching-reader
branch
from
October 13, 2026 15:22
65d885f to
e1fec65
Compare
cpsievert
added a commit
that referenced
this pull request
Oct 13, 2026
The prompt ships inside a specific querychat version, so the hedged 'works where querychat executes through a ggsql caching reader' wording only confused the model — and this version's execution path (R and Python) rejects layer-level FROM outright. State the rule plainly and stop advertising MAPPING ... FROM source in the grammar blocks. The caching-reader PRs (#321 + R equivalent) can relax this when they land.
cpsievert
changed the title
This was referenced Oct 13, 2026
cpsievert
commented
Oct 13, 2026
cpsievert
added a commit
that referenced
this pull request
Oct 13, 2026
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
branch
2 times, most recently
from
October 13, 2026 16:51
5826500 to
9d33e00
Compare
Replace the hand-rolled SQL/VISUALISE split in _viz_ggsql.py with whole-query execution through ggsql.CachingReader wrapping the querychat executor: - Base reads go to the executor (database pushdown preserved) and are memoized for the cache TTL, so iterative chart refinement over one shared base read costs 1 source read instead of N. - Derived tables live in the reader's private in-memory DuckDB cache, so layer-level sources (DRAW ... FROM <table>) now run end-to-end. - execute_ggsql() takes the raw query string; callers wanting validation feedback run ggsql.validate() first. - Each chat client's viz-tool closure owns one CachingReader (created lazily on first viz call), so memoized reads are shared across the session and freed when the client is dropped. - Bookmark restore (restore_viz_widgets()) runs through one CachingReader per restore pass. Tests: layer-level sources, shared-source memoization, and bookmark-restore regression coverage in test_ggsql.py.
Note 3 of the Python ggsql-syntax.md system prompt now documents attaching FROM <source> to a layer's MAPPING, since execution through a caching reader makes the pattern work end-to-end. The example shows raw points from the final SELECT alongside a summary line drawn from a CTE, and notes that FROM belongs to MAPPING (a bare 'DRAW line FROM t' is a parse error). The R copy of the prompt ships with the R caching-reader counterpart.
New Caching section in visualize.qmd covering the 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 the GGSQL_CACHE_TTL / GGSQL_CACHE_MAX_BYTES / GGSQL_CACHE_DISABLED environment variables.
cpsievert
force-pushed
the
ggsql-caching-reader
branch
from
October 13, 2026 16:52
9d33e00 to
9a5110e
Compare
cpsievert
marked this pull request as ready for review
October 13, 2026 17:35
cpsievert
force-pushed
the
ggsql-caching-reader
branch
from
October 13, 2026 17:51
952cf50 to
54a3b1f
Compare
ExecutorReader.execute_sql now uses as_narwhals(...).to_arrow() instead of to_polars(...).to_arrow(). ggsql hard-requires pyarrow but not polars, and querychat[viz] doesn't install polars either, so routing through polars imposed an undeclared runtime dependency on pandas-only installs. The direct narwhals conversion also avoids an arrow->polars->arrow round-trip for pyarrow-backed results. Also replace the execute_query() Any return on the abstract QueryExecutor with a documented QueryResult alias: narwhals' IntoFrame (an open-ended union of frame protocols, so third-party executors and future narwhals backends are covered without changes) plus lazy ibis tables. Consumers normalize via as_narwhals(). AppState.get_current_data() is widened to match — with an IbisSource it already returned a lazy ibis.Table at runtime, which the previous IntoFrame annotation did not admit. Two generic call sites that rely on the executor backend matching the source's IntoFrameT now say so via cast(). Smaller hardening in the same area: - ExecutorReader.register/unregister now warn instead of passing silently, so a future ggsql that routes writes to the primary reader fails loudly rather than dropping tables. - caching_reader()/ExecutorReader accept a bare DataSource in their annotations, matching the docstring and tests. - restore_viz_widgets() skips reader creation when there are no saved widgets (avoids an idle in-memory DuckDB). Tests: TestExecutorReaderResultTypes covers the conversion across every source/executor combination (DataFrameSource/DuckDBExecutor with pandas/polars/pyarrow backends, PolarsLazySource, PolarsSQLExecutor, SQLAlchemySource, IbisSource, DataSourceExecutor) plus empty results, NULL columns, the register/unregister warnings, and the known duplicate-column-name limitation.
cpsievert
force-pushed
the
ggsql-caching-reader
branch
from
October 13, 2026 18:14
54a3b1f to
2ddfa98
Compare
caching_reader() and execute_ggsql() were one-line wrappers whose docstrings mostly restated CachingReader behavior already covered by ExecutorReader's class docs and the user-facing visualize.qmd. Inline CachingReader(ExecutorReader(executor)) and reader.execute(query) at both call sites and keep ExecutorReader as the only export of _viz_ggsql. Tests: add a local make_reader() helper in test_ggsql.py, and fix the test_viz_footer.py monkeypatch, which targeted querychat._viz_ggsql.execute_ggsql but never took effect (_viz_tools binds the name at import). It now patches ggsql.CachingReader.execute, the seam the code actually uses.
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
Replaces querychat's hand-rolled SQL/VISUALISE split (
_viz_ggsql.py) with whole-query execution throughggsql.CachingReaderwrapping the querychat executor:DRAW ... FROM <table>) now run end-to-end — previously querychat rejected these outright.execute_ggsql()takes the raw query string; callers wanting validation feedback runggsql.validate()first (aValidatedsplits layer-level sources into overlapping SQL/visual parts that can't be concatenated back).ggsql.CachingReader(created lazily on first viz call), so memoized reads are shared across the session and the cache is freed when the client is dropped. Bookmark restore (restore_viz_widgets()) runs through oneCachingReaderper restore pass, so restored widgets share the same caching behavior.Prompt change
Note 3 of the Python
ggsql-syntax.mdsystem prompt now documents layer-levelFROMsources as supported, since execution through a caching reader makes them work end-to-end. The example shows raw points from the final SELECT alongside a summary line drawn from a CTE, and notes thatFROMattaches toMAPPING(a bareDRAW line FROM tis a parse error). Verified end-to-end throughggsql.CachingReader0.5.2. The corresponding R prompt change ships with the R caching-reader counterpart in #327.Docs
New "Caching" section in
pkg-py/docs/visualize.qmdcovering the 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.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: nine chart refinements over one shared 3M-row 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, session wall time dropped 51.1s → 33.1s (~35%) with no cold-path regression, and the multi-source query ran. The benefit is smaller for cheap local sources and largest for remote databases, where every eliminated read is a network round trip.
Full per-call numbers: benchmark results in ggsql-python#10.
Test plan
pkg-py/tests/test_ggsql.py: 20 passed, including layer-level source, shared-source memoization, and bookmark-restore regression tests.