From 29c919d92a367720928e8477009d383e2071fe59 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 17 Sep 2026 15:02:57 +0200 Subject: [PATCH 1/7] Unify SPAN/PARTITION BY/FACET column lists into a shared column_list rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SPAN's columns, PARTITION BY's columns, and FACET's row/column vars were four identical `identifier, (',' identifier)*` grammar rules with matching duplicate extraction logic in builder.rs. Collapse them into one column_list rule and one parse_column_list() function. project_aesthetics stays separate since it names aesthetics, not columns, despite the identical shape. Also correct two CLAUDE.md files that claimed the generated tree-sitter parser files are committed to git — they're gitignored and regenerated by the Rust build script instead. Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 2 +- src/parser/builder.rs | 26 +++++++++++++---------- tree-sitter-ggsql/CLAUDE.md | 2 +- tree-sitter-ggsql/grammar.js | 28 +++++++++---------------- tree-sitter-ggsql/test/corpus/basic.txt | 28 ++++++++++++------------- 5 files changed, 41 insertions(+), 45 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index acdc28ab..c6084631 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -48,7 +48,7 @@ For details — module layout, traits, where extension points live — see [`src ## Building -**Prerequisite: `tree-sitter-cli`.** Any Rust build regenerates the parser from `grammar.js` via `tree-sitter-ggsql`'s build script, which runs `tree-sitter generate` and **fails if `tree-sitter-cli` is not on `PATH`**. Install it once with `npm install -g tree-sitter-cli`. To build against the committed `tree-sitter-ggsql/src/parser.c` without the CLI (e.g. if you're not touching the grammar), set `GGSQL_SKIP_GENERATE=1`. +**Prerequisite: `tree-sitter-cli`.** Any Rust build regenerates the parser from `grammar.js` via `tree-sitter-ggsql`'s build script, which runs `tree-sitter generate` and **fails if `tree-sitter-cli` is not on `PATH`**. Install it once with `npm install -g tree-sitter-cli`. To build against a pre-generated `tree-sitter-ggsql/src/parser.c` without the CLI (e.g. if you're not touching the grammar), set `GGSQL_SKIP_GENERATE=1`. ```sh # Rust workspace (default members: tree-sitter-ggsql, src, ggsql-cli, ggsql-jupyter) diff --git a/src/parser/builder.rs b/src/parser/builder.rs index c1edba5a..c37deda2 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -401,7 +401,10 @@ fn build_span_clause(node: &Node, source: &SourceTree) -> Result { } }; - let columns = source.find_texts(node, "(span_columns (identifier) @col)"); + let columns_node = source + .find_node(node, "(column_list) @cols") + .ok_or_else(|| GgsqlError::ParseError("Missing columns in SPAN clause".to_string()))?; + let columns = parse_column_list(&columns_node, source)?; let settings = match source.find_node(node, "(setting_clause) @s") { Some(setting_node) => parse_setting_clause(&setting_node, source)?, @@ -689,11 +692,12 @@ fn parse_parameter_assignment( /// Parse a partition_clause: PARTITION BY col1, col2, ... fn parse_partition_clause(node: &Node, source: &SourceTree) -> Result> { - let query = r#" - (partition_columns - (identifier) @col) - "#; - Ok(source.find_texts(node, query)) + let columns_node = source + .find_node(node, "(column_list) @cols") + .ok_or_else(|| { + GgsqlError::ParseError("Missing columns in PARTITION BY clause".to_string()) + })?; + parse_column_list(&columns_node, source) } /// Parse a filter_clause: FILTER @@ -1012,9 +1016,9 @@ fn build_facet(node: &Node, source: &SourceTree) -> Result { "facet_by" => { next_vars_are_cols = true; } - "facet_vars" => { + "column_list" => { // Parse list of variable names - let vars = parse_facet_vars(&child, source)?; + let vars = parse_column_list(&child, source)?; if next_vars_are_cols { column_vars = vars; } else { @@ -1048,9 +1052,9 @@ fn build_facet(node: &Node, source: &SourceTree) -> Result { }) } -/// Parse facet variables from a facet_vars node -fn parse_facet_vars(node: &Node, source: &SourceTree) -> Result> { - let query = "(identifier) @var"; +/// Parse identifier texts out of a column_list node +fn parse_column_list(node: &Node, source: &SourceTree) -> Result> { + let query = "(identifier) @col"; Ok(source.find_texts(node, query)) } diff --git a/tree-sitter-ggsql/CLAUDE.md b/tree-sitter-ggsql/CLAUDE.md index 02ad2b58..c57043ee 100644 --- a/tree-sitter-ggsql/CLAUDE.md +++ b/tree-sitter-ggsql/CLAUDE.md @@ -28,7 +28,7 @@ tree-sitter-ggsql/ └── corpus/ Tree-sitter test corpus (parser test cases) ``` -The files under `src/` are generated by `tree-sitter generate` from `grammar.js` — they are committed so downstream consumers don't need the tree-sitter CLI to build. +The files under `src/` are generated by `tree-sitter generate` from `grammar.js` — they are gitignored, not committed. `bindings/rust/build.rs` regenerates them on every Rust build by default. ## Grammar at a glance diff --git a/tree-sitter-ggsql/grammar.js b/tree-sitter-ggsql/grammar.js index 2589905d..2676a768 100644 --- a/tree-sitter-ggsql/grammar.js +++ b/tree-sitter-ggsql/grammar.js @@ -698,13 +698,13 @@ module.exports = grammar({ // Reuses label_assignment's value shape. field('label', choice($.string, $.null_literal)), caseInsensitive('ACROSS'), - $.span_columns, + $.column_list, optional($.setting_clause) ), - // The group of columns this SPAN covers — same plain identifier-list - // shape as partition_columns/facet_vars/project_aesthetics. - span_columns: $ => seq( + // Shared comma-separated identifier list: SPAN's columns, PARTITION BY's + // columns, FACET's row/column vars, FORMAT's columns. + column_list: $ => seq( $.identifier, repeat(seq(',', $.identifier)) ), @@ -843,12 +843,7 @@ module.exports = grammar({ partition_clause: $ => seq( caseInsensitive('PARTITION'), caseInsensitive('BY'), - $.partition_columns - ), - - partition_columns: $ => seq( - $.identifier, - repeat(seq(',', $.identifier)) + $.column_list ), // FILTER clause for layer filtering: FILTER @@ -1054,21 +1049,16 @@ module.exports = grammar({ // Single variable = wrap layout, BY clause = grid layout facet_clause: $ => seq( caseInsensitive('FACET'), - $.facet_vars, + $.column_list, optional(seq( alias(caseInsensitive('BY'), $.facet_by), - $.facet_vars + $.column_list )), optional($.setting_clause) // Reuse from DRAW/SCALE ), facet_by: $ => 'BY', - facet_vars: $ => seq( - $.identifier, - repeat(seq(',', $.identifier)) - ), - // PROJECT clause - PROJECT [aesthetics] TO coord_type [SETTING prop => value, ...] // Examples: // PROJECT TO cartesian (defaults to x, y) @@ -1085,7 +1075,9 @@ module.exports = grammar({ optional(seq(caseInsensitive('SETTING'), $.project_properties)) ), - // Optional list of position aesthetic names for PROJECT clause + // Optional list of position aesthetic names for PROJECT clause. Kept + // separate from column_list even though the grammar shape is identical: + // this names aesthetics (x, y, angle, radius, ...), not columns. project_aesthetics: $ => seq( $.identifier, repeat(seq(',', $.identifier)) diff --git a/tree-sitter-ggsql/test/corpus/basic.txt b/tree-sitter-ggsql/test/corpus/basic.txt index 8afc0403..a8603f3e 100644 --- a/tree-sitter-ggsql/test/corpus/basic.txt +++ b/tree-sitter-ggsql/test/corpus/basic.txt @@ -1281,7 +1281,7 @@ DRAW line PARTITION BY category (draw_clause (geom_type) (partition_clause - (partition_columns + (column_list (identifier (bare_identifier)))))))) @@ -1317,7 +1317,7 @@ DRAW line PARTITION BY category, region (draw_clause (geom_type) (partition_clause - (partition_columns + (column_list (identifier (bare_identifier)) (identifier @@ -1369,7 +1369,7 @@ DRAW line SETTING opacity => 0.5 FILTER year > 2020 PARTITION BY category (filter_token (number)))) (partition_clause - (partition_columns + (column_list (identifier (bare_identifier)))))))) @@ -2013,7 +2013,7 @@ DRAW line PARTITION BY category ORDER BY date ASC (draw_clause (geom_type) (partition_clause - (partition_columns + (column_list (identifier (bare_identifier)))) (order_clause @@ -2069,7 +2069,7 @@ DRAW line SETTING opacity => 0.5 FILTER year > 2020 PARTITION BY region ORDER BY (filter_token (number)))) (partition_clause - (partition_columns + (column_list (identifier (bare_identifier)))) (order_clause @@ -2911,7 +2911,7 @@ FACET region (geom_type))) (viz_clause (facet_clause - (facet_vars + (column_list (identifier (bare_identifier))))))) @@ -2949,11 +2949,11 @@ FACET region BY category (geom_type))) (viz_clause (facet_clause - (facet_vars + (column_list (identifier (bare_identifier))) (facet_by) - (facet_vars + (column_list (identifier (bare_identifier))))))) @@ -2991,7 +2991,7 @@ FACET region SETTING scales => 'free_y', ncol => 3 (geom_type))) (viz_clause (facet_clause - (facet_vars + (column_list (identifier (bare_identifier))) (setting_clause @@ -4611,7 +4611,7 @@ TABULATE FROM sales SPAN 'Pretty Name' ACROSS foo, bar, baz (tab_clause (span_clause label: (string) - (span_columns + (column_list (identifier (bare_identifier)) (identifier @@ -4637,7 +4637,7 @@ TABULATE FROM sales SPAN 'A' ACROSS foo (tab_clause (span_clause label: (string) - (span_columns + (column_list (identifier (bare_identifier))))))) @@ -4659,7 +4659,7 @@ TABULATE FROM sales SPAN NULL ACROSS foo, bar (tab_clause (span_clause label: (null_literal) - (span_columns + (column_list (identifier (bare_identifier)) (identifier @@ -4683,7 +4683,7 @@ TABULATE FROM sales SPAN 'W' ACROSS foo SETTING width => '40%' (tab_clause (span_clause label: (string) - (span_columns + (column_list (identifier (bare_identifier))) (setting_clause @@ -4719,7 +4719,7 @@ TABULATE FROM sales LABEL id => 'ID' SPAN 'A' ACROSS foo, bar (tab_clause (span_clause label: (string) - (span_columns + (column_list (identifier (bare_identifier)) (identifier From ef415aad0a666161608a4ad347efdc00922c4429 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 17 Sep 2026 15:52:19 +0200 Subject: [PATCH 2/7] Add TABULATE FORMAT clause grammar and AST construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FORMAT configures cell formatting for a group of columns: FORMAT col, ... [SETTING ...] [RENAMING ...], repeatable like SPAN. SETTING's semantics are deferred — parsed and stored but not yet validated or applied. RENAMING reuses SCALE's exact clause (scale_renaming_clause renamed to the generic renaming_clause) so FORMAT gets identical explicit-value and wildcard-template renaming behavior for free. Table gains a `formats: Vec` field. Format stores value_mapping/ value_template rather than reusing Scale's label_mapping/label_template names, since Table::labels already claims "label" for column headers. Both fields share one default_template() helper in format.rs instead of each struct defining its own serde default function. Co-Authored-By: Claude Sonnet 5 --- src/format.rs | 9 +++ src/lib.rs | 2 +- src/parser/builder.rs | 88 +++++++++++++++++++++++-- src/plot/scale/types.rs | 7 +- src/table/mod.rs | 34 +++++++++- tree-sitter-ggsql/CLAUDE.md | 2 +- tree-sitter-ggsql/grammar.js | 19 ++++-- tree-sitter-ggsql/test/corpus/basic.txt | 42 ++++++++++-- 8 files changed, 182 insertions(+), 21 deletions(-) diff --git a/src/format.rs b/src/format.rs index d0d9df1e..df7c8f4d 100644 --- a/src/format.rs +++ b/src/format.rs @@ -154,6 +154,15 @@ fn format_number_with_spec(value: &str, fmt: &str) -> String { value.to_string() } +/// Default template: passes a value through unchanged. Shared `serde` +/// default for every `*_template` field using this module's placeholder +/// syntax (`Scale::label_template`, `Format::value_template`) — their +/// default can't just be `String::default()`, since `""` would blank every +/// value instead of passing it through. +pub(crate) fn default_template() -> String { + "{}".to_string() +} + /// Apply a label template to an array of break values. /// /// Each break value is formatted using the template string. diff --git a/src/lib.rs b/src/lib.rs index f0765166..ce093dc6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -72,7 +72,7 @@ pub use plot::{ // Re-export the parse-time Plot/Table result and the Table stub pub use spec::Spec; -pub use table::{Spanner, Table}; +pub use table::{Format, Spanner, Table}; // Re-export aesthetic classification utilities pub use plot::aesthetic::{ diff --git a/src/parser/builder.rs b/src/parser/builder.rs index c37deda2..44ebb2d0 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, Spanner, Spec, Table}; +use crate::{Format, GgsqlError, Result, Spanner, Spec, Table}; use std::collections::HashMap; use tree_sitter::Node; @@ -378,6 +378,9 @@ fn process_tab_clause(node: &Node, source: &SourceTree, table: &mut Table) -> Re "span_clause" => { table.spans.push(build_span_clause(&child, source)?); } + "format_clause" => { + table.formats.push(build_format_clause(&child, source)?); + } _ => {} } } @@ -418,6 +421,36 @@ fn build_span_clause(node: &Node, source: &SourceTree) -> Result { }) } +/// Build a Format from a format_clause node: FORMAT col, ... [SETTING ...] [RENAMING ...] +fn build_format_clause(node: &Node, source: &SourceTree) -> Result { + let columns_node = source + .find_node(node, "(column_list) @cols") + .ok_or_else(|| GgsqlError::ParseError("Missing columns in FORMAT clause".to_string()))?; + let columns = parse_column_list(&columns_node, source)?; + + let settings = match source.find_node(node, "(setting_clause) @s") { + Some(setting_node) => parse_setting_clause(&setting_node, source)?, + None => Parameters::new(), + }; + + let mut value_mapping = None; + let mut value_template = "{}".to_string(); + if let Some(renaming_node) = source.find_node(node, "(renaming_clause) @r") { + let (mappings, template) = parse_renaming_clause(&renaming_node, source)?; + if !mappings.is_empty() { + value_mapping = Some(mappings); + } + value_template = template; + } + + Ok(Format { + columns, + settings, + value_mapping, + value_template, + }) +} + /// Process a visualization clause node fn process_viz_clause(node: &Node, source: &SourceTree, spec: &mut Plot) -> Result<()> { let mut cursor = node.walk(); @@ -808,9 +841,9 @@ fn build_scale(node: &Node, source: &SourceTree) -> Result { // Reuse existing setting_clause parser properties = parse_setting_clause(&child, source)?; } - "scale_renaming_clause" => { + "renaming_clause" => { // Parse RENAMING 'A' => 'Alpha', 'B' => 'Beta', * => '{} units' - let (mappings, template) = parse_scale_renaming_clause(&child, source)?; + let (mappings, template) = parse_renaming_clause(&child, source)?; if !mappings.is_empty() { label_mapping = Some(mappings); } @@ -935,7 +968,7 @@ fn parse_scale_via_clause(node: &Node, source: &SourceTree) -> Result /// Returns a tuple of: /// - HashMap where: Key = original value, Value = Some(label) or None for suppressed labels /// - Template string for wildcard mappings (* => '...'), defaults to "{}" -fn parse_scale_renaming_clause( +fn parse_renaming_clause( node: &Node, source: &SourceTree, ) -> Result<(HashMap>, String)> { @@ -1453,6 +1486,53 @@ mod tests { assert_eq!(table.spans[1].columns, vec!["baz"]); } + #[test] + fn test_tabulate_format_basic_has_no_settings_or_renaming() { + let specs = parse_test_specs("TABULATE FROM sales FORMAT foo, bar").unwrap(); + let table = specs[0].as_table().expect("expected a Table spec"); + + assert_eq!(table.formats.len(), 1); + assert_eq!(table.formats[0].columns, vec!["foo", "bar"]); + assert!(table.formats[0].settings.is_empty()); + assert_eq!(table.formats[0].value_mapping, None); + assert_eq!(table.formats[0].value_template, "{}"); + } + + #[test] + fn test_tabulate_format_with_setting_and_renaming() { + let specs = parse_test_specs( + "TABULATE FROM sales FORMAT price SETTING width => '20%' \ + RENAMING null => '-', * => '{:num %.2f}'", + ) + .unwrap(); + let table = specs[0].as_table().expect("expected a Table spec"); + + assert_eq!(table.formats[0].columns, vec!["price"]); + assert_eq!( + table.formats[0].settings.get("width"), + Some(&ParameterValue::String("20%".to_string())) + ); + assert_eq!( + table.formats[0].value_mapping, + Some(HashMap::from([("null".to_string(), Some("-".to_string()))])) + ); + assert_eq!(table.formats[0].value_template, "{:num %.2f}"); + } + + #[test] + fn test_tabulate_multiple_format_clauses_produce_separate_formats() { + let specs = parse_test_specs( + "TABULATE FROM sales FORMAT foo RENAMING * => '{:num %.0f}' FORMAT bar", + ) + .unwrap(); + let table = specs[0].as_table().expect("expected a Table spec"); + + assert_eq!(table.formats.len(), 2); + assert_eq!(table.formats[0].columns, vec!["foo"]); + assert_eq!(table.formats[0].value_template, "{:num %.0f}"); + assert_eq!(table.formats[1].columns, vec!["bar"]); + } + // ======================================== // PROJECT Property Validation Tests // ======================================== diff --git a/src/plot/scale/types.rs b/src/plot/scale/types.rs index 3d9786ea..fcd6006d 100644 --- a/src/plot/scale/types.rs +++ b/src/plot/scale/types.rs @@ -9,11 +9,6 @@ use super::super::types::{ArrayElement, ParameterValue, Parameters}; use super::scale_type::ScaleType; use super::transform::Transform; -/// Default label template - passes through values unchanged -fn default_label_template() -> String { - "{}".to_string() -} - /// One bin of a resolved binned scale: the edges bounding it and the text that /// names it. See [`Scale::binned_bins`]. #[derive(Debug, Clone, PartialEq)] @@ -78,7 +73,7 @@ pub struct Scale { /// Default is "{}" which passes through the value unchanged. /// The `{}` placeholder is replaced with each value at resolution time. /// Example: "{} units" -> {"0": "0 units", "25": "25 units", ...} - #[serde(default = "default_label_template")] + #[serde(default = "crate::format::default_template")] pub label_template: String, } diff --git a/src/table/mod.rs b/src/table/mod.rs index 74380b6e..44178b37 100644 --- a/src/table/mod.rs +++ b/src/table/mod.rs @@ -3,9 +3,11 @@ //! Defines the typed `Table` structure that represents parsed `TABULATE` //! statements, parallel to how `plot` defines `Plot` for `VISUALISE` //! statements: `source` (from `TABULATE FROM`), `labels` (from `TABULATE -//! LABEL`), and `spans` (from `TABULATE SPAN`) are populated so far. +//! LABEL`), `spans` (from `TABULATE SPAN`), and `formats` (from `TABULATE +//! FORMAT`) are populated so far. use serde::{Deserialize, Serialize}; +use std::collections::HashMap; use crate::plot::{ validate_parameter, DefaultParamValue, Labels, ParamConstraint, ParamDefinition, Parameters, @@ -31,6 +33,9 @@ pub struct Table { /// not one `Spanner` with several groups (`SPAN` itself never bundles /// more than one group per clause). pub spans: Vec, + /// Cell formatting (from `TABULATE FORMAT`), one per `FORMAT` clause + /// written — same one-clause-per-group model as `spans`. + pub formats: Vec, } impl Table { @@ -40,6 +45,7 @@ impl Table { source: None, labels: Labels::default(), spans: Vec::new(), + formats: Vec::new(), } } } @@ -157,6 +163,32 @@ impl Spanner { } } +/// One `FORMAT` clause: cell formatting for a group of columns. +/// +/// `settings` is parsed but not yet validated or applied anywhere — `SETTING` +/// semantics for `FORMAT` land in a later change; only the grammar shape is +/// wired up so far. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct Format { + /// The columns this FORMAT applies to, in the order written. + pub columns: Vec, + /// `SETTING` parameters for this FORMAT (e.g. `width => '20%'`). + pub settings: Parameters, + /// Value mappings for custom cell display (`RENAMING` clause). Maps a raw + /// cell value to its display text; `None` suppresses the cell's text. + /// Same shape as `Scale::label_mapping` — named `value_mapping` rather + /// than `label_mapping` here because `Table::labels` already uses + /// "label" for column headers, a different concept from a cell's value. + #[serde(default)] + pub value_mapping: Option>>, + /// Template for generating display text from cell values (e.g. + /// `"{:num %.2f}"`), applied to values with no specific `value_mapping` + /// entry. Default `"{}"` passes the value through unchanged. Same shape + /// as `Scale::label_template`. + #[serde(default = "crate::format::default_template")] + pub value_template: String, +} + #[cfg(test)] mod tests { use super::*; diff --git a/tree-sitter-ggsql/CLAUDE.md b/tree-sitter-ggsql/CLAUDE.md index c57043ee..27601c9d 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(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 still a **work in progress**: it recognises the keyword, an optional `single_source_from` (the same single-source `FROM` rule VISUALISE uses), and a repeated `tab_clause` — `LABEL` and `SPAN`, in any order, the same "any order, repeated" shape `viz_clause` gives VISUALISE's own clauses. Only `LABEL` is wired into the typed `Table` AST by [`/src/parser/builder.rs`](../src/parser/builder.rs) so far — `SPAN` parses but isn't consumed yet. (Revisit this paragraph's level of detail once most TABULATE clauses exist — describing two clauses inline is fine for now, but won't scale the way VISUALISE's own clause list needed a dedicated table instead.) +`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 parsed the same way, with a smaller clause set so far, so [`/src/parser/builder.rs`](../src/parser/builder.rs) can build a typed `Table` from it. For ggsql language semantics, see [`/doc/syntax/`](../doc/syntax/) — this package only defines *how text is parsed*, not what the resulting tree means. diff --git a/tree-sitter-ggsql/grammar.js b/tree-sitter-ggsql/grammar.js index 2676a768..b2cc0db6 100644 --- a/tree-sitter-ggsql/grammar.js +++ b/tree-sitter-ggsql/grammar.js @@ -685,6 +685,17 @@ module.exports = grammar({ tab_clause: $ => choice( $.label_clause, $.span_clause, + $.format_clause, + ), + + // FORMAT — configures cell formatting for a group of columns. Multiple + // FORMAT clauses repeat (FORMAT ... FORMAT ...) for different column + // groups, the same model SPAN uses. + format_clause: $ => seq( + caseInsensitive('FORMAT'), + $.column_list, + optional($.setting_clause), + optional($.renaming_clause) ), // SPAN — groups columns under one spanner cell. Multiple spanners repeat @@ -993,12 +1004,12 @@ module.exports = grammar({ optional($.scale_to_clause), optional($.scale_via_clause), optional($.setting_clause), // reuse existing setting_clause from DRAW - optional($.scale_renaming_clause) // custom label mappings + optional($.renaming_clause) // custom label mappings ), - // RENAMING clause for custom axis/legend labels - // Syntax: RENAMING 'A' => 'Alpha', 'B' => 'Beta', 'C' => NULL - scale_renaming_clause: $ => seq( + // RENAMING clause: SCALE uses it for custom axis/legend labels, FORMAT + // for custom cell labels. Syntax: RENAMING 'A' => 'Alpha', 'B' => 'Beta', 'C' => NULL + renaming_clause: $ => seq( caseInsensitive('RENAMING'), $.renaming_assignment, repeat(seq(',', $.renaming_assignment)) diff --git a/tree-sitter-ggsql/test/corpus/basic.txt b/tree-sitter-ggsql/test/corpus/basic.txt index a8603f3e..c08441de 100644 --- a/tree-sitter-ggsql/test/corpus/basic.txt +++ b/tree-sitter-ggsql/test/corpus/basic.txt @@ -2593,7 +2593,7 @@ SCALE DISCRETE x RENAMING 'A' => 'Alpha', 'B' => 'Beta' (scale_clause (scale_type_identifier) (aesthetic_name) - (scale_renaming_clause + (renaming_clause (renaming_assignment name: (string) value: (string)) @@ -2631,7 +2631,7 @@ SCALE DISCRETE x RENAMING 'internal' => NULL (scale_clause (scale_type_identifier) (aesthetic_name) - (scale_renaming_clause + (renaming_clause (renaming_assignment name: (string) value: (null_literal))))))) @@ -2666,7 +2666,7 @@ SCALE CONTINUOUS x RENAMING * => '{} units' (scale_clause (scale_type_identifier) (aesthetic_name) - (scale_renaming_clause + (renaming_clause (renaming_assignment value: (string))))))) @@ -2700,7 +2700,7 @@ SCALE DISCRETE x RENAMING 'A' => 'Alpha', * => 'Category {}' (scale_clause (scale_type_identifier) (aesthetic_name) - (scale_renaming_clause + (renaming_clause (renaming_assignment name: (string) value: (string)) @@ -4724,3 +4724,37 @@ TABULATE FROM sales LABEL id => 'ID' SPAN 'A' ACROSS foo, bar (bare_identifier)) (identifier (bare_identifier))))))) + +================================================================================ +TABULATE FORMAT clause +================================================================================ + +TABULATE FROM sales FORMAT foo, bar SETTING width => '20%' RENAMING null => '-' + +-------------------------------------------------------------------------------- + +(query + (tabulate_statement + (tabulate_keyword) + (single_source_from + source: (qualified_name + (identifier + (bare_identifier)))) + (tab_clause + (format_clause + (column_list + (identifier + (bare_identifier)) + (identifier + (bare_identifier))) + (setting_clause + (parameter_assignment + name: (parameter_name + (identifier + (bare_identifier))) + value: (parameter_value + (string)))) + (renaming_clause + (renaming_assignment + name: (null_literal) + value: (string))))))) From f16b412d99d83981ad27a64671757b02ebb92591 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 17 Sep 2026 17:17:40 +0200 Subject: [PATCH 3/7] Apply TABULATE FORMAT's RENAMING to resolved table cell values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wires Format.value_mapping/value_template into actual cell display: a new table_format module replaces each FORMAT-covered column in the resolved DataFrame with its resolved display text before column/cell layout runs, so create_body's ordinary value_to_string rendering already reflects any RENAMING and never needs to know FORMAT exists. A later FORMAT clause wins over an earlier one naming the same column; an unknown column errors. format.rs gains resolve_column_values, which works directly off a column's typed Arrow values rather than boxing each row into an ArrayElement first (the path apply_label_template uses for Scale's much smaller break lists) — avoiding that allocation for what can be a much larger number of table rows. compute_numeric_precision now takes plain f64s so both call sites can share it without going through ArrayElement, and the string/numeric column conversion in format_dataframe_column is factored out into column_to_strings for both to reuse. Co-Authored-By: Claude Sonnet 5 --- src/execute/mod.rs | 3 + src/execute/table.rs | 49 +++++++-- src/execute/table_format.rs | 152 +++++++++++++++++++++++++++ src/format.rs | 204 +++++++++++++++++++++++++++++------- 4 files changed, 360 insertions(+), 48 deletions(-) create mode 100644 src/execute/table_format.rs diff --git a/src/execute/mod.rs b/src/execute/mod.rs index eef00362..eea72eae 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -11,6 +11,8 @@ //! - `scale`: Scale creation, resolution, type coercion, and OOB handling //! - `table`: Table (TABULATE) resolution //! - `table_spanner`: `TABULATE SPAN` resolution, called from `table` +//! - `table_format`: `TABULATE FORMAT` resolution (replaces a column's +//! values with its resolved display text), called from `table` mod casting; mod cte; @@ -19,6 +21,7 @@ mod position; mod scale; mod schema; mod table; +mod table_format; mod table_spanner; // Re-export public API diff --git a/src/execute/table.rs b/src/execute/table.rs index c4df3f5a..15ee723a 100644 --- a/src/execute/table.rs +++ b/src/execute/table.rs @@ -4,12 +4,15 @@ //! resolution, or facet handling to do here — just the one query that //! produces `body`, plus resolving that data into positioned `TableCell`s. //! `SPAN`-specific resolution (column reordering, header-row assignment) -//! lives in the sibling `table_spanner` module, 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. -//! `Table::resolve_spanner_ids` is the exception — it needs no `DataFrame`, -//! so it lives on `Table` itself, reachable from `validate()` too. - +//! lives in the sibling `table_spanner` module, and `FORMAT`-specific +//! resolution (replacing a column's values with its resolved display text) +//! lives in `table_format`, 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. `Table::resolve_spanner_ids` is the +//! exception — it needs no `DataFrame`, so it lives on `Table` itself, +//! reachable from `validate()` too. + +use super::table_format::apply_formats; use super::table_spanner::{create_spanners, reorder_table_columns}; use crate::array_util::value_to_string; use crate::parser::{self, SourceTree}; @@ -80,12 +83,14 @@ fn build_cells(df: &DataFrame, table: &Table) -> Result> { .resolve_spanner_ids() .map_err(GgsqlError::ValidationError)?; - let columns = create_table_columns(df, &table.labels); + let df = apply_formats(df, &table.formats)?; + + let columns = create_table_columns(&df, &table.labels); let columns = reorder_table_columns(columns, &spans)?; let spanners = create_spanners(&columns, &spans)?; let column_labels = create_column_labels(&columns); let header = compose_header(spanners, column_labels); - let table_body = create_body(df, &columns); + let table_body = create_body(&df, &columns); let cells = rowbind_cells(header, table_body); validate_overlaps(&cells)?; @@ -787,6 +792,34 @@ mod layout_tests { .all(|c| c.top == 2)); } + #[test] + fn build_cells_applies_a_formats_renaming_to_body_cells() { + let frame = df! { + "price" => vec![0.0f64, 5.0], + } + .unwrap(); + let mut table = Table::new(); + table.formats = vec![crate::Format { + columns: vec!["price".to_string()], + settings: Parameters::new(), + value_mapping: Some(std::collections::HashMap::from([( + "0".to_string(), + Some("-".to_string()), + )])), + value_template: "${:num %.2f}".to_string(), + }]; + + let cells = build_cells(&frame, &table).unwrap(); + + let mut body_cells: Vec<_> = cells + .iter() + .filter(|c| c.kind == TableCellKind::Body) + .collect(); + body_cells.sort_by_key(|c| c.top); + assert_eq!(body_cells[0].content, "-"); + assert_eq!(body_cells[1].content, "$5.00"); + } + #[test] fn build_cells_reorders_columns_for_a_spanner_before_building_labels_and_body() { let frame = df! { diff --git a/src/execute/table_format.rs b/src/execute/table_format.rs new file mode 100644 index 00000000..e7eca3b0 --- /dev/null +++ b/src/execute/table_format.rs @@ -0,0 +1,152 @@ +//! `TABULATE FORMAT` resolution: replacing each `FORMAT`-covered column in a +//! table's `DataFrame` with its resolved display text, called from +//! `table::build_cells` before column/cell layout runs — so `create_body`'s +//! ordinary `value_to_string` rendering already reflects any `RENAMING`, and +//! never needs to know `FORMAT` exists. + +use std::collections::HashMap; + +use crate::array_util::{new_str_array, value_to_string}; +use crate::{DataFrame, Format, GgsqlError, Result}; + +/// Replace every `FORMAT`-covered column in `df` with its resolved display +/// text. A later `FORMAT` clause wins over an earlier one naming the same +/// column. A column whose `FORMAT` has no `RENAMING` at all (`value_template` +/// is the default `"{}"` and `value_mapping` is empty) is left untouched. +pub(crate) fn apply_formats(df: &DataFrame, formats: &[Format]) -> Result { + let mut by_column: HashMap<&str, &Format> = HashMap::new(); + for format in formats { + for name in &format.columns { + if df.column(name).is_err() { + return Err(GgsqlError::ValidationError(format!( + "FORMAT references unknown column '{name}'" + ))); + } + by_column.insert(name.as_str(), format); + } + } + + let mut df = df.clone(); + for (name, format) in by_column { + let has_renaming = format.value_template != "{}" + || format.value_mapping.as_ref().is_some_and(|m| !m.is_empty()); + if !has_renaming { + // Skip the per-row formatting pass for a column nobody asked to + // reformat — the common `FORMAT ... SETTING ...` case. + continue; + } + + let array = df + .column(name) + .expect("name was just checked above, before df was cloned") + .clone(); + let resolved = crate::format::resolve_column_values( + &array, + &format.value_template, + &format.value_mapping, + ) + .map_err(|e| GgsqlError::ValidationError(format!("FORMAT column '{name}': {e}")))?; + + let display: Vec> = resolved + .into_iter() + .enumerate() + .map(|(row, value)| Some(value.unwrap_or_else(|| value_to_string(&array, row)))) + .collect(); + let display_refs: Vec> = display.iter().map(|s| s.as_deref()).collect(); + + df = df.with_column(name, new_str_array(display_refs))?; + } + + Ok(df) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::array_util::value_to_string; + use crate::df; + + fn format_with( + columns: &[&str], + template: &str, + mapping: Option>>, + ) -> Format { + Format { + columns: columns.iter().map(|s| s.to_string()).collect(), + settings: crate::plot::Parameters::new(), + value_mapping: mapping, + value_template: template.to_string(), + } + } + + fn column_content(df: &DataFrame, name: &str) -> Vec { + let array = df.column(name).unwrap(); + (0..df.height()) + .map(|row| value_to_string(array, row)) + .collect() + } + + #[test] + fn leaves_a_column_untouched_when_its_format_has_no_renaming() { + let frame = df! { "price" => vec![1.0f64, 2.0] }.unwrap(); + let formats = vec![format_with(&["price"], "{}", None)]; + + let result = apply_formats(&frame, &formats).unwrap(); + + assert_eq!(column_content(&result, "price"), vec!["1", "2"]); + } + + #[test] + fn applies_a_wildcard_template() { + let frame = df! { "price" => vec![1.0f64, 2.0] }.unwrap(); + let formats = vec![format_with(&["price"], "${:num %.2f}", None)]; + + let result = apply_formats(&frame, &formats).unwrap(); + + assert_eq!(column_content(&result, "price"), vec!["$1.00", "$2.00"]); + } + + #[test] + fn explicit_mapping_wins_over_the_template() { + let frame = df! { "price" => vec![0.0f64, 5.0] }.unwrap(); + let mapping = Some(HashMap::from([("0".to_string(), Some("-".to_string()))])); + let formats = vec![format_with(&["price"], "{:num %.2f}", mapping)]; + + let result = apply_formats(&frame, &formats).unwrap(); + + assert_eq!(column_content(&result, "price"), vec!["-", "5.00"]); + } + + #[test] + fn later_format_wins_for_a_column_named_by_two_formats() { + let frame = df! { "price" => vec![1.0f64] }.unwrap(); + let formats = vec![ + format_with(&["price"], "{:num %.0f}", None), + format_with(&["price"], "{:num %.2f}", None), + ]; + + let result = apply_formats(&frame, &formats).unwrap(); + + assert_eq!(column_content(&result, "price"), vec!["1.00"]); + } + + #[test] + fn errors_on_a_format_naming_an_unknown_column() { + let frame = df! { "price" => vec![1.0f64] }.unwrap(); + let formats = vec![format_with(&["typo"], "{}", None)]; + + let error = apply_formats(&frame, &formats).unwrap_err(); + assert!(matches!(error, GgsqlError::ValidationError(msg) + if msg.contains("FORMAT references unknown column 'typo'"))); + } + + #[test] + fn a_null_with_no_explicit_entry_still_falls_back_to_value_to_string() { + let frame = df! { "price" => vec![Some(1.0f64), None] }.unwrap(); + let formats = vec![format_with(&["price"], "{:num %.2f}", None)]; + + let result = apply_formats(&frame, &formats).unwrap(); + + assert_eq!(column_content(&result, "price"), vec!["1.00", "null"]); + } +} diff --git a/src/format.rs b/src/format.rs index df7c8f4d..24118daf 100644 --- a/src/format.rs +++ b/src/format.rs @@ -8,11 +8,14 @@ //! - `{:time %fmt}` - DateTime strftime format (e.g., `{:time %b %Y}` -> "Jan 2024") //! - `{:num %fmt}` - Number printf format (e.g., `{:num %.2f}` -> "25.50") +use arrow::array::{Array, ArrayRef}; +use arrow::datatypes::DataType; use chrono::{NaiveDate, NaiveDateTime, NaiveTime}; use regex::Regex; use std::collections::HashMap; use std::sync::OnceLock; +use crate::array_util::{as_f64, as_str, cast_array}; use crate::plot::ArrayElement; /// Placeholder types supported in label templates @@ -200,7 +203,14 @@ pub fn apply_label_template( .any(|p| matches!(p.placeholder, Placeholder::Plain)) || placeholders.is_empty(); let numeric_precision = if has_plain { - compute_numeric_precision(breaks) + let numbers: Vec = breaks + .iter() + .filter_map(|e| match e { + ArrayElement::Number(n) => Some(*n), + _ => None, + }) + .collect(); + compute_numeric_precision(&numbers) } else { None }; @@ -226,20 +236,14 @@ pub fn apply_label_template( result } -/// Determine the number of decimal places needed to display numeric breaks -/// consistently. Based on the algorithm from R's `scales::precision()`: -/// uses the smallest inter-break difference to derive display precision. -/// Returns `None` if there are fewer than 2 numeric breaks or all are integers. -fn compute_numeric_precision(breaks: &[ArrayElement]) -> Option { +/// Determine the number of decimal places needed to display numeric values +/// consistently. Based on the algorithm from R's `scales::precision()`: uses +/// the smallest inter-value difference to derive display precision. Returns +/// `None` if there are fewer than 2 numbers or all are integers. +fn compute_numeric_precision(numbers: &[f64]) -> Option { let tol = f64::EPSILON.sqrt(); - let mut numbers: Vec = breaks - .iter() - .filter_map(|e| match e { - ArrayElement::Number(n) => Some(*n), - _ => None, - }) - .collect(); + let mut numbers: Vec = numbers.to_vec(); if numbers.len() < 2 { return None; @@ -296,19 +300,39 @@ pub fn format_dataframe_column( column_name: &str, template: &str, ) -> Result { - use crate::array_util::{as_f64, as_str, cast_array, new_str_array}; - use arrow::array::Array; - use arrow::datatypes::DataType; + use crate::array_util::new_str_array; // Get the column let column = df .column(column_name) .map_err(|e| format!("Column '{}' not found: {}", column_name, e))?; - // Step 1: Convert entire column to strings - let string_values: Vec> = if let Ok(str_col) = as_str(column) { + let string_values = column_to_strings(column).map_err(|e| { + format!("{e} in column '{column_name}'. Try string or numeric types instead.") + })?; + + // Step 2: Apply formatting template to all string values + let placeholders = parse_placeholders(template); + let formatted_owned: Vec> = string_values + .into_iter() + .map(|opt| opt.map(|s| format_value(&s, template, &placeholders, None))) + .collect(); + + let formatted_refs: Vec> = + formatted_owned.iter().map(|opt| opt.as_deref()).collect(); + let formatted_col = new_str_array(formatted_refs); + + // Replace column in DataFrame + df.with_column(column_name, formatted_col) + .map_err(|e| format!("Failed to replace column: {}", e)) +} + +/// Convert an Arrow column to one `String` per row (`None` for a null), +/// supporting string and numeric columns. +fn column_to_strings(column: &ArrayRef) -> Result>, String> { + if let Ok(str_col) = as_str(column) { // String column (includes temporal data auto-converted to ISO format) - (0..str_col.len()) + Ok((0..str_col.len()) .map(|i| { if str_col.is_null(i) { None @@ -316,14 +340,14 @@ pub fn format_dataframe_column( Some(str_col.value(i).to_string()) } }) - .collect() + .collect()) } else if let Ok(cast) = cast_array(column, &DataType::Float64) { // Numeric column - use shared format_number helper for clean integer formatting use crate::plot::format_number; let f64_col = as_f64(&cast).map_err(|e| format!("Failed to cast column to f64: {}", e))?; - (0..f64_col.len()) + Ok((0..f64_col.len()) .map(|i| { if f64_col.is_null(i) { None @@ -331,29 +355,74 @@ pub fn format_dataframe_column( Some(format_number(f64_col.value(i))) } }) - .collect() + .collect()) } else { - return Err(format!( - "Formatting doesn't support type {:?} in column '{}'. Try string or numeric types instead.", - column.data_type(), - column_name - )); - }; + Err(format!( + "Formatting doesn't support type {:?}", + column.data_type() + )) + } +} - // Step 2: Apply formatting template to all string values - let placeholders = parse_placeholders(template); - let formatted_owned: Vec> = string_values - .into_iter() - .map(|opt| opt.map(|s| format_value(&s, template, &placeholders, None))) - .collect(); +/// Look up `key` in `mapping`. `Some(_)` (possibly empty, for an explicit +/// `=> NULL` suppression) means the mapping resolves this key outright; +/// `None` means there is no entry for it at all. +fn resolve_override( + mapping: &Option>>, + key: &str, +) -> Option { + match mapping.as_ref().and_then(|m| m.get(key)) { + Some(Some(display)) => Some(display.clone()), + Some(None) => Some(String::new()), + None => None, + } +} - let formatted_refs: Vec> = - formatted_owned.iter().map(|opt| opt.as_deref()).collect(); - let formatted_col = new_str_array(formatted_refs); +/// Resolve one display value per row of a column, for `TABULATE FORMAT`'s +/// `RENAMING` clause: an explicit `mapping` entry wins outright (including +/// suppressing a cell to blank text via `=> NULL`); every other value gets +/// `template` applied, except a null cell with no explicit `"null"` entry, +/// which is left unresolved. +/// +/// `Ok(None)` at a given row means "no resolution, use the caller's own +/// default content for that row"; `Ok(Some(s))` (possibly empty) is the +/// resolved display text. +pub fn resolve_column_values( + column: &ArrayRef, + template: &str, + mapping: &Option>>, +) -> Result>, String> { + let string_values = column_to_strings(column)?; - // Replace column in DataFrame - df.with_column(column_name, formatted_col) - .map_err(|e| format!("Failed to replace column: {}", e)) + let placeholders = parse_placeholders(template); + let has_plain = placeholders + .iter() + .any(|p| matches!(p.placeholder, Placeholder::Plain)) + || placeholders.is_empty(); + let numeric_precision = if has_plain { + let numbers: Vec = string_values + .iter() + .filter_map(|v| v.as_deref().and_then(|s| s.parse::().ok())) + .collect(); + compute_numeric_precision(&numbers) + } else { + None + }; + + Ok(string_values + .into_iter() + .map(|value| match value { + None => resolve_override(mapping, "null"), + Some(raw) => resolve_override(mapping, &raw).or_else(|| { + Some(format_value( + &raw, + template, + &placeholders, + numeric_precision, + )) + }), + }) + .collect()) } /// Format a single value using template and parsed placeholders @@ -638,4 +707,59 @@ mod tests { assert_eq!(result.get("0.05"), Some(&Some("0.05".to_string()))); assert_eq!(result.get("0.1"), Some(&Some("0.10".to_string()))); } + + #[test] + fn resolve_column_values_applies_template_to_unmapped_values() { + use crate::array_util::new_str_array; + + let column = new_str_array(vec![Some("a"), Some("b")]); + let result = resolve_column_values(&column, "[{}]", &None).unwrap(); + + assert_eq!( + result, + vec![Some("[a]".to_string()), Some("[b]".to_string())] + ); + } + + #[test] + fn resolve_column_values_explicit_mapping_wins_over_template() { + use crate::array_util::new_f64_array; + + let column = new_f64_array(vec![Some(0.0), Some(5.0)]); + let mapping = Some(HashMap::from([("0".to_string(), Some("-".to_string()))])); + let result = resolve_column_values(&column, "{:num %.2f}", &mapping).unwrap(); + + assert_eq!( + result, + vec![Some("-".to_string()), Some("5.00".to_string())] + ); + } + + #[test] + fn resolve_column_values_null_with_explicit_entry_is_resolved() { + use crate::array_util::new_f64_array; + + let column = new_f64_array(vec![None, Some(1.0)]); + let mapping = Some(HashMap::from([("null".to_string(), None)])); // => NULL suppresses + + let result = resolve_column_values(&column, "{:num %.0f}", &mapping).unwrap(); + + assert_eq!(result[0], Some(String::new())); // suppressed, not templated + assert_eq!(result[1], Some("1".to_string())); + } + + #[test] + fn resolve_column_values_null_without_an_explicit_entry_is_left_unresolved() { + use crate::array_util::new_f64_array; + + let column = new_f64_array(vec![None, Some(1.0)]); + + let result = resolve_column_values(&column, "{:num %.2f}", &None).unwrap(); + + // Caller falls back to its own default rendering for this row, the + // same way an unmapped null break is left untouched by + // apply_label_template. + assert_eq!(result[0], None); + assert_eq!(result[1], Some("1.00".to_string())); + } } From a8d703137242f678c87e5f60f8fb61f3c6dfdfb6 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 18 Sep 2026 16:11:58 +0200 Subject: [PATCH 4/7] Resolve FORMAT's hjust SETTING and render it in HtmlWriter Validates FORMAT's SETTING against a real FORMAT_PARAMS list (previously accepted-and-ignored): hjust takes "left"/"right"/"center"/"centre" or a 0-1 number, via a new ParamConstraint::string_option_or_number shared with any future param needing the same keyword-or-continuous shape. TableColumn and TableCell gain a `properties: Parameters` field. resolve_column_properties resolves each column's hjust exactly once, standardised to a number (left/center/right = 0.0/0.5/1.0; an explicit number passes through; an absent setting defaults from the column's Arrow dtype, numeric right, else left) so every writer buckets one representation instead of re-implementing keyword handling. Column-label cells inherit their column's properties, matching gt's own convention. HtmlWriter reads a cell's resolved hjust and renders it as an inline `style` attribute (text-align, plus font-variant-numeric: tabular-nums when right-aligned) through one translation function, so a future property is one more mapper rather than a change to render_cell itself. The numeric bucketing (0.25/0.75 thresholds) matches VegaLiteWriter's own convert_hjust, so hjust means the same alignment in every writer. Since dtype-derived defaults apply to every TABULATE output regardless of FORMAT, several pre-existing HtmlWriter/CLI/Jupyter tests asserting bare ``/`` strings needed updating; loosened them (and the new cell tests) to check tag content and alignment keywords rather than exact attribute strings, so future properties don't compound into unreadable literals. Co-Authored-By: Claude Sonnet 5 --- ggsql-cli/src/writers.rs | 4 +- ggsql-jupyter/src/executor.rs | 2 +- src/execute/table.rs | 198 ++++++++++++++++++++++++---------- src/execute/table_format.rs | 194 +++++++++++++++++++++++++-------- src/execute/table_spanner.rs | 33 +++--- src/plot/types.rs | 15 +++ src/table/mod.rs | 81 +++++++++++++- src/validate.rs | 10 ++ src/writer/html.rs | 130 +++++++++++++++++----- 9 files changed, 521 insertions(+), 146 deletions(-) diff --git a/ggsql-cli/src/writers.rs b/ggsql-cli/src/writers.rs index 552c610e..b7a30f45 100644 --- a/ggsql-cli/src/writers.rs +++ b/ggsql-cli/src/writers.rs @@ -768,8 +768,8 @@ mod tests { panic!("expected Output::Text from the html writer"); }; assert!(html.starts_with(""), "{html}"); - assert!(html.contains(""), "{html}"); - assert!(html.contains(""), "{html}"); + assert!(html.contains(">id"), "{html}"); + assert!(html.contains(">a"), "{html}"); } #[cfg(all(feature = "html", feature = "duckdb"))] diff --git a/ggsql-jupyter/src/executor.rs b/ggsql-jupyter/src/executor.rs index 613680e2..bc0e6c3b 100644 --- a/ggsql-jupyter/src/executor.rs +++ b/ggsql-jupyter/src/executor.rs @@ -344,7 +344,7 @@ mod tests { match result { ExecutionResult::Table { html } => { assert!(html.contains("
ida
")); - assert!(html.contains("")); + assert!(html.contains(">x")); } other => panic!("expected Table, got {other:?}"), } diff --git a/src/execute/table.rs b/src/execute/table.rs index 15ee723a..eef6d9a0 100644 --- a/src/execute/table.rs +++ b/src/execute/table.rs @@ -12,14 +12,16 @@ //! exception — it needs no `DataFrame`, so it lives on `Table` itself, //! reachable from `validate()` too. -use super::table_format::apply_formats; +use std::collections::HashMap; + +use super::table_format::{apply_formats, reshape_formats, resolve_column_properties}; use super::table_spanner::{create_spanners, reorder_table_columns}; use crate::array_util::value_to_string; use crate::parser::{self, SourceTree}; -use crate::plot::Labels; +use crate::plot::{Labels, Parameters}; use crate::reader::{Reader, ResolvedTable}; use crate::validate::{validate, ValidationWarning}; -use crate::{DataFrame, GgsqlError, Result, Spec, Table}; +use crate::{DataFrame, Format, GgsqlError, Result, Spec, Table}; /// Resolve a TABULATE query into a `ResolvedTable`. /// @@ -83,10 +85,21 @@ fn build_cells(df: &DataFrame, table: &Table) -> Result> { .resolve_spanner_ids() .map_err(GgsqlError::ValidationError)?; - let df = apply_formats(df, &table.formats)?; + for (idx, format) in table.formats.iter().enumerate() { + format + .validate_settings() + .map_err(|e| GgsqlError::ValidationError(format!("FORMAT {}: {}", idx + 1, e)))?; + } + + // `table.formats` reshaped to one `Format` per column — the shape both + // create_table_columns (SETTING) and apply_formats (RENAMING) read from. + let formats = reshape_formats(df, &table.formats)?; - let columns = create_table_columns(&df, &table.labels); + let columns = create_table_columns(df, &table.labels, &formats); let columns = reorder_table_columns(columns, &spans)?; + + let df = apply_formats(df, &formats)?; + let spanners = create_spanners(&columns, &spans)?; let column_labels = create_column_labels(&columns); let header = compose_header(spanners, column_labels); @@ -97,29 +110,34 @@ fn build_cells(df: &DataFrame, table: &Table) -> Result> { Ok(cells) } -/// One column's identity within a table layout: its source name and resolved -/// display label. `create_table_columns` is the one place `Labels` gets consulted — -/// `create_column_labels` and `create_body` both work off `name`/`label` -/// directly instead of asking `Labels` again, and both follow `columns`' -/// order rather than `df`'s raw column order, so a future spanner-driven -/// reordering of this list carries through to cell positions automatically. -/// -/// `dtype: DataType` is expected to join this once column alignment is -/// tackled — left out for now since nothing would read it yet, and an unread -/// struct field is a dead-code warning, not just an early add. +/// One column's identity within a table layout: its source name and +/// resolved display label/properties. `create_table_columns` is the one +/// place `Labels` and `FORMAT`'s `SETTING` get consulted — `create_column_labels` +/// and `create_body` both work off this resolved data directly instead of +/// asking again, and both follow `columns`' order rather than `df`'s raw +/// column order, so a future spanner-driven reordering of this list carries +/// through to cell positions automatically. pub(crate) struct TableColumn { /// The column's name in the resolved `DataFrame` — used to look its /// values up in `create_body`, independent of display order. pub(crate) name: String, /// The resolved `ColumnLabel` cell content for this column. pub(crate) label: String, + /// Resolved `SETTING` properties for this column's cells (e.g. `hjust`), + /// carried onto every `ColumnLabel`/`Body` cell in this column. + pub(crate) properties: Parameters, } /// Build one `TableColumn` per `DataFrame` column, in the `DataFrame`'s own /// order — `reorder_table_columns` is what may reorder this list -/// afterward, not this function. `labels` (from a `TABULATE LABEL` clause) -/// is the one authority for a column's label. -fn create_table_columns(df: &DataFrame, labels: &Labels) -> Vec { +/// afterward, not this function. `labels` is the one authority for a +/// column's label; `formats` (already reshaped to one `Format` per column +/// by `reshape_formats`) is the one authority for its properties. +fn create_table_columns( + df: &DataFrame, + labels: &Labels, + formats: &HashMap, +) -> Vec { df.get_column_names() .into_iter() .map(|name| { @@ -128,7 +146,18 @@ fn create_table_columns(df: &DataFrame, labels: &Labels) -> Vec { Some(None) => String::new(), Some(Some(label)) => label.clone(), }; - TableColumn { name, label } + // Not stored on TableColumn: nothing needs it once `properties` + // (which may default from it) is resolved. + let dtype = df + .column(&name) + .expect("name comes from df's own columns") + .data_type(); + let properties = resolve_column_properties(dtype, formats.get(&name)); + TableColumn { + name, + label, + properties, + } }) .collect() } @@ -151,13 +180,16 @@ 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: column.label.clone(), + .map(|(index, column)| { + TableCell::new( + TableCellKind::ColumnLabel, + 0, + 0, + index, + index, + column.label.clone(), + ) + .with_properties(column.properties.clone()) }) .collect() } @@ -179,14 +211,17 @@ fn create_body(df: &DataFrame, columns: &[TableColumn]) -> Vec { .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(array, row), - }); + cells.push( + TableCell::new( + TableCellKind::Body, + row, + row, + index, + index, + value_to_string(array, row), + ) + .with_properties(column.properties.clone()), + ); } } @@ -360,8 +395,7 @@ impl std::fmt::Display for TableCellKind { /// 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. +/// directly. #[derive(Debug, Clone)] pub struct TableCell { /// What role this cell plays (column label, body, ...). @@ -376,9 +410,44 @@ pub struct TableCell { pub right: usize, /// The cell's text content. pub content: String, + /// Display properties for this cell (e.g. `hjust`), resolved from its + /// column's `FORMAT` `SETTING`. `ColumnLabel` and `Body` cells inherit + /// their column's properties; a `Spanner` cell covers several columns + /// at once, so it has none of its own. + pub properties: Parameters, } impl TableCell { + /// Build a cell with no display properties — the common case for a + /// `Spanner` or filler cell, which covers several columns rather than + /// resolving from one. A `ColumnLabel`/`Body` cell should follow this + /// with `with_properties` instead of leaving the default. + pub(crate) fn new( + kind: TableCellKind, + top: usize, + bottom: usize, + left: usize, + right: usize, + content: String, + ) -> Self { + Self { + kind, + top, + bottom, + left, + right, + content, + properties: Parameters::new(), + } + } + + /// Set this cell's display properties, e.g. to a column's resolved + /// `FORMAT` `SETTING`. + pub(crate) fn with_properties(mut self, properties: Parameters) -> Self { + self.properties = properties; + self + } + /// 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) { @@ -428,6 +497,7 @@ mod layout_tests { TableColumn { name: name.to_string(), label: label.to_string(), + properties: Parameters::new(), } } @@ -447,7 +517,7 @@ mod layout_tests { labels.labels.insert("name".to_string(), None); // "extra" has no entry at all: no LABEL clause mentioned it. - let columns = create_table_columns(&frame, &labels); + let columns = create_table_columns(&frame, &labels, &HashMap::new()); assert_eq!(columns[0].name, "id"); assert_eq!(columns[0].label, "ID"); // overridden @@ -502,7 +572,7 @@ mod layout_tests { "name" => vec!["a".to_string(), "b".to_string()], } .unwrap(); - let columns = create_table_columns(&frame, &Labels::default()); + let columns = create_table_columns(&frame, &Labels::default(), &HashMap::new()); let body = create_body(&frame, &columns); @@ -547,14 +617,7 @@ mod layout_tests { } fn cell(kind: TableCellKind, top: usize, bottom: usize, content: &str) -> TableCell { - TableCell { - kind, - top, - bottom, - left: 0, - right: 0, - content: content.to_string(), - } + TableCell::new(kind, top, bottom, 0, 0, content.to_string()) } #[test] @@ -704,14 +767,7 @@ mod layout_tests { left: usize, right: usize, ) -> TableCell { - TableCell { - kind, - top, - bottom, - left, - right, - content: String::new(), - } + TableCell::new(kind, top, bottom, left, right, String::new()) } #[test] @@ -820,6 +876,40 @@ mod layout_tests { assert_eq!(body_cells[1].content, "$5.00"); } + #[test] + fn build_cells_defaults_hjust_by_dtype_on_label_and_body_cells() { + let frame = df! { + "price" => vec![1.0f64], + "name" => vec!["a".to_string()], + } + .unwrap(); + let table = Table::new(); + + let cells = build_cells(&frame, &table).unwrap(); + + let hjust = |left: usize, kind: TableCellKind| { + cells + .iter() + .find(|c| c.left == left && c.kind == kind) + .unwrap() + .properties + .get("hjust") + .cloned() + }; + assert_eq!( + hjust(0, TableCellKind::ColumnLabel), + Some(ParameterValue::Number(1.0)) + ); + assert_eq!( + hjust(0, TableCellKind::Body), + Some(ParameterValue::Number(1.0)) + ); + assert_eq!( + hjust(1, TableCellKind::Body), + Some(ParameterValue::Number(0.0)) + ); + } + #[test] fn build_cells_reorders_columns_for_a_spanner_before_building_labels_and_body() { let frame = df! { diff --git a/src/execute/table_format.rs b/src/execute/table_format.rs index e7eca3b0..1c905b02 100644 --- a/src/execute/table_format.rs +++ b/src/execute/table_format.rs @@ -1,20 +1,26 @@ -//! `TABULATE FORMAT` resolution: replacing each `FORMAT`-covered column in a -//! table's `DataFrame` with its resolved display text, called from -//! `table::build_cells` before column/cell layout runs — so `create_body`'s -//! ordinary `value_to_string` rendering already reflects any `RENAMING`, and -//! never needs to know `FORMAT` exists. +//! `TABULATE FORMAT` resolution: reshaping `Table.formats` (one entry per +//! `FORMAT` clause, each naming several columns) into one `Format` per +//! column (`reshape_formats`), then applying that per column — `RENAMING` +//! by replacing the column's values in the `DataFrame` (`apply_formats`), +//! `SETTING` by resolving display properties for `TableColumn` +//! (`resolve_column_properties`). Called from `table::build_cells`. use std::collections::HashMap; +use arrow::datatypes::DataType; + use crate::array_util::{new_str_array, value_to_string}; +use crate::plot::{ParameterValue, Parameters}; use crate::{DataFrame, Format, GgsqlError, Result}; -/// Replace every `FORMAT`-covered column in `df` with its resolved display -/// text. A later `FORMAT` clause wins over an earlier one naming the same -/// column. A column whose `FORMAT` has no `RENAMING` at all (`value_template` -/// is the default `"{}"` and `value_mapping` is empty) is left untouched. -pub(crate) fn apply_formats(df: &DataFrame, formats: &[Format]) -> Result { - let mut by_column: HashMap<&str, &Format> = HashMap::new(); +/// Reshape `formats` (one `FORMAT` clause per entry, each naming several +/// columns) into one `Format` per column it covers — a later `FORMAT` +/// clause wins over an earlier one naming the same column. +pub(crate) fn reshape_formats( + df: &DataFrame, + formats: &[Format], +) -> Result> { + let mut resolved = HashMap::new(); for format in formats { for name in &format.columns { if df.column(name).is_err() { @@ -22,12 +28,25 @@ pub(crate) fn apply_formats(df: &DataFrame, formats: &[Format]) -> Result, +) -> Result { let mut df = df.clone(); - for (name, format) in by_column { + for (name, format) in formats { let has_renaming = format.value_template != "{}" || format.value_mapping.as_ref().is_some_and(|m| !m.is_empty()); if !has_renaming { @@ -38,7 +57,7 @@ pub(crate) fn apply_formats(df: &DataFrame, formats: &[Format]) -> Result Result) -> Parameters { + let mut properties = format.map(|f| f.settings.clone()).unwrap_or_default(); + + let hjust = match properties.get("hjust") { + Some(ParameterValue::String(s)) if s == "left" => 0.0, + Some(ParameterValue::String(s)) if s == "right" => 1.0, + Some(ParameterValue::String(_)) => 0.5, // "center" or "centre" + Some(ParameterValue::Number(n)) => *n, + _ => { + if dtype.is_numeric() { + 1.0 + } else { + 0.0 + } + } + }; + properties.insert("hjust".to_string(), ParameterValue::Number(hjust)); + + properties +} + #[cfg(test)] mod tests { use super::*; use crate::array_util::value_to_string; use crate::df; - fn format_with( - columns: &[&str], - template: &str, - mapping: Option>>, - ) -> Format { + fn format_with(template: &str, mapping: Option>>) -> Format { Format { - columns: columns.iter().map(|s| s.to_string()).collect(), - settings: crate::plot::Parameters::new(), + columns: Vec::new(), + settings: Parameters::new(), value_mapping: mapping, value_template: template.to_string(), } @@ -86,10 +129,44 @@ mod tests { .collect() } + #[test] + fn reshape_formats_lets_a_later_clause_win_and_clears_columns() { + let frame = df! { "price" => vec![1.0f64] }.unwrap(); + let formats = vec![ + Format { + columns: vec!["price".to_string()], + ..format_with("{:num %.0f}", None) + }, + Format { + columns: vec!["price".to_string()], + ..format_with("{:num %.2f}", None) + }, + ]; + + let resolved = reshape_formats(&frame, &formats).unwrap(); + + let price = resolved.get("price").unwrap(); + assert_eq!(price.value_template, "{:num %.2f}"); + assert!(price.columns.is_empty()); + } + + #[test] + fn reshape_formats_errors_on_an_unknown_column() { + let frame = df! { "price" => vec![1.0f64] }.unwrap(); + let formats = vec![Format { + columns: vec!["typo".to_string()], + ..format_with("{}", None) + }]; + + let error = reshape_formats(&frame, &formats).unwrap_err(); + assert!(matches!(error, GgsqlError::ValidationError(msg) + if msg.contains("FORMAT references unknown column 'typo'"))); + } + #[test] fn leaves_a_column_untouched_when_its_format_has_no_renaming() { let frame = df! { "price" => vec![1.0f64, 2.0] }.unwrap(); - let formats = vec![format_with(&["price"], "{}", None)]; + let formats = HashMap::from([("price".to_string(), format_with("{}", None))]); let result = apply_formats(&frame, &formats).unwrap(); @@ -99,7 +176,7 @@ mod tests { #[test] fn applies_a_wildcard_template() { let frame = df! { "price" => vec![1.0f64, 2.0] }.unwrap(); - let formats = vec![format_with(&["price"], "${:num %.2f}", None)]; + let formats = HashMap::from([("price".to_string(), format_with("${:num %.2f}", None))]); let result = apply_formats(&frame, &formats).unwrap(); @@ -110,7 +187,7 @@ mod tests { fn explicit_mapping_wins_over_the_template() { let frame = df! { "price" => vec![0.0f64, 5.0] }.unwrap(); let mapping = Some(HashMap::from([("0".to_string(), Some("-".to_string()))])); - let formats = vec![format_with(&["price"], "{:num %.2f}", mapping)]; + let formats = HashMap::from([("price".to_string(), format_with("{:num %.2f}", mapping))]); let result = apply_formats(&frame, &formats).unwrap(); @@ -118,35 +195,66 @@ mod tests { } #[test] - fn later_format_wins_for_a_column_named_by_two_formats() { - let frame = df! { "price" => vec![1.0f64] }.unwrap(); - let formats = vec![ - format_with(&["price"], "{:num %.0f}", None), - format_with(&["price"], "{:num %.2f}", None), - ]; + fn a_null_with_no_explicit_entry_still_falls_back_to_value_to_string() { + let frame = df! { "price" => vec![Some(1.0f64), None] }.unwrap(); + let formats = HashMap::from([("price".to_string(), format_with("{:num %.2f}", None))]); let result = apply_formats(&frame, &formats).unwrap(); - assert_eq!(column_content(&result, "price"), vec!["1.00"]); + assert_eq!(column_content(&result, "price"), vec!["1.00", "null"]); } - #[test] - fn errors_on_a_format_naming_an_unknown_column() { - let frame = df! { "price" => vec![1.0f64] }.unwrap(); - let formats = vec![format_with(&["typo"], "{}", None)]; + fn format_with_hjust(hjust: ParameterValue) -> Format { + let mut settings = Parameters::new(); + settings.insert("hjust".to_string(), hjust); + Format { + columns: Vec::new(), + settings, + value_mapping: None, + value_template: "{}".to_string(), + } + } - let error = apply_formats(&frame, &formats).unwrap_err(); - assert!(matches!(error, GgsqlError::ValidationError(msg) - if msg.contains("FORMAT references unknown column 'typo'"))); + #[test] + fn resolve_column_properties_defaults_hjust_by_dtype_when_absent() { + assert_eq!( + resolve_column_properties(&DataType::Float64, None).get("hjust"), + Some(&ParameterValue::Number(1.0)) + ); + assert_eq!( + resolve_column_properties(&DataType::Utf8, None).get("hjust"), + Some(&ParameterValue::Number(0.0)) + ); } #[test] - fn a_null_with_no_explicit_entry_still_falls_back_to_value_to_string() { - let frame = df! { "price" => vec![Some(1.0f64), None] }.unwrap(); - let formats = vec![format_with(&["price"], "{:num %.2f}", None)]; + fn resolve_column_properties_standardises_keywords_to_numbers() { + let format = format_with_hjust(ParameterValue::String("left".to_string())); + assert_eq!( + resolve_column_properties(&DataType::Utf8, Some(&format)).get("hjust"), + Some(&ParameterValue::Number(0.0)) + ); - let result = apply_formats(&frame, &formats).unwrap(); + let format = format_with_hjust(ParameterValue::String("centre".to_string())); + assert_eq!( + resolve_column_properties(&DataType::Utf8, Some(&format)).get("hjust"), + Some(&ParameterValue::Number(0.5)) + ); - assert_eq!(column_content(&result, "price"), vec!["1.00", "null"]); + let format = format_with_hjust(ParameterValue::String("right".to_string())); + assert_eq!( + resolve_column_properties(&DataType::Utf8, Some(&format)).get("hjust"), + Some(&ParameterValue::Number(1.0)) + ); + } + + #[test] + fn resolve_column_properties_keeps_an_explicit_number_unchanged() { + let format = format_with_hjust(ParameterValue::Number(0.25)); + + assert_eq!( + resolve_column_properties(&DataType::Float64, Some(&format)).get("hjust"), + Some(&ParameterValue::Number(0.25)) + ); } } diff --git a/src/execute/table_spanner.rs b/src/execute/table_spanner.rs index 95b0497f..9771927f 100644 --- a/src/execute/table_spanner.rs +++ b/src/execute/table_spanner.rs @@ -203,14 +203,14 @@ pub(crate) fn create_spanners( (true, None) => run_start = Some(index), // Column not in span, end run and push cell (false, Some(start)) => { - cells.push(TableCell { - kind: TableCellKind::Spanner, - top: row, - bottom: row, - left: start, - right: index - 1, - content: label.clone(), - }); + cells.push(TableCell::new( + TableCellKind::Spanner, + row, + row, + start, + index - 1, + label.clone(), + )); run_start = None; } _ => {} @@ -218,14 +218,14 @@ pub(crate) fn create_spanners( } // Started but not ended: last column if let Some(start) = run_start { - cells.push(TableCell { - kind: TableCellKind::Spanner, - top: row, - bottom: row, - left: start, - right: columns.len() - 1, - content: label.clone(), - }); + cells.push(TableCell::new( + TableCellKind::Spanner, + row, + row, + start, + columns.len() - 1, + label.clone(), + )); } } @@ -241,6 +241,7 @@ mod tests { TableColumn { name: name.to_string(), label: label.to_string(), + properties: Parameters::new(), } } diff --git a/src/plot/types.rs b/src/plot/types.rs index e3e98b3f..a8b4be31 100644 --- a/src/plot/types.rs +++ b/src/plot/types.rs @@ -1372,6 +1372,21 @@ impl ParamConstraint { } } + /// String enum or Number within a range - for parameters like `hjust` + /// that accept either a named keyword or a continuous value. + pub const fn string_option_or_number( + values: &'static [&'static str], + num: NumberConstraint, + ) -> Self { + Self { + number: TypeConstraint::Constrained(num), + string: TypeConstraint::Constrained(StringConstraint::one_of(values)), + boolean: TypeConstraint::Forbidden, + array: TypeConstraint::Forbidden, + allow_null: true, + } + } + /// String enum or Array of strings from same enum - for `free` parameter #[allow(dead_code)] pub const fn string_or_string_array(values: &'static [&'static str]) -> Self { diff --git a/src/table/mod.rs b/src/table/mod.rs index 44178b37..3c3d002a 100644 --- a/src/table/mod.rs +++ b/src/table/mod.rs @@ -10,7 +10,8 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; use crate::plot::{ - validate_parameter, DefaultParamValue, Labels, ParamConstraint, ParamDefinition, Parameters, + validate_parameter, DefaultParamValue, Labels, NumberConstraint, ParamConstraint, + ParamDefinition, Parameters, }; use crate::DataSource; @@ -165,14 +166,13 @@ impl Spanner { /// One `FORMAT` clause: cell formatting for a group of columns. /// -/// `settings` is parsed but not yet validated or applied anywhere — `SETTING` -/// semantics for `FORMAT` land in a later change; only the grammar shape is -/// wired up so far. +/// `settings` is validated against `FORMAT_PARAMS` and resolved into each +/// covered column's `TableCell` properties — not yet consumed by any writer. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Format { /// The columns this FORMAT applies to, in the order written. pub columns: Vec, - /// `SETTING` parameters for this FORMAT (e.g. `width => '20%'`). + /// `SETTING` parameters for this FORMAT (e.g. `hjust => 'right'`). pub settings: Parameters, /// Value mappings for custom cell display (`RENAMING` clause). Maps a raw /// cell value to its display text; `None` suppresses the cell's text. @@ -189,6 +189,38 @@ pub struct Format { pub value_template: String, } +/// `SETTING` parameters `FORMAT` accepts. +const FORMAT_PARAMS: &[ParamDefinition] = &[ParamDefinition { + name: "hjust", + default: DefaultParamValue::Number(0.5), + // Both spellings accepted here; resolve_column_properties standardises + // "centre" to "center" when it builds a column's TableCell properties. + constraint: ParamConstraint::string_option_or_number( + &["left", "right", "centre", "center"], + NumberConstraint::range(0.0, 1.0), + ), +}]; + +impl Format { + /// Validate `settings` against `FORMAT_PARAMS`. + pub fn validate_settings(&self) -> Result<(), String> { + let valid: Vec<&str> = FORMAT_PARAMS.iter().map(|p| p.name).collect(); + + for (name, value) in &self.settings { + let Some(param) = FORMAT_PARAMS.iter().find(|p| p.name == name.as_str()) else { + return Err(format!( + "FORMAT setting should be {}, not '{}'", + crate::or_list_quoted(&valid, '\''), + name + )); + }; + validate_parameter(name, value, ¶m.constraint)?; + } + + Ok(()) + } +} + #[cfg(test)] mod tests { use super::*; @@ -221,4 +253,43 @@ mod tests { assert!(spanner_with_settings(settings).validate_settings().is_err()); } + + fn format_with_settings(settings: Parameters) -> Format { + Format { + columns: vec!["a".to_string()], + settings, + value_mapping: None, + value_template: "{}".to_string(), + } + } + + #[test] + fn format_validate_settings_accepts_a_string_hjust() { + let mut settings = Parameters::new(); + settings.insert( + "hjust".to_string(), + ParameterValue::String("right".to_string()), + ); + + assert!(format_with_settings(settings).validate_settings().is_ok()); + } + + #[test] + fn format_validate_settings_accepts_a_numeric_hjust() { + let mut settings = Parameters::new(); + settings.insert("hjust".to_string(), ParameterValue::Number(0.25)); + + assert!(format_with_settings(settings).validate_settings().is_ok()); + } + + #[test] + fn format_validate_settings_rejects_an_unrecognized_string() { + let mut settings = Parameters::new(); + settings.insert( + "hjust".to_string(), + ParameterValue::String("up".to_string()), + ); + + assert!(format_with_settings(settings).validate_settings().is_err()); + } } diff --git a/src/validate.rs b/src/validate.rs index c554d3d5..bf6e241f 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -315,6 +315,16 @@ pub fn validate(query: &str) -> Result { location: None, }); } + + // Validate each FORMAT's SETTING parameters the same way. + for (idx, format) in table.formats.iter().enumerate() { + if let Err(e) = format.validate_settings() { + errors.push(ValidationError { + message: format!("FORMAT {}: {}", idx + 1, e), + location: None, + }); + } + } } Ok(Validated { diff --git a/src/writer/html.rs b/src/writer/html.rs index 1769b960..c47beed1 100644 --- a/src/writer/html.rs +++ b/src/writer/html.rs @@ -1,6 +1,7 @@ //! A minimal HTML table writer. //! -//! Renders a `ResolvedTable`'s cells as a bare `
x
` — no styling, no +//! Renders a `ResolvedTable`'s cells as a bare `
` — inline `style` +//! attributes from each cell's resolved `FORMAT` properties, but no //! footnotes, since `Table` has no fields to describe those yet. Spanner //! rows are rendered (as `colspan`, one `` per level, above the column //! labels); `render_cell`/`render_row` can also render a `rowspan` cell, @@ -15,6 +16,7 @@ use std::collections::BTreeMap; use std::collections::HashMap; use std::collections::HashSet; +use crate::plot::{ParameterValue, Parameters}; use crate::util::escape_html; use crate::writer::{Writer, WriterOptions}; use crate::{DataFrame, GgsqlError, Plot, Result, TableCell}; @@ -118,7 +120,8 @@ impl Writer for HtmlWriter { /// Render one `TableCell` as an HTML tag — `"); } + #[test] + fn render_cell_emits_a_style_attribute_from_properties() { + // The exact declarations a given `hjust` produces are `cell_style`'s + // own tests' job — this just checks render_cell embeds whatever + // `cell_style` returns as a `style="..."` attribute. + let mut c = cell(TableCellKind::Body, 0, 0, 0, 0); + c.properties + .insert("hjust".to_string(), ParameterValue::Number(1.0)); + let style = cell_style(&c.properties).unwrap(); + assert_eq!(render_cell(&c), format!("")); + } + + #[test] + fn text_align_buckets_a_resolved_hjust_number() { + assert_eq!(text_align(None), None); + assert_eq!(text_align(Some(&ParameterValue::Number(0.0))), Some("left")); + assert_eq!(text_align(Some(&ParameterValue::Number(0.1))), Some("left")); + assert_eq!( + text_align(Some(&ParameterValue::Number(0.5))), + Some("center") + ); + assert_eq!( + text_align(Some(&ParameterValue::Number(0.9))), + Some("right") + ); + assert_eq!( + text_align(Some(&ParameterValue::Number(1.0))), + Some("right") + ); + } + + #[test] + fn cell_style_only_adds_tabular_nums_when_right_aligned() { + let mut left = Parameters::new(); + left.insert("hjust".to_string(), ParameterValue::Number(0.0)); + assert_eq!(cell_style(&left), Some("text-align: left".to_string())); + + let mut right = Parameters::new(); + right.insert("hjust".to_string(), ParameterValue::Number(1.0)); + assert_eq!( + cell_style(&right), + Some("text-align: right; font-variant-numeric: tabular-nums".to_string()) + ); + + assert_eq!(cell_style(&Parameters::new()), None); + } + #[test] fn render_row_skips_a_column_occupied_by_a_rowspan_from_above() { let a = cell(TableCellKind::ColumnLabel, 1, 1, 0, 0); @@ -303,10 +374,15 @@ mod tests { let writer = HtmlWriter::new(); let html = writer.render(&spec).unwrap(); + // Precise per-dtype alignment is covered directly by + // resolve_column_properties's/cell_style's own tests — this just + // checks the columns render and escaping works end to end. assert!(html.starts_with("
`/`` from /// `cell.is_header()`, with a `colspan`/`rowspan` attribute only when the -/// cell actually spans more than one column/row. +/// cell actually spans more than one column/row, and a `style` attribute +/// only when `cell.properties` resolves to one. fn render_cell(cell: &TableCell) -> String { let tag = if cell.is_header() { "th" } else { "td" }; let colspan = cell.width(); @@ -130,9 +133,44 @@ fn render_cell(cell: &TableCell) -> String { if rowspan > 1 { attrs.push_str(&format!(" rowspan=\"{rowspan}\"")); } + if let Some(style) = cell_style(&cell.properties) { + attrs.push_str(&format!(" style=\"{style}\"")); + } format!("<{tag}{attrs}>{}", escape_html(&cell.content)) } +/// Translate a cell's resolved `FORMAT` properties into a `style` attribute +/// value, or `None` if none of them produce a CSS declaration. +fn cell_style(properties: &Parameters) -> Option { + let mut declarations = Vec::new(); + + if let Some(align) = text_align(properties.get("hjust")) { + declarations.push(format!("text-align: {align}")); + // Right-aligned data reads as numeric — keep digit widths uniform + // so they still line up under one another. + if align == "right" { + declarations.push("font-variant-numeric: tabular-nums".to_string()); + } + } + + (!declarations.is_empty()).then(|| declarations.join("; ")) +} + +/// Map a resolved `hjust` number to a CSS `text-align` keyword, bucketed +/// with the same `0.25`/`0.75` thresholds `VegaLiteWriter`'s `convert_hjust` +/// uses for its own `align` conversion, so `hjust` means the same alignment +/// in both writers. `resolve_column_properties` already standardises every +/// `hjust` (however the user wrote it) to a number before it reaches a +/// `TableCell`, so a non-`Number` here is unreachable in practice. +fn text_align(hjust: Option<&ParameterValue>) -> Option<&'static str> { + match hjust? { + ParameterValue::Number(n) if *n <= 0.25 => Some("left"), + ParameterValue::Number(n) if *n >= 0.75 => Some("right"), + ParameterValue::Number(_) => Some("center"), + _ => None, + } +} + /// Per row, the column positions already covered by a cell that started in /// an earlier row and hasn't ended yet (a rowspan declared above that row) — /// those get no cell and no filler when the row is rendered, since the @@ -184,14 +222,7 @@ fn render_row(mut cells: Vec<&TableCell>, ncol: usize, occupied: &HashSet // by another spanner's occupancy) renders through `render_cell` // too, so one place decides which tag a `TableCellKind` gets, // not two. - let filler = TableCell { - kind, - top: cells[0].top, - bottom: cells[0].top, - left: col, - right: col, - content: String::new(), - }; + let filler = TableCell::new(kind, cells[0].top, cells[0].top, col, col, String::new()); html.push_str(&render_cell(&filler)); col += 1; } @@ -213,14 +244,7 @@ mod render_tests { left: usize, right: usize, ) -> TableCell { - TableCell { - kind, - top, - bottom, - left, - right, - content: String::new(), - } + TableCell::new(kind, top, bottom, left, right, String::new()) } #[test] @@ -234,6 +258,53 @@ mod render_tests { assert_eq!(render_cell(&c), "
")); - assert!(html.contains("")); - assert!(html.contains("")); - assert!(html.contains("")); + assert!(html.contains(">id")); + assert!(html.contains(">name")); + assert!(html.contains(">1")); + assert!(html.contains("text-align: right")); // "id"/1, numeric + assert!(html.contains("text-align: left")); // "name", text assert!(html.contains("<b>a</b>")); assert!(!html.contains("a")); } @@ -328,11 +404,15 @@ mod tests { // "amount" has no spanner, so its label stretches up into the // spanner row (rowspan) instead of a blank filler cell there. - assert!(html.contains("")); - assert!(html.contains("")); - assert!(html.contains("")); + // Alignment styling is incidental here (amount/id are numeric) and + // covered precisely by resolve_column_properties's own tests — this + // checks colspan/rowspan/ordering, not exact style content. + assert!(html.contains("")); + assert!(html.contains(">id")); + assert!(html.contains(">name")); // The spanner row renders above the column-label row. - assert!(html.find("Info").unwrap() < html.find("").unwrap()); + assert!(html.find("Info").unwrap() < html.find(">id").unwrap()); } #[test] From f676b2af8e0deb618eaad02ec50b85efe2279747 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 18 Sep 2026 16:51:48 +0200 Subject: [PATCH 5/7] Document TABULATE FORMAT and add its doc-site syntax highlighting Adds a FORMAT section to doc/syntax/clause/tabulate.qmd (clause syntax, SETTING's hjust, RENAMING linked to SCALE's own RENAMING/break-formatting docs rather than duplicated) plus a runnable example combining hjust alignment with null/template RENAMING against ggsql:penguins. doc/ggsql.xml gains a FORMAT keyword and FormatClause highlighting context, mirroring SpanClause, so the doc site's ```ggsql fences highlight it correctly; verified with a full quarto render of the page (after rebuilding and reinstalling the ggsql-jupyter kernel, since the previously-installed one predated the FORMAT grammar). CHANGELOG.md's existing TABULATE bullet list gains a FORMAT entry at the same one-line-per-clause detail level as LABEL/SPAN. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 1 + doc/ggsql.xml | 43 +++++++++++++++++++++++++++ doc/syntax/clause/tabulate.qmd | 53 ++++++++++++++++++++++++++++++++-- 3 files changed, 95 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d79b83f..e910a6bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,7 @@ following clauses: - `LABEL` overrides the display of column labels - `SPAN` groups columns under a shared header cell (spanner). + - `FORMAT` configures how a column's cells are displayed. ### Changed diff --git a/doc/ggsql.xml b/doc/ggsql.xml index 32c07b42..bc78e016 100644 --- a/doc/ggsql.xml +++ b/doc/ggsql.xml @@ -99,6 +99,7 @@ VISUALIZE TABULATE SPAN + FORMAT @@ -665,6 +666,7 @@ + @@ -699,6 +701,7 @@ + @@ -727,6 +730,7 @@ + @@ -751,6 +755,45 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/doc/syntax/clause/tabulate.qmd b/doc/syntax/clause/tabulate.qmd index 3c6c3bad..d4350dd4 100644 --- a/doc/syntax/clause/tabulate.qmd +++ b/doc/syntax/clause/tabulate.qmd @@ -20,9 +20,12 @@ TABULATE FROM LABEL => , ... SPAN ACROSS , ... SETTING => , ... + FORMAT , ... + SETTING => , ... + RENAMING => , ... ``` -`LABEL` and `SPAN` may each appear more than once, in any order, after the source. +`LABEL`, `SPAN` and `FORMAT` may each appear more than once, in any order, after the source. ### `FROM` ```ggsql @@ -83,6 +86,41 @@ TABULATE FROM sales `H1` ends up covering `jan, feb, mar, apr, may, jun`. Only an `id` declared by an *earlier* `SPAN` clause is recognised — referencing one declared later, or a typo, is treated as an unknown column, the same error as any other bad `ACROSS` entry. Every `id` must be unique across the whole query, and can't collide with an actual column name. +### `FORMAT` +```ggsql +FORMAT , ... + SETTING => , ... + RENAMING => , ... +``` + +Configures cell display for one or more columns. Give several `FORMAT` clauses for different column groups; if two `FORMAT` clauses name the same column, the later one wins for it. + +```ggsql +TABULATE FROM sales + FORMAT price SETTING hjust => 'right' +``` + +#### `SETTING` +```ggsql +SETTING => , ... +``` + +* `hjust` (default: right for numeric columns, left otherwise): horizontal alignment for the column's cells. One of `'left'`, `'right'`, `'center'`/`'centre'`, or a number between 0 and 1 (0 is left, 1 is right, 0.5 is centered). + +#### `RENAMING` +```ggsql +RENAMING => , ... +``` + +Works the same way as [`SCALE`'s `RENAMING`](scale.qmd#renaming), just applied to the columns' own cell values instead of a scale's breaks: give a value on the left and its display text on the right, or `* => '...'` to format every value with [a template](scale.qmd#break-formatting). `NULL` on the right suppresses a cell's text. + +`null` (unquoted) matches an actual missing value, which makes `RENAMING` a convenient way to blank out missing data or zeroes: + +```ggsql +TABULATE FROM sales + FORMAT price RENAMING null => '-', 0 => '-' +``` + ### Examples ```{ggsql} @@ -98,7 +136,18 @@ SELECT * FROM ggsql:penguins LIMIT 5 TABULATE SPAN 'Bill' ACROSS bill_len, bill_dep - LABEL + LABEL bill_len => 'Length', bill_dep => 'Depth' ``` + + + +```{ggsql} +SELECT * FROM ggsql:penguins LIMIT 5 + +TABULATE + FORMAT body_mass + SETTING hjust => 'right' + RENAMING null => '-', * => '{:num %.0f}g' +``` From b84885046ea4572ef80306556c51897c2ed8847e Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 21 Sep 2026 11:22:18 +0200 Subject: [PATCH 6/7] Plumb per-column and per-row properties through ResolvedTable/Writer Table writers now receive resolved TableColumn/TableRow slices alongside cells, so a future whole-column setting like FORMAT's width can be read once per column instead of duplicated across every cell. Also drops ResolvedTable's vestigial table: Table field, made dead once cells/columns absorbed everything it used to carry. Co-Authored-By: Claude Sonnet 5 --- ggsql-jupyter/src/executor.rs | 3 +- src/doc/API.md | 2 +- src/execute/mod.rs | 2 +- src/execute/table.rs | 116 +++++++++++++++++++++++----------- src/execute/table_format.rs | 42 ++++++------ src/lib.rs | 2 +- src/reader/mod.rs | 23 ++++--- src/reader/spec.rs | 28 +++++--- src/writer/html.rs | 17 +++-- src/writer/mod.rs | 21 ++++-- 10 files changed, 170 insertions(+), 86 deletions(-) diff --git a/ggsql-jupyter/src/executor.rs b/ggsql-jupyter/src/executor.rs index bc0e6c3b..9d93c424 100644 --- a/ggsql-jupyter/src/executor.rs +++ b/ggsql-jupyter/src/executor.rs @@ -301,7 +301,8 @@ impl QueryExecutor { tracing::warn!("{}", warning.message); } - let html = HtmlWriter::new().write_table(table.cells())?; + let html = + HtmlWriter::new().write_table(table.cells(), table.columns(), table.rows())?; 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 83397de3..dfbc8b91 100644 --- a/src/doc/API.md +++ b/src/doc/API.md @@ -25,7 +25,7 @@ This document provides a comprehensive reference for the ggsql public API. fn execute(&self, query: &str) -> Result ``` -Execute a ggsql query for visualization or tabulation. This is the main entry point - a required method on the Reader trait. `ResolvedSpec` is `Plot(Box)` or `Table(ResolvedTable)`, depending on whether the query used `VISUALISE` or `TABULATE`; `.as_plot()` / `.as_table()` (or the consuming `.into_plot()` / `.into_table()`) narrow it. +Execute a ggsql query for visualization or tabulation. This is the main entry point - a required method on the Reader trait. `ResolvedSpec` is `Plot(Box)` or `Table(Box)`, depending on whether the query used `VISUALISE` or `TABULATE`; `.as_plot()` / `.as_table()` (or the consuming `.into_plot()` / `.into_table()`) narrow it. **What happens during execution:** diff --git a/src/execute/mod.rs b/src/execute/mod.rs index eea72eae..af57f8e5 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -28,7 +28,7 @@ mod table_spanner; pub use casting::TypeRequirement; pub use cte::CteDefinition; pub use schema::TypeInfo; -pub use table::{resolve_table_with_reader, TableCell, TableCellKind}; +pub use table::{resolve_table_with_reader, TableCell, TableCellKind, TableColumn, TableRow}; use crate::naming; use crate::parser; diff --git a/src/execute/table.rs b/src/execute/table.rs index eef6d9a0..35a7d009 100644 --- a/src/execute/table.rs +++ b/src/execute/table.rs @@ -14,14 +14,14 @@ use std::collections::HashMap; -use super::table_format::{apply_formats, reshape_formats, resolve_column_properties}; +use super::table_format::{apply_formats, resolve_column_properties, setup_formats}; use super::table_spanner::{create_spanners, reorder_table_columns}; use crate::array_util::value_to_string; use crate::parser::{self, SourceTree}; use crate::plot::{Labels, Parameters}; use crate::reader::{Reader, ResolvedTable}; use crate::validate::{validate, ValidationWarning}; -use crate::{DataFrame, Format, GgsqlError, Result, Spec, Table}; +use crate::{DataFrame, Format, GgsqlError, Result, Spanner, Spec, Table}; /// Resolve a TABULATE query into a `ResolvedTable`. /// @@ -65,17 +65,34 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result Result> { +/// Resolve a table's columns: validated SPAN settings, spanner ids +/// expanded, one `TableColumn` per `DataFrame` column (labelled, with +/// `formats`' resolved `SETTING` properties), reordered for any +/// `gather`-ing spanner. Also returns `spans` alongside `columns` — already +/// resolved here, and still needed by `build_cells` for the spanner cells +/// themselves, so recomputing it there would just repeat this work. +fn setup_columns( + df: &DataFrame, + table: &Table, + formats: &HashMap, +) -> Result<(Vec, Vec)> { for (idx, spanner) in table.spans.iter().enumerate() { spanner .validate_settings() @@ -85,25 +102,27 @@ fn build_cells(df: &DataFrame, table: &Table) -> Result> { .resolve_spanner_ids() .map_err(GgsqlError::ValidationError)?; - for (idx, format) in table.formats.iter().enumerate() { - format - .validate_settings() - .map_err(|e| GgsqlError::ValidationError(format!("FORMAT {}: {}", idx + 1, e)))?; - } - - // `table.formats` reshaped to one `Format` per column — the shape both - // create_table_columns (SETTING) and apply_formats (RENAMING) read from. - let formats = reshape_formats(df, &table.formats)?; - - let columns = create_table_columns(df, &table.labels, &formats); + let columns = create_table_columns(df, &table.labels, formats); let columns = reorder_table_columns(columns, &spans)?; - let df = apply_formats(df, &formats)?; + Ok((columns, spans)) +} - let spanners = create_spanners(&columns, &spans)?; - let column_labels = create_column_labels(&columns); +/// Build the resolved cell layout for a table from its already-resolved +/// `columns` and already-`FORMAT`-applied `df` — spanner rows, column +/// labels, and body, composed together and checked for overlaps. Split out +/// from `resolve_table_with_reader` so the layout pipeline can be tested +/// directly against a `df!()`-built `DataFrame` and a `Table`, without +/// needing a `Reader`/real SQL execution. +fn build_cells( + df: &DataFrame, + columns: &[TableColumn], + spans: &[Spanner], +) -> Result> { + let spanners = create_spanners(columns, spans)?; + let column_labels = create_column_labels(columns); let header = compose_header(spanners, column_labels); - let table_body = create_body(&df, &columns); + let table_body = create_body(df, columns); let cells = rowbind_cells(header, table_body); validate_overlaps(&cells)?; @@ -117,22 +136,35 @@ fn build_cells(df: &DataFrame, table: &Table) -> Result> { /// asking again, and both follow `columns`' order rather than `df`'s raw /// column order, so a future spanner-driven reordering of this list carries /// through to cell positions automatically. -pub(crate) struct TableColumn { +#[derive(Debug, Clone)] +pub struct TableColumn { /// The column's name in the resolved `DataFrame` — used to look its /// values up in `create_body`, independent of display order. - pub(crate) name: String, + pub name: String, /// The resolved `ColumnLabel` cell content for this column. - pub(crate) label: String, + pub label: String, /// Resolved `SETTING` properties for this column's cells (e.g. `hjust`), - /// carried onto every `ColumnLabel`/`Body` cell in this column. - pub(crate) properties: Parameters, + /// carried onto every `ColumnLabel`/`Body` cell in this column. A + /// writer wanting a whole-column property (e.g. `width`) reads it here + /// instead of the same value repeated across the column's cells. + pub properties: Parameters, +} + +/// One row's resolved properties within a table layout. No row-wide +/// `TABULATE` clause exists yet to populate anything beyond `properties` +/// staying empty — the type exists so `ResolvedTable`/`Writer::write_table` +/// have a row-wide counterpart to `TableColumn` ready before one is needed. +#[derive(Debug, Clone, Default)] +pub struct TableRow { + /// Resolved properties for this row's cells. + pub properties: Parameters, } /// Build one `TableColumn` per `DataFrame` column, in the `DataFrame`'s own /// order — `reorder_table_columns` is what may reorder this list /// afterward, not this function. `labels` is the one authority for a /// column's label; `formats` (already reshaped to one `Format` per column -/// by `reshape_formats`) is the one authority for its properties. +/// by `setup_formats`) is the one authority for its properties. fn create_table_columns( df: &DataFrame, labels: &Labels, @@ -493,6 +525,16 @@ mod layout_tests { use crate::plot::{ParameterValue, Parameters}; use crate::Spanner; + /// Runs the same steps `resolve_table_with_reader` does, minus the + /// `Reader`/SQL execution — lets a test build a `Table`'s resolved cells + /// directly against a `df!()`-built `DataFrame`. + fn resolve_cells(df: &DataFrame, table: &Table) -> Result> { + let formats = setup_formats(df, &table.formats)?; + let (columns, spans) = setup_columns(df, table, &formats)?; + let df = apply_formats(df, &formats)?; + build_cells(&df, &columns, &spans) + } + fn column(name: &str, label: &str) -> TableColumn { TableColumn { name: name.to_string(), @@ -827,7 +869,7 @@ mod layout_tests { let mut table = Table::new(); table.spans = vec![labeled_spanner(&["a", "b"], "G")]; - let cells = build_cells(&frame, &table).unwrap(); + let cells = resolve_cells(&frame, &table).unwrap(); let spanner_cell = cells .iter() @@ -865,7 +907,7 @@ mod layout_tests { value_template: "${:num %.2f}".to_string(), }]; - let cells = build_cells(&frame, &table).unwrap(); + let cells = resolve_cells(&frame, &table).unwrap(); let mut body_cells: Vec<_> = cells .iter() @@ -885,7 +927,7 @@ mod layout_tests { .unwrap(); let table = Table::new(); - let cells = build_cells(&frame, &table).unwrap(); + let cells = resolve_cells(&frame, &table).unwrap(); let hjust = |left: usize, kind: TableCellKind| { cells @@ -921,7 +963,7 @@ mod layout_tests { let mut table = Table::new(); table.spans = vec![labeled_spanner(&["a", "b"], "G")]; - let cells = build_cells(&frame, &table).unwrap(); + let cells = resolve_cells(&frame, &table).unwrap(); let mut label_cells: Vec<_> = cells .iter() @@ -958,7 +1000,7 @@ mod layout_tests { spanner_with_setting(&["b", "c"], "level", ParameterValue::Number(1.0)), ]; - assert!(build_cells(&frame, &table).is_err()); + assert!(resolve_cells(&frame, &table).is_err()); } #[test] @@ -972,7 +1014,7 @@ mod layout_tests { table.labels.labels.insert("id".to_string(), None); table.labels.labels.insert("name".to_string(), None); - let cells = build_cells(&frame, &table).unwrap(); + let cells = resolve_cells(&frame, &table).unwrap(); assert!(!cells.iter().any(|c| c.kind == TableCellKind::ColumnLabel)); assert!(cells diff --git a/src/execute/table_format.rs b/src/execute/table_format.rs index 1c905b02..47069aae 100644 --- a/src/execute/table_format.rs +++ b/src/execute/table_format.rs @@ -1,9 +1,10 @@ -//! `TABULATE FORMAT` resolution: reshaping `Table.formats` (one entry per -//! `FORMAT` clause, each naming several columns) into one `Format` per -//! column (`reshape_formats`), then applying that per column — `RENAMING` -//! by replacing the column's values in the `DataFrame` (`apply_formats`), -//! `SETTING` by resolving display properties for `TableColumn` -//! (`resolve_column_properties`). Called from `table::build_cells`. +//! `TABULATE FORMAT` resolution: validating `Table.formats`' `SETTING` +//! parameters and reshaping it (one entry per `FORMAT` clause, each naming +//! several columns) into one `Format` per column (`setup_formats`), then +//! applying that per column — `RENAMING` by replacing the column's values in +//! the `DataFrame` (`apply_formats`), `SETTING` by resolving display +//! properties for `TableColumn` (`resolve_column_properties`). `setup_formats` +//! and `apply_formats` are both called from `table::resolve_table_with_reader`. use std::collections::HashMap; @@ -13,13 +14,18 @@ use crate::array_util::{new_str_array, value_to_string}; use crate::plot::{ParameterValue, Parameters}; use crate::{DataFrame, Format, GgsqlError, Result}; -/// Reshape `formats` (one `FORMAT` clause per entry, each naming several -/// columns) into one `Format` per column it covers — a later `FORMAT` -/// clause wins over an earlier one naming the same column. -pub(crate) fn reshape_formats( - df: &DataFrame, - formats: &[Format], -) -> Result> { +/// Validate every FORMAT's `SETTING` parameters, then reshape `formats` +/// into one `Format` per column it covers. +pub(crate) fn setup_formats(df: &DataFrame, formats: &[Format]) -> Result> { + for (idx, format) in formats.iter().enumerate() { + format + .validate_settings() + .map_err(|e| GgsqlError::ValidationError(format!("FORMAT {}: {}", idx + 1, e)))?; + } + + // `formats` has one entry per FORMAT clause, each naming several + // columns — reshaped here into one `Format` per column, where a later + // clause wins over an earlier one naming the same column. let mut resolved = HashMap::new(); for format in formats { for name in &format.columns { @@ -57,7 +63,7 @@ pub(crate) fn apply_formats( let array = df .column(name) - .expect("reshape_formats already checked this column exists") + .expect("setup_formats already checked this column exists") .clone(); let resolved = crate::format::resolve_column_values( &array, @@ -130,7 +136,7 @@ mod tests { } #[test] - fn reshape_formats_lets_a_later_clause_win_and_clears_columns() { + fn setup_formats_lets_a_later_clause_win_and_clears_columns() { let frame = df! { "price" => vec![1.0f64] }.unwrap(); let formats = vec![ Format { @@ -143,7 +149,7 @@ mod tests { }, ]; - let resolved = reshape_formats(&frame, &formats).unwrap(); + let resolved = setup_formats(&frame, &formats).unwrap(); let price = resolved.get("price").unwrap(); assert_eq!(price.value_template, "{:num %.2f}"); @@ -151,14 +157,14 @@ mod tests { } #[test] - fn reshape_formats_errors_on_an_unknown_column() { + fn setup_formats_errors_on_an_unknown_column() { let frame = df! { "price" => vec![1.0f64] }.unwrap(); let formats = vec![Format { columns: vec!["typo".to_string()], ..format_with("{}", None) }]; - let error = reshape_formats(&frame, &formats).unwrap_err(); + let error = setup_formats(&frame, &formats).unwrap_err(); assert!(matches!(error, GgsqlError::ValidationError(msg) if msg.contains("FORMAT references unknown column 'typo'"))); } diff --git a/src/lib.rs b/src/lib.rs index ce093dc6..3c01666d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -92,7 +92,7 @@ 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}; +pub use execute::{TableCell, TableCellKind, TableColumn, TableRow}; /// Main library error type #[derive(thiserror::Error, Debug)] diff --git a/src/reader/mod.rs b/src/reader/mod.rs index fe99230c..154731f9 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, TableCell}; +use crate::{naming, DataFrame, GgsqlError, Result, Spec, TableCell, TableColumn, TableRow}; // ============================================================================= // SQL Dialect @@ -708,14 +708,21 @@ pub struct Metadata { /// Result of executing a ggsql TABULATE query, ready for rendering. pub struct ResolvedTable { - /// The resolved table specification - pub(crate) table: Table, /// 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, + /// Resolved per-column properties, for a writer wanting a whole-column + /// value (e.g. `width`) rather than the same value repeated across a + /// column's cells. `None` if resolution never got as far as building + /// columns (not currently possible via the public API, but kept + /// optional so a future construction path doesn't need one). + pub(crate) columns: Option>, + /// Resolved per-row properties, symmetric with `columns`. Always `None` + /// today — no row-wide `TABULATE` clause exists yet to populate it. + pub(crate) rows: Option>, /// The SQL query that was executed to produce `cells` pub(crate) sql: String, /// Validation warnings from preparation @@ -729,11 +736,11 @@ pub struct ResolvedTable { /// Result of executing a ggsql query: either a resolved plot or a resolved /// table, mirroring the parse-time `Spec` (`Plot` or `Table`). pub enum ResolvedSpec { - // Boxed for the same reason `Spec::Plot` is: `ResolvedPlot` is far larger - // than `ResolvedTable`, and clippy flags the resulting size gap - // (`large_enum_variant`) otherwise. + // Both boxed, mirroring `Spec`'s own two variants: whichever of + // `ResolvedPlot`/`ResolvedTable` is larger, an unboxed size gap between + // them trips clippy's `large_enum_variant` lint. Plot(Box), - Table(ResolvedTable), + Table(Box), } // ============================================================================ @@ -1048,7 +1055,7 @@ pub fn execute_with_reader(reader: &dyn Reader, query: &str) -> Result { let resolved = resolve_table_with_reader(query, reader)?; - Ok(ResolvedSpec::Table(resolved)) + Ok(ResolvedSpec::Table(Box::new(resolved))) } _ => { let resolved = resolve_plot_with_reader(reader, query)?; diff --git a/src/reader/spec.rs b/src/reader/spec.rs index f35ed489..a11bc151 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, TableCell}; +use crate::{DataFrame, TableCell, TableColumn, TableRow}; use super::{Metadata, ResolvedPlot, ResolvedSpec, ResolvedTable}; @@ -114,29 +114,39 @@ impl ResolvedPlot { impl ResolvedTable { /// Create a new ResolvedTable. pub(crate) fn new( - table: Table, cells: Vec, + columns: Option>, + rows: Option>, sql: String, warnings: Vec, ) -> Self { Self { - table, cells, + columns, + rows, sql, warnings, } } - /// Get the resolved table specification. - pub fn table(&self) -> &Table { - &self.table - } - /// Get the resolved layout: one cell per column label and per data value. pub fn cells(&self) -> &[TableCell] { &self.cells } + /// Resolved per-column properties, if any were built — a writer wanting + /// a whole-column value (e.g. `width`) reads it here instead of the + /// same value repeated across the column's cells. + pub fn columns(&self) -> Option<&[TableColumn]> { + self.columns.as_deref() + } + + /// Resolved per-row properties, symmetric with `columns`. Always `None` + /// today — no row-wide `TABULATE` clause exists yet to populate it. + pub fn rows(&self) -> Option<&[TableRow]> { + self.rows.as_deref() + } + /// 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 @@ -196,7 +206,7 @@ impl ResolvedSpec { pub fn into_table(self) -> Option { match self { ResolvedSpec::Plot(_) => None, - ResolvedSpec::Table(table) => Some(table), + ResolvedSpec::Table(table) => Some(*table), } } } diff --git a/src/writer/html.rs b/src/writer/html.rs index c47beed1..d0fa13c7 100644 --- a/src/writer/html.rs +++ b/src/writer/html.rs @@ -19,7 +19,7 @@ use std::collections::HashSet; use crate::plot::{ParameterValue, Parameters}; use crate::util::escape_html; use crate::writer::{Writer, WriterOptions}; -use crate::{DataFrame, GgsqlError, Plot, Result, TableCell}; +use crate::{DataFrame, GgsqlError, Plot, Result, TableCell, TableColumn, TableRow}; /// Renders a resolved table as a bare HTML `
idname1
Infoamount
idname
Infoamountid
`. Does not support plots. #[derive(Debug, Default)] @@ -53,7 +53,16 @@ impl Writer for HtmlWriter { )) } - fn write_table(&self, cells: &[TableCell]) -> Result { + fn write_table( + &self, + cells: &[TableCell], + columns: Option<&[TableColumn]>, + rows: Option<&[TableRow]>, + ) -> Result { + // Not consumed yet — no property needs whole-column/whole-row + // rendering (like `width`) rather than a per-cell one yet. + let _ = (columns, rows); + let ncol = cells .iter() .map(|cell| cell.right) @@ -323,7 +332,7 @@ mod render_tests { cell(TableCellKind::ColumnLabel, 0, 0, 1, 1), ]; - assert!(HtmlWriter::new().write_table(&cells).is_err()); + assert!(HtmlWriter::new().write_table(&cells, None, None).is_err()); } #[test] @@ -338,7 +347,7 @@ mod render_tests { cell(TableCellKind::Body, 2, 2, 1, 1), ]; - let html = HtmlWriter::new().write_table(&cells).unwrap(); + let html = HtmlWriter::new().write_table(&cells, None, None).unwrap(); assert_eq!( html, diff --git a/src/writer/mod.rs b/src/writer/mod.rs index c19b040a..fdfff832 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, TableCell}; +use crate::{DataFrame, GgsqlError, Plot, Result, TableCell, TableColumn, TableRow}; use std::collections::HashMap; pub mod options; @@ -174,20 +174,27 @@ pub trait Writer { /// 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. + /// building `cells`. `columns`/`rows` cover a whole-column or whole-row + /// property (e.g. `width`) that can't be expressed as a per-cell value. /// /// # Arguments /// /// * `cells` - The resolved table layout — see `TableCell` for the /// position/kind conventions + /// * `columns` - Resolved per-column properties, if any were built + /// * `rows` - Resolved per-row properties, if any were built /// /// # Errors /// /// Returns `GgsqlError::WriterError` if this writer doesn't support /// tables, or output generation fails. - fn write_table(&self, cells: &[TableCell]) -> Result { - let _ = cells; + fn write_table( + &self, + cells: &[TableCell], + columns: Option<&[TableColumn]>, + rows: Option<&[TableRow]>, + ) -> Result { + let _ = (cells, columns, rows); Err(GgsqlError::WriterError( "this writer does not support tables".to_string(), )) @@ -224,7 +231,9 @@ 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.cells()), + ResolvedSpec::Table(table) => { + self.write_table(table.cells(), table.columns(), table.rows()) + } } } } From f7280b09ee79b3d903fd0af0589f3f8d9b65507b Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 21 Sep 2026 13:17:47 +0200 Subject: [PATCH 7/7] Add FORMAT's width SETTING and render it as an HTML Adds a numeric-with-unit ParamConstraint (px/%) for FORMAT's width SETTING, and has HtmlWriter emit a with a per-column width style, mirroring gt's own colgroup output (checked directly against gt's source: width is the only thing it ever puts there). Co-Authored-By: Claude Sonnet 5 --- src/plot/types.rs | 102 +++++++++++++++++++++++++++++++++--- src/table/mod.rs | 48 +++++++++++++---- src/writer/html.rs | 125 ++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 252 insertions(+), 23 deletions(-) diff --git a/src/plot/types.rs b/src/plot/types.rs index a8b4be31..fcd7ea94 100644 --- a/src/plot/types.rs +++ b/src/plot/types.rs @@ -1168,13 +1168,21 @@ impl NumberConstraint { pub struct StringConstraint { /// String must be one of these values (empty = any string allowed) pub allowed_values: &'static [&'static str], + /// String must be a number immediately followed by one of these units + /// (empty = no restriction) — for a CSS-style measurement like + /// `width => '20%'`/`'240px'`, where the numeric part is unbounded but + /// the unit isn't. Checked by stripping a candidate unit and parsing + /// what's left as `f64`, not just a suffix match, so `'pxpx'` or a bare + /// `'%'` are rejected too. + pub numeric_units: &'static [&'static str], } impl StringConstraint { - /// Any string allowed (empty slice = no restriction) + /// Any string allowed (empty slices = no restriction) pub const fn unconstrained() -> Self { Self { allowed_values: &[], + numeric_units: &[], } } @@ -1182,6 +1190,15 @@ impl StringConstraint { pub const fn one_of(values: &'static [&'static str]) -> Self { Self { allowed_values: values, + numeric_units: &[], + } + } + + /// String must be a number immediately followed by one of the given units + pub const fn numeric_with_unit(units: &'static [&'static str]) -> Self { + Self { + allowed_values: &[], + numeric_units: units, } } } @@ -1328,6 +1345,19 @@ impl ParamConstraint { } } + /// String only, constrained to a number immediately followed by one of + /// the given units — for a CSS-style measurement like + /// `width => '20%'`/`'240px'`. + pub const fn string_numeric_with_unit(units: &'static [&'static str]) -> Self { + Self { + number: TypeConstraint::Forbidden, + string: TypeConstraint::Constrained(StringConstraint::numeric_with_unit(units)), + boolean: TypeConstraint::Forbidden, + array: TypeConstraint::Forbidden, + allow_null: true, + } + } + /// String only pub const fn string() -> Self { Self { @@ -1530,11 +1560,7 @@ fn validate_number(name: &str, n: f64, c: &NumberConstraint) -> Result<(), Strin } fn validate_string(name: &str, s: &str, c: &StringConstraint) -> Result<(), String> { - // Empty allowed_values = unconstrained (any string) - if c.allowed_values.is_empty() { - return Ok(()); - } - if !c.allowed_values.contains(&s) { + if !c.allowed_values.is_empty() && !c.allowed_values.contains(&s) { return Err(format!( "'{}' should be {}, not '{}'", name, @@ -1542,6 +1568,20 @@ fn validate_string(name: &str, s: &str, c: &StringConstraint) -> Result<(), Stri s )); } + if !c.numeric_units.is_empty() { + let has_valid_unit = c.numeric_units.iter().any(|unit| { + s.strip_suffix(unit) + .is_some_and(|prefix| prefix.parse::().is_ok()) + }); + if !has_valid_unit { + return Err(format!( + "'{}' should be a number followed by {}, not '{}'", + name, + crate::or_list_quoted(c.numeric_units, '\''), + s + )); + } + } Ok(()) } @@ -2196,6 +2236,56 @@ mod tests { assert!(result.unwrap_err().contains("should be String")); } + #[test] + fn test_string_numeric_with_unit_accepts_a_number_with_an_allowed_unit() { + let constraint = ParamConstraint::string_numeric_with_unit(&["px", "%"]); + assert!(validate_parameter( + "width", + &ParameterValue::String("20%".to_string()), + &constraint + ) + .is_ok()); + assert!(validate_parameter( + "width", + &ParameterValue::String("240px".to_string()), + &constraint + ) + .is_ok()); + } + + #[test] + fn test_string_numeric_with_unit_rejects_a_non_numeric_prefix() { + let constraint = ParamConstraint::string_numeric_with_unit(&["px", "%"]); + let result = validate_parameter( + "width", + &ParameterValue::String("wide%".to_string()), + &constraint, + ); + assert!(result.is_err()); + } + + #[test] + fn test_string_numeric_with_unit_rejects_a_disallowed_unit() { + let constraint = ParamConstraint::string_numeric_with_unit(&["px", "%"]); + let result = validate_parameter( + "width", + &ParameterValue::String("20em".to_string()), + &constraint, + ); + assert!(result.is_err()); + } + + #[test] + fn test_string_numeric_with_unit_rejects_a_bare_unit() { + let constraint = ParamConstraint::string_numeric_with_unit(&["px", "%"]); + let result = validate_parameter( + "width", + &ParameterValue::String("%".to_string()), + &constraint, + ); + assert!(result.is_err()); + } + #[test] fn test_boolean_accepts_valid() { let constraint = ParamConstraint::boolean(); diff --git a/src/table/mod.rs b/src/table/mod.rs index 3c3d002a..ce59d4a7 100644 --- a/src/table/mod.rs +++ b/src/table/mod.rs @@ -167,7 +167,7 @@ impl Spanner { /// One `FORMAT` clause: cell formatting for a group of columns. /// /// `settings` is validated against `FORMAT_PARAMS` and resolved into each -/// covered column's `TableCell` properties — not yet consumed by any writer. +/// covered column's `TableCell` properties. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Format { /// The columns this FORMAT applies to, in the order written. @@ -190,16 +190,25 @@ pub struct Format { } /// `SETTING` parameters `FORMAT` accepts. -const FORMAT_PARAMS: &[ParamDefinition] = &[ParamDefinition { - name: "hjust", - default: DefaultParamValue::Number(0.5), - // Both spellings accepted here; resolve_column_properties standardises - // "centre" to "center" when it builds a column's TableCell properties. - constraint: ParamConstraint::string_option_or_number( - &["left", "right", "centre", "center"], - NumberConstraint::range(0.0, 1.0), - ), -}]; +const FORMAT_PARAMS: &[ParamDefinition] = &[ + ParamDefinition { + name: "hjust", + default: DefaultParamValue::Number(0.5), + // Both spellings accepted here; resolve_column_properties standardises + // "centre" to "center" when it builds a column's TableCell properties. + constraint: ParamConstraint::string_option_or_number( + &["left", "right", "centre", "center"], + NumberConstraint::range(0.0, 1.0), + ), + }, + ParamDefinition { + name: "width", + // No default: an unset width leaves column sizing to the writer, + // not to a value resolved here. + default: DefaultParamValue::Null, + constraint: ParamConstraint::string_numeric_with_unit(&["px", "%"]), + }, +]; impl Format { /// Validate `settings` against `FORMAT_PARAMS`. @@ -292,4 +301,21 @@ mod tests { assert!(format_with_settings(settings).validate_settings().is_err()); } + + #[test] + fn format_validate_settings_accepts_a_percent_or_pixel_width() { + let mut settings = Parameters::new(); + settings.insert( + "width".to_string(), + ParameterValue::String("20%".to_string()), + ); + assert!(format_with_settings(settings).validate_settings().is_ok()); + + let mut settings = Parameters::new(); + settings.insert( + "width".to_string(), + ParameterValue::String("240px".to_string()), + ); + assert!(format_with_settings(settings).validate_settings().is_ok()); + } } diff --git a/src/writer/html.rs b/src/writer/html.rs index d0fa13c7..16d2a380 100644 --- a/src/writer/html.rs +++ b/src/writer/html.rs @@ -1,8 +1,15 @@ //! A minimal HTML table writer. //! -//! Renders a `ResolvedTable`'s cells as a bare `
` — inline `style` -//! attributes from each cell's resolved `FORMAT` properties, but no -//! footnotes, since `Table` has no fields to describe those yet. Spanner +//! Maps `ResolvedTable`'s three parts onto distinct pieces of the `
`: +//! - `cells` → ``/`` rows; each cell's resolved `FORMAT` +//! properties become an inline `style` attribute (e.g. `hjust` becomes +//! `text-align`). +//! - `columns` → a ``, one `` per column, with a `style` +//! attribute from that column's resolved `width` (a bare `` for a +//! column with none). +//! - `rows` → not consumed yet; no row-wide property exists to render. +//! +//! No footnotes, since `Table` has no fields to describe those yet. Spanner //! rows are rendered (as `colspan`, one `` per level, above the column //! labels); `render_cell`/`render_row` can also render a `rowspan` cell, //! though nothing in the resolution pipeline produces one yet, so a column @@ -59,9 +66,8 @@ impl Writer for HtmlWriter { columns: Option<&[TableColumn]>, rows: Option<&[TableRow]>, ) -> Result { - // Not consumed yet — no property needs whole-column/whole-row - // rendering (like `width`) rather than a per-cell one yet. - let _ = (columns, rows); + // Not consumed yet — no property needs whole-row rendering yet. + let _ = rows; let ncol = cells .iter() @@ -105,6 +111,10 @@ impl Writer for HtmlWriter { let mut html = String::from("
\n"); + if let Some(colgroup) = render_colgroup(columns) { + html.push_str(&colgroup); + } + if !header_rows.is_empty() { html.push_str("\n"); for (row, row_cells) in header_rows { @@ -180,6 +190,42 @@ fn text_align(hjust: Option<&ParameterValue>) -> Option<&'static str> { } } +/// Render a `` block, one `` per column, or `None` if +/// `columns` is absent or none of them resolve a `style`. Skipping the block +/// entirely in that case avoids emitting a run of bare, attribute-less +/// `` tags that would render identically to omitting them. +fn render_colgroup(columns: Option<&[TableColumn]>) -> Option { + let styles: Vec> = columns? + .iter() + .map(|c| column_style(&c.properties)) + .collect(); + if styles.iter().all(Option::is_none) { + return None; + } + + let mut html = String::from("\n"); + for style in styles { + match style { + Some(style) => html.push_str(&format!("\n")), + None => html.push_str("\n"), + } + } + html.push_str("\n"); + Some(html) +} + +/// Translate a column's resolved `FORMAT` properties into a ``'s +/// `style` attribute value, or `None` if it has no `width`. `width`'s value +/// is already validated (by `ParamConstraint::string_numeric_with_unit`) to +/// be a number immediately followed by `px` or `%`, which is exactly CSS's +/// own `width` syntax, so it's used as-is. +fn column_style(properties: &Parameters) -> Option { + match properties.get("width") { + Some(ParameterValue::String(width)) => Some(format!("width: {width}")), + _ => None, + } +} + /// Per row, the column positions already covered by a cell that started in /// an earlier row and hasn't ended yet (a rowspan declared above that row) — /// those get no cell and no filler when the row is rendered, since the @@ -314,6 +360,52 @@ mod render_tests { assert_eq!(cell_style(&Parameters::new()), None); } + #[test] + fn column_style_reads_a_string_width_as_a_css_width() { + let mut properties = Parameters::new(); + properties.insert( + "width".to_string(), + ParameterValue::String("20%".to_string()), + ); + assert_eq!(column_style(&properties), Some("width: 20%".to_string())); + + assert_eq!(column_style(&Parameters::new()), None); + } + + fn column(properties: Parameters) -> TableColumn { + TableColumn { + name: String::new(), + label: String::new(), + properties, + } + } + + #[test] + fn render_colgroup_returns_none_without_columns() { + assert_eq!(render_colgroup(None), None); + } + + #[test] + fn render_colgroup_returns_none_when_no_column_has_a_width() { + let columns = vec![column(Parameters::new()), column(Parameters::new())]; + assert_eq!(render_colgroup(Some(&columns)), None); + } + + #[test] + fn render_colgroup_emits_a_bare_col_for_a_column_with_no_width() { + let mut widened = Parameters::new(); + widened.insert( + "width".to_string(), + ParameterValue::String("20%".to_string()), + ); + let columns = vec![column(widened), column(Parameters::new())]; + + assert_eq!( + render_colgroup(Some(&columns)).unwrap(), + "\n\n\n\n" + ); + } + #[test] fn render_row_skips_a_column_occupied_by_a_rowspan_from_above() { let a = cell(TableCellKind::ColumnLabel, 1, 1, 0, 0); @@ -424,6 +516,27 @@ mod tests { assert!(html.find("Info").unwrap() < html.find(">id").unwrap()); } + #[test] + fn test_write_table_renders_a_colgroup_for_a_formats_width() { + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + reader + .execute_sql("CREATE TABLE sales AS SELECT * FROM (VALUES (1, 'a')) AS t(id, name)") + .unwrap(); + let spec = reader + .execute("TABULATE FROM sales FORMAT id SETTING width => '20%'") + .unwrap(); + + let writer = HtmlWriter::new(); + let html = writer.render(&spec).unwrap(); + + assert!(html.contains("")); + assert!(html.contains("")); + // "name" has no FORMAT width, so its stays bare. + assert!(html.contains("")); + // comes before , per the HTML spec. + assert!(html.find("").unwrap() < html.find("").unwrap()); + } + #[test] fn test_write_table_omits_tbody_for_a_zero_row_result() { let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap();