Repository navigation
Conversation
A single-folder, uv-runnable Python example for demoing querychat's multi-table support without needing the repo checked out or set up -- useful for customer demos. Data is synthetic (seeded, deterministic) and generated once to a local parquet cache on first run.
Pulls in querychat's viz extra so the chat can generate charts, not just tables -- previously the visualize tool was silently disabled with a warning.
Drops pandas as a dependency entirely -- polars' native parquet I/O and nullable Int64 inference handle everything generate_data.py needs without it.
DATE_TRUNC(...)-derived columns hit a ggsql bug where SCALE ... VIA date can't coerce a Timestamp column (posit-dev/ggsql#559). Add extra_instructions telling the LLM to CAST such expressions to DATE, so monthly/weekly revenue charts keep working until the upstream fix lands.
execute_ggsql now executes the whole query (SQL + VISUALISE) through a CachingReader wrapping the querychat executor, replacing the hand-rolled split path (executor SQL -> polars -> fresh DuckDBReader per call). Base reads keep database pushdown and are memoized across viz calls within a session (one reader per executor, weakly held); derived tables stage in the reader's private cache, so layer-level DRAW...FROM sources now work instead of raising. Signature change: execute_ggsql(executor, query: str) takes the raw query string. A Validated's sql()/visual() parts overlap for VISUALISE...FROM queries (validate normalizes the FROM into both) and cannot be concatenated back into a valid query.
Completes bc0b804: execute_ggsql now takes the whole query string (a Validated splits layer-level sources into overlapping parts that cannot be concatenated back), but these call sites still passed ggsql.validate(query).
_viz_ggsql.py lazily imports ggsql.CachingReader at first viz call; older releases would fail with ImportError at runtime. CachingReader first ships in ggsql 0.5.2 (posit-dev/ggsql-python#10).
The module-level WeakKeyDictionary never actually released readers: each value (CachingReader -> ExecutorReader) holds a strong reference to its key (the executor), so the weak key could never die and entries accumulated for the life of the process — the leak the weak keys were meant to prevent. The viz tool closure is created once per chat client and already has exactly the memo's desired lifetime, so the reader now lives there, created lazily on first viz call. execute_ggsql() takes the reader explicitly, making cache sharing visible at the call site; a new test guards against reintroducing an implicit shared registry.
restore_viz_widgets() passed the executor itself to execute_ggsql(), whose signature now takes a ggsql.CachingReader. The AttributeError was swallowed by the per-entry except, so every viz widget silently failed to restore from bookmarks. Create one reader per restore pass (restored widgets sharing a base read also reuse the memo) and add regression tests covering restore, memo sharing, and invalid-entry skipping.
Memoized base reads assume the data doesn't change mid-session; with update queries enabled or an externally-written live database, charts can lag changes by up to the TTL. Document the caveat and the GGSQL_CACHE_TTL / GGSQL_CACHE_MAX_BYTES / GGSQL_CACHE_DISABLED knobs in the visualize guide and the caching_reader() docstring.
) * fix(pkg-r): skip tests that assume Suggests packages are installed CRAN's r-devel-linux-x86_64-fedora-clang flavor has RSQLite but not duckdb installed, which caused the test suite to ERROR: - 92 errors: tests requesting engine = "duckdb" (explicitly or via the local_data_frame_source() fixture default) hard-failed in check_installed("duckdb"), including one variant where the non-interactive check surfaced as "menu() cannot be used non-interactively" - 4 failures: tests using the default engine silently fell back to SQLite and then failed DuckDB-specific expectations skip_if_no_dataframe_engine() only skipped when *neither* engine was installed, so it never fired on that machine, and it had no way to express "this test requires a specific engine". It is now engine-aware: - with engine = NULL, keep the old any-engine semantics (mirroring how DataFrameSource resolves a default engine) - with a specific engine, skip unless that engine's package is installed, so tests no longer silently degrade to another engine local_data_frame_source() and local_recording_data_frame_source() now call it with their requested engine, and local_querychat() skips for plain data.frame inputs, so fixture-based tests skip automatically. Tests that construct DataFrameSource or QueryChat directly gained skip_if_no_dataframe_engine() guards (6 describe blocks and 3 test_that() blocks in test-QueryChat.R, 2 describe blocks in test-QueryChatSystemPrompt.R, plus 3 explicit skip_if_not_installed ("duckdb") calls in DuckDB-asserting tests). Audited all Suggests packages by running the full suite with every Suggests dependency (except the testthat/withr harness and later, which is a hard dependency of shiny) shadowed by broken stubs. That surfaced 46 additional tests failing on "no compatible database engine" with both engines absent, all fixed by the guards above. Verified by simulating the CRAN condition (FAIL 0, SKIP 98, PASS 716 for the duckdb-only case), a full Suggests blackout (FAIL 0, SKIP 199, PASS 1198), and with everything installed (FAIL 0, SKIP 0, PASS 2195, twice). * docs(pkg-r): add NEWS bullet for CRAN test skip fix
* v0.4.1 * docs(pkg-r): drop dev-mode bullet from release_bullets() * docs(pkg-r): update cran-comments for 0.4.1 resubmission
Contributor
Author
|
Superseded by #326. This PR's diff contained the full stacked history (multi-table demo, CachingReader, maps capability, plus duplicated main commits from a bad rebase) because its head branch |
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
2 participants
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
make py-check-types(pyright 1.1.414) currently fails onmainwith 4 errors in_handoff_prompt.py: the dynamically builtcreate_modelfield specs (language,referenced_tables) are inferred astuple[Literal, Any], which nocreate_modeloverload accepts. This blocks CI on every open PR touchingpkg-py(e.g. #322, #323, #324).The file already notes that static typing cannot express runtime-built
Literalvalues; this just widens the two field-spec variables totuple[Any, Any]so the call sites check cleanly. No runtime change.Verification
uv run pyright pkg-py/src/querychat/_handoff_prompt.py: 0 errors (was 4).