From 587a9579693b9097e5c1a3f11148b44215d245bd Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 14 Sep 2026 11:28:50 +0200 Subject: [PATCH 1/5] Replace ResolvedTable's DataFrame body with a resolved cell layout TableCell (execute/table.rs) positions each column label and data value on an inclusive row/column grid, tagged ColumnLabel/Body (TableCellKind, naming borrowed from R's gt) so a writer can tell them apart without relying on position. ResolvedTable now holds Vec instead of a DataFrame; nrow()/ncol() are computed from cells rather than stored, so there's one source of truth for the table's shape. Writer::write_table drops its Table parameter entirely: Table's only field (source) is already consumed before cells exist, so there's nothing left for a writer to use it for. HtmlWriter, ggsql-cli and ggsql-jupyter are updated to match. --- ggsql-cli/src/main.rs | 4 +- ggsql-jupyter/src/executor.rs | 6 +- src/doc/API.md | 6 +- src/execute/mod.rs | 2 +- src/execute/table.rs | 111 +++++++++++++++++++++++++++++++--- src/lib.rs | 5 ++ src/reader/mod.rs | 19 +++--- src/reader/spec.rs | 33 +++++++--- src/writer/html.rs | 47 ++++++++------ src/writer/mod.rs | 21 ++++--- 10 files changed, 188 insertions(+), 66 deletions(-) diff --git a/ggsql-cli/src/main.rs b/ggsql-cli/src/main.rs index e27fdb5a..2ca80fca 100644 --- a/ggsql-cli/src/main.rs +++ b/ggsql-cli/src/main.rs @@ -430,8 +430,8 @@ fn render_spec(spec: ResolvedSpec, args: &RenderArgs, writer: &WriterSpec) { ResolvedSpec::Table(table) => { if args.verbose { eprintln!("\nQuery executed:"); - eprintln!(" Rows: {}", table.body().height()); - eprintln!(" Columns: {}", table.body().width()); + eprintln!(" Rows: {}", table.nrow()); + eprintln!(" Columns: {}", table.ncol()); } } } diff --git a/ggsql-jupyter/src/executor.rs b/ggsql-jupyter/src/executor.rs index 6e79b2ba..613680e2 100644 --- a/ggsql-jupyter/src/executor.rs +++ b/ggsql-jupyter/src/executor.rs @@ -294,14 +294,14 @@ impl QueryExecutor { ResolvedSpec::Table(table) => { tracing::info!( "Query executed: {} rows, {} cols", - table.body().height(), - table.body().width() + table.nrow(), + table.ncol() ); for warning in table.warnings() { tracing::warn!("{}", warning.message); } - let html = HtmlWriter::new().write_table(table.table(), table.body())?; + let html = HtmlWriter::new().write_table(table.cells())?; tracing::debug!("Generated HTML table: {} chars", html.len()); Ok(ExecutionResult::Table { html }) diff --git a/src/doc/API.md b/src/doc/API.md index c9449059..83397de3 100644 --- a/src/doc/API.md +++ b/src/doc/API.md @@ -408,9 +408,9 @@ pub trait Writer { /// Check whether a plot can be rendered by this writer, without rendering it fn validate_plot(&self, spec: &Plot) -> Result<()>; - /// Render a resolved table and its body data. Defaults to an "unsupported" - /// error; only `HtmlWriter` overrides it as of this writing. - fn write_table(&self, table: &Table, body: &DataFrame) -> Result { .. } + /// Render a resolved table's cells. Defaults to an "unsupported" error; + /// only `HtmlWriter` overrides it as of this writing. + fn write_table(&self, cells: &[TableCell]) -> Result { .. } /// Render a `ResolvedSpec` from `reader.execute()` — the usual entry point. /// Dispatches to `write_plot`/`write_table` depending on the variant. diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 9ada5a9a..456251f4 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -23,7 +23,7 @@ mod table; pub use casting::TypeRequirement; pub use cte::CteDefinition; pub use schema::TypeInfo; -pub use table::resolve_table_with_reader; +pub use table::{resolve_table_with_reader, TableCell, TableCellKind}; use crate::naming; use crate::parser; diff --git a/src/execute/table.rs b/src/execute/table.rs index 8c6ebb3f..9a1c1cf8 100644 --- a/src/execute/table.rs +++ b/src/execute/table.rs @@ -2,12 +2,17 @@ //! //! A Table has no layers, so there's no per-layer CTE materialization, scale //! resolution, or facet handling to do here — just the one query that -//! produces `body`. +//! produces `body`, plus (as `Table` grows headings/spanners/footnotes) +//! resolving that data into positioned `TableCell`s. As those concerns grow +//! they're expected to split into sibling files here, the way Plot's own +//! resolution logic is split across `schema.rs`/`casting.rs`/`layer.rs`/ +//! `scale.rs`/`position.rs`/`cte.rs` rather than left in one file. +use crate::array_util::value_to_string; use crate::parser::{self, SourceTree}; use crate::reader::{Reader, ResolvedTable}; use crate::validate::{validate, ValidationWarning}; -use crate::{GgsqlError, Result, Spec}; +use crate::{DataFrame, GgsqlError, Result, Spec}; /// Resolve a TABULATE query into a `ResolvedTable`. /// @@ -51,8 +56,99 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result Vec { + let mut cells = Vec::new(); + + for (index, name) in body.get_column_names().into_iter().enumerate() { + cells.push(TableCell { + kind: TableCellKind::ColumnLabel, + top: 0, + bottom: 0, + left: index, + right: index, + content: name, + }); + } + + let columns = body.get_columns(); + for row in 0..body.height() { + for (index, column) in columns.iter().enumerate() { + cells.push(TableCell { + kind: TableCellKind::Body, + top: row + 1, + bottom: row + 1, + left: index, + right: index, + content: value_to_string(column, row), + }); + } + } + + cells +} + +// ============================================================================= +// Public API: TableCell +// ============================================================================= + +/// What role a `TableCell` plays in the table's layout. +/// +/// Naming follows R's gt package (`column_labels`, `body`, ...), since ggsql's +/// table grammar is expected to keep drawing on its part vocabulary as more +/// of it (spanners, stub, footnotes, source notes) gets built out here. +/// +/// Lets a writer tell cells apart (e.g. `` vs ``) without relying on +/// position — a column label is a `ColumnLabel` cell, not "whatever's in row +/// 0". A caption is expected to become a `TableCell` too once `Table` can +/// resolve one (still just text with a position, spanning the full width) — +/// not added yet, since nothing produces one today. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum TableCellKind { + /// A column label (gt's `column_labels`). + ColumnLabel, + /// A data value (gt's `body`). + Body, +} + +/// A single positioned cell within a resolved table layout. +/// +/// Parallel to `PreparedData` on the Plot side (an intermediate resolution +/// type, not the final `ResolvedTable` envelope) — but there is no Plot-side +/// equivalent to the shape itself, since Plot resolves at `DataFrame` +/// granularity, not per-cell. +/// +/// Position is an inclusive grid rectangle: `top`/`bottom` are row indices, +/// `left`/`right` are column indices, 0-based, inclusive on both ends. A +/// non-spanning cell has `top == bottom` and `left == right`. Colspan/rowspan +/// and adjacency helpers are expected to live elsewhere and account for the +/// inclusive convention themselves, rather than each caller doing `+ 1` +/// arithmetic against these fields directly. Style/formatting fields are +/// deliberately not included yet — add them once a feature needs them. +#[derive(Debug, Clone)] +pub struct TableCell { + /// What role this cell plays (column label, body, ...). + pub kind: TableCellKind, + /// Top row index (inclusive). + pub top: usize, + /// Bottom row index (inclusive). + pub bottom: usize, + /// Left column index (inclusive). + pub left: usize, + /// Right column index (inclusive). + pub right: usize, + /// The cell's text content. + pub content: String, } #[cfg(test)] @@ -75,8 +171,8 @@ mod tests { let resolved = resolve_table_with_reader("TABULATE FROM sales", &reader).unwrap(); assert_eq!(resolved.sql(), "SELECT * FROM sales"); - assert_eq!(resolved.body().height(), 3); - assert_eq!(resolved.body().width(), 2); + assert_eq!(resolved.nrow(), 3); + assert_eq!(resolved.ncol(), 2); } #[test] @@ -88,10 +184,7 @@ mod tests { resolve_table_with_reader("SELECT * FROM sales TABULATE", &reader).unwrap(); assert_eq!(from_only.sql(), select_then_tabulate.sql()); - assert_eq!( - from_only.body().height(), - select_then_tabulate.body().height() - ); + assert_eq!(from_only.nrow(), select_then_tabulate.nrow()); } #[test] diff --git a/src/lib.rs b/src/lib.rs index 99f11a16..0ff8b1b3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -89,6 +89,11 @@ pub use util::{and_list, and_list_quoted, or_list, or_list_quoted}; // DataFrame abstraction (wraps Arrow RecordBatch) pub use dataframe::DataFrame; +// Re-export the resolved table layout Writer::write_table needs — the +// Table-side counterpart to DataFrame, not to the plot:: AST vocabulary +// above, since Table has no specification vocabulary of its own yet. +pub use execute::{TableCell, TableCellKind}; + /// Main library error type #[derive(thiserror::Error, Debug)] pub enum GgsqlError { diff --git a/src/reader/mod.rs b/src/reader/mod.rs index 23891e6f..fe99230c 100644 --- a/src/reader/mod.rs +++ b/src/reader/mod.rs @@ -37,7 +37,7 @@ use crate::execute::{prepare_data_with_reader, resolve_table_with_reader}; use crate::parser::{self, SourceTree}; use crate::plot::{CastTargetType, Plot}; use crate::validate::{validate, ValidationWarning}; -use crate::{naming, DataFrame, GgsqlError, Result, Spec, Table}; +use crate::{naming, DataFrame, GgsqlError, Result, Spec, Table, TableCell}; // ============================================================================= // SQL Dialect @@ -710,16 +710,13 @@ pub struct Metadata { pub struct ResolvedTable { /// The resolved table specification pub(crate) table: Table, - // PROVISIONAL, NOT A FINAL DESIGN DECISION: a plain `DataFrame` is enough - // to design the execution plumbing against, but this was never settled - // as the real representation. It will very likely need to become a - // table-specific intermediate representation once real table writers - // exist (e.g. an HTML/gt-style writer) and we know what they actually - // need `body` to carry. Don't build on this shape assuming it's final. - /// The data resolved from `table.source` (or the main SQL if there was no - /// TABULATE FROM) - pub(crate) body: DataFrame, - /// The SQL query that was executed to produce `body` + /// The resolved layout: one cell per column label and per data value, + /// resolved from `table.source` (or the main SQL if there was no + /// TABULATE FROM). See `TableCell` for the position/kind conventions. + /// `nrow()`/`ncol()` are computed from this rather than stored + /// separately, so there's one source of truth for the table's shape. + pub(crate) cells: Vec, + /// The SQL query that was executed to produce `cells` pub(crate) sql: String, /// Validation warnings from preparation pub(crate) warnings: Vec, diff --git a/src/reader/spec.rs b/src/reader/spec.rs index 6cd9dec1..f35ed489 100644 --- a/src/reader/spec.rs +++ b/src/reader/spec.rs @@ -5,7 +5,7 @@ use std::collections::HashMap; use crate::naming; use crate::plot::Plot; use crate::validate::ValidationWarning; -use crate::{DataFrame, Table}; +use crate::{DataFrame, Table, TableCell}; use super::{Metadata, ResolvedPlot, ResolvedSpec, ResolvedTable}; @@ -115,13 +115,13 @@ impl ResolvedTable { /// Create a new ResolvedTable. pub(crate) fn new( table: Table, - body: DataFrame, + cells: Vec, sql: String, warnings: Vec, ) -> Self { Self { table, - body, + cells, sql, warnings, } @@ -132,14 +132,29 @@ impl ResolvedTable { &self.table } - /// Get the resolved body data. See the PROVISIONAL note on the `body` - /// field in `reader::mod` — this accessor's return type will likely - /// change once real table writers exist. - pub fn body(&self) -> &DataFrame { - &self.body + /// Get the resolved layout: one cell per column label and per data value. + pub fn cells(&self) -> &[TableCell] { + &self.cells } - /// The SQL query that was executed to produce `body`. + /// Number of data rows (not counting the column-label row), computed + /// from `cells`. The column-label row is always `bottom == 0`, so it + /// only determines this max when there are no data rows, where it + /// correctly gives `0`. + pub fn nrow(&self) -> usize { + self.cells.iter().map(|cell| cell.bottom).max().unwrap_or(0) + } + + /// Number of columns, computed from `cells`. + pub fn ncol(&self) -> usize { + self.cells + .iter() + .map(|cell| cell.right) + .max() + .map_or(0, |right| right + 1) + } + + /// The SQL query that was executed to produce `cells`. pub fn sql(&self) -> &str { &self.sql } diff --git a/src/writer/html.rs b/src/writer/html.rs index 9d362e60..8faa3cab 100644 --- a/src/writer/html.rs +++ b/src/writer/html.rs @@ -1,19 +1,18 @@ //! A minimal HTML table writer. //! -//! Renders a `ResolvedTable`'s body as a bare `` — no styling, no -//! headings/spanners/footnotes, since `Table` has no fields to describe -//! those yet. This is a stub to prove the Table → writer plumbing end to -//! end, not the real grammar-of-tables output; it deliberately does not -//! reuse `ggsql-jupyter`'s existing `dataframe_to_html`, since that's built -//! around `DataFrame` specifically, and `ResolvedTable.body`'s type is -//! itself still provisional (see the note on that field). - +//! Renders a `ResolvedTable`'s cells as a bare `
` — no styling, no +//! spanners/footnotes, since `Table` has no fields to describe those yet. +//! This is a stub to prove the Table → writer plumbing end to end, not the +//! real grammar-of-tables output; it deliberately does not reuse +//! `ggsql-jupyter`'s existing `dataframe_to_html`, which works directly off +//! a `DataFrame` rather than resolved `TableCell`s. + +use std::collections::BTreeMap; 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}; +use crate::{DataFrame, GgsqlError, Plot, Result, TableCell, TableCellKind}; /// Renders a resolved table as a bare HTML `
`. Does not support plots. #[derive(Debug, Default)] @@ -47,21 +46,29 @@ impl Writer for HtmlWriter { )) } - fn write_table(&self, _table: &Table, body: &DataFrame) -> Result { + fn write_table(&self, cells: &[TableCell]) -> Result { + let mut column_labels: Vec<&TableCell> = cells + .iter() + .filter(|cell| cell.kind == TableCellKind::ColumnLabel) + .collect(); + column_labels.sort_by_key(|cell| cell.left); + + let mut body_rows: BTreeMap> = BTreeMap::new(); + for cell in cells.iter().filter(|cell| cell.kind == TableCellKind::Body) { + body_rows.entry(cell.top).or_default().push(cell); + } + let mut html = String::from("
\n\n"); - for name in body.get_column_names() { - html.push_str(&format!("", escape_html(&name))); + for cell in column_labels { + html.push_str(&format!("", escape_html(&cell.content))); } html.push_str("\n\n\n"); - let columns = body.get_columns(); - for row in 0..body.height() { + for (_, mut row) in body_rows { + row.sort_by_key(|cell| cell.left); html.push_str(""); - for column in columns { - html.push_str(&format!( - "", - escape_html(&value_to_string(column, row)) - )); + for cell in row { + html.push_str(&format!("", escape_html(&cell.content))); } html.push_str("\n"); } diff --git a/src/writer/mod.rs b/src/writer/mod.rs index ac6091a6..c19b040a 100644 --- a/src/writer/mod.rs +++ b/src/writer/mod.rs @@ -29,7 +29,7 @@ //! without knowing which writer they picked. use crate::reader::ResolvedSpec; -use crate::{DataFrame, GgsqlError, Plot, Result, Table}; +use crate::{DataFrame, GgsqlError, Plot, Result, TableCell}; use std::collections::HashMap; pub mod options; @@ -164,25 +164,30 @@ pub trait Writer { /// Ok(()) if the spec is compatible, otherwise an error fn validate_plot(&self, spec: &Plot) -> Result<()>; - /// Generate output from a resolved table specification and its body data + /// Generate output from a resolved table's cells /// /// The table-side counterpart to `write_plot()`. Defaults to rejecting /// every table, so a writer that only supports Plot output (every writer, /// as of this writing) needs no changes; a writer that does support /// tables overrides this instead. /// + /// Unlike `write_plot`, there is no AST parameter: `Table` (the parsed + /// `TABULATE` spec) has nothing left that a writer needs by the time + /// `cells` exists — its only field (`source`) is already consumed + /// building `cells`. If `Table` grows something a writer genuinely needs + /// that isn't itself expressible as a cell, add it back then. + /// /// # Arguments /// - /// * `table` - The parsed TABULATE specification - /// * `body` - The resolved data (see the PROVISIONAL note on - /// `ResolvedTable.body` — this parameter's type may change) + /// * `cells` - The resolved table layout — see `TableCell` for the + /// position/kind conventions /// /// # Errors /// /// Returns `GgsqlError::WriterError` if this writer doesn't support /// tables, or output generation fails. - fn write_table(&self, table: &Table, body: &DataFrame) -> Result { - let _ = (table, body); + fn write_table(&self, cells: &[TableCell]) -> Result { + let _ = cells; Err(GgsqlError::WriterError( "this writer does not support tables".to_string(), )) @@ -219,7 +224,7 @@ pub trait Writer { fn render(&self, spec: &ResolvedSpec) -> Result { match spec { ResolvedSpec::Plot(plot) => self.write_plot(plot.plot(), plot.data()), - ResolvedSpec::Table(table) => self.write_table(table.table(), table.body()), + ResolvedSpec::Table(table) => self.write_table(table.cells()), } } } From 270a02608063e1ae0c44ce8cfcb7f6df6b19a6f9 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 14 Sep 2026 13:17:49 +0200 Subject: [PATCH 2/5] Split table-cell construction into pure column-label/body/composer steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit create_column_labels and create_body each build their half of the layout independently, both numbering rows from 0 — neither knows where it sits relative to the other. compose_cells is a pure function of the two (no DataFrame/SQL knowledge), computing the row offset from the column labels' actual extent and shifting the body via TableCell::offset_rows/offset_cols rather than each caller touching top/bottom or left/right separately. This is what lets more intermediate composers (header, footer, ...) join compose_cells later as Table grows, and keeps cells_from_dataframe useful as a place to test offset arithmetic in isolation from any real query — added layout_tests covering create_column_labels, create_body, and compose_cells (including a label section spanning more than one row, to pin down that the offset comes from the labels' real extent rather than an assumed single row) using the df! macro, no duckdb feature or Reader needed. --- src/execute/table.rs | 197 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 174 insertions(+), 23 deletions(-) diff --git a/src/execute/table.rs b/src/execute/table.rs index 9a1c1cf8..13feef07 100644 --- a/src/execute/table.rs +++ b/src/execute/table.rs @@ -55,39 +55,47 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result Vec { - let mut cells = Vec::new(); - - for (index, name) in body.get_column_names().into_iter().enumerate() { - cells.push(TableCell { +/// Build one `ColumnLabel` cell per column, numbered from `top == 0`. +/// +/// Row numbering here is local to this function alone — `compose_cells` +/// is what decides where this sits relative to the body, not this function. +fn create_column_labels(df: &DataFrame) -> Vec { + df.get_column_names() + .into_iter() + .enumerate() + .map(|(index, name)| TableCell { kind: TableCellKind::ColumnLabel, top: 0, bottom: 0, left: index, right: index, content: name, - }); - } + }) + .collect() +} - let columns = body.get_columns(); - for row in 0..body.height() { +/// Build one `Body` cell per `DataFrame` value, numbered from `top == 0`. +/// +/// Row numbering here is local to this function alone, the same as +/// `create_column_labels` — see that function's doc comment. +fn create_body(df: &DataFrame) -> Vec { + let mut cells = Vec::new(); + let columns = df.get_columns(); + + for row in 0..df.height() { for (index, column) in columns.iter().enumerate() { cells.push(TableCell { kind: TableCellKind::Body, - top: row + 1, - bottom: row + 1, + top: row, + bottom: row, left: index, right: index, content: value_to_string(column, row), @@ -98,6 +106,29 @@ fn cells_from_dataframe(body: &DataFrame) -> Vec { cells } +/// Compose a column-label row and a body into one layout: a pure function of +/// its two arguments, with no `DataFrame`/SQL knowledge of its own. Shifts +/// `body` down by however many rows `column_labels` occupies — today always +/// one, but this is what lets the two stay ignorant of each other's size. As +/// `Table` grows (headings/spanners/footnotes), more of these intermediate +/// composers are expected alongside this one (e.g. for a header or footer), +/// each combining a subset of parts the same way. +fn compose_cells(column_labels: Vec, mut body: Vec) -> Vec { + let row_offset = column_labels + .iter() + .map(|cell| cell.bottom) + .max() + .map_or(0, |bottom| bottom + 1); + + for cell in &mut body { + cell.offset_rows(row_offset); + } + + let mut cells = column_labels; + cells.extend(body); + cells +} + // ============================================================================= // Public API: TableCell // ============================================================================= @@ -131,10 +162,11 @@ pub enum TableCellKind { /// Position is an inclusive grid rectangle: `top`/`bottom` are row indices, /// `left`/`right` are column indices, 0-based, inclusive on both ends. A /// non-spanning cell has `top == bottom` and `left == right`. Colspan/rowspan -/// and adjacency helpers are expected to live elsewhere and account for the -/// inclusive convention themselves, rather than each caller doing `+ 1` -/// arithmetic against these fields directly. Style/formatting fields are -/// deliberately not included yet — add them once a feature needs them. +/// and adjacency helpers beyond `offset_rows`/`offset_cols` are expected to +/// live elsewhere and account for the inclusive convention themselves, +/// rather than each caller doing `+ 1` arithmetic against these fields +/// directly. Style/formatting fields are deliberately not included yet — add +/// them once a feature needs them. #[derive(Debug, Clone)] pub struct TableCell { /// What role this cell plays (column label, body, ...). @@ -151,6 +183,125 @@ pub struct TableCell { pub content: String, } +impl TableCell { + /// Shift this cell down by `rows`, moving `top` and `bottom` together so + /// a spanning cell keeps its height. + pub fn offset_rows(&mut self, rows: usize) { + self.top += rows; + self.bottom += rows; + } + + /// Shift this cell right by `cols`, moving `left` and `right` together + /// so a spanning cell keeps its width. + pub fn offset_cols(&mut self, cols: usize) { + self.left += cols; + self.right += cols; + } +} + +#[cfg(test)] +mod layout_tests { + use super::*; + use crate::df; + + #[test] + fn create_column_labels_builds_one_cell_per_column_at_row_zero() { + let frame = df! { + "id" => vec![1i32, 2], + "name" => vec!["a".to_string(), "b".to_string()], + } + .unwrap(); + + let labels = create_column_labels(&frame); + + assert_eq!(labels.len(), 2); + assert_eq!(labels[0].kind, TableCellKind::ColumnLabel); + assert_eq!(labels[0].top, 0); + assert_eq!(labels[0].bottom, 0); + assert_eq!(labels[0].left, 0); + assert_eq!(labels[0].right, 0); + assert_eq!(labels[0].content, "id"); + assert_eq!(labels[1].left, 1); + assert_eq!(labels[1].right, 1); + assert_eq!(labels[1].content, "name"); + } + + #[test] + fn create_body_numbers_rows_from_zero() { + let frame = df! { + "id" => vec![1i32, 2], + "name" => vec!["a".to_string(), "b".to_string()], + } + .unwrap(); + + let body = create_body(&frame); + + assert_eq!(body.len(), 4); + assert!(body.iter().all(|cell| cell.kind == TableCellKind::Body)); + // Row 0 + assert_eq!(body[0].top, 0); + assert_eq!(body[0].bottom, 0); + assert_eq!(body[0].left, 0); + assert_eq!(body[0].content, "1"); + assert_eq!(body[1].top, 0); + assert_eq!(body[1].left, 1); + assert_eq!(body[1].content, "a"); + // Row 1 + assert_eq!(body[2].top, 1); + assert_eq!(body[2].bottom, 1); + assert_eq!(body[2].left, 0); + assert_eq!(body[2].content, "2"); + assert_eq!(body[3].top, 1); + assert_eq!(body[3].left, 1); + assert_eq!(body[3].content, "b"); + } + + fn cell(kind: TableCellKind, top: usize, bottom: usize, content: &str) -> TableCell { + TableCell { + kind, + top, + bottom, + left: 0, + right: 0, + content: content.to_string(), + } + } + + #[test] + fn compose_cells_shifts_the_body_below_a_single_label_row() { + let column_labels = vec![cell(TableCellKind::ColumnLabel, 0, 0, "id")]; + let body = vec![ + cell(TableCellKind::Body, 0, 0, "1"), + cell(TableCellKind::Body, 1, 1, "2"), + ]; + + let cells = compose_cells(column_labels, body); + + assert_eq!(cells.len(), 3); + assert_eq!(cells[0].kind, TableCellKind::ColumnLabel); + assert_eq!(cells[0].top, 0); + assert_eq!(cells[1].kind, TableCellKind::Body); + assert_eq!(cells[1].top, 1); + assert_eq!(cells[1].bottom, 1); + assert_eq!(cells[2].top, 2); + assert_eq!(cells[2].bottom, 2); + } + + #[test] + fn compose_cells_offsets_by_the_label_rows_actual_extent_not_a_hardcoded_one() { + // Nothing produces a multi-row column-label section today, but + // `compose_cells` computes the offset from `column_labels` itself + // rather than assuming exactly one row — pin that down directly. + let column_labels = vec![cell(TableCellKind::ColumnLabel, 0, 1, "id")]; + let body = vec![cell(TableCellKind::Body, 0, 0, "1")]; + + let cells = compose_cells(column_labels, body); + + assert_eq!(cells[1].top, 2); + assert_eq!(cells[1].bottom, 2); + } +} + #[cfg(test)] #[cfg(feature = "duckdb")] mod tests { From 30ea884e24a84d13086c581f77ea57a507002316 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 14 Sep 2026 16:29:33 +0200 Subject: [PATCH 3/5] Add TABULATE LABEL clause for custom column display labels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tabulate_statement now accepts an optional label_clause (reused unchanged from VISUALISE), so TABULATE FROM sales LABEL id => 'ID' parses. Table.labels: Labels (not Option — an empty Labels already means "no overrides", so there's no third state an Option would distinguish, and every consumer's logic is the same either way). build_tabulate_statement now mirrors build_visualise_statement's structure: a process_tab_clause function parallels process_viz_clause, ready to grow an arm per future TABULATE clause (FACET/SCALE). execute::table::create_column_labels resolves each column's content against Table.labels with three outcomes: a column absent from labels keeps its name, an explicit LABEL col => NULL empties the cell, and LABEL col => 'text' overrides it. Verified end-to-end via a real ggsql exec --writer html invocation, plus a unit test covering all three outcomes. Co-Authored-By: Claude Sonnet 5 --- src/execute/table.rs | 57 ++++++++++++++++++++----- src/parser/builder.rs | 33 +++++++++++--- src/plot/main.rs | 2 +- src/table/mod.rs | 15 +++++-- tree-sitter-ggsql/grammar.js | 6 +-- tree-sitter-ggsql/test/corpus/basic.txt | 22 ++++++++++ 6 files changed, 111 insertions(+), 24 deletions(-) diff --git a/src/execute/table.rs b/src/execute/table.rs index 13feef07..0c3d117d 100644 --- a/src/execute/table.rs +++ b/src/execute/table.rs @@ -10,6 +10,7 @@ use crate::array_util::value_to_string; use crate::parser::{self, SourceTree}; +use crate::plot::Labels; use crate::reader::{Reader, ResolvedTable}; use crate::validate::{validate, ValidationWarning}; use crate::{DataFrame, GgsqlError, Result, Spec}; @@ -56,7 +57,7 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result Result Vec { - df.get_column_names() - .into_iter() - .enumerate() - .map(|(index, name)| TableCell { +/// +/// `labels` (from a `TABULATE LABEL` clause) is the one authority for a +/// column's content. Three outcomes: a name absent from `labels` keeps the +/// column name; an explicit `LABEL col => NULL` empties the cell; +/// `LABEL col => 'text'` sets it to `text`. +fn create_column_labels(df: &DataFrame, labels: &Labels) -> Vec { + let mut cells = Vec::new(); + + for (index, name) in df.get_column_names().into_iter().enumerate() { + let content = match labels.labels.get(&name) { + None => name, + Some(None) => String::new(), + Some(Some(label)) => label.clone(), + }; + + cells.push(TableCell { kind: TableCellKind::ColumnLabel, top: 0, bottom: 0, left: index, right: index, - content: name, - }) - .collect() + content, + }); + } + + cells } /// Build one `Body` cell per `DataFrame` value, numbered from `top == 0`. @@ -212,7 +226,7 @@ mod layout_tests { } .unwrap(); - let labels = create_column_labels(&frame); + let labels = create_column_labels(&frame, &Labels::default()); assert_eq!(labels.len(), 2); assert_eq!(labels[0].kind, TableCellKind::ColumnLabel); @@ -226,6 +240,29 @@ mod layout_tests { assert_eq!(labels[1].content, "name"); } + #[test] + fn create_column_labels_resolves_default_suppress_and_override() { + let frame = df! { + "id" => vec![1i32], + "name" => vec!["a".to_string()], + "extra" => vec![true], + } + .unwrap(); + + let mut labels = Labels::default(); + labels + .labels + .insert("id".to_string(), Some("ID".to_string())); + labels.labels.insert("name".to_string(), None); + // "extra" has no entry at all: no LABEL clause mentioned it. + + let column_labels = create_column_labels(&frame, &labels); + + assert_eq!(column_labels[0].content, "ID"); // overridden + assert_eq!(column_labels[1].content, ""); // explicitly suppressed + assert_eq!(column_labels[2].content, "extra"); // absent: kept as-is + } + #[test] fn create_body_numbers_rows_from_zero() { let frame = df! { diff --git a/src/parser/builder.rs b/src/parser/builder.rs index ea1aa53d..bc5591e6 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -244,7 +244,7 @@ pub fn build_ast(source: &SourceTree) -> Result> { ) } "tabulate_statement" => { - let table = build_tabulate_statement(&stmt_node, source); + let table = build_tabulate_statement(&stmt_node, source)?; (table.source.is_some(), "TABULATE", Spec::Table(table)) } other => { @@ -343,19 +343,40 @@ fn build_visualise_statement(node: &Node, source: &SourceTree) -> Result { } /// Build a single Table from a tabulate_statement node -fn build_tabulate_statement(node: &Node, source: &SourceTree) -> Table { +fn build_tabulate_statement(node: &Node, source: &SourceTree) -> Result
{}{}
{}{}
{ let mut table = Table::new(); let mut cursor = node.walk(); for child in node.children(&mut cursor) { - if child.kind() == "single_source_from" { - if let Some(source_node) = child.child_by_field_name("source") { - table.source = Some(parse_data_source(&source_node, source)); + match child.kind() { + "single_source_from" => { + if let Some(source_node) = child.child_by_field_name("source") { + table.source = Some(parse_data_source(&source_node, source)); + } } + "label_clause" => { + process_tab_clause(&child, source, &mut table)?; + } + _ => {} } } - table + Ok(table) +} + +/// Process a table clause node +// A single arm today, deliberately: this mirrors process_viz_clause's match +// shape so a second TABULATE clause (FACET/SCALE) only needs a new arm. +#[allow(clippy::single_match)] +fn process_tab_clause(node: &Node, source: &SourceTree, table: &mut Table) -> Result<()> { + match node.kind() { + "label_clause" => { + table.labels = build_labels(node, source)?; + } + _ => {} + } + + Ok(()) } /// Process a visualization clause node diff --git a/src/plot/main.rs b/src/plot/main.rs index 3c0b1ffa..bfa4c351 100644 --- a/src/plot/main.rs +++ b/src/plot/main.rs @@ -74,7 +74,7 @@ pub struct Plot { } /// Text labels (from LABELS clause) -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Default)] pub struct Labels { /// Label assignments (label type → text, None = suppress) pub labels: HashMap>, diff --git a/src/table/mod.rs b/src/table/mod.rs index 75198a78..a88e81f2 100644 --- a/src/table/mod.rs +++ b/src/table/mod.rs @@ -2,11 +2,12 @@ //! //! 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 minimal: only `source` (from `TABULATE FROM`) -//! is populated so far. +//! statements. Still minimal: `source` (from `TABULATE FROM`) and `labels` +//! (from `TABULATE LABEL`) are populated so far. use serde::{Deserialize, Serialize}; +use crate::plot::Labels; use crate::DataSource; /// Complete ggsql table specification. @@ -18,12 +19,20 @@ pub struct Table { /// `Plot`, there are no layers to hold a per-layer source override, so /// this is the only place a `TABULATE`'s data source can come from. pub source: Option, + /// Column display labels (from `TABULATE LABEL`). Reuses `plot::Labels` + /// as-is — the same "name → text, None = suppress" shape applies + /// unchanged, just keyed by column name instead of aesthetic name. An + /// empty `Labels` means no overrides at all. + pub labels: Labels, } impl Table { /// Create a new empty Table. pub fn new() -> Self { - Self { source: None } + Self { + source: None, + labels: Labels::default(), + } } } diff --git a/tree-sitter-ggsql/grammar.js b/tree-sitter-ggsql/grammar.js index 293d0f56..d36b5e21 100644 --- a/tree-sitter-ggsql/grammar.js +++ b/tree-sitter-ggsql/grammar.js @@ -668,13 +668,11 @@ module.exports = grammar({ caseInsensitive("VISUALIZE") ))), - // TABULATE — placeholder for tabular output, parallel to VISUALISE: an - // optional FROM right after the keyword, same single_source_from as - // visualise_statement (no joins, no comma list). No other clauses yet: - // the Table AST it builds has no fields to populate. + // TABULATE — still incomplete, more clauses expected as Table grows. tabulate_statement: $ => prec.dynamic(1, seq( $.tabulate_keyword, optional($.single_source_from), + optional($.label_clause), )), // TABULATE keyword as explicit high-precedence token (mirrors visualise_keyword) diff --git a/tree-sitter-ggsql/test/corpus/basic.txt b/tree-sitter-ggsql/test/corpus/basic.txt index 6652e9dd..dc6d7d81 100644 --- a/tree-sitter-ggsql/test/corpus/basic.txt +++ b/tree-sitter-ggsql/test/corpus/basic.txt @@ -4569,3 +4569,25 @@ TABULATE FROM sales source: (qualified_name (identifier (bare_identifier)))))) + +================================================================================ +TABULATE LABEL clause +================================================================================ + +TABULATE FROM sales LABEL id => 'ID' + +-------------------------------------------------------------------------------- + +(query + (tabulate_statement + (tabulate_keyword) + (single_source_from + source: (qualified_name + (identifier + (bare_identifier)))) + (label_clause + (label_assignment + name: (label_type + (identifier + (bare_identifier))) + value: (string))))) From d219de0dba0fcfc3521233ead50d815f7a2ca483 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 15 Sep 2026 09:38:05 +0200 Subject: [PATCH 4/5] Reject resolved table layouts with overlapping cells Groundwork ahead of column spanners, the feature expected to make cell overlap actually reachable: validate_overlaps walks each cell's grid footprint into a position set and errors on the first collision. Co-Authored-By: Claude Sonnet 5 --- src/execute/table.rs | 83 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/src/execute/table.rs b/src/execute/table.rs index 0c3d117d..9284c0b0 100644 --- a/src/execute/table.rs +++ b/src/execute/table.rs @@ -60,6 +60,7 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result, mut body: Vec) -> Vec cells } +/// Check that no two cells in a resolved layout claim the same grid position. +/// +/// Walks every cell's full footprint (`top..=bottom` × `left..=right`, not +/// just its corners) into a set of occupied positions, erroring as soon as a +/// position is claimed twice. `O(total cell area)` rather than the O(n²) cost +/// of comparing every pair of cells — cheap for the common case (one 1x1 +/// cell per data value, so area == cell count) and only grows with the +/// footprint spanning cells actually cover, not with `cells.len()` squared. +fn validate_overlaps(cells: &[TableCell]) -> Result<()> { + let mut occupied = std::collections::HashSet::new(); + + for cell in cells { + for row in cell.top..=cell.bottom { + for col in cell.left..=cell.right { + if !occupied.insert((row, col)) { + return Err(GgsqlError::ValidationError(format!( + "Table layout has more than one cell at row {row}, column {col}" + ))); + } + } + } + } + + Ok(()) +} + // ============================================================================= // Public API: TableCell // ============================================================================= @@ -337,6 +364,62 @@ mod layout_tests { assert_eq!(cells[1].top, 2); assert_eq!(cells[1].bottom, 2); } + + fn cell_at( + kind: TableCellKind, + top: usize, + bottom: usize, + left: usize, + right: usize, + ) -> TableCell { + TableCell { + kind, + top, + bottom, + left, + right, + content: String::new(), + } + } + + #[test] + fn validate_overlaps_accepts_a_disjoint_layout() { + let cells = vec![ + cell_at(TableCellKind::ColumnLabel, 0, 0, 0, 0), + cell_at(TableCellKind::ColumnLabel, 0, 0, 1, 1), + cell_at(TableCellKind::Body, 1, 1, 0, 0), + cell_at(TableCellKind::Body, 1, 1, 1, 1), + ]; + + assert!(validate_overlaps(&cells).is_ok()); + } + + #[test] + fn validate_overlaps_rejects_two_cells_at_the_same_position() { + let cells = vec![ + cell_at(TableCellKind::Body, 0, 0, 0, 0), + cell_at(TableCellKind::Body, 0, 0, 0, 0), + ]; + + let error = validate_overlaps(&cells).unwrap_err(); + assert!( + matches!(error, GgsqlError::ValidationError(msg) if msg.contains("row 0, column 0")) + ); + } + + #[test] + fn validate_overlaps_rejects_a_spanning_cell_overlapping_a_later_one() { + // A cell spanning columns 0..=1 on row 0 overlapping a second cell + // that only touches column 1 on the same row — the shape a spanner + // bug or a bad spanner declaration would produce, not something + // disjoint labels/body can create on their own. + let cells = vec![ + cell_at(TableCellKind::ColumnLabel, 0, 0, 0, 1), + cell_at(TableCellKind::Body, 0, 0, 1, 1), + ]; + + assert!(validate_overlaps(&cells).is_err()); + } } #[cfg(test)] From cbfb9e6e4d3aa0671b8c92df606097903772d692 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 15 Sep 2026 10:39:56 +0200 Subject: [PATCH 5/5] Track resolved column name/label as TableColumn ahead of spanners create_table_columns is now the one place a TABULATE LABEL clause gets resolved; create_column_labels and create_body both build cells off its order instead of the DataFrame's raw column order, so a future spanner- driven column rearrangement reaches cell positions without either function changing. create_body also now looks columns up by name and fetches each column's array once (outside the row loop) rather than once per cell. Co-Authored-By: Claude Sonnet 5 --- src/execute/table.rs | 190 ++++++++++++++++++++++++++++--------------- 1 file changed, 125 insertions(+), 65 deletions(-) diff --git a/src/execute/table.rs b/src/execute/table.rs index 9284c0b0..1b01a34d 100644 --- a/src/execute/table.rs +++ b/src/execute/table.rs @@ -57,63 +57,98 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result NULL` empties the cell; +/// column's label. Three outcomes: a name absent from `labels` keeps the +/// column name; an explicit `LABEL col => NULL` empties the label; /// `LABEL col => 'text'` sets it to `text`. -fn create_column_labels(df: &DataFrame, labels: &Labels) -> Vec { - let mut cells = Vec::new(); - - for (index, name) in df.get_column_names().into_iter().enumerate() { - let content = match labels.labels.get(&name) { - None => name, - Some(None) => String::new(), - Some(Some(label)) => label.clone(), - }; +fn create_table_columns(df: &DataFrame, labels: &Labels) -> Vec { + df.get_column_names() + .into_iter() + .map(|name| { + let label = match labels.labels.get(&name) { + None => name.clone(), + Some(None) => String::new(), + Some(Some(label)) => label.clone(), + }; + TableColumn { name, label } + }) + .collect() +} - cells.push(TableCell { +/// Build one `ColumnLabel` cell per column, numbered from `top == 0`, in +/// `columns`' order. +/// +/// Row numbering here is local to this function alone — `compose_cells` +/// is what decides where this sits relative to the body, not this function. +fn create_column_labels(columns: &[TableColumn]) -> Vec { + columns + .iter() + .enumerate() + .map(|(index, column)| TableCell { kind: TableCellKind::ColumnLabel, top: 0, bottom: 0, left: index, right: index, - content, - }); - } - - cells + content: column.label.clone(), + }) + .collect() } -/// Build one `Body` cell per `DataFrame` value, numbered from `top == 0`. -/// -/// Row numbering here is local to this function alone, the same as -/// `create_column_labels` — see that function's doc comment. -fn create_body(df: &DataFrame) -> Vec { +/// Build one `Body` cell per `DataFrame` value, numbered from `top == 0`, in +/// `columns`' order rather than `df`'s raw column order — the same seam +/// `create_column_labels` uses, so the two stay in sync under a future +/// reordering. Looks each column up in `df` **by name**, not position, since +/// `columns` may already be reordered relative to `df` by the time this runs. +fn create_body(df: &DataFrame, columns: &[TableColumn]) -> Vec { let mut cells = Vec::new(); - let columns = df.get_columns(); - for row in 0..df.height() { - for (index, column) in columns.iter().enumerate() { + for (index, column) in columns.iter().enumerate() { + // Looked up once per column, outside the row loop: `DataFrame::column` + // is an `O(ncol)` scan over the schema, so doing this per row instead + // would cost `O(nrow * ncol)` lookups rather than `O(ncol)`. + let array = df + .column(&column.name) + .expect("TableColumn.name always names a column of df"); + + for row in 0..df.height() { cells.push(TableCell { kind: TableCellKind::Body, top: row, bottom: row, left: index, right: index, - content: value_to_string(column, row), + content: value_to_string(array, row), }); } } @@ -245,30 +280,15 @@ mod layout_tests { use super::*; use crate::df; - #[test] - fn create_column_labels_builds_one_cell_per_column_at_row_zero() { - let frame = df! { - "id" => vec![1i32, 2], - "name" => vec!["a".to_string(), "b".to_string()], + fn column(name: &str, label: &str) -> TableColumn { + TableColumn { + name: name.to_string(), + label: label.to_string(), } - .unwrap(); - - let labels = create_column_labels(&frame, &Labels::default()); - - assert_eq!(labels.len(), 2); - assert_eq!(labels[0].kind, TableCellKind::ColumnLabel); - assert_eq!(labels[0].top, 0); - assert_eq!(labels[0].bottom, 0); - assert_eq!(labels[0].left, 0); - assert_eq!(labels[0].right, 0); - assert_eq!(labels[0].content, "id"); - assert_eq!(labels[1].left, 1); - assert_eq!(labels[1].right, 1); - assert_eq!(labels[1].content, "name"); } #[test] - fn create_column_labels_resolves_default_suppress_and_override() { + fn create_table_columns_resolves_default_suppress_and_override() { let frame = df! { "id" => vec![1i32], "name" => vec!["a".to_string()], @@ -283,11 +303,30 @@ mod layout_tests { labels.labels.insert("name".to_string(), None); // "extra" has no entry at all: no LABEL clause mentioned it. - let column_labels = create_column_labels(&frame, &labels); + let columns = create_table_columns(&frame, &labels); - assert_eq!(column_labels[0].content, "ID"); // overridden - assert_eq!(column_labels[1].content, ""); // explicitly suppressed - assert_eq!(column_labels[2].content, "extra"); // absent: kept as-is + assert_eq!(columns[0].name, "id"); + assert_eq!(columns[0].label, "ID"); // overridden + assert_eq!(columns[1].label, ""); // explicitly suppressed + assert_eq!(columns[2].label, "extra"); // absent: kept as-is + } + + #[test] + fn create_column_labels_builds_one_cell_per_column_at_row_zero() { + let columns = vec![column("id", "id"), column("name", "name")]; + + let labels = create_column_labels(&columns); + + assert_eq!(labels.len(), 2); + assert_eq!(labels[0].kind, TableCellKind::ColumnLabel); + assert_eq!(labels[0].top, 0); + assert_eq!(labels[0].bottom, 0); + assert_eq!(labels[0].left, 0); + assert_eq!(labels[0].right, 0); + assert_eq!(labels[0].content, "id"); + assert_eq!(labels[1].left, 1); + assert_eq!(labels[1].right, 1); + assert_eq!(labels[1].content, "name"); } #[test] @@ -297,29 +336,50 @@ mod layout_tests { "name" => vec!["a".to_string(), "b".to_string()], } .unwrap(); + let columns = create_table_columns(&frame, &Labels::default()); - let body = create_body(&frame); + let body = create_body(&frame, &columns); assert_eq!(body.len(), 4); assert!(body.iter().all(|cell| cell.kind == TableCellKind::Body)); - // Row 0 + // Column 0 ("id"): both rows, before column 1 starts — cells are + // pushed column-major, not row-major (see create_body's inline + // comment on why `array` is looked up once per column). assert_eq!(body[0].top, 0); assert_eq!(body[0].bottom, 0); assert_eq!(body[0].left, 0); assert_eq!(body[0].content, "1"); - assert_eq!(body[1].top, 0); - assert_eq!(body[1].left, 1); - assert_eq!(body[1].content, "a"); - // Row 1 - assert_eq!(body[2].top, 1); - assert_eq!(body[2].bottom, 1); - assert_eq!(body[2].left, 0); - assert_eq!(body[2].content, "2"); + assert_eq!(body[1].top, 1); + assert_eq!(body[1].bottom, 1); + assert_eq!(body[1].left, 0); + assert_eq!(body[1].content, "2"); + // Column 1 ("name") + assert_eq!(body[2].top, 0); + assert_eq!(body[2].left, 1); + assert_eq!(body[2].content, "a"); assert_eq!(body[3].top, 1); assert_eq!(body[3].left, 1); assert_eq!(body[3].content, "b"); } + #[test] + fn create_body_looks_up_columns_by_name_not_position() { + // `columns` reordered relative to `frame`'s own column order — + // `create_body` must follow `columns`, not `df`'s raw position, for + // spanner-driven reordering to actually reach the body. + let frame = df! { + "id" => vec![1i32], + "name" => vec!["a".to_string()], + } + .unwrap(); + let columns = vec![column("name", "name"), column("id", "id")]; + + let body = create_body(&frame, &columns); + + assert_eq!(body[0].content, "a"); // "name" column, placed first + assert_eq!(body[1].content, "1"); // "id" column, placed second + } + fn cell(kind: TableCellKind, top: usize, bottom: usize, content: &str) -> TableCell { TableCell { kind,