chore(pkg-py): prepare querychat for ggsql 0.3.0#225
Conversation
020ab01 to
e8ed1dd
Compare
There was a problem hiding this comment.
Pull request overview
Updates QueryChat’s Python visualization (“ggsql”) integration to align with ggsql 0.3.0 syntax/validation, refreshing the LLM-facing prompt guidance and adding regression tests around validation failures.
Changes:
- Switch
ggsqloptional/dev dependency to a temporary direct Git reference while 0.3.0 is not yet on the package index. - Refresh ggsql prompt/syntax reference (e.g.,
rangevs deprecatederrorbar,FROM ... VISUALISE ..., segment endpoint requirements). - Simplify
_viz_tools.pyvalidation by relying on ggsql 0.3 validation errors and extend test coverage for malformed visual queries.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Updates viz/dev dependency specs to use a temporary direct ggsql Git reference. |
pkg-py/tests/test_viz_tools.py |
Adds tests asserting updated prompt/syntax guidance and improved error surfacing behavior. |
pkg-py/tests/test_ggsql.py |
Adds validation behavior tests for new ggsql query forms and upstream error reporting. |
pkg-py/src/querychat/prompts/tool-visualize.md |
Updates tool guidance to match ggsql 0.3 rules and terminology. |
pkg-py/src/querychat/prompts/prompt.md |
Minor prompt wording tweak for VISUALISE formatting. |
pkg-py/src/querychat/prompts/ggsql-syntax.md |
Updates syntax reference for range, FROM ... VISUALISE ..., and segment requirements. |
pkg-py/src/querychat/_viz_tools.py |
Adjusts validation flow to raise ggsql upstream errors when invalid. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| validated = validate(ggsql) | ||
| if not validated.has_visual(): | ||
| # When VISUALISE contains SQL expressions (e.g., CAST()), | ||
| # ggsql silently treats the entire query as plain SQL: | ||
| # valid()=True, has_visual()=False, no errors. This | ||
| # heuristic catches that case so we can guide the LLM. | ||
| # Remove when ggsql reports this as a parse error: | ||
| # https://github.com/posit-dev/ggsql/issues/256 | ||
| has_keyword = ( | ||
| "VISUALISE" in ggsql.upper() or "VISUALIZE" in ggsql.upper() | ||
| ) | ||
| if has_keyword: | ||
| raise ValueError( | ||
| "VISUALISE clause was not recognized. " | ||
| "VISUALISE and MAPPING accept column names only — " | ||
| "no SQL expressions, CAST(), or functions. " | ||
| "Move all data transformations to the SELECT clause, " | ||
| "then reference the resulting column by name in VISUALISE." | ||
| ) | ||
| raise ValueError( | ||
| "Query must include a VISUALISE clause. " | ||
| "Use querychat_query for queries without visualization." | ||
| ) | ||
|
|
||
| if not validated.valid(): | ||
| raise ValueError( | ||
| "\n".join(error["message"] for error in validated.errors()) | ||
| ) |
There was a problem hiding this comment.
In visualize(), the has_visual() check happens before validated.valid(). If ggsql.validate() returns has_visual()==False for queries that contain VISUALISE but fail to parse (or otherwise validate), this will raise the generic “Query must include a VISUALISE clause” error and hide the upstream parse/validation messages in validated.errors(). Consider checking validated.valid() first (surfacing/joining validated.errors()), then enforcing has_visual() only for the case where the query is otherwise valid SQL but lacks a VISUALISE clause.
| viz = [ | ||
| # Temporary direct reference until ggsql 0.3.0 is available on the package index. | ||
| "ggsql @ git+https://github.com/posit-dev/ggsql-python.git@refs/pull/5/head", | ||
| "altair>=6.0", | ||
| "shinywidgets>=0.8.0", | ||
| "vl-convert-python>=1.9.0", | ||
| ] |
There was a problem hiding this comment.
The ggsql dependency is now a direct GitHub PR ref (refs/pull/5/head), which is mutable and not reproducible (the referenced commit can change over time). To keep installs deterministic, please pin to an immutable commit SHA (or a tag) and keep the “temporary until 0.3.0 is on PyPI” note. Also, the PR description says the metadata is bumped to >=0.3.0, but the current change doesn’t reflect that constraint—consider aligning the description and/or the dependency spec once 0.3.0 is available.
| dev = [ | ||
| "ruff>=0.6.5", | ||
| "pyright>=1.1.401", | ||
| "tox-uv>=1.11.4", | ||
| "pytest>=8.4.0", | ||
| "polars>=1.0.0", | ||
| "pyarrow>=14.0.0", | ||
| "ibis-framework[duckdb]>=9.0.0", | ||
| # Temporary direct reference until ggsql 0.3.0 is available on the package index. | ||
| "ggsql @ git+https://github.com/posit-dev/ggsql-python.git@refs/pull/5/head", | ||
| "altair>=6.0", | ||
| "shinywidgets>=0.8.0", | ||
| "vl-convert-python>=1.9.0", | ||
| ] |
There was a problem hiding this comment.
Same reproducibility concern as the viz extra: the dev dependency group points at refs/pull/5/head, which is mutable. Pinning the direct reference to a commit SHA (or tag) will make local dev/test environments deterministic and avoid unexpected breakages if the PR branch is force-pushed.
Follow up to #201
Summary
>=0.3.0range, fullsegmentendpoints, andFROM ... VISUALISE ..._viz_tools.pyto use ggsql 0.3 validation where safe, with regression coverage for malformed visual queriesTest Plan
.venv/bin/pytest pkg-py/tests/test_viz_tools.py pkg-py/tests/test_ggsql.py -v.venv/bin/ruff check pkg-py/src/querychat/_viz_tools.py pkg-py/tests/test_viz_tools.py pkg-py/tests/test_ggsql.pyNotes
../ggsql-pythoninstalled into the worktree venv.uv runresolution is still blocked untilggsql>=0.3.0is available on the normal resolver path.