Table plumbing - #546
Draft
teunbrand wants to merge 17 commits into
Draft
Conversation
Frees up the `Spec` name for future enum meaning both table and plot output kinds.
Parse query / build AST will now produce Spec::{Plot,Table}. Up- and downstream plumbing still missing.
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.
Table's first real field: source: Option<DataSource>, populated from TABULATE FROM the same way Plot.source is from VISUALISE FROM (same single_source_from grammar node, same parse_data_source parsing). New build_tabulate_statement mirrors build_visualise_statement's own inline single_source_from handling rather than sharing a helper with it — the extraction is only 3 lines at 2 call sites, and the other existing parse_data_source caller (mapping_clause's layer_source) uses a different field name, so a shared wrapper or a field-name parameter would've been a new pattern not otherwise used in this file. build_ast's TABULATE arm now checks table.source.is_some() directly instead of querying the CST, so the "FROM after a trailing SELECT" validation is symmetric with VISUALISE's own check — only the error message text is still duplicated between the two arms (left as a TODO).
extract_sql's "append SELECT * FROM <source>" rewrite was anchored to visualise_statement only, so a TABULATE FROM query lost its source entirely from the extracted SQL text. Both statement kinds now get the same treatment, via a query explicitly anchored to visualise_statement/tabulate_statement (rather than a bare single_source_from match) so this can't silently start matching some unrelated future use of that node. TABULATE FROM x and SELECT * FROM x TABULATE now extract to the identical SQL, mirroring the equivalence VISUALISE FROM already has with a bare SELECT. Updates the validate.rs golden test that was locking in the old (gap) behavior; the remaining gap — no actual validation of a Table's contents — is unchanged and still documented.
ResolvedTable (table, body, sql, warnings) and ResolvedSpec (Plot(Box<ResolvedPlot>) | Table(ResolvedTable)) are the execution-time counterparts to Table and Spec, mirroring the existing ResolvedPlot shape. ResolvedTable's impl is colocated in reader/spec.rs rather than a new file, since it's small enough not to warrant one yet. body's type (a plain DataFrame) is explicitly marked provisional, not a settled design — it's enough to design the execution plumbing against, but will likely need to become a table-specific representation once real table writers exist. Not yet wired up: Reader::execute() still returns Result<ResolvedPlot> and Writer::render() still takes &ResolvedPlot. ResolvedTable::new has no call site yet (temporary dead_code warning, same as ResolvedPlot:: new's own single-call-site pattern) until the execute/table.rs resolver and the Plot/Table dispatch in execute_with_reader exist — that dispatch and the Reader/Writer signature change land together as one atomic step, since every caller does execute() then render() back to back.
execute::table::resolve_table_with_reader(query, reader) -> Result<ResolvedTable> is the Table-side substitute for prepare_data_with_reader + execute_with_reader combined — Table has no per-layer/scale/facet resolution step to justify a separate intermediate struct the way Plot's PreparedData does, so both collapse into one function here. Parses, takes the first Table spec, gets SQL via extract_sql() (now shared with VISUALISE), runs it via reader.execute_sql(), and wraps the result into ResolvedTable. Confirms end to end that "TABULATE FROM x" and "SELECT * FROM x TABULATE" resolve identically, not just that they extract to the same SQL text. Known gap, called out in the doc comment: unlike the Plot pipeline, setup statements (INSTALL, LOAD, SET, etc.) ahead of a TABULATE aren't executed here yet. Not yet wired up: no caller exists for this function until the Reader::execute()/Writer::render() dispatch step lands.
Reader::execute() now returns Result<ResolvedSpec> and dispatches (via renamed execute_with_reader, which peeks at the query's first Spec) to either resolve_plot_with_reader (renamed from the old execute_with_reader) or resolve_table_with_reader. Writer::render() takes &ResolvedSpec and returns a clean WriterError for the Table case — no writer renders tables yet. Both were tried as a single default-method consolidation first; abandoned once it hit a real constraint, not a style choice: a default method can't unsize &Self into &dyn Reader without a Self: Sized bound, which would remove execute from dyn Reader's vtable. execute() stays a required method, with the 8 implementations as one-liners. Updates ~60 call sites accordingly. Tests route through the new as_plot()/as_table()/into_plot()/into_table() accessors (mirroring Spec's own API) to keep exercising ResolvedPlot-specific assertions while still passing the original ResolvedSpec through to render(); each test binds `let plot = spec.as_plot().unwrap();` once rather than repeating the chain per line. Real call sites (ggsql-cli, ggsql-jupyter, the doc-example harness) each get a deliberate, clear error/skip for the Table case instead of a blind unwrap; ggsql-wasm needed no changes. Adds one integration test confirming the dispatch itself (not just its two pieces) routes correctly and that rendering a table fails cleanly rather than panicking.
write_plot/validate_plot are now the real, required methods; write_table is a new default method (returns WriterError) that render() dispatches to for ResolvedSpec::Table, mirroring how write_plot handles the Plot case. Every writer name now says which side it's for, matching the naming clarity already applied on the Reader side (resolve_plot_with_reader/resolve_table_with_reader).
HtmlWriter is the first writer to actually implement write_table: a bare <table>, no styling/headings/spanners/footnotes, since Table has no fields to describe those yet. Proves the Table -> ResolvedTable -> Writer plumbing end to end; not the real grammar-of-tables output. Deliberately does not reuse ggsql-jupyter's existing dataframe_to_html (used for its pure-SQL fallback display) - that function is built around DataFrame specifically, and ResolvedTable.body's type is itself still provisional, so tying a new writer to it now would be reuse for its own sake rather than because the shapes are known to match long-term. Has its own small escape_html for the same reason; noted as a near-term follow-up that this duplicates logic already present in ggsql-jupyter (and, more loosely, ggsql-cli's doc-example harness) and is worth consolidating into a shared utility in core ggsql. No feature flag of its own (pure string formatting, no new dependencies) - reachable wherever the writer module already is.
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")].
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 <table> needs none.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR to the
table-featurebranch sets up the plumbing for rendering tables.In the grammar there is a new
TABULATEstatement, parallel toVISUALISEstatements.At the AST level
Plotgains siblingTableand together they are in the renamedSpecenum.At the executed state, the old
Specstruct is nowResolvedPlotand has sisterResolvedTable, together in theResolvedSpecenum.Writers take a
ResolvedSpecand have separate wirings for the Plot/Table variants, so that eventually also hephaestus writers can render tables once table cells are supported.There is also a stubby
HtmlWriterwhich refuses plots but renders tables as a<table>html tag.Both CLI and Jupyter should be able to use
HtmlWriter.The actual table feature are absent, this PR is just to get the plumbing in place to build out features later.