From aece2e97142e3ca258437a7f58d940d2b19c3296 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 8 Sep 2026 14:02:54 +0200 Subject: [PATCH 01/17] Rename render::Spec to ResolvedPlot Frees up the `Spec` name for future enum meaning both table and plot output kinds. --- ggsql-cli/src/main.rs | 8 ++++---- src/CLAUDE.md | 10 +++++----- src/doc/API.md | 14 +++++++------- src/execute/cte.rs | 2 +- src/lib.rs | 2 +- src/reader/adbc.rs | 2 +- src/reader/cache.rs | 4 ++-- src/reader/cache_equivalence.rs | 4 ++-- src/reader/duckdb.rs | 2 +- src/reader/mod.rs | 22 +++++++++++----------- src/reader/odbc/mod.rs | 2 +- src/reader/spec.rs | 8 ++++---- src/reader/sqlite.rs | 2 +- src/writer/hephaestus/CLAUDE.md | 6 +++--- src/writer/hephaestus/mod.rs | 2 +- src/writer/mod.rs | 8 ++++---- 16 files changed, 49 insertions(+), 49 deletions(-) diff --git a/ggsql-cli/src/main.rs b/ggsql-cli/src/main.rs index f04592cd7..a274d3dc7 100644 --- a/ggsql-cli/src/main.rs +++ b/ggsql-cli/src/main.rs @@ -5,7 +5,7 @@ Provides commands for executing ggsql queries with various data sources and outp */ use clap::{Parser, Subcommand, ValueEnum}; -use ggsql::reader::{Reader, Spec}; +use ggsql::reader::{Reader, ResolvedPlot}; use ggsql::validate::validate; use ggsql::writer::{Writer, WriterOptions}; use ggsql::{parser, VERSION}; @@ -376,7 +376,7 @@ fn exec_with_reader( render_spec(spec, writer, output, verbose); } -fn render_spec(spec: Spec, writer: &WriterSpec, output: Option, verbose: bool) { +fn render_spec(spec: ResolvedPlot, writer: &WriterSpec, output: Option, verbose: bool) { if verbose { let metadata = spec.metadata(); eprintln!("\nQuery executed:"); @@ -784,7 +784,7 @@ fn cmd_skill(format: Option) { } } -fn render_vegalite(spec: &Spec, options: &WriterOptions) -> Output { +fn render_vegalite(spec: &ResolvedPlot, options: &WriterOptions) -> Output { #[cfg(feature = "vegalite")] { // Configure from --writer-option, then render @@ -805,7 +805,7 @@ fn render_vegalite(spec: &Spec, options: &WriterOptions) -> Output { } } -fn render_png(spec: &Spec, options: &WriterOptions) -> Output { +fn render_png(spec: &ResolvedPlot, options: &WriterOptions) -> Output { #[cfg(feature = "png")] { // Configure from --writer-option, then render diff --git a/src/CLAUDE.md b/src/CLAUDE.md index 06d8f93e9..bb2996d52 100644 --- a/src/CLAUDE.md +++ b/src/CLAUDE.md @@ -23,7 +23,7 @@ src/ ├── parser/ Tree-sitter integration → typed AST (Plot) ├── plot/ AST: Plot, Layer, Geom, Scale, Facet, Projection, Mappings (see plot/CLAUDE.md) ├── reader/ Reader trait + drivers (DuckDB, SQLite, ODBC, Snowflake, …) -├── execute/ Pipeline that turns Plot + Reader → executed Spec +├── execute/ Pipeline that turns Plot + Reader → ResolvedPlot ├── writer/ Writer trait + Vega-Lite implementation (see writer/vegalite/CLAUDE.md) ├── data/ Bundled sample datasets (penguins, airquality) └── doc/ API.md — public Rust API reference @@ -49,7 +49,7 @@ Grammar lives in [`/tree-sitter-ggsql/`](../tree-sitter-ggsql/) — when adding | `odbc.rs` | ODBC | `odbc` (default) | | `cache.rs` | `CachingReader` — wraps any primary `Reader` with an in-memory cache | `duckdb` or `sqlite` | | `connection.rs` | Connection-string parsing for all of the above | — | -| `spec.rs` | `Spec` type returned by `execute()`, plus DataFrame conversion | — | +| `spec.rs` | `ResolvedPlot` type returned by `execute()`, plus DataFrame conversion | — | | `data.rs` | Bundled sample datasets — the `ggsql:` builtins | `builtin-data` | `SqlDialect` trait in `mod.rs` lets each driver supply its own type names, information-schema queries, and spatial helper methods (`sql_st_transform`, `sql_geometry_to_wkb`, `sql_geometry_bbox`, `sql_ensure_geometry`, `sql_select_replace`, `sql_spatial_setup`). @@ -58,7 +58,7 @@ Grammar lives in [`/tree-sitter-ggsql/`](../tree-sitter-ggsql/) — when adding ### `execute/` -The pipeline that takes a parsed `Plot` plus a `Reader` and produces a fully-resolved `Spec` (typed data per layer, scales resolved, casts applied). Submodules: +The pipeline that takes a parsed `Plot` plus a `Reader` and produces a `ResolvedPlot` (typed data per layer, scales resolved, casts applied). Submodules: - `mod.rs` — top-level `prepare_data_with_reader()` and validation glue. - `cte.rs` — CTE extraction / materialization for shared subqueries. @@ -83,13 +83,13 @@ Sufficiently large to have its own [`plot/CLAUDE.md`](plot/CLAUDE.md). It holds ### `doc/` -Just `API.md` — the public Rust API reference for `Reader::execute`, `Writer::render`, `validate`, `Spec`, `Validated`, `Metadata`. End-user docs live in `/doc/`, not here. +Just `API.md` — the public Rust API reference for `Reader::execute`, `Writer::render`, `validate`, `ResolvedPlot`, `Validated`, `Metadata`. End-user docs live in `/doc/`, not here. ## Public API quick reference Two-stage pipeline: -1. **`reader.execute(query)`** → `Spec` (parses, runs SQL, resolves mappings, applies stats). +1. **`reader.execute(query)`** → `ResolvedPlot` (parses, runs SQL, resolves mappings, applies stats). 2. **`writer.render(&spec)`** → output (Vega-Lite JSON for `VegaLiteWriter`). `validate(query)` performs syntax + semantic checks without touching a reader. diff --git a/src/doc/API.md b/src/doc/API.md index 09e0c2ba4..3d3e45c41 100644 --- a/src/doc/API.md +++ b/src/doc/API.md @@ -4,7 +4,7 @@ This document provides a comprehensive reference for the ggsql public API. ## Overview -- **Stage 1: `reader.execute()`** - Parse query, execute SQL, resolve mappings, create Spec +- **Stage 1: `reader.execute()`** - Parse query, execute SQL, resolve mappings, create ResolvedPlot - **Stage 2: `writer.render()`** - Generate output (Vega-Lite JSON, etc.) ### API Functions @@ -12,7 +12,7 @@ This document provides a comprehensive reference for the ggsql public API. | Function | Use Case | | ------------------ | ---------------------------------------------------- | | `reader.execute()` | Main entry point - full visualization pipeline | -| `writer.render()` | Generate output from Spec | +| `writer.render()` | Generate output from ResolvedPlot | | `validate()` | Validate syntax + semantics, inspect query structure | --- @@ -22,7 +22,7 @@ This document provides a comprehensive reference for the ggsql public API. ### `Reader::execute` ```rust -fn execute(&self, query: &str) -> Result +fn execute(&self, query: &str) -> Result ``` Execute a ggsql query for visualization. This is the main entry point - a default method on the Reader trait. @@ -43,7 +43,7 @@ Execute a ggsql query for visualization. This is the main entry point - a defaul **Returns:** -- `Ok(Spec)` - Ready for rendering +- `Ok(ResolvedPlot)` - Ready for rendering - `Err(GgsqlError)` - Parse, validation, or execution error **Example:** @@ -183,7 +183,7 @@ if let Some(tree) = validated.tree() { --- -### `Spec` +### `ResolvedPlot` Result of executing a ggsql query, ready for rendering. @@ -401,8 +401,8 @@ pub trait Writer { /// Check whether a spec can be rendered by this writer, without rendering it fn validate(&self, spec: &Plot) -> Result<()>; - /// Render a prepared `Spec` from `reader.execute()` — the usual entry point - fn render(&self, spec: &Spec) -> Result; + /// Render a `ResolvedPlot` from `reader.execute()` — the usual entry point + fn render(&self, spec: &ResolvedPlot) -> Result; } ``` diff --git a/src/execute/cte.rs b/src/execute/cte.rs index 65b343681..6c1a8920b 100644 --- a/src/execute/cte.rs +++ b/src/execute/cte.rs @@ -738,7 +738,7 @@ mod tests { fn register(&self, _name: &str, _df: crate::DataFrame, _replace: bool) -> Result<()> { Ok(()) } - fn execute(&self, _query: &str) -> Result { + fn execute(&self, _query: &str) -> Result { unreachable!() } fn caches_sources(&self) -> bool { diff --git a/src/lib.rs b/src/lib.rs index 17cabcef1..b7353bb4d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -761,7 +761,7 @@ mod integration_tests { // Verify the spec has the facet configuration assert!( prepared.specs[0].facet.is_some(), - "Spec should have facet configuration" + "ResolvedPlot should have facet configuration" ); } diff --git a/src/reader/adbc.rs b/src/reader/adbc.rs index 203d31e26..509f8a7d8 100644 --- a/src/reader/adbc.rs +++ b/src/reader/adbc.rs @@ -282,7 +282,7 @@ where Ok(()) } - fn execute(&self, query: &str) -> Result { + fn execute(&self, query: &str) -> Result { crate::reader::execute_with_reader(self, query) } diff --git a/src/reader/cache.rs b/src/reader/cache.rs index 411205cff..c09a33368 100644 --- a/src/reader/cache.rs +++ b/src/reader/cache.rs @@ -12,7 +12,7 @@ //! - [`Reader::dialect`] returns the cache dialect. use crate::array_util::{as_i64, as_str}; -use crate::reader::{execute_with_reader, ColumnInfo, Reader, Spec, SqlDialect, TableInfo}; +use crate::reader::{execute_with_reader, ColumnInfo, Reader, ResolvedPlot, SqlDialect, TableInfo}; use crate::{naming, DataFrame, Result}; use arrow::array::Array; use std::cell::{Cell, RefCell}; @@ -456,7 +456,7 @@ impl Reader for CachingReader { Ok(()) } - fn execute(&self, query: &str) -> Result { + fn execute(&self, query: &str) -> Result { execute_with_reader(self, query) } diff --git a/src/reader/cache_equivalence.rs b/src/reader/cache_equivalence.rs index 789bae65e..9337452f2 100644 --- a/src/reader/cache_equivalence.rs +++ b/src/reader/cache_equivalence.rs @@ -272,7 +272,7 @@ mod adbc_mode { use super::*; use crate::reader::sqlite::SqliteDialect; use crate::reader::test_support::assert_dataframes_equal; - use crate::reader::{AdbcReader, Spec, SqlDialect}; + use crate::reader::{AdbcReader, ResolvedPlot, SqlDialect}; use crate::{DataFrame, Result}; use adbc_core::options::{AdbcVersion, OptionDatabase, OptionValue}; use adbc_core::LOAD_FLAG_DEFAULT; @@ -329,7 +329,7 @@ mod adbc_mode { fn unregister(&self, name: &str) -> Result<()> { self.inner.unregister(name) } - fn execute(&self, query: &str) -> Result { + fn execute(&self, query: &str) -> Result { crate::reader::execute_with_reader(self, query) } fn dialect(&self) -> &dyn SqlDialect { diff --git a/src/reader/duckdb.rs b/src/reader/duckdb.rs index cde53e32e..2ce66f725 100644 --- a/src/reader/duckdb.rs +++ b/src/reader/duckdb.rs @@ -503,7 +503,7 @@ impl Reader for DuckDBReader { Ok(()) } - fn execute(&self, query: &str) -> Result { + fn execute(&self, query: &str) -> Result { super::execute_with_reader(self, query) } diff --git a/src/reader/mod.rs b/src/reader/mod.rs index aaa961a83..11d183447 100644 --- a/src/reader/mod.rs +++ b/src/reader/mod.rs @@ -7,7 +7,7 @@ //! //! All readers implement the `Reader` trait, which provides: //! - SQL query execution → DataFrame conversion -//! - Visualization query execution → Spec +//! - Visualization query execution → ResolvedPlot //! - Optional DataFrame registration for queryable tables //! - Connection management and error handling //! @@ -502,7 +502,7 @@ pub(crate) fn returns_rows(sql: &str) -> bool { #[cfg(test)] pub(crate) mod test_support { use super::{ - execute_with_reader, returns_rows, ColumnInfo, Reader, Spec, SqlDialect, TableInfo, + execute_with_reader, returns_rows, ColumnInfo, Reader, ResolvedPlot, SqlDialect, TableInfo, }; use crate::{DataFrame, GgsqlError, Result}; use std::sync::{Arc, Mutex}; @@ -541,7 +541,7 @@ pub(crate) mod test_support { fn unregister(&self, name: &str) -> Result<()> { self.inner.unregister(name) } - fn execute(&self, query: &str) -> Result { + fn execute(&self, query: &str) -> Result { execute_with_reader(self, query) } fn dialect(&self) -> &dyn SqlDialect { @@ -591,7 +591,7 @@ pub(crate) mod test_support { fn unregister(&self, _name: &str) -> Result<()> { Err(Self::refuse("unregister")) } - fn execute(&self, query: &str) -> Result { + fn execute(&self, query: &str) -> Result { execute_with_reader(self, query) } fn dialect(&self) -> &dyn SqlDialect { @@ -670,11 +670,11 @@ pub(crate) mod test_support { } // ============================================================================ -// Spec - Result of reader.execute() +// ResolvedPlot - Result of reader.execute() // ============================================================================ /// Result of executing a ggsql query, ready for rendering. -pub struct Spec { +pub struct ResolvedPlot { /// Single resolved plot specification pub(crate) plot: Plot, /// Internal data map (global + layer-specific DataFrames) @@ -797,7 +797,7 @@ pub trait Reader { /// Execute a ggsql query and return the visualization specification. /// /// This is the main entry point for creating visualizations. It parses the query, - /// executes the SQL portion, and returns a `Spec` ready for rendering. + /// executes the SQL portion, and returns a `ResolvedPlot` ready for rendering. /// /// # Arguments /// @@ -805,7 +805,7 @@ pub trait Reader { /// /// # Returns /// - /// A `Spec` containing the resolved visualization specification and data. + /// A `ResolvedPlot` containing the visualization specification and data. /// /// # Errors /// @@ -826,7 +826,7 @@ pub trait Reader { /// let writer = VegaLiteWriter::new(); /// let json = writer.render(&spec)?; /// ``` - fn execute(&self, query: &str) -> Result; + fn execute(&self, query: &str) -> Result; /// Get the SQL dialect for this reader. /// @@ -967,7 +967,7 @@ pub struct ColumnInfo { /// This is the shared implementation behind `Reader::execute()`. Concrete /// readers delegate to this so the trait stays object-safe (no `Self: Sized` /// bound on `execute`). -pub fn execute_with_reader(reader: &dyn Reader, query: &str) -> Result { +pub fn execute_with_reader(reader: &dyn Reader, query: &str) -> Result { let validated = validate(query)?; let warnings: Vec = validated.warnings().to_vec(); @@ -981,7 +981,7 @@ pub fn execute_with_reader(reader: &dyn Reader, query: &str) -> Result { let layer_sql = vec![None; plot.layers.len()]; let stat_sql = vec![None; plot.layers.len()]; - Ok(Spec::new( + Ok(ResolvedPlot::new( plot, prepared_data.data, prepared_data.sql, diff --git a/src/reader/odbc/mod.rs b/src/reader/odbc/mod.rs index 740912e26..2f14fe032 100644 --- a/src/reader/odbc/mod.rs +++ b/src/reader/odbc/mod.rs @@ -226,7 +226,7 @@ impl Reader for OdbcReader { Ok(()) } - fn execute(&self, query: &str) -> Result { + fn execute(&self, query: &str) -> Result { super::execute_with_reader(self, query) } diff --git a/src/reader/spec.rs b/src/reader/spec.rs index aee78f4db..7709d51cd 100644 --- a/src/reader/spec.rs +++ b/src/reader/spec.rs @@ -1,4 +1,4 @@ -//! Implementation of Spec methods. +//! Implementation of ResolvedPlot methods. use std::collections::HashMap; @@ -7,10 +7,10 @@ use crate::plot::Plot; use crate::validate::ValidationWarning; use crate::DataFrame; -use super::{Metadata, Spec}; +use super::{Metadata, ResolvedPlot}; -impl Spec { - /// Create a new Spec from PreparedData +impl ResolvedPlot { + /// Create a new ResolvedPlot from PreparedData pub(crate) fn new( plot: Plot, data: HashMap, diff --git a/src/reader/sqlite.rs b/src/reader/sqlite.rs index 6df181633..54c144c9d 100644 --- a/src/reader/sqlite.rs +++ b/src/reader/sqlite.rs @@ -568,7 +568,7 @@ impl Reader for SqliteReader { Ok(()) } - fn execute(&self, query: &str) -> Result { + fn execute(&self, query: &str) -> Result { super::execute_with_reader(self, query) } diff --git a/src/writer/hephaestus/CLAUDE.md b/src/writer/hephaestus/CLAUDE.md index 3360d5718..0b53e6b87 100644 --- a/src/writer/hephaestus/CLAUDE.md +++ b/src/writer/hephaestus/CLAUDE.md @@ -1,6 +1,6 @@ # `writer/hephaestus/` — PNG writer internals -`PngWriter` renders a resolved ggsql `Spec` to **PNG bytes** via +`PngWriter` renders a ggsql `ResolvedPlot` to **PNG bytes** via [hephaestus](https://github.com/posit-dev/hephaestus), a 2D scene renderer with a grammar-of-graphics plot API. Behind the non-default `png` cargo feature. @@ -22,7 +22,7 @@ sibling writer's internals, [`../vegalite/CLAUDE.md`](../vegalite/CLAUDE.md). ## The governing principle **ggsql owns every scale domain; the writer never computes its own extents.** -The `Spec` arrives with each `Scale` fully resolved — type, domain (already +The `ResolvedPlot` arrives with each `Scale` fully resolved — type, domain (already expanded, transform-aware, trained globally over all layers and the whole position family), transform, breaks, formatted labels, and a concrete output range for material aesthetics. The writer's job is to *pass those through* to @@ -465,7 +465,7 @@ small omission that only shows up across the whole feature surface — use the visual-test harness instead of one-off queries. It renders every executable ```` ```{ggsql} ```` cell in [`/doc/`](../../../doc/) (≈190 in `doc/syntax/` alone) and writes one HTML report pairing each query with its render, optionally -beside the Vega-Lite render of the same `Spec`: +beside the Vega-Lite render of the same `ResolvedPlot`: ```sh cargo run -p ggsql-cli --features png --example visual_test -- --compare diff --git a/src/writer/hephaestus/mod.rs b/src/writer/hephaestus/mod.rs index 4d8edf7a1..0c4b755c4 100644 --- a/src/writer/hephaestus/mod.rs +++ b/src/writer/hephaestus/mod.rs @@ -1,6 +1,6 @@ //! PNG raster writer. //! -//! Renders a resolved ggsql `Spec` to PNG bytes via the [`hephaestus`] 2D scene +//! Renders a ggsql `ResolvedPlot` to PNG bytes via the [`hephaestus`] 2D scene //! renderer. Only [`PngWriter`] is public; the renderer behind it is an //! implementation detail. //! diff --git a/src/writer/mod.rs b/src/writer/mod.rs index a0aad469a..66fe68831 100644 --- a/src/writer/mod.rs +++ b/src/writer/mod.rs @@ -6,7 +6,7 @@ //! # Architecture //! //! All writers implement the `Writer` trait, which provides: -//! - Spec + Data → Output conversion +//! - ResolvedPlot + Data → Output conversion //! - Validation for writer compatibility //! - Format-specific rendering logic //! @@ -28,7 +28,7 @@ //! key–value [`WriterOptions`] when a frontend collects settings from a user //! without knowing which writer they picked. -use crate::reader::Spec; +use crate::reader::ResolvedPlot; use crate::{DataFrame, Plot, Result}; use std::collections::HashMap; @@ -114,7 +114,7 @@ pub trait Writer { /// Ok(()) if the spec is compatible, otherwise an error fn validate(&self, spec: &Plot) -> Result<()>; - /// Render a Spec to output format + /// Render a ResolvedPlot to output format /// /// This is the main entry point for generating visualization output. /// @@ -138,7 +138,7 @@ pub trait Writer { /// let writer = VegaLiteWriter::new(); /// let json = writer.render(&spec)?; /// ``` - fn render(&self, spec: &Spec) -> Result { + fn render(&self, spec: &ResolvedPlot) -> Result { self.write(spec.plot(), spec.data()) } } From daced07678f8e1f0ea30abcbe9495d60defcaca4 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 8 Sep 2026 14:44:48 +0200 Subject: [PATCH 02/17] Add Spec enum for Plot/Table plumbing Parse query / build AST will now produce Spec::{Plot,Table}. Up- and downstream plumbing still missing. --- ggsql-cli/src/main.rs | 19 +++++++---- src/CLAUDE.md | 8 +++-- src/execute/mod.rs | 16 ++++++--- src/lib.rs | 6 ++++ src/parser/builder.rs | 11 +++--- src/parser/mod.rs | 59 +++++++++++++++++++------------- src/spec.rs | 79 +++++++++++++++++++++++++++++++++++++++++++ src/table/mod.rs | 15 ++++++++ src/validate.rs | 13 ++++--- 9 files changed, 182 insertions(+), 44 deletions(-) create mode 100644 src/spec.rs create mode 100644 src/table/mod.rs diff --git a/ggsql-cli/src/main.rs b/ggsql-cli/src/main.rs index a274d3dc7..438c6a162 100644 --- a/ggsql-cli/src/main.rs +++ b/ggsql-cli/src/main.rs @@ -461,12 +461,19 @@ fn cmd_parse(query: String, format: String) { "pretty" => { println!("ggsql Specifications: {} total", specs.len()); for (i, spec) in specs.iter().enumerate() { - println!("\nVisualization #{}:", i + 1); - println!(" Global Mappings: {:?}", spec.global_mappings); - println!(" Layers: {}", spec.layers.len()); - println!(" Scales: {}", spec.scales.len()); - if spec.facet.is_some() { - println!(" Faceting: Yes"); + match spec { + ggsql::Spec::Plot(plot) => { + println!("\nVisualization #{}:", i + 1); + println!(" Global Mappings: {:?}", plot.global_mappings); + println!(" Layers: {}", plot.layers.len()); + println!(" Scales: {}", plot.scales.len()); + if plot.facet.is_some() { + println!(" Faceting: Yes"); + } + } + ggsql::Spec::Table(_) => { + println!("\nTable #{}:", i + 1); + } } } } diff --git a/src/CLAUDE.md b/src/CLAUDE.md index bb2996d52..1c81394a6 100644 --- a/src/CLAUDE.md +++ b/src/CLAUDE.md @@ -17,11 +17,13 @@ src/ ├── dataframe.rs DataFrame wrapper around arrow RecordBatch ├── format.rs Label/number/date formatting ├── naming.rs Internal column-name conventions (__ggsql_*) +├── spec.rs Spec: parse-time result of one VISUALISE/TABULATE statement (Plot or Table) ├── util.rs String helpers (and_list, or_list, …) ├── validate.rs validate(): syntax + semantic checks without SQL execution │ -├── parser/ Tree-sitter integration → typed AST (Plot) +├── parser/ Tree-sitter integration → typed AST (Spec: Plot or Table) ├── plot/ AST: Plot, Layer, Geom, Scale, Facet, Projection, Mappings (see plot/CLAUDE.md) +├── table/ AST stub for TABULATE, parallel to plot/ (no fields yet) ├── reader/ Reader trait + drivers (DuckDB, SQLite, ODBC, Snowflake, …) ├── execute/ Pipeline that turns Plot + Reader → ResolvedPlot ├── writer/ Writer trait + Vega-Lite implementation (see writer/vegalite/CLAUDE.md) @@ -31,9 +33,9 @@ src/ ### `parser/` -- `mod.rs` exposes `parse_query()` which builds a `Vec` from a query string. +- `mod.rs` exposes `parse_query()` which builds a `Vec` from a query string — one `Spec` per `VISUALISE`/`TABULATE` statement, in source order. Today `build_ast` only ever produces `Spec::Plot`; `TABULATE` isn't wired into the grammar yet. - `source_tree.rs` is the parse-once wrapper: holds the tree-sitter `Tree`, source text, and language; offers a declarative query API (`find_node`, `find_text`, …) plus lazy `extract_sql()` / `extract_visualise()` extractors. It also handles the `VISUALISE FROM ` shorthand by injecting `SELECT * FROM `. -- `builder.rs` walks the CST and produces typed `Plot` values. This is where new grammar nodes become `Plot` fields. +- `builder.rs` walks the CST and produces typed `Spec` values (`Plot`, boxed for size, or `Table`). This is where new grammar nodes become `Plot`/`Table` fields. - `sql.rs` extracts structure from SQL fragments over the parse tree. Grammar lives in [`/tree-sitter-ggsql/`](../tree-sitter-ggsql/) — when adding syntax, edit `grammar.js`, regenerate, then teach `builder.rs` about the new nodes. diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 9f633d10f..33014a7f4 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -29,7 +29,7 @@ use crate::plot::facet::{resolve_properties as resolve_facet_properties, FacetDa use crate::plot::layer::is_transposed; use crate::plot::projection::resolve_projection_properties; use crate::plot::{AestheticValue, Layer, Scale, ScaleTypeKind, Schema}; -use crate::{DataFrame, DataSource, GgsqlError, Plot, Result}; +use crate::{DataFrame, DataSource, GgsqlError, Plot, Result, Spec}; use std::collections::{HashMap, HashSet}; use crate::reader::Reader; @@ -1108,7 +1108,10 @@ pub fn prepare_data_with_reader(query: &str, reader: &dyn Reader) -> Result Result = parser::build_ast(&source_tree)? + .into_iter() + .filter_map(Spec::into_plot) + .collect(); if specs.is_empty() { return Err(GgsqlError::ValidationError( diff --git a/src/lib.rs b/src/lib.rs index b7353bb4d..6abe94791 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -42,6 +42,8 @@ pub mod format; pub mod naming; pub mod parser; pub mod plot; +pub mod spec; +pub mod table; pub mod util; pub mod reader; @@ -59,6 +61,10 @@ pub use plot::{ SqlExpression, }; +// Re-export the parse-time Plot/Table result and the Table stub +pub use spec::Spec; +pub use table::Table; + // Re-export aesthetic classification utilities pub use plot::aesthetic::{ is_position_aesthetic, AestheticContext, MATERIAL_AESTHETICS, POSITION_SUFFIXES, diff --git a/src/parser/builder.rs b/src/parser/builder.rs index d2f44e735..6f11ab7f4 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -8,7 +8,7 @@ use crate::plot::layer::geom::Geom; use crate::plot::projection::resolve_coord; use crate::plot::scale::{color_to_hex, is_color_aesthetic, is_user_facet_aesthetic, Transform}; use crate::plot::*; -use crate::{GgsqlError, Result}; +use crate::{GgsqlError, Result, Spec}; use std::collections::HashMap; use tree_sitter::Node; @@ -194,7 +194,7 @@ fn parse_literal_value(node: &Node, source: &SourceTree) -> Result Result> { +pub fn build_ast(source: &SourceTree) -> Result> { let root = source.root(); // Check if root is a query node @@ -234,7 +234,7 @@ pub fn build_ast(source: &SourceTree) -> Result> { )); } - specs.push(spec); + specs.push(Spec::Plot(Box::new(spec))); } if specs.is_empty() { @@ -1216,7 +1216,10 @@ mod tests { let source = SourceTree::new(query)?; source.validate()?; - build_ast(&source) + Ok(build_ast(&source)? + .into_iter() + .filter_map(Spec::into_plot) + .collect()) } // ======================================== diff --git a/src/parser/mod.rs b/src/parser/mod.rs index a9171c64f..15bab19db 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -19,7 +19,7 @@ the visualization specification into a typed AST. ```rust # use ggsql::parser::parse_query; -# use ggsql::Geom; +# use ggsql::{Geom, Spec}; # fn main() -> Result<(), Box> { let query = r#" SELECT date, revenue, region FROM sales WHERE year = 2024 @@ -31,14 +31,17 @@ let query = r#" let specs = parse_query(query)?; assert_eq!(specs.len(), 1); -assert_eq!(specs[0].layers.len(), 1); -assert_eq!(specs[0].layers[0].geom, Geom::line()); +let Spec::Plot(plot) = &specs[0] else { + panic!("expected a Plot"); +}; +assert_eq!(plot.layers.len(), 1); +assert_eq!(plot.layers[0].geom, Geom::line()); # Ok(()) # } ``` */ -use crate::{Plot, Result}; +use crate::{Result, Spec}; pub mod builder; pub mod source_tree; @@ -55,7 +58,7 @@ pub use sql::{ /// /// Takes a complete ggsql query (SQL + VISUALISE) and returns a vector of /// parsed specifications (one per VISUALISE statement). -pub fn parse_query(query: &str) -> Result> { +pub fn parse_query(query: &str) -> Result> { // Parse the full query and create SourceTree let source_tree = SourceTree::new(query)?; @@ -72,7 +75,17 @@ pub fn parse_query(query: &str) -> Result> { mod tests { use super::*; use crate::plot::ParameterValue; - use crate::{AestheticValue, DataSource, Geom}; + use crate::{AestheticValue, DataSource, Geom, Plot}; + + /// Test helper: `parse_query`, then unwrap every result down to its `Plot`. + /// These tests predate the `Table` variant and only ever exercise VISUALISE + /// queries, so every `Spec` here is a `Spec::Plot`. + fn parse_query_plots(query: &str) -> Result> { + Ok(parse_query(query)? + .into_iter() + .filter_map(Spec::into_plot) + .collect()) + } #[test] fn test_simple_query_parsing() { @@ -82,7 +95,7 @@ mod tests { DRAW point "#; - let result = parse_query(query); + let result = parse_query_plots(query); assert!(result.is_ok(), "Failed to parse simple query: {:?}", result); let specs = result.unwrap(); @@ -115,7 +128,7 @@ mod tests { DRAW point MAPPING z AS y, 'value' AS color "#; - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 1); assert_eq!(specs[0].layers.len(), 2); // First layer is line, second layer is point @@ -140,7 +153,7 @@ mod tests { DRAW bar MAPPING x AS x, y AS y "#; - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 2); assert_eq!(specs[0].layers.len(), 1); assert_eq!(specs[1].layers.len(), 1); @@ -154,7 +167,7 @@ mod tests { DRAW point "#; - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 1); assert_eq!(specs[0].layers[0].geom, Geom::point()); } @@ -171,7 +184,7 @@ mod tests { DRAW point "#; - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 3); assert_eq!(specs[0].layers.len(), 1); assert_eq!(specs[1].layers.len(), 1); @@ -186,7 +199,7 @@ mod tests { DRAW point MAPPING x AS x, y AS y "#; - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 1); assert!(specs[0].global_mappings.is_empty()); } @@ -202,7 +215,7 @@ mod tests { DRAW bar MAPPING x AS x, y AS y "#; - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 2); // First viz should have layers and labels @@ -226,7 +239,7 @@ mod tests { DRAW bar MAPPING x AS x, y AS y "#; - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 3); assert_eq!(specs[0].layers[0].geom, Geom::line()); assert_eq!(specs[1].layers[0].geom, Geom::point()); @@ -249,7 +262,7 @@ mod tests { DRAW point MAPPING date AS x, revenue AS y "#; - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 3); // Plot with 2 layers, scale, labels @@ -288,7 +301,7 @@ mod tests { source_tree ); - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 1); } @@ -300,7 +313,7 @@ mod tests { DRAW point "#; - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 1); assert!(specs[0].global_mappings.wildcard); assert!(specs[0].global_mappings.aesthetics.is_empty()); @@ -313,7 +326,7 @@ mod tests { DRAW line "#; - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 1); let mapping = &specs[0].global_mappings; assert!(!mapping.wildcard); @@ -339,7 +352,7 @@ mod tests { DRAW point "#; - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 1); let mapping = &specs[0].global_mappings; assert!(!mapping.wildcard); @@ -363,7 +376,7 @@ mod tests { DRAW point "#; - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 1); let mapping = &specs[0].global_mappings; assert!(!mapping.wildcard); @@ -390,7 +403,7 @@ mod tests { DRAW bar "#; - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 1); assert_eq!( specs[0].source, @@ -406,7 +419,7 @@ mod tests { DRAW point "#; - let specs = parse_query(query).unwrap(); + let specs = parse_query_plots(query).unwrap(); assert_eq!(specs.len(), 1); assert_eq!( specs[0].source, @@ -423,7 +436,7 @@ mod tests { PLACE text SETTING x => 5, y => 10, label => 'Hello' "#; - let result = parse_query(query); + let result = parse_query_plots(query); assert!(result.is_ok(), "Failed to parse PLACE clause: {:?}", result); let specs = result.unwrap(); diff --git a/src/spec.rs b/src/spec.rs new file mode 100644 index 000000000..d96725907 --- /dev/null +++ b/src/spec.rs @@ -0,0 +1,79 @@ +//! The parse-time result of a single VISUALISE/TABULATE statement. + +use serde::{Deserialize, Serialize}; + +use crate::{Plot, Table}; + +/// One parsed statement from a ggsql query: either a visualization or a table. +/// +/// A query may contain several `VISUALISE`/`TABULATE` statements +/// (`parser::parse_query` returns one `Spec` per statement, in source order). +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum Spec { + // Boxed: `Plot` is far larger than `Table`, and clippy flags the + // resulting size gap (`large_enum_variant`) otherwise. + Plot(Box), + Table(Table), +} + +impl Spec { + /// Borrow the inner `Plot`, or `None` if this is a `Table`. + pub fn as_plot(&self) -> Option<&Plot> { + match self { + Spec::Plot(plot) => Some(plot), + Spec::Table(_) => None, + } + } + + /// Borrow the inner `Table`, or `None` if this is a `Plot`. + pub fn as_table(&self) -> Option<&Table> { + match self { + Spec::Plot(_) => None, + Spec::Table(table) => Some(table), + } + } + + /// Consume this `Spec`, returning the inner `Plot`, or `None` if it was a `Table`. + pub fn into_plot(self) -> Option { + match self { + Spec::Plot(plot) => Some(*plot), + Spec::Table(_) => None, + } + } + + /// Consume this `Spec`, returning the inner `Table`, or `None` if it was a `Plot`. + pub fn into_table(self) -> Option { + match self { + Spec::Plot(_) => None, + Spec::Table(table) => Some(table), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn as_plot_and_as_table_are_mutually_exclusive() { + let plot = Spec::Plot(Box::default()); + assert!(plot.as_plot().is_some()); + assert!(plot.as_table().is_none()); + + let table = Spec::Table(Table {}); + assert!(table.as_plot().is_none()); + assert!(table.as_table().is_some()); + } + + #[test] + fn into_plot_and_into_table_are_mutually_exclusive() { + let plot = Spec::Plot(Box::default()); + assert!(plot.into_plot().is_some()); + + let table = Spec::Table(Table {}); + assert!(table.into_table().is_some()); + + assert!(Spec::Plot(Box::default()).into_table().is_none()); + assert!(Spec::Table(Table {}).into_plot().is_none()); + } +} diff --git a/src/table/mod.rs b/src/table/mod.rs new file mode 100644 index 000000000..a77943e71 --- /dev/null +++ b/src/table/mod.rs @@ -0,0 +1,15 @@ +//! Table types for ggsql specification +//! +//! This module will define the typed `Table` structure that represents parsed +//! `TABULATE` statements, parallel to how `plot` defines `Plot` for `VISUALISE` +//! statements. It is currently a stub: no clauses parse into it yet. + +use serde::{Deserialize, Serialize}; + +/// Complete ggsql table specification. +/// +/// Parallel to [`crate::Plot`], but for `TABULATE` statements. No fields yet — +/// this exists so the rest of the pipeline (parser, [`crate::Spec`]) has a +/// concrete type to route through before any table-specific syntax lands. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct Table {} diff --git a/src/validate.rs b/src/validate.rs index 2199bb956..8f2850966 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -4,7 +4,7 @@ //! any SQL. Use this for IDE integration, syntax checking, and query inspection. use crate::parser; -use crate::Result; +use crate::{Plot, Result, Spec}; // ============================================================================ // Core Types @@ -186,9 +186,14 @@ pub fn validate(query: &str) -> Result { }); } - // Build AST from existing tree for validation - let plots = match parser::build_ast(&source_tree) { - Ok(p) => p, + // Build AST from existing tree for validation. Table specs are silently + // dropped here: there is no table-validation path yet, so a TABULATE-only + // query ends up with an empty `plots`, skips the `if let Some(plot) = + // plots.first()` block below entirely, and is reported `valid: true` + // without anything having actually been validated. Known gap, not a + // deliberate choice. + let plots: Vec = match parser::build_ast(&source_tree) { + Ok(specs) => specs.into_iter().filter_map(Spec::into_plot).collect(), Err(e) => { errors.push(ValidationError { message: e.to_string(), From 40ad1d250f314cd24cb00720983e98973614d24b Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 9 Sep 2026 11:58:19 +0200 Subject: [PATCH 03/17] Wire TABULATE into grammer. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wire TABULATE into the grammar Adds tabulate_statement/tabulate_keyword to grammar.js, parallel to VISUALISE, so build_ast constructs real Spec::Table values instead of the type existing but never being reachable from source text. TABULATE FROM reuses the same restricted single-source grammar as VISUALISE FROM (no joins/comma-lists) via a shared single_source_from rule; Table itself stays fieldless, so a parsed FROM source is discarded rather than stored. Generalizes the SQL/Spec boundary detection that VISUALISE previously owned alone: SourceTree::extract_visualise() (now extract_spec(), to match the Spec enum), extract_sql()'s boundary check, and check_no_double_from() all recognize TABULATE too, so a TABULATE-only query no longer has its keyword silently swallowed into "SQL" text. Renames Validated::has_visual to has_spec for the same reason. Table's own validation/execution paths remain known gaps (see inline "Known gap" comments and golden tests in validate.rs/execute/mod.rs) — this is grammar and parse-tree plumbing only. --- ggsql-cli/CLAUDE.md | 2 +- ggsql-cli/examples/visual_test.rs | 6 +- ggsql-cli/src/main.rs | 2 +- ggsql-jupyter/src/executor.rs | 2 +- ggsql-wasm/src/lib.rs | 2 +- src/CLAUDE.md | 2 +- src/doc/API.md | 6 +- src/execute/cte.rs | 4 +- src/execute/mod.rs | 22 +++- src/parser/builder.rs | 131 ++++++++++++++++++++---- src/parser/source_tree.rs | 87 +++++++++------- src/validate.rs | 55 ++++++---- tree-sitter-ggsql/grammar.js | 25 +++-- tree-sitter-ggsql/test/corpus/basic.txt | 49 +++++++-- 14 files changed, 289 insertions(+), 106 deletions(-) diff --git a/ggsql-cli/CLAUDE.md b/ggsql-cli/CLAUDE.md index eee23793d..dc43b095d 100644 --- a/ggsql-cli/CLAUDE.md +++ b/ggsql-cli/CLAUDE.md @@ -94,7 +94,7 @@ open target/visual-test/index.html Four properties are worth preserving when changing it: -- **One reader per source file, cells in document order.** Doc pages build a table in one cell and plot it in the next, so per-cell isolation would break the corpus. A cell with no `VISUALISE` (`validate(..).has_visual()` is false) runs as setup through `execute_sql`. +- **One reader per source file, cells in document order.** Doc pages build a table in one cell and plot it in the next, so per-cell isolation would break the corpus. A cell with no `VISUALISE`/`TABULATE` (`validate(..).has_spec()` is false) runs as setup through `execute_sql`. - **Cells run in their own page's directory**, as Quarto runs them, so a query reading `FROM 'minard_troops.csv'` finds the CSV sitting beside the `.qmd`. The report and its `assets/` are resolved to an absolute path up front, since they outlive that switch. - **Nothing aborts the run.** Execution errors, render errors and *panics* inside a writer are captured per cell (`capture`), so one report surfaces every problem in the corpus at once. This is the point of the tool — a run that stops at the first failure tells you almost nothing. - **Renders are files, specs are inline.** PNGs are written to `assets/`; Vega-Lite specs are embedded in `"), - "<script>alert('xss')</script>" - ); - } - fn positron_console() -> RenderHints { RenderHints { is_notebook: false, diff --git a/src/util.rs b/src/util.rs index 9c7b5a073..f257ea228 100644 --- a/src/util.rs +++ b/src/util.rs @@ -87,6 +87,16 @@ pub fn set_union(mut old: Vec, new: &[String]) -> Vec { old } +/// Escape HTML special characters. `&` must be replaced first, or the +/// entities inserted for the others would themselves get escaped. +pub fn escape_html(s: &str) -> String { + s.replace('&', "&") + .replace('<', "<") + .replace('>', ">") + .replace('"', """) + .replace('\'', "'") +} + #[cfg(test)] mod tests { use super::*; @@ -237,4 +247,15 @@ mod tests { assert_eq!(and_list(&[1, 2, 3]), "1, 2, and 3"); assert_eq!(or_list(&[42, 99]), "42 or 99"); } + + #[test] + fn test_escape_html() { + assert_eq!( + escape_html(""), + "<script>alert('xss')</script>" + ); + assert_eq!(escape_html("a & b"), "a & b"); + assert_eq!(escape_html("say \"hi\""), "say "hi""); + assert_eq!(escape_html("plain text"), "plain text"); + } } diff --git a/src/writer/html.rs b/src/writer/html.rs index 0fe378538..9d362e60a 100644 --- a/src/writer/html.rs +++ b/src/writer/html.rs @@ -11,6 +11,7 @@ use std::collections::HashMap; use crate::array_util::value_to_string; +use crate::util::escape_html; use crate::writer::{Writer, WriterOptions}; use crate::{DataFrame, GgsqlError, Plot, Result, Table}; @@ -70,16 +71,6 @@ impl Writer for HtmlWriter { } } -/// Escape HTML special characters. `&` must be replaced first, or the -/// entities inserted for the others would themselves get escaped. -fn escape_html(s: &str) -> String { - s.replace('&', "&") - .replace('<', "<") - .replace('>', ">") - .replace('"', """) - .replace('\'', "'") -} - #[cfg(test)] #[cfg(feature = "duckdb")] mod tests { From c978c37303a28aff3b83fe613f34f037a8a50bb4 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 10 Sep 2026 12:06:54 +0200 Subject: [PATCH 12/17] Wire ggsql-cli to HtmlWriter; unblock writer module from vegalite ggsql exec/run now accept --writer html, rendering a TABULATE result via HtmlWriter. render_spec matches on ResolvedSpec::Plot/Table directly instead of rejecting Table upfront; Plot-only pre-checks (metadata, empty-layer) now only run for the Plot case, and the Text-output message no longer hardcodes "Vega-Lite JSON" now that PNG and HTML share the same output path. Wiring HtmlWriter through the CLI kept needing awkward per-call-site vegalite gates, because lib.rs's pub mod writer was entirely gated on `feature = "vegalite"` even though the Writer trait, WriterOptions, PngWriter and HtmlWriter have no real dependency on it. Fixed the root cause instead of accumulating workarounds: the gate is removed; VegaLiteWriter still gates itself internally, untouched. That fix surfaced one real, previously-silent test gap: data.rs's test_ribbon_transposed_vegalite_encoding used VegaLiteWriter unconditionally inside a test module gated only on duckdb+builtin-data. Given its own #[cfg(feature = "vegalite")]. --- ggsql-cli/src/main.rs | 76 +++++++++++++++++++++++++------------------ src/lib.rs | 1 - src/reader/data.rs | 1 + src/writer/mod.rs | 4 +-- 4 files changed, 47 insertions(+), 35 deletions(-) diff --git a/ggsql-cli/src/main.rs b/ggsql-cli/src/main.rs index d6e97e4c5..2a096a9f8 100644 --- a/ggsql-cli/src/main.rs +++ b/ggsql-cli/src/main.rs @@ -7,7 +7,7 @@ Provides commands for executing ggsql queries with various data sources and outp use clap::{Parser, Subcommand, ValueEnum}; use ggsql::reader::{Reader, ResolvedSpec}; use ggsql::validate::validate; -use ggsql::writer::{Writer, WriterOptions}; +use ggsql::writer::{HtmlWriter, Writer, WriterOptions}; use ggsql::{parser, VERSION}; use std::io::{IsTerminal, Write}; use std::path::PathBuf; @@ -72,8 +72,9 @@ pub enum Commands { #[arg(long)] cache: Option, - /// Output format: vegalite (JSON), or png (raster image; requires the - /// `png` feature and a GPU adapter) + /// Output format: vegalite (JSON), html (a plain table; TABULATE + /// queries only), or png (raster image; requires the `png` feature + /// and a GPU adapter) #[arg(short, long, default_value = "vegalite")] writer: String, @@ -81,7 +82,7 @@ pub enum Commands { /// flag may carry several settings separated by `;` (quote it, as most /// shells read `;` themselves): `-D 'width=1600;dpi=150'`. The /// png writer takes width, height, units, dpi, and background; - /// the vegalite writer takes none. + /// the vegalite and html writers take none. #[arg( short = 'D', long = "writer-option", @@ -112,8 +113,9 @@ pub enum Commands { #[arg(long)] cache: Option, - /// Output format: vegalite (JSON), or png (raster image; requires the - /// `png` feature and a GPU adapter) + /// Output format: vegalite (JSON), html (a plain table; TABULATE + /// queries only), or png (raster image; requires the `png` feature + /// and a GPU adapter) #[arg(short, long, default_value = "vegalite")] writer: String, @@ -121,7 +123,7 @@ pub enum Commands { /// flag may carry several settings separated by `;` (quote it, as most /// shells read `;` themselves): `-D 'width=1600;dpi=150'`. The /// png writer takes width, height, units, dpi, and background; - /// the vegalite writer takes none. + /// the vegalite and html writers take none. #[arg( short = 'D', long = "writer-option", @@ -377,36 +379,37 @@ fn exec_with_reader( } fn render_spec(spec: ResolvedSpec, writer: &WriterSpec, output: Option, verbose: bool) { - // Known gap: no writer renders tables yet, so bail out here with a - // CLI-specific message rather than letting it flow into Plot-specific - // pre-checks below (metadata, layer checks) or a generic writer error. - let plot = match spec.as_plot() { - Some(plot) => plot, - None => { - eprintln!("TABULATE queries aren't supported by `exec`/`run` yet."); - std::process::exit(1); - } - }; - - if verbose { - let metadata = plot.metadata(); - eprintln!("\nQuery executed:"); - eprintln!(" Rows: {}", metadata.rows); - eprintln!(" Columns: {}", metadata.columns.join(", ")); - eprintln!(" Layers: {}", metadata.layer_count); - } + match &spec { + ResolvedSpec::Plot(plot) => { + if verbose { + let metadata = plot.metadata(); + eprintln!("\nQuery executed:"); + eprintln!(" Rows: {}", metadata.rows); + eprintln!(" Columns: {}", metadata.columns.join(", ")); + eprintln!(" Layers: {}", metadata.layer_count); + } - if plot.plot().layers.is_empty() { - eprintln!("No visualization specifications found"); - std::process::exit(1); + if plot.plot().layers.is_empty() { + eprintln!("No visualization specifications found"); + std::process::exit(1); + } + } + ResolvedSpec::Table(table) => { + if verbose { + eprintln!("\nQuery executed:"); + eprintln!(" Rows: {}", table.body().height()); + eprintln!(" Columns: {}", table.body().width()); + } + } } let render = match writer.name.as_str() { "vegalite" => render_vegalite(&spec, &writer.options), "png" => render_png(&spec, &writer.options), + "html" => render_html(&spec, &writer.options), other => { eprintln!("Unknown writer '{}'", other); - eprintln!("Available writers: png, vegalite"); + eprintln!("Available writers: html, png, vegalite"); std::process::exit(1) } }; @@ -418,7 +421,7 @@ fn render_spec(spec: ResolvedSpec, writer: &WriterSpec, output: Option, (Output::Text(txt), Some(path)) => match std::fs::write(&path, txt) { Ok(_) => { if verbose { - eprintln!("\nVega-Lite JSON written to: {}", path.display()); + eprintln!("\nOutput written to: {}", path.display()); } } Err(e) => { @@ -844,9 +847,20 @@ fn render_png(spec: &ResolvedSpec, options: &WriterOptions) -> Output { } } +fn render_html(spec: &ResolvedSpec, options: &WriterOptions) -> Output { + // Configure from --writer-option, then render + let html_writer = unwrap_writer(HtmlWriter::from_options(options)); + match html_writer.render(spec) { + Ok(html) => Output::Text(html), + Err(e) => { + eprintln!("Failed to generate HTML output: {}", e); + std::process::exit(1); + } + } +} + /// A writer built from its options, or the option error on stderr and a /// non-zero exit — an unusable setting is the user's mistake, not a warning. -#[cfg(any(feature = "vegalite", feature = "png"))] fn unwrap_writer(writer: ggsql::Result) -> W { writer.unwrap_or_else(|e| { eprintln!("{}", e); diff --git a/src/lib.rs b/src/lib.rs index f04f45dde..b00ac8612 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -48,7 +48,6 @@ pub mod util; pub mod reader; -#[cfg(feature = "vegalite")] pub mod writer; pub mod execute; diff --git a/src/reader/data.rs b/src/reader/data.rs index 47ef711a1..26022a2df 100644 --- a/src/reader/data.rs +++ b/src/reader/data.rs @@ -214,6 +214,7 @@ mod duckdb_tests { ); } + #[cfg(feature = "vegalite")] #[test] fn test_ribbon_transposed_vegalite_encoding() { use crate::reader::Reader; diff --git a/src/writer/mod.rs b/src/writer/mod.rs index 47df8365f..713a99108 100644 --- a/src/writer/mod.rs +++ b/src/writer/mod.rs @@ -52,9 +52,7 @@ mod hephaestus; pub use hephaestus::{rgba, Color, PngWriter}; // Pure string formatting, no extra dependencies — unlike vegalite/png, has no -// feature flag of its own. It's still only reachable when `vegalite` is on, -// though, since this whole module is gated on that in lib.rs (a pre-existing -// quirk — see the comment there). +// feature flag of its own. pub mod html; pub use html::HtmlWriter; From b5d9600d42edbbe705ccfd76e6e7d4a53d660759 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 10 Sep 2026 13:30:10 +0200 Subject: [PATCH 13/17] Wire TABULATE output through HtmlWriter in ggsql-jupyter A TABULATE query previously reached the hardcoded VegaLiteWriter and errored. ExecutionResult gains a Table variant carrying pre-rendered HTML; the executor dispatches on ResolvedSpec (Table -> HtmlWriter, Plot -> VegaLiteWriter) and display_data wraps the HTML directly, with no Positron-specific sizing since a plain
needs none. --- ggsql-jupyter/src/display.rs | 26 ++++++++++++++ ggsql-jupyter/src/executor.rs | 66 ++++++++++++++++++++++++----------- 2 files changed, 72 insertions(+), 20 deletions(-) diff --git a/ggsql-jupyter/src/display.rs b/ggsql-jupyter/src/display.rs index 7c81d14fa..5e81abe56 100644 --- a/ggsql-jupyter/src/display.rs +++ b/ggsql-jupyter/src/display.rs @@ -72,6 +72,7 @@ impl RenderHints { pub fn format_display_data(result: ExecutionResult, hints: &RenderHints) -> Option { match result { ExecutionResult::Visualization { spec } => Some(format_vegalite(spec, hints)), + ExecutionResult::Table { html } => Some(format_table(html)), ExecutionResult::DataFrame(df) => { // DDL statements return DataFrames with 0 columns - don't display anything if df.width() == 0 { @@ -86,6 +87,20 @@ pub fn format_display_data(result: ExecutionResult, hints: &RenderHints) -> Opti } } +/// Format a TABULATE result (already rendered to HTML by `HtmlWriter`) as +/// display_data. No `RenderHints` involved — a plain `
` needs no +/// container sizing or Positron-specific wrapping. +fn format_table(html: String) -> Value { + json!({ + "data": { + "text/html": html, + "text/plain": "ggsql table".to_string() + }, + "metadata": {}, + "transient": {} + }) +} + /// Format a connection-changed message fn format_connection_changed(display_name: &str) -> Value { let text = format!("Connected to {}", display_name); @@ -405,6 +420,17 @@ mod tests { assert!(display["data"]["text/plain"].is_string()); } + #[test] + fn test_table_format() { + let html = "
1
".to_string(); + let result = ExecutionResult::Table { html: html.clone() }; + let display = + format_display_data(result, &RenderHints::default()).expect("Table should return Some"); + + assert_eq!(display["data"]["text/html"], html); + assert!(display["data"]["text/plain"].is_string()); + } + #[test] fn test_empty_dataframe_returns_none() { // DDL statements return DataFrames with 0 columns diff --git a/ggsql-jupyter/src/executor.rs b/ggsql-jupyter/src/executor.rs index c23fe4fa3..042470b70 100644 --- a/ggsql-jupyter/src/executor.rs +++ b/ggsql-jupyter/src/executor.rs @@ -9,10 +9,10 @@ use anyhow::Result; use ggsql::{ reader::{ connection::{extract_odbc_value, reader_from_uri}, - Reader, + Reader, ResolvedSpec, }, validate::validate, - writer::{VegaLiteWriter, Writer}, + writer::{HtmlWriter, VegaLiteWriter, Writer}, DataFrame, }; @@ -25,6 +25,8 @@ pub enum ExecutionResult { Visualization { spec: String, // Vega-Lite JSON }, + /// TABULATE query, already rendered as an HTML table via `HtmlWriter`. + Table { html: String }, /// Connection changed via meta-command ConnectionChanged { display_name: String }, } @@ -247,26 +249,35 @@ impl QueryExecutor { // 3. Execute ggsql query using reader let spec = self.reader.execute(code)?; - if let Some(plot) = spec.as_plot() { - tracing::info!( - "Query executed: {} rows, {} layers", - plot.metadata().rows, - plot.metadata().layer_count - ); - } - - // 4. Render to output format. Known gap: a TABULATE query reaches - // here too (it has a Spec, so step 2's has_spec() check doesn't - // divert it to the pure-SQL path), and errors out right here since - // no writer supports ResolvedSpec::Table yet. There is no - // table-specific output path (HTML table, Positron data-explorer, - // etc.) wired up for it — only the pure-SQL branch above gets that. - let vega_json = self.writer.render(&spec)?; + // 4. Render to output format: a table goes through a fresh + // HtmlWriter (bare , no Positron-specific wrapping), a plot + // through the persistent VegaLiteWriter. + match &spec { + ResolvedSpec::Table(table) => { + tracing::info!( + "Query executed: {} rows, {} cols", + table.body().height(), + table.body().width() + ); + + let html = HtmlWriter::new().render(&spec)?; + tracing::debug!("Generated HTML table: {} chars", html.len()); + + Ok(ExecutionResult::Table { html }) + } + ResolvedSpec::Plot(plot) => { + tracing::info!( + "Query executed: {} rows, {} layers", + plot.metadata().rows, + plot.metadata().layer_count + ); - tracing::debug!("Generated Vega-Lite spec: {} chars", vega_json.len()); + let vega_json = self.writer.render(&spec)?; + tracing::debug!("Generated Vega-Lite spec: {} chars", vega_json.len()); - // 5. Return result - Ok(ExecutionResult::Visualization { spec: vega_json }) + Ok(ExecutionResult::Visualization { spec: vega_json }) + } + } } } @@ -283,6 +294,21 @@ mod tests { assert!(matches!(result, ExecutionResult::Visualization { .. })); } + #[test] + fn test_tabulate() { + let mut executor = QueryExecutor::new().unwrap(); + let code = "SELECT 1 AS x, 2 AS y TABULATE"; + let result = executor.execute(code).unwrap(); + + match result { + ExecutionResult::Table { html } => { + assert!(html.contains("
")); + assert!(html.contains("")); + } + other => panic!("expected Table, got {other:?}"), + } + } + #[test] fn test_pure_sql() { let mut executor = QueryExecutor::new().unwrap(); From 933a15f1dbbd3e7fcfe407dfea879fd77045ad19 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 10 Sep 2026 16:26:25 +0200 Subject: [PATCH 14/17] honour setup statements --- src/execute/mod.rs | 39 +++++++++++++++++++++++++++++++++------ src/execute/table.rs | 11 ++++++++--- 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 8419b0d04..9ada5a9a1 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -1093,6 +1093,19 @@ pub struct PreparedData { pub visual: String, } +/// Execute setup statements (INSTALL, LOAD, SET, etc.) ahead of the main +/// query. Shared by the Plot and Table pipelines (`prepare_data_with_reader` +/// and `table::resolve_table_with_reader`). Structured DML (CREATE, INSERT, +/// UPDATE, DELETE) is out of scope here — see `cte::extract_side_effects`, +/// which only the Plot pipeline currently runs. +fn execute_setup_statements(source_tree: &parser::SourceTree, reader: &dyn Reader) -> Result<()> { + let root = source_tree.root(); + for stmt in source_tree.find_texts(&root, "(sql_statement (other_sql_statement) @stmt)") { + reader.execute_sql(&stmt)?; + } + Ok(()) +} + /// Build data map from a query using a Reader /// /// This is the main entry point for preparing visualization data from a ggsql query. @@ -1139,12 +1152,7 @@ pub fn prepare_data_with_reader(query: &str, reader: &dyn Reader) -> Result Result { let validated = validate(query)?; let warnings: Vec = validated.warnings().to_vec(); @@ -39,6 +42,8 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result Date: Thu, 10 Sep 2026 16:32:10 +0200 Subject: [PATCH 15/17] dedup error message efforts --- src/parser/builder.rs | 52 +++++++++++++++++-------------------------- 1 file changed, 21 insertions(+), 31 deletions(-) diff --git a/src/parser/builder.rs b/src/parser/builder.rs index f9727c6b7..841c14565 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -223,43 +223,23 @@ pub fn build_ast(source: &SourceTree) -> Result> { let mut stmt_nodes: Vec = viz_nodes.into_iter().chain(tab_nodes).collect(); stmt_nodes.sort_by_key(|n| n.start_byte()); - // TODO: the "FROM after a trailing SELECT" check below is duplicated - // (with a different keyword) across the two arms. Both now check the same - // `.source.is_some()` shape, so the message-building (only difference - // left) could be factored into a shared `fn(has_from, last_is_select, - // keyword)` helper. let mut specs = Vec::new(); for stmt_node in stmt_nodes { - match stmt_node.kind() { + // Build the spec, then check the shared "FROM after a trailing + // SELECT" restriction once for whichever kind it is — VISUALISE FROM + // and TABULATE FROM both forbid it, differing only in keyword. + let (has_from, keyword, spec) = match stmt_node.kind() { "visualise_statement" => { - let spec = build_visualise_statement(&stmt_node, source)?; - - // Validate VISUALISE FROM usage - if spec.source.is_some() && last_is_select { - return Err(GgsqlError::ParseError( - "Cannot use VISUALISE FROM when the last SQL statement is SELECT. \ - Use either 'SELECT ... VISUALISE' or remove the SELECT and use \ - 'VISUALISE FROM ...'." - .to_string(), - )); - } - - specs.push(Spec::Plot(Box::new(spec))); + let plot = build_visualise_statement(&stmt_node, source)?; + ( + plot.source.is_some(), + "VISUALISE", + Spec::Plot(Box::new(plot)), + ) } "tabulate_statement" => { let table = build_tabulate_statement(&stmt_node, source); - - // Validate TABULATE FROM usage, mirroring VISUALISE FROM above. - if table.source.is_some() && last_is_select { - return Err(GgsqlError::ParseError( - "Cannot use TABULATE FROM when the last SQL statement is SELECT. \ - Use either 'SELECT ... TABULATE' or remove the SELECT and use \ - 'TABULATE FROM ...'." - .to_string(), - )); - } - - specs.push(Spec::Table(table)); + (table.source.is_some(), "TABULATE", Spec::Table(table)) } other => { return Err(GgsqlError::InternalError(format!( @@ -267,7 +247,17 @@ pub fn build_ast(source: &SourceTree) -> Result> { other ))); } + }; + + if has_from && last_is_select { + return Err(GgsqlError::ParseError(format!( + "Cannot use {keyword} FROM when the last SQL statement is SELECT. \ + Use either 'SELECT ... {keyword}' or remove the SELECT and use \ + '{keyword} FROM ...'." + ))); } + + specs.push(spec); } if specs.is_empty() { From fcaa1dc56dca839ed37186008ce14f80ca0a680a Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 11 Sep 2026 15:20:24 +0200 Subject: [PATCH 16/17] Validate TABULATE has a data source before executing A bare TABULATE with no FROM and no preceding SQL previously reported valid: true from validate() and only failed once SQL actually ran, since Table specs were silently dropped from validation entirely. build_ast's specs are now split into both plots and tables so this is caught up front, mirroring resolve_table_with_reader's existing rejection of the same query at execution time. --- src/validate.rs | 55 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 19 deletions(-) diff --git a/src/validate.rs b/src/validate.rs index bff30c22a..3f8b43fd4 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -4,7 +4,7 @@ //! any SQL. Use this for IDE integration, syntax checking, and query inspection. use crate::parser; -use crate::{Plot, Result, Spec}; +use crate::{Plot, Result, Spec, Table}; // ============================================================================ // Core Types @@ -187,14 +187,9 @@ pub fn validate(query: &str) -> Result { }); } - // Build AST from existing tree for validation. Table specs are silently - // dropped here: there is no table-validation path yet, so a TABULATE-only - // query ends up with an empty `plots`, skips the `if let Some(plot) = - // plots.first()` block below entirely, and is reported `valid: true` - // without anything having actually been validated. Known gap, not a - // deliberate choice. - let plots: Vec = match parser::build_ast(&source_tree) { - Ok(specs) => specs.into_iter().filter_map(Spec::into_plot).collect(), + // Build AST from existing tree for validation. + let specs: Vec = match parser::build_ast(&source_tree) { + Ok(specs) => specs, Err(e) => { errors.push(ValidationError { message: e.to_string(), @@ -211,6 +206,8 @@ pub fn validate(query: &str) -> Result { }); } }; + let plots: Vec<&Plot> = specs.iter().filter_map(Spec::as_plot).collect(); + let tables: Vec<&Table> = specs.iter().filter_map(Spec::as_table).collect(); // Validate the single plot (we only support one VISUALISE statement) if let Some(plot) = plots.first() { @@ -271,6 +268,20 @@ pub fn validate(query: &str) -> Result { } } + // Validate the single table (we only support one TABULATE statement). + // `Table` has only `source` today, and `sql_part` already reflects it: + // `extract_sql` synthesizes "SELECT * FROM " for a `TABULATE + // FROM`, so `sql_part` is only empty when there is neither a `FROM` nor + // preceding SQL — the same condition `resolve_table_with_reader` rejects + // at execution time, caught here before any SQL runs. + if !tables.is_empty() && sql_part.trim().is_empty() { + errors.push(ValidationError { + message: "TABULATE has no data source: add a FROM, or a SQL query before it" + .to_string(), + location: None, + }); + } + Ok(Validated { sql: sql_part, visual: viz_part, @@ -308,16 +319,10 @@ mod tests { } #[test] - fn test_validate_tabulate_only_is_known_gap() { - // Documents the known gap in src/validate.rs: a TABULATE-only query - // is recognized as having a Spec (has_spec() is true), and sql() - // correctly reflects the FROM source (extract_sql injects - // "SELECT * FROM " for TABULATE FROM the same way it does - // for VISUALISE FROM) — but nothing about the Table itself is - // actually validated, so it's reported valid with no errors - // regardless of what the Table contains. Update this test (and the - // "Known gap" comment above the `plots: Vec` filter) once table - // validation exists. + fn test_validate_tabulate_from_is_valid() { + // A TABULATE FROM has a data source (extract_sql injects + // "SELECT * FROM " the same way it does for VISUALISE FROM), + // so it's recognized as having a Spec and reported valid. let validated = validate("TABULATE FROM sales").unwrap(); assert!(validated.has_spec()); assert_eq!(validated.sql(), "SELECT * FROM sales"); @@ -326,6 +331,18 @@ mod tests { assert!(validated.errors().is_empty()); } + #[test] + fn test_validate_bare_tabulate_has_no_data_source() { + // A bare TABULATE, with neither a FROM nor a preceding SQL query, has + // no data source — caught here before any SQL runs, mirroring + // `resolve_table_with_reader`'s execution-time rejection of the same + // query. + let validated = validate("TABULATE").unwrap(); + assert!(validated.has_spec()); + assert!(!validated.valid()); + assert!(validated.errors()[0].message.contains("no data source")); + } + #[test] fn test_validate_valid_query() { let validated = From 4dc875ea138d182224dbcf27c368d9a0fca3e34e Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 11 Sep 2026 17:20:58 +0200 Subject: [PATCH 17/17] Fix bugs and gaps found in a pre-emptive table_plumbing review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ran the code-review skill against main...HEAD before sending the branch out for real review; this addresses the fixable findings. extract_sql/extract_spec scoped their FROM-injection query to the whole document root instead of the statement Reader::execute() is actually resolving, so `TABULATE VISUALISE FROM sales DRAW point` silently borrowed the VISUALISE's FROM for the source-less TABULATE instead of erroring. Added first_stmt (returns the first visualise_statement/tabulate_statement node) and scoped the FROM query to it. A query mixing VISUALISE and TABULATE silently resolved only the first spec and dropped the rest with no diagnostic. validate() now warns whenever a query has more than one such statement, threaded through into ResolvedPlot/ResolvedTable's existing warnings() and surfaced via ggsql-cli's stderr and ggsql-jupyter's tracing log. The "use --writer html" hint for a TABULATE query only fired for svg/pdf/hep. All eight Plot-only writers (including the default, vegalite, and the four raster writers) now route through require_plot, so the hint is consistent regardless of --writer. build_ast collected visualise_statement/tabulate_statement nodes via two find_nodes calls plus a sort_by_key; a single alternation query now yields them already in document order in one tree traversal. tree-sitter-ggsql/CLAUDE.md's grammar description omitted TABULATE from the top-level query rule; it now names tabulate_statement explicitly and flags it as a work in progress. Two other findings from the review — HtmlWriter's duplication of ggsql-jupyter's dataframe_to_html, and the missing CHANGELOG entry — were reviewed and left as-is: both are intentional given the Table feature is still incomplete. Co-Authored-By: Claude Sonnet 5 --- ggsql-cli/src/main.rs | 11 ++++++ ggsql-cli/src/writers.rs | 71 +++++++++++++++++++++++++++-------- ggsql-jupyter/src/executor.rs | 6 +++ src/execute/table.rs | 12 ++++++ src/parser/builder.rs | 18 ++++++--- src/parser/source_tree.rs | 62 ++++++++++++++++++++++-------- src/validate.rs | 29 +++++++++++++- tree-sitter-ggsql/CLAUDE.md | 2 +- 8 files changed, 172 insertions(+), 39 deletions(-) diff --git a/ggsql-cli/src/main.rs b/ggsql-cli/src/main.rs index 88f854985..e27fdb5a8 100644 --- a/ggsql-cli/src/main.rs +++ b/ggsql-cli/src/main.rs @@ -436,6 +436,17 @@ fn render_spec(spec: ResolvedSpec, args: &RenderArgs, writer: &WriterSpec) { } } + // Not behind -v: a query that silently dropped a statement (e.g. a mix of + // VISUALISE and TABULATE, only the first of which is resolved) is a + // correctness-relevant fact, not a verbose-only detail. + let query_warnings = match &spec { + ResolvedSpec::Plot(plot) => plot.warnings(), + ResolvedSpec::Table(table) => table.warnings(), + }; + for warning in query_warnings { + eprintln!("warning: {}", warning.message); + } + let info = writer.info; let (render, warnings) = (info.render)(&spec, &writer.options).unwrap_or_else(|e| { eprintln!("Failed to generate {} output: {}", info.label, e); diff --git a/ggsql-cli/src/writers.rs b/ggsql-cli/src/writers.rs index 1e1c77cc8..552c610ec 100644 --- a/ggsql-cli/src/writers.rs +++ b/ggsql-cli/src/writers.rs @@ -113,6 +113,24 @@ fn check_options(options: &WriterOptions) -> Result<(), String> { .map_err(|e| e.to_string()) } +/// The `ResolvedPlot` a Plot-only writer needs, or a clear error for a +/// TABULATE query — every writer except `html` renders only plots. +#[cfg(any( + feature = "vegalite", + feature = "png", + feature = "jpeg", + feature = "tiff", + feature = "webp", + feature = "svg", + feature = "pdf", + feature = "hep" +))] +fn require_plot(spec: &ResolvedSpec) -> Result<&ggsql::reader::ResolvedPlot, String> { + spec.as_plot().ok_or_else(|| { + "this writer does not support TABULATE queries; use --writer html".to_string() + }) +} + pub const WRITERS: &[WriterInfo] = &[ WriterInfo { name: "vegalite", @@ -346,7 +364,10 @@ fn render_vegalite(spec: &ResolvedSpec, options: &WriterOptions) -> Rendered { #[cfg(feature = "vegalite")] { let writer = VegaLiteWriter::from_options(options).map_err(|e| e.to_string())?; - let json = writer.render(spec).map_err(|e| e.to_string())?; + let plot = require_plot(spec)?; + let json = writer + .write_plot(plot.plot(), plot.data()) + .map_err(|e| e.to_string())?; Ok((Output::Text(json), Vec::new())) } #[cfg(not(feature = "vegalite"))] @@ -394,7 +415,10 @@ fn render_png(spec: &ResolvedSpec, options: &WriterOptions) -> Rendered { #[cfg(feature = "png")] { let writer = PngWriter::from_options(options).map_err(|e| e.to_string())?; - let png = writer.render(spec).map_err(|e| e.to_string())?; + let plot = require_plot(spec)?; + let png = writer + .write_plot(plot.plot(), plot.data()) + .map_err(|e| e.to_string())?; Ok((Output::Bin(png), Vec::new())) } #[cfg(not(feature = "png"))] @@ -418,7 +442,10 @@ fn render_jpeg(spec: &ResolvedSpec, options: &WriterOptions) -> Rendered { #[cfg(feature = "jpeg")] { let writer = JpegWriter::from_options(options).map_err(|e| e.to_string())?; - let jpeg = writer.render(spec).map_err(|e| e.to_string())?; + let plot = require_plot(spec)?; + let jpeg = writer + .write_plot(plot.plot(), plot.data()) + .map_err(|e| e.to_string())?; Ok((Output::Bin(jpeg), Vec::new())) } #[cfg(not(feature = "jpeg"))] @@ -442,7 +469,10 @@ fn render_tiff(spec: &ResolvedSpec, options: &WriterOptions) -> Rendered { #[cfg(feature = "tiff")] { let writer = TiffWriter::from_options(options).map_err(|e| e.to_string())?; - let tiff = writer.render(spec).map_err(|e| e.to_string())?; + let plot = require_plot(spec)?; + let tiff = writer + .write_plot(plot.plot(), plot.data()) + .map_err(|e| e.to_string())?; Ok((Output::Bin(tiff), Vec::new())) } #[cfg(not(feature = "tiff"))] @@ -466,7 +496,10 @@ fn render_webp(spec: &ResolvedSpec, options: &WriterOptions) -> Rendered { #[cfg(feature = "webp")] { let writer = WebpWriter::from_options(options).map_err(|e| e.to_string())?; - let webp = writer.render(spec).map_err(|e| e.to_string())?; + let plot = require_plot(spec)?; + let webp = writer + .write_plot(plot.plot(), plot.data()) + .map_err(|e| e.to_string())?; Ok((Output::Bin(webp), Vec::new())) } #[cfg(not(feature = "webp"))] @@ -476,16 +509,6 @@ fn render_webp(spec: &ResolvedSpec, options: &WriterOptions) -> Rendered { } } -/// The `ResolvedPlot` a Plot-only writer needs, or a clear error for a -/// TABULATE query — `svg`, `pdf` and `hep` render only plots; `html` is the -/// only writer that renders a table. -#[cfg(any(feature = "svg", feature = "pdf", feature = "hep"))] -fn require_plot(spec: &ResolvedSpec) -> Result<&ggsql::reader::ResolvedPlot, String> { - spec.as_plot().ok_or_else(|| { - "this writer does not support TABULATE queries; use --writer html".to_string() - }) -} - fn check_svg(options: &WriterOptions) -> Result<(), String> { #[cfg(feature = "svg")] return check_options::(options); @@ -762,4 +785,22 @@ mod tests { let info = find("html").unwrap(); assert!((info.render)(&spec, &WriterOptions::new()).is_err()); } + + #[cfg(all(feature = "vegalite", feature = "duckdb"))] + #[test] + fn the_default_writer_hints_at_html_for_a_tabulate_query() { + use ggsql::reader::{DuckDBReader, Reader}; + + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + reader + .execute_sql("CREATE TABLE sales AS SELECT * FROM (VALUES (1)) AS t(id)") + .unwrap(); + let spec = reader.execute("TABULATE FROM sales").unwrap(); + + let info = find(DEFAULT_WRITER).unwrap(); + let Err(err) = (info.render)(&spec, &WriterOptions::new()) else { + panic!("expected the default writer to reject a TABULATE query"); + }; + assert!(err.contains("--writer html"), "{err}"); + } } diff --git a/ggsql-jupyter/src/executor.rs b/ggsql-jupyter/src/executor.rs index b000626df..6e79b2ba6 100644 --- a/ggsql-jupyter/src/executor.rs +++ b/ggsql-jupyter/src/executor.rs @@ -297,6 +297,9 @@ impl QueryExecutor { table.body().height(), table.body().width() ); + for warning in table.warnings() { + tracing::warn!("{}", warning.message); + } let html = HtmlWriter::new().write_table(table.table(), table.body())?; tracing::debug!("Generated HTML table: {} chars", html.len()); @@ -309,6 +312,9 @@ impl QueryExecutor { plot.metadata().rows, plot.metadata().layer_count ); + for warning in plot.warnings() { + tracing::warn!("{}", warning.message); + } Ok(ExecutionResult::Visualization(plot)) } diff --git a/src/execute/table.rs b/src/execute/table.rs index 7dbe58cc4..8c6ebb3fa 100644 --- a/src/execute/table.rs +++ b/src/execute/table.rs @@ -100,4 +100,16 @@ mod tests { let result = resolve_table_with_reader("TABULATE", &reader); assert!(result.is_err()); } + + #[test] + fn test_tabulate_does_not_borrow_a_later_visualise_from() { + // A source-less TABULATE followed by an unrelated VISUALISE FROM must + // still error "no data source", not silently resolve against the + // VISUALISE's FROM — regression for a bug where extract_sql matched + // any statement's FROM in the whole query, not just the one being + // resolved. + let reader = reader_with_sales(); + let result = resolve_table_with_reader("TABULATE VISUALISE FROM sales DRAW point", &reader); + assert!(result.is_err()); + } } diff --git a/src/parser/builder.rs b/src/parser/builder.rs index 841c14565..ea1aa53dd 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -216,12 +216,18 @@ pub fn build_ast(source: &SourceTree) -> Result> { false }; - // Find all visualise_statement and tabulate_statement nodes, in source - // order (they can be interleaved, e.g. `VISUALISE ... TABULATE ...`). - let viz_nodes = source.find_nodes(&root, "(visualise_statement) @viz"); - let tab_nodes = source.find_nodes(&root, "(tabulate_statement) @tab"); - let mut stmt_nodes: Vec = viz_nodes.into_iter().chain(tab_nodes).collect(); - stmt_nodes.sort_by_key(|n| n.start_byte()); + // A single alternation query visits the tree once and yields + // visualise_statement/tabulate_statement nodes already in document order, + // so they arrive correctly interleaved (e.g. `VISUALISE ... TABULATE ...`). + let stmt_nodes = source.find_nodes( + &root, + r#" + [ + (visualise_statement) @stmt + (tabulate_statement) @stmt + ] + "#, + ); let mut specs = Vec::new(); for stmt_node in stmt_nodes { diff --git a/src/parser/source_tree.rs b/src/parser/source_tree.rs index 0e1bdf255..99f7edd6b 100644 --- a/src/parser/source_tree.rs +++ b/src/parser/source_tree.rs @@ -134,17 +134,22 @@ impl<'a> SourceTree<'a> { .collect() } - /// The start byte of the first VISUALISE or TABULATE statement, whichever - /// comes first. `None` if the query has neither. - fn first_stmt_start(&self, root: &Node) -> Option { - let viz = self - .find_node(root, "(visualise_statement) @viz") - .map(|n| n.start_byte()); - let tab = self - .find_node(root, "(tabulate_statement) @tab") - .map(|n| n.start_byte()); + /// The first VISUALISE or TABULATE statement node, whichever comes first. + /// `None` if the query has neither. + /// + /// This is the statement `Reader::execute()`'s dispatch actually resolves + /// — it always acts on the first top-level `Spec`, of whichever kind — so + /// it is also the only statement `extract_sql`'s FROM-injection should + /// ever look inside. + fn first_stmt<'b>(&self, root: &Node<'b>) -> Option> { + let viz = self.find_node(root, "(visualise_statement) @viz"); + let tab = self.find_node(root, "(tabulate_statement) @tab"); match (viz, tab) { - (Some(a), Some(b)) => Some(a.min(b)), + (Some(a), Some(b)) => Some(if a.start_byte() <= b.start_byte() { + a + } else { + b + }), (Some(a), None) | (None, Some(a)) => Some(a), (None, None) => None, } @@ -173,11 +178,13 @@ impl<'a> SourceTree<'a> { pub fn extract_sql(&self) -> Option { let root = self.root(); - // Check if there's any VISUALISE or TABULATE statement - if self.first_stmt_start(&root).is_none() { - // Neither at all - return entire source as SQL + // The statement `Reader::execute()` will actually resolve — the only + // one a FROM belonging to some *other*, later statement should ever + // be allowed to feed into. + let Some(first_stmt) = self.first_stmt(&root) else { + // Neither VISUALISE nor TABULATE at all - return entire source as SQL return Some(self.source.to_string()); - } + }; // Find sql_portion node and extract its text let sql_portion_node = self.find_node(&root, "(sql_portion) @sql"); @@ -214,8 +221,16 @@ impl<'a> SourceTree<'a> { // silently start matching some unrelated future use of // single_source_from — today it's only ever a child of one of these // two, but this doesn't rely on that staying true. + // + // Scoped to `first_stmt` specifically, not `root`: a query can have + // several VISUALISE/TABULATE statements (interleaved, even), and only + // the first one is ever actually resolved. Searching the whole tree + // here would let a *later* statement's FROM leak into the one being + // resolved — e.g. `TABULATE VISUALISE FROM sales DRAW point` would + // silently pick up the VISUALISE's `FROM sales` for the source-less + // TABULATE instead of correctly reporting no data source. let stmt_from = self.find_text( - &root, + &first_stmt, r#" [ (visualise_statement (single_source_from source: (_) @source)) @@ -246,7 +261,7 @@ impl<'a> SourceTree<'a> { pub fn extract_spec(&self) -> Option { let root = self.root(); - let spec_start = self.first_stmt_start(&root)?; + let spec_start = self.first_stmt(&root)?.start_byte(); // Extract spec text from first VISUALISE/TABULATE onwards let spec_text = &self.source[spec_start..]; @@ -339,6 +354,21 @@ mod tests { ); } + #[test] + fn test_extract_sql_does_not_borrow_a_later_statements_from() { + // A source-less TABULATE followed by an unrelated VISUALISE FROM must + // not pick up the VISUALISE's FROM — extract_sql is scoped to the + // first statement (the one Reader::execute() actually resolves), not + // the whole tree. + let tree = SourceTree::new("TABULATE VISUALISE FROM sales DRAW point").unwrap(); + assert_eq!(tree.extract_sql(), None); + + // Same in the other order: a source-less VISUALISE must not borrow a + // later TABULATE's FROM either. + let tree = SourceTree::new("VISUALISE DRAW point TABULATE FROM sales").unwrap(); + assert_eq!(tree.extract_sql(), None); + } + #[test] fn test_extract_sql_visualise_from_jinja_ref() { let query = "VISUALISE FROM {{ ref('fct_orders') }} DRAW point MAPPING x AS x, y AS y"; diff --git a/src/validate.rs b/src/validate.rs index 3f8b43fd4..b15fe7a52 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -97,7 +97,7 @@ fn has_error_ancestor(node: &tree_sitter::Node) -> bool { /// Validate query syntax and semantics without executing SQL. pub fn validate(query: &str) -> Result { let mut errors = Vec::new(); - let warnings = Vec::new(); + let mut warnings = Vec::new(); // Parse once and create SourceTree let source_tree = match parser::SourceTree::new(query) { @@ -209,6 +209,20 @@ pub fn validate(query: &str) -> Result { let plots: Vec<&Plot> = specs.iter().filter_map(Spec::as_plot).collect(); let tables: Vec<&Table> = specs.iter().filter_map(Spec::as_table).collect(); + // Reader::execute() resolves only the first VISUALISE/TABULATE statement + // and silently drops the rest — a query with more than one gets no + // diagnostic otherwise. This warning is the only signal a caller has that + // part of their query was ignored. + if specs.len() > 1 { + warnings.push(ValidationWarning { + message: format!( + "Query has {} VISUALISE/TABULATE statements; only the first is resolved, the rest are ignored.", + specs.len() + ), + location: None, + }); + } + // Validate the single plot (we only support one VISUALISE statement) if let Some(plot) = plots.first() { // Validate each layer @@ -318,6 +332,19 @@ mod tests { assert!(validated.valid()); } + #[test] + fn test_validate_warns_on_multiple_spec_statements() { + // Reader::execute() only ever resolves the first VISUALISE/TABULATE + // statement; a query with more than one should warn rather than + // silently drop the rest with no diagnostic at all. + let validated = validate("SELECT 1 AS x VISUALISE x DRAW point TABULATE").unwrap(); + assert!(validated.valid()); + assert!(!validated.warnings().is_empty()); + assert!(validated.warnings()[0] + .message + .contains("only the first is resolved")); + } + #[test] fn test_validate_tabulate_from_is_valid() { // A TABULATE FROM has a data source (extract_sql injects diff --git a/tree-sitter-ggsql/CLAUDE.md b/tree-sitter-ggsql/CLAUDE.md index 4c6ea1b75..c31624350 100644 --- a/tree-sitter-ggsql/CLAUDE.md +++ b/tree-sitter-ggsql/CLAUDE.md @@ -32,7 +32,7 @@ The files under `src/` are generated by `tree-sitter generate` from `grammar.js` ## Grammar at a glance -`grammar.js` defines a `query` rule that is `optional(sql_portion) + repeat(visualise_statement)`. The SQL portion is recognised structurally enough to know where it ends (statement boundaries, recursive subqueries, WITH compound statements) without re-implementing a SQL parser — the actual SQL is handed off to the configured `Reader`. The VISUALISE side is parsed in detail (clauses, layer types, mappings, settings, scales, facets, projections, labels) so [`/src/parser/builder.rs`](../src/parser/builder.rs) can build a typed AST from it. +`grammar.js` defines a `query` rule that is `optional(sql_portion) + repeat(choice(visualise_statement, tabulate_statement))`. The SQL portion is recognised structurally enough to know where it ends (statement boundaries, recursive subqueries, WITH compound statements) without re-implementing a SQL parser — the actual SQL is handed off to the configured `Reader`. The VISUALISE side is parsed in detail (clauses, layer types, mappings, settings, scales, facets, projections, labels) so [`/src/parser/builder.rs`](../src/parser/builder.rs) can build a typed AST from it. `tabulate_statement` (the plain-table counterpart to VISUALISE — a query result with no plot) is a **work in progress**: today it only recognises the keyword plus an optional `single_source_from` (the same single-source `FROM` rule VISUALISE uses), with no clauses of its own yet. For ggsql language semantics, see [`/doc/syntax/`](../doc/syntax/) — this package only defines *how text is parsed*, not what the resulting tree means.
x