From 0cb0623d78f2ab548e9cf753e897083d5e737ea5 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 13 Oct 2026 13:29:47 +0200 Subject: [PATCH 01/25] Accept FACET clause in TABULATE grammar --- tree-sitter-ggsql/grammar.js | 1 + tree-sitter-ggsql/test/corpus/basic.txt | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/tree-sitter-ggsql/grammar.js b/tree-sitter-ggsql/grammar.js index 25aa6cf1f..e77132996 100644 --- a/tree-sitter-ggsql/grammar.js +++ b/tree-sitter-ggsql/grammar.js @@ -693,6 +693,7 @@ module.exports = grammar({ $.label_clause, $.span_clause, $.format_clause, + $.facet_clause, ), // FORMAT — configures cell formatting for a group of columns. Multiple diff --git a/tree-sitter-ggsql/test/corpus/basic.txt b/tree-sitter-ggsql/test/corpus/basic.txt index a942acd76..4c880ad60 100644 --- a/tree-sitter-ggsql/test/corpus/basic.txt +++ b/tree-sitter-ggsql/test/corpus/basic.txt @@ -4834,6 +4834,31 @@ TABULATE * FROM sales FORMAT foo, bar SETTING width => '20%' RENAMING null => '- (identifier (bare_identifier))))))) +================================================================================ +TABULATE FACET clause +================================================================================ + +TABULATE * FROM sales FACET region + +-------------------------------------------------------------------------------- + +(query + (tabulate_statement + (tabulate_keyword) + (column_selection + (mapping_list + (mapping_element + (wildcard_mapping)))) + (single_source_from + source: (qualified_name + (identifier + (bare_identifier)))) + (tab_clause + (facet_clause + (column_list + (identifier + (bare_identifier))))))) + ================================================================================ TABULATE column selection: wildcard, bare column, and a rename ================================================================================ From 2818aa9a4cefc34dae92018e80e24d5e91fe93c1 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 13 Oct 2026 13:41:58 +0200 Subject: [PATCH 02/25] Route TABULATE FACET to Table::facet, rejecting BY --- src/parser/builder.rs | 32 ++++++++++++++++++++++++++++++++ src/table/mod.rs | 9 +++++++-- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/parser/builder.rs b/src/parser/builder.rs index a1b05d3c1..032e5f760 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -450,6 +450,15 @@ fn process_tab_clause(node: &Node, source: &SourceTree, table: &mut Table) -> Re "format_clause" => { table.formats.push(build_format_clause(&child, source)?); } + "facet_clause" => { + let facet = build_facet(&child, source)?; + if facet.is_grid() { + return Err(GgsqlError::ParseError( + "TABULATE FACET does not support BY".to_string(), + )); + } + table.facet = Some(facet); + } _ => {} } } @@ -1575,6 +1584,29 @@ mod tests { ); } + #[test] + fn test_tabulate_facet_wrap_layout() { + let specs = parse_test_specs("TABULATE * FROM sales FACET region").unwrap(); + let table = specs[0].as_table().expect("expected a Table spec"); + + let facet = table.facet.as_ref().expect("expected a facet"); + assert!(facet.is_wrap()); + match &facet.layout { + FacetLayout::Wrap { variables } => { + assert_eq!(variables, &vec!["region".to_string()]) + } + _ => unreachable!(), + } + assert!(!facet.resolved); + } + + #[test] + fn test_tabulate_facet_by_is_rejected() { + let result = parse_test_specs("TABULATE * FROM sales FACET region BY category"); + + assert!(result.is_err()); + } + #[test] fn test_tabulate_repeated_label_clauses_merge_rather_than_overwrite() { let specs = parse_test_specs("TABULATE * FROM sales LABEL id => 'ID' LABEL name => 'Name'") diff --git a/src/table/mod.rs b/src/table/mod.rs index 7bdb9c7be..d8c878486 100644 --- a/src/table/mod.rs +++ b/src/table/mod.rs @@ -4,8 +4,8 @@ //! statements, parallel to how `plot` defines `Plot` for `VISUALISE` //! statements: `source` (from `TABULATE FROM`), `selection` (from //! `TABULATE`'s own column list), `labels` (from `TABULATE LABEL`), `spans` -//! (from `TABULATE SPAN`), and `formats` (from `TABULATE FORMAT`) are -//! populated so far. +//! (from `TABULATE SPAN`), `formats` (from `TABULATE FORMAT`), and `facet` +//! (from `TABULATE FACET`) are populated so far. mod layout; mod spanner; @@ -26,6 +26,7 @@ use crate::params::{ validate_parameter, DefaultParamValue, NumberConstraint, ParamConstraint, ParamDefinition, Parameters, }; +use crate::plot::facet::Facet; use crate::DataSource; /// Complete ggsql table specification. @@ -54,6 +55,9 @@ pub struct Table { /// Cell formatting (from `TABULATE FORMAT`), one per `FORMAT` clause /// written — same one-clause-per-group model as `spans`. pub formats: Vec, + /// Row grouping (from `TABULATE FACET`). Only the wrap form is + /// meaningful for tables; the builder rejects `BY`. + pub facet: Option, } /// One item in a TABULATE column selection. @@ -79,6 +83,7 @@ impl Table { labels: Labels::default(), spans: Vec::new(), formats: Vec::new(), + facet: None, } } } From 4189c01b9cb3a4f60e9206f550d1c217ae0beea3 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 13 Oct 2026 14:43:14 +0200 Subject: [PATCH 03/25] Resolve TABULATE FACET settings via table::setup_facet --- src/execute/table/mod.rs | 2 + src/table/facet.rs | 144 +++++++++++++++++++++++++++++++++++++++ src/table/mod.rs | 2 + 3 files changed, 148 insertions(+) create mode 100644 src/table/facet.rs diff --git a/src/execute/table/mod.rs b/src/execute/table/mod.rs index 85d008be7..83356291c 100644 --- a/src/execute/table/mod.rs +++ b/src/execute/table/mod.rs @@ -42,6 +42,8 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result) -> Result, String> { + let Some(facet) = facet else { + return Ok(None); + }; + + let valid: Vec<&str> = TABLE_FACET_PARAMS.iter().map(|p| p.name).collect(); + + for (name, value) in &facet.properties { + let Some(param) = TABLE_FACET_PARAMS.iter().find(|p| p.name == name.as_str()) else { + return Err(format!( + "FACET setting should be {}, not '{}'", + crate::or_list_quoted(&valid, '\''), + name + )); + }; + validate_parameter(name, value, ¶m.constraint)?; + } + + let mut resolved = facet.clone(); + if !resolved.properties.contains_key("place") { + resolved.properties.insert( + "place".to_string(), + ParameterValue::String("top".to_string()), + ); + } + resolved.resolved = true; + Ok(Some(resolved)) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::plot::facet::FacetLayout; + use crate::plot::Parameters; + + fn table_facet(settings: Parameters) -> Facet { + Facet { + layout: FacetLayout::Wrap { + variables: vec!["region".to_string()], + }, + properties: settings, + resolved: false, + } + } + + #[test] + fn setup_defaults_place_to_top() { + let facet = table_facet(Parameters::new()); + + let resolved = setup_facet(&Some(facet)).unwrap().unwrap(); + + assert_eq!( + resolved.properties.get("place"), + Some(&ParameterValue::String("top".to_string())) + ); + assert!(resolved.resolved); + } + + #[test] + fn setup_accepts_place_left() { + let mut settings = Parameters::new(); + settings.insert( + "place".to_string(), + ParameterValue::String("left".to_string()), + ); + let facet = table_facet(settings); + + let resolved = setup_facet(&Some(facet)).unwrap().unwrap(); + + assert_eq!( + resolved.properties.get("place"), + Some(&ParameterValue::String("left".to_string())) + ); + } + + #[test] + fn setup_rejects_an_unknown_setting() { + let mut settings = Parameters::new(); + settings.insert("ncol".to_string(), ParameterValue::Number(2.0)); + let facet = table_facet(settings); + + let err = setup_facet(&Some(facet)).unwrap_err(); + + assert!(err.contains("ncol")); + } + + #[test] + fn setup_rejects_an_invalid_place_value() { + let mut settings = Parameters::new(); + settings.insert( + "place".to_string(), + ParameterValue::String("right".to_string()), + ); + let facet = table_facet(settings); + + assert!(setup_facet(&Some(facet)).is_err()); + } + + #[test] + fn setup_accepts_missing_repeat_and_null() { + for value in ["repeat", "null"] { + let mut settings = Parameters::new(); + settings.insert( + "missing".to_string(), + ParameterValue::String(value.to_string()), + ); + let facet = table_facet(settings); + + setup_facet(&Some(facet)).unwrap(); + } + } + + #[test] + fn setup_passes_none_through() { + assert_eq!(setup_facet(&None).unwrap(), None); + } +} diff --git a/src/table/mod.rs b/src/table/mod.rs index d8c878486..b129eece0 100644 --- a/src/table/mod.rs +++ b/src/table/mod.rs @@ -7,9 +7,11 @@ //! (from `TABULATE SPAN`), `formats` (from `TABULATE FORMAT`), and `facet` //! (from `TABULATE FACET`) are populated so far. +mod facet; mod layout; mod spanner; +pub(crate) use facet::setup_facet; pub(crate) use layout::{ compose_header, create_caption, create_column_labels, create_heading, extract_heading_labels, move_stub_columns, standardise_hjust, Slot, From b1130620372a0ab1e16707ac306cfd45a617956a Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 13 Oct 2026 16:53:14 +0200 Subject: [PATCH 04/25] Group TABULATE rows by FACET variables Assign each result row a group id from its facet values (nested by variable, nulls in one group per level), reorder rows so groups are contiguous, and stamp body rows with their group id for label-cell generation. --- src/execute/table/facet.rs | 172 ++++++++++++++++++++++++++++++++++++ src/execute/table/layout.rs | 14 +-- src/execute/table/mod.rs | 16 +++- src/table/layout.rs | 4 + 4 files changed, 200 insertions(+), 6 deletions(-) create mode 100644 src/execute/table/facet.rs diff --git a/src/execute/table/facet.rs b/src/execute/table/facet.rs new file mode 100644 index 000000000..24387b296 --- /dev/null +++ b/src/execute/table/facet.rs @@ -0,0 +1,172 @@ +//! `TABULATE FACET` grouping: assigns each `DataFrame` row a group id from +//! its facet values, then reorders rows so groups are contiguous. + +use std::collections::HashMap; + +use arrow::array::UInt32Array; + +use crate::array_util::value_to_string; +use crate::DataFrame; + +/// Assign each `df` row a group id. Groups are ordered nested by variable: +/// each variable's levels are ranked by first appearance, and group ids +/// follow lexicographic order of the rank tuples, so the first variable +/// forms the outermost grouping. Rows that are null in every facet variable +/// share one group. +pub(super) fn setup_groups(variables: &[String], df: &DataFrame) -> Result, String> { + let columns: Vec<_> = variables + .iter() + .map(|name| { + df.column(name) + .map_err(|_| format!("TABULATE FACET: no column '{}' in the query result", name)) + }) + .collect::>()?; + + // Rank each variable's values by first appearance. + let mut levels: Vec, usize>> = vec![HashMap::new(); columns.len()]; + let mut tuples = Vec::with_capacity(df.height()); + for row in 0..df.height() { + let tuple: Vec = columns + .iter() + .zip(&mut levels) + .map(|(column, ranks)| { + let value = if column.is_null(row) { + None + } else { + Some(value_to_string(column, row)) + }; + let next = ranks.len(); + *ranks.entry(value).or_insert(next) + }) + .collect(); + tuples.push(tuple); + } + + // Group ids follow the nested (lexicographic) order of rank tuples. + let mut unique = tuples.clone(); + unique.sort(); + unique.dedup(); + let group_of: HashMap, usize> = unique + .into_iter() + .enumerate() + .map(|(id, tuple)| (tuple, id)) + .collect(); + + Ok(tuples.into_iter().map(|tuple| group_of[&tuple]).collect()) +} + +/// Reorder `df` so rows with the same group id are contiguous, keeping the +/// query's row order within each group. Returns the reordered frame and the +/// group ids in their new order. +pub(super) fn reorder_rows( + df: &DataFrame, + ids: Vec, +) -> Result<(DataFrame, Vec), String> { + let mut order: Vec = (0..df.height() as u32).collect(); + // sort_by_key is stable: within-group row order is preserved. + order.sort_by_key(|&i| ids[i as usize]); + + let df = df + .take(&UInt32Array::from(order.clone())) + .map_err(|e| e.to_string())?; + let ids = order.into_iter().map(|i| ids[i as usize]).collect(); + + Ok((df, ids)) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::df; + + fn variables(names: &[&str]) -> Vec { + names.iter().map(|n| n.to_string()).collect() + } + + #[test] + fn setup_groups_assigns_ids_in_first_appearance_order() { + let frame = df! { + "region" => vec!["south".to_string(), "north".to_string(), "south".to_string()], + "sales" => vec![1i32, 2, 3], + } + .unwrap(); + + let ids = setup_groups(&variables(&["region"]), &frame).unwrap(); + + assert_eq!(ids, vec![0, 1, 0]); + } + + #[test] + fn setup_groups_uses_the_whole_key_for_multiple_variables() { + let frame = df! { + "region" => vec!["north".to_string(), "north".to_string()], + "quarter" => vec!["q1".to_string(), "q2".to_string()], + } + .unwrap(); + + let ids = setup_groups(&variables(&["region", "quarter"]), &frame).unwrap(); + + assert_eq!(ids, vec![0, 1]); + } + + #[test] + fn setup_groups_orders_ids_nested_by_variable() { + // Row order interleaves the variables; ids follow the nested order + // (a,x), (a,y), (b,x), (b,y) instead. + let frame = df! { + "a" => vec!["a".to_string(), "b".to_string(), "a".to_string(), "b".to_string()], + "b" => vec!["x".to_string(), "x".to_string(), "y".to_string(), "y".to_string()], + } + .unwrap(); + + let ids = setup_groups(&variables(&["a", "b"]), &frame).unwrap(); + + assert_eq!(ids, vec![0, 2, 1, 3]); + } + + #[test] + fn setup_groups_errors_on_an_unknown_column() { + let frame = df! { "sales" => vec![1i32] }.unwrap(); + + let err = setup_groups(&variables(&["region"]), &frame).unwrap_err(); + + assert!(err.contains("region")); + } + + #[test] + fn reorder_rows_clusters_groups_and_preserves_within_group_order() { + let frame = df! { + "region" => vec!["south".to_string(), "north".to_string(), "south".to_string()], + "sales" => vec![1i32, 2, 3], + } + .unwrap(); + let ids = setup_groups(&variables(&["region"]), &frame).unwrap(); + + let (frame, ids) = reorder_rows(&frame, ids).unwrap(); + + assert_eq!(ids, vec![0, 0, 1]); + assert_eq!( + frame + .column("sales") + .unwrap() + .as_any() + .downcast_ref::() + .unwrap() + .values(), + &[1, 3, 2] + ); + } + + #[test] + fn setup_groups_buckets_nulls_into_one_group() { + let frame = DataFrame::new(vec![( + "region", + crate::dataframe::into_array_ref(vec![Some("north"), None, None]), + )]) + .unwrap(); + + let ids = setup_groups(&variables(&["region"]), &frame).unwrap(); + + assert_eq!(ids, vec![0, 1, 1]); + } +} diff --git a/src/execute/table/layout.rs b/src/execute/table/layout.rs index f77902bac..274b740a4 100644 --- a/src/execute/table/layout.rs +++ b/src/execute/table/layout.rs @@ -41,6 +41,7 @@ pub(super) fn build_cells( title: Option<&str>, subtitle: Option<&str>, caption: Option<&str>, + groups: Option<&[usize]>, ) -> Result { let spanners = create_spanners(&columns, spans); // Every label empty means `LABEL col => NULL` (or `=> ''`) on every @@ -65,11 +66,13 @@ pub(super) fn build_cells( let header = heading.rowbind(header); let mut body_cells = create_row_labels(df, &columns); body_cells.extend(create_body(df, &columns)); - let table_body = TableLayout::new( - columns.clone(), - vec![TableRow::body(); df.height()], - body_cells, - ); + let body_rows = (0..df.height()) + .map(|row| TableRow { + group: groups.map(|ids| ids[row]), + ..TableRow::body() + }) + .collect(); + let table_body = TableLayout::new(columns.clone(), body_rows, body_cells); let layout = header.rowbind(table_body); let caption_layout = create_caption(caption, &columns); let layout = layout.rowbind(caption_layout); @@ -177,6 +180,7 @@ mod tests { title.as_deref(), subtitle.as_deref(), caption.as_deref(), + None, ) } diff --git a/src/execute/table/mod.rs b/src/execute/table/mod.rs index 83356291c..d0b02d76e 100644 --- a/src/execute/table/mod.rs +++ b/src/execute/table/mod.rs @@ -7,10 +7,12 @@ //! construction in `layout`; the `TableLayout` assembly and `SPAN` resolution //! it composes live in `crate::table`. +mod facet; mod format; mod layout; mod stub; +use facet::{reorder_rows, setup_groups}; use format::{apply_formats, setup_formats}; use layout::{build_cells, setup_columns}; @@ -43,7 +45,7 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result Result { + let ids = + setup_groups(&facet.get_variables(), &df).map_err(GgsqlError::ValidationError)?; + let (df, ids) = reorder_rows(&df, ids).map_err(GgsqlError::ValidationError)?; + (df, Some(ids)) + } + None => (df, None), + }; + let column_names = df.get_column_names(); let spans = table .resolve_spanners(&labels, Some(&column_names)) @@ -81,6 +94,7 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result, } impl TableRow { From 234a132a260612b45c3275906dd9a143be72abcc Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 15 Oct 2026 09:45:05 +0200 Subject: [PATCH 05/25] Render TABULATE FACET groups with interleaved label rows Faceted tables emit one GroupHeading row per changed facet-variable level above each group (hierarchical for several variables), with labels read from the group's first formatted row; the facet columns leave the body. Column-claim conflicts (SPAN/STUB over a facet variable) are rejected by Table::validate_boundaries. place => 'left' is rejected until group-stub columns land. --- src/execute/table/layout.rs | 151 ++++++++++++++++++++++++++++++++---- src/execute/table/mod.rs | 123 +++++++++++++++++++++++++---- src/table/facet.rs | 12 ++- src/table/layout.rs | 38 ++++++++- src/table/mod.rs | 101 +++++++++++++++++------- src/table/spanner.rs | 15 ++-- src/validate.rs | 2 +- src/writer/html.rs | 36 ++++++++- 8 files changed, 403 insertions(+), 75 deletions(-) diff --git a/src/execute/table/layout.rs b/src/execute/table/layout.rs index 274b740a4..89af0799d 100644 --- a/src/execute/table/layout.rs +++ b/src/execute/table/layout.rs @@ -8,40 +8,80 @@ use std::collections::HashMap; use super::format::resolve_column_properties; use super::stub::{create_row_labels, create_stubhead}; use crate::array_util::value_to_string; +use crate::plot::facet::Facet; use crate::plot::Labels; use crate::table::{ compose_header, create_caption, create_column_labels, create_heading, create_spanners, move_stub_columns, reorder_table_columns, TableCell, TableClass, TableColumn, TableLayout, TableRow, }; -use crate::{DataFrame, Format, Result, Spanner}; +use crate::{DataFrame, Format, GgsqlError, Result, Spanner}; /// Resolve a table's columns: one `TableColumn` per `DataFrame` column /// (labelled, with `formats`' resolved `SETTING` properties), reordered for -/// any `gather`-ing spanner and then for STUB columns. `spans` must already -/// be resolved (`Table::resolve_spanners`). +/// any `gather`-ing spanner and then for STUB columns. A `place => 'top'` +/// facet consumes its columns as group labels; `place => 'left'` keeps +/// them. `spans` must already be resolved (`Table::resolve_spanners`). pub(super) fn setup_columns( df: &DataFrame, spans: &[Spanner], labels: &Labels, formats: &HashMap, + facet: Option<&crate::plot::facet::Facet>, ) -> Vec { let columns = create_table_columns(df, labels, formats); let columns = reorder_table_columns(columns, spans); - move_stub_columns(columns) + let mut columns = move_stub_columns(columns); + let Some(facet) = facet else { + return columns; + }; + if facet.properties.get("place") + != Some(&crate::plot::ParameterValue::String("top".to_string())) + { + return columns; + } + columns.retain(|c| !facet.get_variables().contains(&c.name)); + columns +} + +/// Resolve a table's body rows: one `TableRow` per `df` row, stamped with +/// its facet group. A facet groups on the raw query values (before +/// `FORMAT`'s RENAMING rewrites them) and reorders `df` so groups are +/// contiguous. Group-label rows are not part of the result — `build_cells` +/// interleaves those once labels can be read from the formatted frame. +pub(super) fn setup_rows( + df: DataFrame, + facet: Option<&Facet>, +) -> Result<(DataFrame, Vec)> { + let Some(facet) = facet else { + let rows = vec![TableRow::body(); df.height()]; + return Ok((df, rows)); + }; + let ids = super::facet::setup_groups(&facet.get_variables(), &df) + .map_err(GgsqlError::ValidationError)?; + let (df, ids) = super::facet::reorder_rows(&df, ids).map_err(GgsqlError::ValidationError)?; + let rows = ids + .into_iter() + .map(|id| TableRow { + group: Some(id), + ..TableRow::body() + }) + .collect(); + Ok((df, rows)) } /// Build the resolved `TableLayout` for a table from its already-resolved -/// `columns` and already-`FORMAT`-applied `df` — spanner rows, column -/// labels, and body, composed together and validated. +/// `columns`, already-`FORMAT`-applied `df`, and `setup_rows` body rows — +/// spanner rows, column labels, and body, composed together and validated. pub(super) fn build_cells( df: &DataFrame, columns: Vec, + rows: Vec, spans: &[Spanner], title: Option<&str>, subtitle: Option<&str>, caption: Option<&str>, - groups: Option<&[usize]>, + facet: Option<&Facet>, ) -> Result { let spanners = create_spanners(&columns, spans); // Every label empty means `LABEL col => NULL` (or `=> ''`) on every @@ -66,12 +106,10 @@ pub(super) fn build_cells( let header = heading.rowbind(header); let mut body_cells = create_row_labels(df, &columns); body_cells.extend(create_body(df, &columns)); - let body_rows = (0..df.height()) - .map(|row| TableRow { - group: groups.map(|ids| ids[row]), - ..TableRow::body() - }) - .collect(); + let (body_rows, body_cells) = match facet { + None => (rows, body_cells), + Some(facet) => weave_group_labels(df, &columns, &rows, body_cells, facet)?, + }; let table_body = TableLayout::new(columns.clone(), body_rows, body_cells); let layout = header.rowbind(table_body); let caption_layout = create_caption(caption, &columns); @@ -151,6 +189,89 @@ fn create_body(df: &DataFrame, columns: &[TableColumn]) -> Vec { cells } +/// Interleave group-label rows into the body. At each group boundary, emit +/// one `GroupHeading` row per facet-variable level that changed, labels read +/// from the group's first row (so an unchanged outer variable isn't +/// restated). Body cells keep their data-row numbering, shifted down by the +/// label rows before them. +fn weave_group_labels( + df: &DataFrame, + columns: &[TableColumn], + rows: &[TableRow], + body_cells: Vec, + facet: &Facet, +) -> Result<(Vec, Vec)> { + let facet_arrays: Vec<_> = facet + .get_variables() + .iter() + .map(|name| { + df.column(name).map_err(|_| { + GgsqlError::ValidationError(format!( + "TABULATE FACET: no column '{name}' in the query result" + )) + }) + }) + .collect::>()?; + + let mut out_rows = Vec::new(); + let mut label_cells = Vec::new(); + // body_offset[data row] = how many label rows precede it in the grid. + let mut body_offset = vec![0usize; df.height()]; + // None until the first group's labels are read: it emits every level. + let mut previous_labels: Option> = None; + let mut row = 0usize; + + for group_rows in rows.chunk_by(|a, b| a.group == b.group) { + let id = group_rows[0] + .group + .expect("setup_rows stamps every body row"); + let labels: Vec = facet_arrays + .iter() + .map(|array| value_to_string(array, row)) + .collect(); + // When multiple labels exist, we have a nesting pattern. + // We don't need to repeat every level label at every group, + // just from the level(s) that changed onward. + let first_changed = match &previous_labels { + None => 0, + Some(previous) => previous + .iter() + .zip(&labels) + .position(|(old, new)| old != new) + .unwrap_or(labels.len()), + }; + for label in &labels[first_changed..] { + label_cells.push(TableCell::new_group_heading( + out_rows.len(), + columns.len(), + label.clone(), + )); + out_rows.push(TableRow::group_heading(id)); + } + previous_labels = Some(labels); + + for (index, table_row) in group_rows.iter().enumerate() { + let mut table_row = table_row.clone(); + // First row in group needs special class. Index is within chunk + // of groups, so first row in group has index 0. + if index == 0 { + table_row.classes.push(TableClass::RowGroupFirst); + } + body_offset[row] = out_rows.len() - row; + out_rows.push(table_row); + row += 1; + } + } + + let mut body_cells = body_cells; + for cell in &mut body_cells { + cell.offset_rows(body_offset[cell.top]); + } + label_cells.extend(body_cells); + + Ok((out_rows, label_cells)) +} + #[cfg(test)] mod tests { use super::super::format::{apply_formats, setup_formats}; @@ -171,11 +292,13 @@ mod tests { .resolve_spanners(&labels, Some(&column_names)) .map_err(GgsqlError::ValidationError)?; let formats = setup_formats(df, &table.formats, &spans)?; - let columns = setup_columns(df, &spans, &labels, &formats); + let columns = setup_columns(df, &spans, &labels, &formats, None); let df = apply_formats(df, &formats)?; + let rows = vec![TableRow::body(); df.height()]; build_cells( &df, columns, + rows, &spans, title.as_deref(), subtitle.as_deref(), diff --git a/src/execute/table/mod.rs b/src/execute/table/mod.rs index d0b02d76e..657f0a95f 100644 --- a/src/execute/table/mod.rs +++ b/src/execute/table/mod.rs @@ -12,9 +12,8 @@ mod format; mod layout; mod stub; -use facet::{reorder_rows, setup_groups}; use format::{apply_formats, setup_formats}; -use layout::{build_cells, setup_columns}; +use layout::{build_cells, setup_columns, setup_rows}; use crate::table::extract_heading_labels; @@ -66,16 +65,7 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result { - let ids = - setup_groups(&facet.get_variables(), &df).map_err(GgsqlError::ValidationError)?; - let (df, ids) = reorder_rows(&df, ids).map_err(GgsqlError::ValidationError)?; - (df, Some(ids)) - } - None => (df, None), - }; + let (df, rows) = setup_rows(df, facet.as_ref())?; let column_names = df.get_column_names(); let spans = table @@ -85,16 +75,23 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result = resolved.columns().iter().map(|c| c.name.as_str()).collect(); + assert_eq!(names, ["quarter", "sales"]); + + let mut headings: Vec<_> = resolved + .cells() + .iter() + .filter(|c| c.kind == TableCellKind::GroupHeading) + .collect(); + headings.sort_by_key(|c| c.top); + assert_eq!(headings.len(), 2); + // Row 0 is the column-label row; each heading spans both columns. + assert_eq!( + (headings[0].top, headings[0].left, headings[0].right), + (1, 0, 1) + ); + assert_eq!(headings[0].content, "north"); + // north has two data rows, so south's heading sits at row 4. + assert_eq!(headings[1].top, 4); + assert_eq!(headings[1].content, "south"); + + assert_eq!( + resolved + .cells() + .iter() + .filter(|c| c.kind == TableCellKind::Body) + .count(), + 6 + ); + } + + #[test] + fn test_tabulate_facet_nests_label_rows_for_multiple_variables() { + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + reader + .execute_sql( + "CREATE TABLE t AS SELECT * FROM (VALUES \ + ('north', 'q1', 1), ('north', 'q2', 2), ('south', 'q1', 3)) \ + AS t(region, quarter, sales)", + ) + .unwrap(); + + let resolved = + resolve_table_with_reader("TABULATE * FROM t FACET region, quarter", &reader).unwrap(); + + let mut headings: Vec<_> = resolved + .cells() + .iter() + .filter(|c| c.kind == TableCellKind::GroupHeading) + .collect(); + headings.sort_by_key(|c| c.top); + let texts: Vec<&str> = headings.iter().map(|c| c.content.as_str()).collect(); + // q1 is not restated while region stays north; south restates q1 + // under it. One data row per group: headings interleave singly. + assert_eq!(texts, ["north", "q1", "q2", "south", "q1"]); + let tops: Vec = headings.iter().map(|c| c.top).collect(); + assert_eq!(tops, [1, 2, 4, 6, 7]); + } + + #[test] + fn test_tabulate_facet_covering_every_column_errors() { + let reader = reader_with_sales(); + let result = resolve_table_with_reader("TABULATE name FROM sales FACET name", &reader); + assert!(result.is_err()); + } + + #[test] + fn test_tabulate_facet_rejects_a_spanner_over_a_facet_column() { + let reader = reader_with_sales(); + let result = + resolve_table_with_reader("TABULATE * FROM sales FACET name SPAN G OVER name", &reader); + assert!(result.is_err()); + } + + #[test] + fn test_tabulate_facet_place_left_is_not_supported_yet() { + let reader = reader_with_sales(); + let result = resolve_table_with_reader( + "TABULATE * FROM sales FACET name SETTING place => 'left'", + &reader, + ); + assert!(result.is_err()); + } + #[test] fn test_tabulate_span_unknown_column_errors_even_with_gather_disabled() { // Table::resolve_spanners checks every OVER entry regardless of diff --git a/src/table/facet.rs b/src/table/facet.rs index fba8e784a..0404d95cd 100644 --- a/src/table/facet.rs +++ b/src/table/facet.rs @@ -49,6 +49,9 @@ pub(crate) fn setup_facet(facet: &Option) -> Result, String "place".to_string(), ParameterValue::String("top".to_string()), ); + } else if resolved.properties.get("place") == Some(&ParameterValue::String("left".to_string())) + { + return Err("TABULATE FACET SETTING place => 'left' is not supported yet".to_string()); } resolved.resolved = true; Ok(Some(resolved)) @@ -84,7 +87,7 @@ mod tests { } #[test] - fn setup_accepts_place_left() { + fn setup_rejects_place_left_until_group_stub_columns_land() { let mut settings = Parameters::new(); settings.insert( "place".to_string(), @@ -92,12 +95,7 @@ mod tests { ); let facet = table_facet(settings); - let resolved = setup_facet(&Some(facet)).unwrap().unwrap(); - - assert_eq!( - resolved.properties.get("place"), - Some(&ParameterValue::String("left".to_string())) - ); + assert!(setup_facet(&Some(facet)).is_err()); } #[test] diff --git a/src/table/layout.rs b/src/table/layout.rs index 7d17939e5..8e2875f11 100644 --- a/src/table/layout.rs +++ b/src/table/layout.rs @@ -44,6 +44,9 @@ pub enum TableCellKind { /// real cell reaches. Distinct from whatever kind it visually stands in /// for, so kind-based filtering can't mistake it for real structure. Filler, + /// A group label row above a group of body rows (`TABULATE FACET`, + /// gt's `gt_group_heading`). Spans all columns; a body row, not a header. + GroupHeading, } impl TableCellKind { @@ -76,6 +79,7 @@ impl std::fmt::Display for TableCellKind { TableCellKind::Subtitle => "subtitle", TableCellKind::Caption => "caption", TableCellKind::Filler => "filler", + TableCellKind::GroupHeading => "group heading", }; write!(f, "{text}") } @@ -117,6 +121,12 @@ pub enum TableClass { ColHeadingRow, /// The `` wrapping a spanner row (gt's `gt_spanner_row`). SpannerRow, + /// The `` wrapping a `GroupHeading` cell (gt's + /// `gt_group_heading_row`). + GroupHeadingRow, + /// The first body row of a facet group (gt's `gt_row_group_first`) — + /// carries the thick border separating a group from what precedes it. + RowGroupFirst, // -------------------------- Cell-scoped ------------------------- // Recorded on `TableCell::classes`, rendered on the ``/``. @@ -141,6 +151,9 @@ pub enum TableClass { Subtitle, /// The caption cell (gt's `gt_caption`). Caption, + /// A group-label cell (`TableCellKind::GroupHeading`, gt's + /// `gt_group_heading`). + GroupHeading, /// Left/center/right-aligned cell content (gt's /// `gt_left`/`gt_center`/`gt_right`). AlignLeft, @@ -170,6 +183,9 @@ impl std::fmt::Display for TableClass { TableClass::Subtitle => "subtitle", TableClass::Caption => "caption", TableClass::Heading => "heading", + TableClass::GroupHeading => "group_heading", + TableClass::GroupHeadingRow => "group_heading_row", + TableClass::RowGroupFirst => "row_group_first", }; write!(f, "{text}") } @@ -280,6 +296,13 @@ impl TableCell { .with_classes(vec![TableClass::Caption]) } + /// A group-label cell (`TableClass::GroupHeading`), spanning all `ncol` + /// columns on its `row`. + pub(crate) fn new_group_heading(row: usize, ncol: usize, content: String) -> Self { + Self::new(TableCellKind::GroupHeading, row, row, 0, ncol - 1, content) + .with_classes(vec![TableClass::GroupHeading]) + } + /// 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 { @@ -426,6 +449,16 @@ impl TableRow { pub fn body() -> Self { Self::default() } + + /// A group-label row (`TableClass::GroupHeadingRow`) belonging to + /// `group` — a body row, like the rows it labels. + pub fn group_heading(group: usize) -> Self { + Self { + classes: vec![TableClass::GroupHeadingRow], + group: Some(group), + ..Self::body() + } + } } /// The row extent a set of cells implies — the highest `bottom` plus one, @@ -477,6 +510,7 @@ mod tests { assert_eq!(TableCellKind::Subtitle.to_string(), "subtitle"); assert_eq!(TableCellKind::Caption.to_string(), "caption"); assert_eq!(TableCellKind::Filler.to_string(), "filler"); + assert_eq!(TableCellKind::GroupHeading.to_string(), "group heading"); } #[test] @@ -897,7 +931,7 @@ pub(crate) fn extract_heading_labels( /// Move every STUB-targeted column to the front, preserving each group's /// relative order. Runs after `reorder_table_columns`, as the final word on /// column position — a SPAN can never cover a stub column -/// (`Table::validate_span_stub_boundary`). +/// (`Table::validate_boundaries`). pub(crate) fn move_stub_columns(columns: Vec) -> Vec { let (mut stub, body): (Vec<_>, Vec<_>) = columns.into_iter().partition(TableColumn::is_stub); stub.extend(body); @@ -948,7 +982,7 @@ fn stretch_unspanned_column_labels( for label in header .cells .iter_mut() - // A stub column can never be spanned (`Table::validate_span_stub_boundary`), + // A stub column can never be spanned (`Table::validate_boundaries`), // so its stretch always reaches the full spanner height. .filter(|c| matches!(c.kind, TableCellKind::ColumnLabel | TableCellKind::StubHead)) { diff --git a/src/table/mod.rs b/src/table/mod.rs index b129eece0..f056ebb64 100644 --- a/src/table/mod.rs +++ b/src/table/mod.rs @@ -21,7 +21,7 @@ pub use spanner::Spanner; pub(crate) use spanner::{create_spanners, reorder_table_columns}; use serde::{Deserialize, Serialize}; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use crate::labels::Labels; use crate::params::{ @@ -138,45 +138,47 @@ impl Table { Ok(resolved) } - /// Reject any SPAN naming a column that any FORMAT clause ever targeted - /// at STUB — spanners over the stub aren't supported yet. No - /// last-clause-wins resolution needed: a column that was ever declared - /// STUB is off-limits to SPAN, regardless of whether a later FORMAT - /// clause retargeted it to BODY. Takes already-`resolve_spanner_ids`- - /// resolved spans, so a SPAN's `OVER` id reference is checked against - /// the real columns it expands to, not the literal id text — and a - /// STUB format's own column list gets the same treatment, expanding any - /// entry that names a SPAN id into the columns it covers, so `FORMAT - /// STUB ` is checked against real columns too rather than the - /// literal id, which would never appear in a span's own `columns`. - /// - /// Needs no DataFrame: FORMAT/SPAN are both plain AST, so this runs from - /// both `validate()` and real execution (`resolve_spanners`), mirroring - /// `resolve_spanner_ids`. - pub fn validate_span_stub_boundary(&self, resolved_spans: &[Spanner]) -> Result<(), String> { - let stub_columns: HashSet<&str> = self + /// Reject columns claimed by two table features at once: a SPAN over a + /// STUB or FACET column, or a FORMAT STUB over a FACET column. Takes + /// already-`resolve_spanner_ids`-resolved spans, so every check compares + /// real column names, never a SPAN id. Needs no DataFrame, so this runs + /// from both `validate()` and real execution (`resolve_spanners`). + pub fn validate_boundaries(&self, resolved_spans: &[Spanner]) -> Result<(), String> { + let stub_columns: Vec = self .formats .iter() .filter(|format| format.target.is_stub()) .flat_map(|format| &format.columns) .flat_map( |name| match resolved_spans.iter().find(|span| &span.id == name) { - Some(span) => span.columns.iter().map(String::as_str).collect::>(), - None => vec![name.as_str()], + Some(span) => span.columns.clone(), + None => vec![name.clone()], }, ) .collect(); for (idx, span) in resolved_spans.iter().enumerate() { - span.validate_stub_boundary(&stub_columns) + span.validate_boundary(&stub_columns, "stub") + .map_err(|e| format!("SPAN {}: {}", idx + 1, e))?; + } + + let Some(facet) = &self.facet else { + return Ok(()); + }; + let facet_variables = facet.get_variables(); + for (idx, span) in resolved_spans.iter().enumerate() { + span.validate_boundary(&facet_variables, "facet") .map_err(|e| format!("SPAN {}: {}", idx + 1, e))?; } + if let Some(name) = facet_variables.iter().find(|v| stub_columns.contains(v)) { + return Err(format!("FORMAT STUB cannot target FACET group '{name}'")); + } Ok(()) } /// Fully resolve this table's spanners for execution: settings - /// validated, `OVER` ids folded, STUB boundary checked, columns + /// validated, `OVER` ids folded, feature boundaries checked, columns /// checked against the real schema, and each `label` overlaid with any /// `LABEL => ...` entry. pub fn resolve_spanners( @@ -190,7 +192,7 @@ impl Table { .map_err(|e| format!("SPAN {}: {}", idx + 1, e))?; } let spans = self.resolve_spanner_ids()?; - self.validate_span_stub_boundary(&spans)?; + self.validate_boundaries(&spans)?; // None (validate()'s case) skips this: no real schema to check. if let Some(names) = column_names { for span in &spans { @@ -361,7 +363,7 @@ mod tests { } #[test] - fn validate_span_stub_boundary_rejects_a_span_over_a_stub_column() { + fn validate_boundaries_rejects_a_span_over_a_stub_column() { let table = Table { formats: vec![Format { columns: vec!["a".to_string()], @@ -378,12 +380,12 @@ mod tests { }; let resolved = table.resolve_spanner_ids().unwrap(); - let error = table.validate_span_stub_boundary(&resolved).unwrap_err(); + let error = table.validate_boundaries(&resolved).unwrap_err(); assert!(error.contains("cannot include stub column 'a'")); } #[test] - fn validate_span_stub_boundary_rejects_a_format_stub_naming_a_span_id() { + fn validate_boundaries_rejects_a_format_stub_naming_a_span_id() { // FORMAT STUB naming a SPAN id rather than its columns directly must // still be caught: `stub_columns` has to expand "G" into "a"/"b" // before comparing, or this collision goes undetected. @@ -403,7 +405,52 @@ mod tests { }; let resolved = table.resolve_spanner_ids().unwrap(); - let error = table.validate_span_stub_boundary(&resolved).unwrap_err(); + let error = table.validate_boundaries(&resolved).unwrap_err(); assert!(error.contains("cannot include stub column")); } + + fn table_facet(variables: &[&str]) -> crate::plot::facet::Facet { + crate::plot::facet::Facet { + layout: crate::plot::facet::FacetLayout::Wrap { + variables: variables.iter().map(|v| v.to_string()).collect(), + }, + properties: Parameters::new(), + resolved: false, + } + } + + #[test] + fn validate_boundaries_rejects_a_span_over_a_facet_column() { + let table = Table { + facet: Some(table_facet(&["a"])), + spans: vec![Spanner { + id: "S".to_string(), + label: Some("S".to_string()), + columns: vec!["a".to_string(), "sales".to_string()], + settings: Parameters::new(), + }], + ..Table::new() + }; + + let resolved = table.resolve_spanner_ids().unwrap(); + let error = table.validate_boundaries(&resolved).unwrap_err(); + assert!(error.contains("cannot include facet column 'a'")); + } + + #[test] + fn validate_boundaries_rejects_a_format_stub_targeting_a_facet_column() { + let table = Table { + facet: Some(table_facet(&["a"])), + formats: vec![Format { + columns: vec!["a".to_string()], + target: ColumnSection::Stub, + ..format_with_settings(Parameters::new()) + }], + ..Table::new() + }; + + let resolved = table.resolve_spanner_ids().unwrap(); + let error = table.validate_boundaries(&resolved).unwrap_err(); + assert!(error.contains("FORMAT STUB cannot target FACET group 'a'")); + } } diff --git a/src/table/spanner.rs b/src/table/spanner.rs index da63b1938..a8cb872bd 100644 --- a/src/table/spanner.rs +++ b/src/table/spanner.rs @@ -3,7 +3,6 @@ //! assignment. use serde::{Deserialize, Serialize}; -use std::collections::HashSet; use super::layout::{TableCell, TableClass, TableColumn, TableLayout, TableRow}; use crate::labels::Labels; @@ -69,14 +68,12 @@ impl Spanner { Ok(()) } - /// Check this spanner's columns against the table's STUB columns. - pub fn validate_stub_boundary(&self, stub_columns: &HashSet<&str>) -> Result<(), String> { - if let Some(name) = self - .columns - .iter() - .find(|name| stub_columns.contains(name.as_str())) - { - return Err(format!("cannot include stub column '{name}'.")); + /// Check this spanner's columns against `off_limits`, columns claimed + /// by another feature — `context` names that feature in the error + /// (e.g. `"stub"`, `"facet"`). + pub fn validate_boundary(&self, off_limits: &[String], context: &str) -> Result<(), String> { + if let Some(name) = self.columns.iter().find(|name| off_limits.contains(name)) { + return Err(format!("cannot include {context} column '{name}'.")); } Ok(()) diff --git a/src/validate.rs b/src/validate.rs index e9c91fa2f..65de3d0d9 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -311,7 +311,7 @@ pub fn validate(query: &str) -> Result { match table.resolve_spanner_ids() { Ok(resolved_spans) => { - if let Err(e) = table.validate_span_stub_boundary(&resolved_spans) { + if let Err(e) = table.validate_boundaries(&resolved_spans) { errors.push(ValidationError { message: e, location: None, diff --git a/src/writer/html.rs b/src/writer/html.rs index 6a748af53..19ecd2d18 100644 --- a/src/writer/html.rs +++ b/src/writer/html.rs @@ -316,6 +316,36 @@ fn class_declarations(class: TableClass) -> &'static [(&'static str, &'static st ("border-right-width", "1px"), ("border-right-color", "#D3D3D3"), ], + TableClass::GroupHeading => &[ + ("padding-top", "8px"), + ("padding-bottom", "8px"), + ("padding-left", "5px"), + ("padding-right", "5px"), + ("color", "#333333"), + ("background-color", "#FFFFFF"), + ("font-size", "100%"), + ("font-weight", "initial"), + ("border-top-style", "solid"), + ("border-top-width", "2px"), + ("border-top-color", "#D3D3D3"), + ("border-bottom-style", "solid"), + ("border-bottom-width", "2px"), + ("border-bottom-color", "#D3D3D3"), + ("border-left-style", "none"), + ("border-left-width", "1px"), + ("border-left-color", "#D3D3D3"), + ("border-right-style", "none"), + ("border-right-width", "1px"), + ("border-right-color", "#D3D3D3"), + ("vertical-align", "middle"), + ("text-align", "left"), + ], + TableClass::GroupHeadingRow => &[], + TableClass::RowGroupFirst => &[ + ("border-top-style", "solid"), + ("border-top-width", "2px"), + ("border-top-color", "#D3D3D3"), + ], TableClass::AlignLeft => &[("text-align", "left")], TableClass::AlignCenter => &[("text-align", "center")], TableClass::AlignRight => &[ @@ -345,6 +375,9 @@ const STYLED_CLASSES: &[TableClass] = &[ TableClass::Subtitle, TableClass::Caption, TableClass::Heading, + TableClass::GroupHeading, + TableClass::GroupHeadingRow, + TableClass::RowGroupFirst, TableClass::AlignLeft, TableClass::AlignCenter, TableClass::AlignRight, @@ -477,7 +510,8 @@ fn header_scope(kind: TableCellKind, colspan: usize) -> Option<&'static str> { | TableCellKind::Title | TableCellKind::Subtitle | TableCellKind::Caption - | TableCellKind::Filler => None, + | TableCellKind::Filler + | TableCellKind::GroupHeading => None, } } From 5932522fac44ce22f530054c14c4e69ee5f3af02 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 15 Oct 2026 14:39:54 +0200 Subject: [PATCH 06/25] Render TABULATE FACET place => 'left' as rowspanned group stub columns --- src/execute/table/layout.rs | 155 +++++++++++++++++++++++++++++++----- src/execute/table/mod.rs | 48 +++++++++-- src/table/facet.rs | 19 +++-- src/table/layout.rs | 31 ++++++-- src/table/mod.rs | 29 +++++-- 5 files changed, 240 insertions(+), 42 deletions(-) diff --git a/src/execute/table/layout.rs b/src/execute/table/layout.rs index 89af0799d..cc3df0959 100644 --- a/src/execute/table/layout.rs +++ b/src/execute/table/layout.rs @@ -12,35 +12,35 @@ use crate::plot::facet::Facet; use crate::plot::Labels; use crate::table::{ compose_header, create_caption, create_column_labels, create_heading, create_spanners, - move_stub_columns, reorder_table_columns, TableCell, TableClass, TableColumn, TableLayout, - TableRow, + groups_are_woven, move_stub_columns, reorder_table_columns, TableCell, TableClass, TableColumn, + TableLayout, TableRow, }; -use crate::{DataFrame, Format, GgsqlError, Result, Spanner}; +use crate::{ColumnSection, DataFrame, Format, GgsqlError, Result, Spanner}; /// Resolve a table's columns: one `TableColumn` per `DataFrame` column /// (labelled, with `formats`' resolved `SETTING` properties), reordered for -/// any `gather`-ing spanner and then for STUB columns. A `place => 'top'` -/// facet consumes its columns as group labels; `place => 'left'` keeps -/// them. `spans` must already be resolved (`Table::resolve_spanners`). +/// any `gather`-ing spanner and then for stub columns. A `place => 'top'` +/// facet consumes its columns as group labels; a `place => 'left'` facet +/// keeps them as group stub columns. `spans` must already be resolved +/// (`Table::resolve_spanners`). pub(super) fn setup_columns( df: &DataFrame, spans: &[Spanner], labels: &Labels, formats: &HashMap, - facet: Option<&crate::plot::facet::Facet>, + facet: Option<&Facet>, ) -> Vec { - let columns = create_table_columns(df, labels, formats); + let columns = create_table_columns(df, labels, formats, facet); let columns = reorder_table_columns(columns, spans); let mut columns = move_stub_columns(columns); let Some(facet) = facet else { return columns; }; - if facet.properties.get("place") - != Some(&crate::plot::ParameterValue::String("top".to_string())) - { - return columns; + if groups_are_woven(facet) { + // If we weave groups as rows, we omit the group columns + let variables = facet.get_variables(); + columns.retain(|c| !variables.contains(&c.name)); } - columns.retain(|c| !facet.get_variables().contains(&c.name)); columns } @@ -108,7 +108,13 @@ pub(super) fn build_cells( body_cells.extend(create_body(df, &columns)); let (body_rows, body_cells) = match facet { None => (rows, body_cells), - Some(facet) => weave_group_labels(df, &columns, &rows, body_cells, facet)?, + Some(facet) => { + if groups_are_woven(facet) { + weave_group_labels(df, &columns, &rows, body_cells, facet)? + } else { + (rows, merge_group_stub_cells(body_cells, &columns, df)) + } + } }; let table_body = TableLayout::new(columns.clone(), body_rows, body_cells); let layout = header.rowbind(table_body); @@ -124,12 +130,18 @@ pub(super) fn build_cells( /// order — `reorder_table_columns` is what may reorder this list /// afterward, not this function. `labels` is the one authority for an /// explicit column label; `formats` (already reshaped to one `Format` per -/// column by `setup_formats`) is the one authority for its properties. +/// column by `setup_formats`) is the one authority for its properties. A +/// `place => 'left'` facet marks its variables `GroupStub`. fn create_table_columns( df: &DataFrame, labels: &Labels, formats: &HashMap, + facet: Option<&Facet>, ) -> Vec { + let group_stub_variables = match facet { + Some(facet) if !groups_are_woven(facet) => facet.get_variables(), + _ => Vec::new(), + }; let mut columns = Vec::new(); for name in df.get_column_names() { // Not stored on TableColumn: nothing needs it once `properties` @@ -140,7 +152,12 @@ fn create_table_columns( .data_type(); let format = formats.get(&name); let properties = resolve_column_properties(dtype, format); - let target = format.map(|f| f.target).unwrap_or_default(); + // Group stubs record nesting order + let target = group_stub_variables + .iter() + .position(|v| *v == name) + .map(ColumnSection::GroupStub) + .unwrap_or_else(|| format.map(|f| f.target).unwrap_or_default()); let label = match labels.labels.get(&name) { // A stub head is a row-label header, not a data column header — // default it blank rather than to the column's own name. @@ -272,6 +289,64 @@ fn weave_group_labels( Ok((out_rows, label_cells)) } +/// Merge each group stub column's row labels into rowspanned cells. A cell +/// at depth `d` spans rows while the labels of every variable from the +/// outermost through `d` stay unchanged, so a group never merges into a +/// cell opened under a different outer level even when its own label +/// repeats. +fn merge_group_stub_cells( + cells: Vec, + columns: &[TableColumn], + df: &DataFrame, +) -> Vec { + let mut group_stub_columns: Vec<&TableColumn> = columns + .iter() + .filter(|c| matches!(c.target, ColumnSection::GroupStub(_))) + .collect(); + if group_stub_columns.is_empty() { + return cells; + } + group_stub_columns.sort_by_key(|c| c.target.sort_order()); + + // Prefix keys: keys[d][row] joins the labels of the outermost `d + 1` + // facet variables; equal keys on adjacent rows mean the cell may span + // the boundary. + let mut keys: Vec> = Vec::new(); + for column in &group_stub_columns { + let array = df + .column(&column.name) + .expect("TableColumn.name always names a column of df"); + let values = (0..df.height()).map(|row| value_to_string(array, row)); + let key = match keys.last() { + None => values.collect(), + Some(prefix) => prefix + .iter() + .zip(values) + .map(|(prefix, value)| format!("{prefix}\u{1f}{value}")) + .collect(), + }; + keys.push(key); + } + + // Column index of a group stub column -> position of its open cell in + // `out`, for the column whose cells are currently being walked. + let mut open: HashMap = HashMap::new(); + let mut out: Vec = Vec::with_capacity(cells.len()); + for cell in cells { + if let ColumnSection::GroupStub(depth) = columns[cell.left].target { + if let Some(&index) = open.get(&cell.left) { + if keys[depth][cell.top] == keys[depth][cell.top - 1] { + out[index].bottom = cell.bottom; + continue; + } + } + open.insert(cell.left, out.len()); + } + out.push(cell); + } + out +} + #[cfg(test)] mod tests { use super::super::format::{apply_formats, setup_formats}; @@ -344,7 +419,7 @@ mod 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, &HashMap::new()); + let columns = create_table_columns(&frame, &labels, &HashMap::new(), None); assert_eq!(columns[0].name, "id"); assert_eq!(columns[0].label, "ID"); // overridden @@ -369,14 +444,14 @@ mod tests { }, )]); - let columns = create_table_columns(&frame, &Labels::default(), &formats); + let columns = create_table_columns(&frame, &Labels::default(), &formats, None); assert_eq!(columns[0].label, ""); let mut labels = Labels::default(); labels .labels .insert("region".to_string(), Some("Region".to_string())); - let columns = create_table_columns(&frame, &labels, &formats); + let columns = create_table_columns(&frame, &labels, &formats, None); assert_eq!(columns[0].label, "Region"); } @@ -403,6 +478,46 @@ mod tests { spanner_with(columns, Some(label), Parameters::new()) } + fn left_facet(variables: &[&str]) -> Facet { + let mut properties = Parameters::new(); + properties.insert( + "place".to_string(), + ParameterValue::String("left".to_string()), + ); + Facet { + layout: crate::plot::facet::FacetLayout::Wrap { + variables: variables.iter().map(|s| s.to_string()).collect(), + }, + properties, + resolved: true, + } + } + + #[test] + fn setup_columns_left_facet_leads_with_group_stubs_in_variable_order() { + let frame = df! { + "region" => vec!["north".to_string()], + "quarter" => vec!["q1".to_string()], + "sales" => vec![1i32], + } + .unwrap(); + let facet = left_facet(&["quarter", "region"]); + + let columns = setup_columns( + &frame, + &[], + &Labels::default(), + &HashMap::new(), + Some(&facet), + ); + + let names: Vec<&str> = columns.iter().map(|c| c.name.as_str()).collect(); + assert_eq!(names, ["quarter", "region", "sales"]); + assert_eq!(columns[0].target, ColumnSection::GroupStub(0)); + assert_eq!(columns[1].target, ColumnSection::GroupStub(1)); + assert_eq!(columns[2].target, ColumnSection::Body); + } + #[test] fn create_body_numbers_rows_from_zero() { let frame = df! { @@ -410,7 +525,7 @@ mod tests { "name" => vec!["a".to_string(), "b".to_string()], } .unwrap(); - let columns = create_table_columns(&frame, &Labels::default(), &HashMap::new()); + let columns = create_table_columns(&frame, &Labels::default(), &HashMap::new(), None); let body = create_body(&frame, &columns); diff --git a/src/execute/table/mod.rs b/src/execute/table/mod.rs index 657f0a95f..25789762a 100644 --- a/src/execute/table/mod.rs +++ b/src/execute/table/mod.rs @@ -567,13 +567,51 @@ mod integration_tests { } #[test] - fn test_tabulate_facet_place_left_is_not_supported_yet() { - let reader = reader_with_sales(); - let result = resolve_table_with_reader( - "TABULATE * FROM sales FACET name SETTING place => 'left'", + fn test_tabulate_facet_place_left_merges_group_labels_into_rowspans() { + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + reader + .execute_sql( + "CREATE TABLE t AS SELECT * FROM (VALUES \ + ('north', 'q1', 1), ('north', 'q2', 2), ('south', 'q2', 3)) \ + AS t(region, quarter, sales)", + ) + .unwrap(); + + let resolved = resolve_table_with_reader( + "TABULATE * FROM t FACET region, quarter SETTING place => 'left'", &reader, + ) + .unwrap(); + + // Facet columns lead as group stubs, in nesting order. + let names: Vec<&str> = resolved.columns().iter().map(|c| c.name.as_str()).collect(); + assert_eq!(names, ["region", "quarter", "sales"]); + assert!(resolved.columns()[0].is_stub()); + assert!(resolved.columns()[1].is_stub()); + + let mut labels: Vec<_> = resolved + .cells() + .iter() + .filter(|c| c.kind == TableCellKind::StubRowLabel) + .collect(); + labels.sort_by_key(|c| (c.left, c.top)); + let extents: Vec<(&str, usize, usize, usize)> = labels + .iter() + .map(|c| (c.content.as_str(), c.left, c.top, c.bottom)) + .collect(); + // Row 0 is the column-label row. region spans both north rows; + // quarter's q2 splits at the region boundary even though the label + // repeats. + assert_eq!( + extents, + [ + ("north", 0, 1, 2), + ("south", 0, 3, 3), + ("q1", 1, 1, 1), + ("q2", 1, 2, 2), + ("q2", 1, 3, 3), + ] ); - assert!(result.is_err()); } #[test] diff --git a/src/table/facet.rs b/src/table/facet.rs index 0404d95cd..31a0d6a84 100644 --- a/src/table/facet.rs +++ b/src/table/facet.rs @@ -49,14 +49,18 @@ pub(crate) fn setup_facet(facet: &Option) -> Result, String "place".to_string(), ParameterValue::String("top".to_string()), ); - } else if resolved.properties.get("place") == Some(&ParameterValue::String("left".to_string())) - { - return Err("TABULATE FACET SETTING place => 'left' is not supported yet".to_string()); } resolved.resolved = true; Ok(Some(resolved)) } +/// Whether a resolved table facet weaves its group labels in as interleaved +/// rows (`place => 'top'`) rather than group stub columns (`place => +/// 'left'`). +pub(crate) fn groups_are_woven(facet: &Facet) -> bool { + facet.properties.get("place") == Some(&ParameterValue::String("top".to_string())) +} + #[cfg(test)] mod tests { use super::*; @@ -87,7 +91,7 @@ mod tests { } #[test] - fn setup_rejects_place_left_until_group_stub_columns_land() { + fn setup_accepts_place_left() { let mut settings = Parameters::new(); settings.insert( "place".to_string(), @@ -95,7 +99,12 @@ mod tests { ); let facet = table_facet(settings); - assert!(setup_facet(&Some(facet)).is_err()); + let resolved = setup_facet(&Some(facet)).unwrap().unwrap(); + + assert_eq!( + resolved.properties.get("place"), + Some(&ParameterValue::String("left".to_string())) + ); } #[test] diff --git a/src/table/layout.rs b/src/table/layout.rs index 8e2875f11..55b0c943d 100644 --- a/src/table/layout.rs +++ b/src/table/layout.rs @@ -928,14 +928,14 @@ pub(crate) fn extract_heading_labels( (title, subtitle, caption) } -/// Move every STUB-targeted column to the front, preserving each group's +/// Move every stub column to the front — group stub columns first, in +/// nesting-depth order, then STUB columns — preserving each section's /// relative order. Runs after `reorder_table_columns`, as the final word on /// column position — a SPAN can never cover a stub column /// (`Table::validate_boundaries`). -pub(crate) fn move_stub_columns(columns: Vec) -> Vec { - let (mut stub, body): (Vec<_>, Vec<_>) = columns.into_iter().partition(TableColumn::is_stub); - stub.extend(body); - stub +pub(crate) fn move_stub_columns(mut columns: Vec) -> Vec { + columns.sort_by_key(|c| c.target.sort_order()); + columns } /// Stack spanner rows above column labels, letting a column with no spanner /// covering it stretch its own label upward over the gap, then fill the @@ -1094,6 +1094,27 @@ mod assembly_tests { ); } + #[test] + fn move_stub_columns_puts_group_stubs_first_in_depth_order() { + let group_stub = |name: &str, depth: usize| TableColumn { + target: ColumnSection::GroupStub(depth), + ..column(name, name) + }; + let columns = vec![ + column("a", "a"), + stub_column("b"), + group_stub("c", 1), + group_stub("d", 0), + ]; + + let result = move_stub_columns(columns); + + assert_eq!( + result.iter().map(|c| c.name.as_str()).collect::>(), + vec!["d", "c", "b", "a"] + ); + } + #[test] fn move_stub_columns_is_a_no_op_with_no_stub_columns() { let columns = vec![column("a", "a"), column("b", "b")]; diff --git a/src/table/mod.rs b/src/table/mod.rs index f056ebb64..1f42393fb 100644 --- a/src/table/mod.rs +++ b/src/table/mod.rs @@ -11,7 +11,7 @@ mod facet; mod layout; mod spanner; -pub(crate) use facet::setup_facet; +pub(crate) use facet::{groups_are_woven, setup_facet}; pub(crate) use layout::{ compose_header, create_caption, create_column_labels, create_heading, extract_heading_labels, move_stub_columns, standardise_hjust, Slot, @@ -210,23 +210,38 @@ impl Table { } } -/// Which section of the table a `FORMAT` clause's columns belong to. +/// Which section of the table a column belongs to. /// -/// `FORMAT STUB` is the only thing that assigns a column to the table's -/// stub; `FORMAT BODY`, or no target identifier at all, leaves columns in -/// the regular body — `Body` is this enum's default for that reason. +/// `FORMAT STUB` assigns a column to the table's stub and a `TABULATE +/// FACET` with `place => 'left'` assigns its variables to group stubs; +/// `FORMAT BODY`, or no target identifier at all, leaves columns in the +/// regular body — `Body` is this enum's default for that reason. +/// `GroupStub` is never reachable from a `FORMAT` target. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] pub enum ColumnSection { #[default] Body, Stub, + /// A `TABULATE FACET` variable's column with `place => 'left'`; the + /// payload is the variable's nesting depth (0 is outermost). + GroupStub(usize), } impl ColumnSection { - /// Whether this is the `STUB` section. + /// Whether this is a stub section (`STUB` or a facet group stub). pub fn is_stub(self) -> bool { - self == ColumnSection::Stub + matches!(self, ColumnSection::Stub | ColumnSection::GroupStub(_)) + } + + /// Section ordering for column placement: group stub columns first (by + /// nesting depth), then STUB, then body. + pub(crate) fn sort_order(self) -> (usize, usize) { + match self { + ColumnSection::GroupStub(depth) => (0, depth), + ColumnSection::Stub => (1, 0), + ColumnSection::Body => (2, 0), + } } } From 954e16969489e619d1710e71f99ae6428d6147a8 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 15 Oct 2026 15:03:09 +0200 Subject: [PATCH 07/25] Identify TABULATE FACET groups by per-variable rank keys --- src/execute/table/facet.rs | 84 ++++++++++++++++++------------------- src/execute/table/layout.rs | 74 ++++++++++++-------------------- src/execute/table/mod.rs | 31 ++++++++++++++ src/table/layout.rs | 17 ++++---- 4 files changed, 106 insertions(+), 100 deletions(-) diff --git a/src/execute/table/facet.rs b/src/execute/table/facet.rs index 24387b296..31801bf0e 100644 --- a/src/execute/table/facet.rs +++ b/src/execute/table/facet.rs @@ -1,4 +1,4 @@ -//! `TABULATE FACET` grouping: assigns each `DataFrame` row a group id from +//! `TABULATE FACET` grouping: assigns each `DataFrame` row a group key from //! its facet values, then reorders rows so groups are contiguous. use std::collections::HashMap; @@ -8,12 +8,15 @@ use arrow::array::UInt32Array; use crate::array_util::value_to_string; use crate::DataFrame; -/// Assign each `df` row a group id. Groups are ordered nested by variable: -/// each variable's levels are ranked by first appearance, and group ids -/// follow lexicographic order of the rank tuples, so the first variable -/// forms the outermost grouping. Rows that are null in every facet variable -/// share one group. -pub(super) fn setup_groups(variables: &[String], df: &DataFrame) -> Result, String> { +/// Assign each `df` row its group key: one rank per facet variable, levels +/// ranked by first appearance. Lexicographic comparison of keys gives the +/// nested group order (first variable outermost); a key's prefix identifies +/// the row's group at that nesting depth. Rows that are null in a variable +/// share one rank for it. +pub(super) fn setup_groups( + variables: &[String], + df: &DataFrame, +) -> Result>, String> { let columns: Vec<_> = variables .iter() .map(|name| { @@ -24,9 +27,9 @@ pub(super) fn setup_groups(variables: &[String], df: &DataFrame) -> Result, usize>> = vec![HashMap::new(); columns.len()]; - let mut tuples = Vec::with_capacity(df.height()); + let mut keys = Vec::with_capacity(df.height()); for row in 0..df.height() { - let tuple: Vec = columns + let key: Vec = columns .iter() .zip(&mut levels) .map(|(column, ranks)| { @@ -39,39 +42,32 @@ pub(super) fn setup_groups(variables: &[String], df: &DataFrame) -> Result, usize> = unique - .into_iter() - .enumerate() - .map(|(id, tuple)| (tuple, id)) - .collect(); - - Ok(tuples.into_iter().map(|tuple| group_of[&tuple]).collect()) + Ok(keys) } -/// Reorder `df` so rows with the same group id are contiguous, keeping the -/// query's row order within each group. Returns the reordered frame and the -/// group ids in their new order. +/// Reorder `df` so rows with equal group keys are contiguous in +/// lexicographic key order, keeping the query's row order within each +/// group. Returns the reordered frame and the keys in their new order. pub(super) fn reorder_rows( df: &DataFrame, - ids: Vec, -) -> Result<(DataFrame, Vec), String> { + keys: Vec>, +) -> Result<(DataFrame, Vec>), String> { let mut order: Vec = (0..df.height() as u32).collect(); - // sort_by_key is stable: within-group row order is preserved. - order.sort_by_key(|&i| ids[i as usize]); + // sort_by is stable: within-group row order is preserved. + order.sort_by(|&a, &b| keys[a as usize].cmp(&keys[b as usize])); let df = df .take(&UInt32Array::from(order.clone())) .map_err(|e| e.to_string())?; - let ids = order.into_iter().map(|i| ids[i as usize]).collect(); + let keys = order + .into_iter() + .map(|i| keys[i as usize].clone()) + .collect(); - Ok((df, ids)) + Ok((df, keys)) } #[cfg(test)] @@ -84,16 +80,16 @@ mod tests { } #[test] - fn setup_groups_assigns_ids_in_first_appearance_order() { + fn setup_groups_assigns_keys_in_first_appearance_order() { let frame = df! { "region" => vec!["south".to_string(), "north".to_string(), "south".to_string()], "sales" => vec![1i32, 2, 3], } .unwrap(); - let ids = setup_groups(&variables(&["region"]), &frame).unwrap(); + let keys = setup_groups(&variables(&["region"]), &frame).unwrap(); - assert_eq!(ids, vec![0, 1, 0]); + assert_eq!(keys, vec![vec![0], vec![1], vec![0]]); } #[test] @@ -104,14 +100,14 @@ mod tests { } .unwrap(); - let ids = setup_groups(&variables(&["region", "quarter"]), &frame).unwrap(); + let keys = setup_groups(&variables(&["region", "quarter"]), &frame).unwrap(); - assert_eq!(ids, vec![0, 1]); + assert_eq!(keys, vec![vec![0, 0], vec![0, 1]]); } #[test] - fn setup_groups_orders_ids_nested_by_variable() { - // Row order interleaves the variables; ids follow the nested order + fn setup_groups_orders_keys_nested_by_variable() { + // Row order interleaves the variables; keys follow the nested order // (a,x), (a,y), (b,x), (b,y) instead. let frame = df! { "a" => vec!["a".to_string(), "b".to_string(), "a".to_string(), "b".to_string()], @@ -119,9 +115,9 @@ mod tests { } .unwrap(); - let ids = setup_groups(&variables(&["a", "b"]), &frame).unwrap(); + let keys = setup_groups(&variables(&["a", "b"]), &frame).unwrap(); - assert_eq!(ids, vec![0, 2, 1, 3]); + assert_eq!(keys, vec![vec![0, 0], vec![1, 0], vec![0, 1], vec![1, 1]]); } #[test] @@ -140,11 +136,11 @@ mod tests { "sales" => vec![1i32, 2, 3], } .unwrap(); - let ids = setup_groups(&variables(&["region"]), &frame).unwrap(); + let keys = setup_groups(&variables(&["region"]), &frame).unwrap(); - let (frame, ids) = reorder_rows(&frame, ids).unwrap(); + let (frame, keys) = reorder_rows(&frame, keys).unwrap(); - assert_eq!(ids, vec![0, 0, 1]); + assert_eq!(keys, vec![vec![0], vec![0], vec![1]]); assert_eq!( frame .column("sales") @@ -165,8 +161,8 @@ mod tests { )]) .unwrap(); - let ids = setup_groups(&variables(&["region"]), &frame).unwrap(); + let keys = setup_groups(&variables(&["region"]), &frame).unwrap(); - assert_eq!(ids, vec![0, 1, 1]); + assert_eq!(keys, vec![vec![0], vec![1], vec![1]]); } } diff --git a/src/execute/table/layout.rs b/src/execute/table/layout.rs index cc3df0959..ebaceb67a 100644 --- a/src/execute/table/layout.rs +++ b/src/execute/table/layout.rs @@ -45,7 +45,7 @@ pub(super) fn setup_columns( } /// Resolve a table's body rows: one `TableRow` per `df` row, stamped with -/// its facet group. A facet groups on the raw query values (before +/// its facet group key. A facet groups on the raw query values (before /// `FORMAT`'s RENAMING rewrites them) and reorders `df` so groups are /// contiguous. Group-label rows are not part of the result — `build_cells` /// interleaves those once labels can be read from the formatted frame. @@ -57,13 +57,13 @@ pub(super) fn setup_rows( let rows = vec![TableRow::body(); df.height()]; return Ok((df, rows)); }; - let ids = super::facet::setup_groups(&facet.get_variables(), &df) + let keys = super::facet::setup_groups(&facet.get_variables(), &df) .map_err(GgsqlError::ValidationError)?; - let (df, ids) = super::facet::reorder_rows(&df, ids).map_err(GgsqlError::ValidationError)?; - let rows = ids + let (df, keys) = super::facet::reorder_rows(&df, keys).map_err(GgsqlError::ValidationError)?; + let rows = keys .into_iter() - .map(|id| TableRow { - group: Some(id), + .map(|key| TableRow { + group: key, ..TableRow::body() }) .collect(); @@ -112,7 +112,8 @@ pub(super) fn build_cells( if groups_are_woven(facet) { weave_group_labels(df, &columns, &rows, body_cells, facet)? } else { - (rows, merge_group_stub_cells(body_cells, &columns, df)) + let body_cells = merge_group_stub_cells(body_cells, &columns, &rows); + (rows, body_cells) } } }; @@ -234,14 +235,12 @@ fn weave_group_labels( let mut label_cells = Vec::new(); // body_offset[data row] = how many label rows precede it in the grid. let mut body_offset = vec![0usize; df.height()]; - // None until the first group's labels are read: it emits every level. - let mut previous_labels: Option> = None; + // None until the first group's key is read: it emits every level. + let mut previous_key: Option<&[usize]> = None; let mut row = 0usize; for group_rows in rows.chunk_by(|a, b| a.group == b.group) { - let id = group_rows[0] - .group - .expect("setup_rows stamps every body row"); + let key = group_rows[0].group.as_slice(); let labels: Vec = facet_arrays .iter() .map(|array| value_to_string(array, row)) @@ -249,13 +248,13 @@ fn weave_group_labels( // When multiple labels exist, we have a nesting pattern. // We don't need to repeat every level label at every group, // just from the level(s) that changed onward. - let first_changed = match &previous_labels { + let first_changed = match previous_key { None => 0, Some(previous) => previous .iter() - .zip(&labels) + .zip(key) .position(|(old, new)| old != new) - .unwrap_or(labels.len()), + .unwrap_or(key.len()), }; for label in &labels[first_changed..] { label_cells.push(TableCell::new_group_heading( @@ -263,9 +262,9 @@ fn weave_group_labels( columns.len(), label.clone(), )); - out_rows.push(TableRow::group_heading(id)); + out_rows.push(TableRow::group_heading(key.to_vec())); } - previous_labels = Some(labels); + previous_key = Some(key); for (index, table_row) in group_rows.iter().enumerate() { let mut table_row = table_row.clone(); @@ -290,43 +289,20 @@ fn weave_group_labels( } /// Merge each group stub column's row labels into rowspanned cells. A cell -/// at depth `d` spans rows while the labels of every variable from the -/// outermost through `d` stay unchanged, so a group never merges into a -/// cell opened under a different outer level even when its own label -/// repeats. +/// at depth `d` spans rows while the `d + 1` outermost ranks of the rows' +/// group keys are equal, so a group never merges into a cell opened under +/// a different outer level even when its label repeats. fn merge_group_stub_cells( cells: Vec, columns: &[TableColumn], - df: &DataFrame, + rows: &[TableRow], ) -> Vec { - let mut group_stub_columns: Vec<&TableColumn> = columns + if !columns .iter() - .filter(|c| matches!(c.target, ColumnSection::GroupStub(_))) - .collect(); - if group_stub_columns.is_empty() { + .any(|c| matches!(c.target, ColumnSection::GroupStub(_))) + { return cells; } - group_stub_columns.sort_by_key(|c| c.target.sort_order()); - - // Prefix keys: keys[d][row] joins the labels of the outermost `d + 1` - // facet variables; equal keys on adjacent rows mean the cell may span - // the boundary. - let mut keys: Vec> = Vec::new(); - for column in &group_stub_columns { - let array = df - .column(&column.name) - .expect("TableColumn.name always names a column of df"); - let values = (0..df.height()).map(|row| value_to_string(array, row)); - let key = match keys.last() { - None => values.collect(), - Some(prefix) => prefix - .iter() - .zip(values) - .map(|(prefix, value)| format!("{prefix}\u{1f}{value}")) - .collect(), - }; - keys.push(key); - } // Column index of a group stub column -> position of its open cell in // `out`, for the column whose cells are currently being walked. @@ -335,7 +311,9 @@ fn merge_group_stub_cells( for cell in cells { if let ColumnSection::GroupStub(depth) = columns[cell.left].target { if let Some(&index) = open.get(&cell.left) { - if keys[depth][cell.top] == keys[depth][cell.top - 1] { + let current_key = &rows[cell.top].group[..=depth]; + let previous_key = &rows[cell.top - 1].group[..=depth]; + if current_key == previous_key { out[index].bottom = cell.bottom; continue; } diff --git a/src/execute/table/mod.rs b/src/execute/table/mod.rs index 25789762a..c5e3047a8 100644 --- a/src/execute/table/mod.rs +++ b/src/execute/table/mod.rs @@ -551,6 +551,37 @@ mod integration_tests { assert_eq!(tops, [1, 2, 4, 6, 7]); } + #[test] + fn test_tabulate_facet_place_left_splits_groups_with_equal_labels() { + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + reader + .execute_sql( + "CREATE TABLE t AS SELECT * FROM (VALUES ('null', 1), (NULL, 2)) \ + AS t(name, sales)", + ) + .unwrap(); + + // NULL and the string 'null' render identically but are distinct + // groups; their cells must not merge into one rowspan. + let resolved = resolve_table_with_reader( + "TABULATE * FROM t FACET name SETTING place => 'left'", + &reader, + ) + .unwrap(); + + let mut labels: Vec<_> = resolved + .cells() + .iter() + .filter(|c| c.kind == TableCellKind::StubRowLabel) + .collect(); + labels.sort_by_key(|c| c.top); + let extents: Vec<(&str, usize, usize)> = labels + .iter() + .map(|c| (c.content.as_str(), c.top, c.bottom)) + .collect(); + assert_eq!(extents, [("null", 1, 1), ("null", 2, 2)]); + } + #[test] fn test_tabulate_facet_covering_every_column_errors() { let reader = reader_with_sales(); diff --git a/src/table/layout.rs b/src/table/layout.rs index 55b0c943d..be91585df 100644 --- a/src/table/layout.rs +++ b/src/table/layout.rs @@ -422,10 +422,11 @@ pub struct TableRow { /// Whether this row is the caption's — always the layout's last row and /// never also a header (`TableLayout::validate` enforces both). pub is_caption: bool, - /// Group this body row belongs to (from `TABULATE FACET`), `None` for - /// header rows and tables without a facet. Group labels live only in - /// cells, so this is just an id. - pub group: Option, + /// This body row's group key (from `TABULATE FACET`): one rank per + /// facet variable, outermost first; a prefix identifies the row's group + /// at that nesting depth. Empty for header rows and tables without a + /// facet. Group labels live only in cells, so this is just a key. + pub group: Vec, } impl TableRow { @@ -450,12 +451,12 @@ impl TableRow { Self::default() } - /// A group-label row (`TableClass::GroupHeadingRow`) belonging to - /// `group` — a body row, like the rows it labels. - pub fn group_heading(group: usize) -> Self { + /// A group-label row (`TableClass::GroupHeadingRow`) keyed by `group` — + /// a body row, like the rows it labels. + pub fn group_heading(group: Vec) -> Self { Self { classes: vec![TableClass::GroupHeadingRow], - group: Some(group), + group, ..Self::body() } } From 191d80f1872c39d174a427aea56a046bd71ccf87 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 15 Oct 2026 16:44:24 +0200 Subject: [PATCH 08/25] Style left-placed TABULATE FACET labels as stub row groups --- src/execute/table/layout.rs | 25 ++++++++++++------------- src/execute/table/mod.rs | 29 ++++++++++++++++++++--------- src/execute/table/stub.rs | 16 ++++++++++------ src/table/layout.rs | 10 ++++++++++ src/writer/html.rs | 14 ++++++++++++++ 5 files changed, 66 insertions(+), 28 deletions(-) diff --git a/src/execute/table/layout.rs b/src/execute/table/layout.rs index ebaceb67a..dfed040de 100644 --- a/src/execute/table/layout.rs +++ b/src/execute/table/layout.rs @@ -60,13 +60,18 @@ pub(super) fn setup_rows( let keys = super::facet::setup_groups(&facet.get_variables(), &df) .map_err(GgsqlError::ValidationError)?; let (df, keys) = super::facet::reorder_rows(&df, keys).map_err(GgsqlError::ValidationError)?; - let rows = keys - .into_iter() - .map(|key| TableRow { + let mut rows = Vec::with_capacity(keys.len()); + for key in keys { + let first_in_group = rows.last().is_none_or(|row: &TableRow| row.group != key); + let mut row = TableRow { group: key, ..TableRow::body() - }) - .collect(); + }; + if first_in_group { + row.classes.push(TableClass::RowGroupFirst); + } + rows.push(row); + } Ok((df, rows)) } @@ -266,15 +271,9 @@ fn weave_group_labels( } previous_key = Some(key); - for (index, table_row) in group_rows.iter().enumerate() { - let mut table_row = table_row.clone(); - // First row in group needs special class. Index is within chunk - // of groups, so first row in group has index 0. - if index == 0 { - table_row.classes.push(TableClass::RowGroupFirst); - } + for table_row in group_rows.iter() { body_offset[row] = out_rows.len() - row; - out_rows.push(table_row); + out_rows.push(table_row.clone()); row += 1; } } diff --git a/src/execute/table/mod.rs b/src/execute/table/mod.rs index c5e3047a8..612dc8ba0 100644 --- a/src/execute/table/mod.rs +++ b/src/execute/table/mod.rs @@ -252,7 +252,7 @@ fn wrap_selection(dialect: &dyn SqlDialect, selection: &[SelectionItem], query: mod integration_tests { use super::*; use crate::reader::DuckDBReader; - use crate::table::TableCellKind; + use crate::table::{TableCellKind, TableClass}; fn reader_with_sales() -> DuckDBReader { let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); @@ -572,7 +572,7 @@ mod integration_tests { let mut labels: Vec<_> = resolved .cells() .iter() - .filter(|c| c.kind == TableCellKind::StubRowLabel) + .filter(|c| c.kind == TableCellKind::GroupHeading) .collect(); labels.sort_by_key(|c| c.top); let extents: Vec<(&str, usize, usize)> = labels @@ -603,7 +603,8 @@ mod integration_tests { reader .execute_sql( "CREATE TABLE t AS SELECT * FROM (VALUES \ - ('north', 'q1', 1), ('north', 'q2', 2), ('south', 'q2', 3)) \ + ('north', 'q1', 1), ('north', 'q2', 2), ('south', 'q2', 3), \ + ('south', 'q2', 4)) \ AS t(region, quarter, sales)", ) .unwrap(); @@ -623,26 +624,36 @@ mod integration_tests { let mut labels: Vec<_> = resolved .cells() .iter() - .filter(|c| c.kind == TableCellKind::StubRowLabel) + .filter(|c| c.kind == TableCellKind::GroupHeading) .collect(); labels.sort_by_key(|c| (c.left, c.top)); let extents: Vec<(&str, usize, usize, usize)> = labels .iter() .map(|c| (c.content.as_str(), c.left, c.top, c.bottom)) .collect(); - // Row 0 is the column-label row. region spans both north rows; - // quarter's q2 splits at the region boundary even though the label - // repeats. + // Row 0 is the column-label row. region spans both north rows and + // both south rows; quarter's q2 splits at the region boundary even + // though the label repeats, and merges within south's group. assert_eq!( extents, [ ("north", 0, 1, 2), - ("south", 0, 3, 3), + ("south", 0, 3, 4), ("q1", 1, 1, 1), ("q2", 1, 2, 2), - ("q2", 1, 3, 3), + ("q2", 1, 3, 4), ] ); + + // The first body row of each group carries the group separator. + let firsts: Vec = resolved + .rows() + .iter() + .enumerate() + .filter(|(_, row)| row.classes.contains(&TableClass::RowGroupFirst)) + .map(|(index, _)| index) + .collect(); + assert_eq!(firsts, [1, 2, 3]); } #[test] diff --git a/src/execute/table/stub.rs b/src/execute/table/stub.rs index 3a0f427ea..fbcbcbf42 100644 --- a/src/execute/table/stub.rs +++ b/src/execute/table/stub.rs @@ -3,7 +3,7 @@ use crate::array_util::value_to_string; use crate::table::{TableCell, TableColumn}; -use crate::DataFrame; +use crate::{ColumnSection, DataFrame}; /// Build one `StubHead` cell per stub column, numbered from `top == 0`. /// Skips every non-stub column. @@ -19,8 +19,10 @@ pub(crate) fn create_stubhead(columns: &[TableColumn]) -> Vec { .collect() } -/// Build one `StubRowLabel` cell per `DataFrame` value in every stub -/// column, numbered from `top == 0`. Skips every non-stub column. +/// Build one row-label cell per `DataFrame` value in every stub column, +/// numbered from `top == 0`: `StubRowLabel` cells for `STUB` columns, +/// `GroupHeading` cells for group stub columns. Skips every non-stub +/// column. pub(crate) fn create_row_labels(df: &DataFrame, columns: &[TableColumn]) -> Vec { let mut cells = Vec::new(); @@ -34,10 +36,12 @@ pub(crate) fn create_row_labels(df: &DataFrame, columns: &[TableColumn]) -> Vec< .expect("TableColumn.name always names a column of df"); for row in 0..df.height() { - cells.push( + let cell = if matches!(column.target, ColumnSection::GroupStub(_)) { + TableCell::new_stub_row_group(row, index, value_to_string(array, row)) + } else { TableCell::new_stub_row_label(row, index, value_to_string(array, row)) - .with_properties(column.properties.clone()), - ); + }; + cells.push(cell.with_properties(column.properties.clone())); } } diff --git a/src/table/layout.rs b/src/table/layout.rs index be91585df..3da843d14 100644 --- a/src/table/layout.rs +++ b/src/table/layout.rs @@ -134,6 +134,9 @@ pub enum TableClass { Row, /// A row-label cell in the stub (`TableCellKind::StubRowLabel`). Stub, + /// A group-label cell in a group stub column (`TableCellKind:: + /// GroupHeading`, gt's `gt_stub_row_group`). + StubRowGroup, /// The stub's own header cell (`TableCellKind::StubHead`). No gt /// equivalent. StubHead, @@ -170,6 +173,7 @@ impl std::fmt::Display for TableClass { TableClass::TableBody => "table_body", TableClass::Row => "row", TableClass::Stub => "stub", + TableClass::StubRowGroup => "stub_row_group", TableClass::ColHeading => "col_heading", TableClass::ColHeadingRow => "col_heading_row", TableClass::SpannerRow => "spanner_row", @@ -268,6 +272,12 @@ impl TableCell { .with_classes(vec![TableClass::Stub]) } + /// A group-stub group-label cell (`TableClass::StubRowGroup`). + pub(crate) fn new_stub_row_group(row: usize, index: usize, content: String) -> Self { + Self::new(TableCellKind::GroupHeading, row, row, index, index, content) + .with_classes(vec![TableClass::StubRowGroup, TableClass::Row]) + } + /// A spanner cell (`TableClass::Spanner`, `SpannerLabel`, centered). pub(crate) fn new_spanner(row: usize, left: usize, right: usize, content: String) -> Self { Self::new(TableCellKind::Spanner, row, row, left, right, content).with_classes(vec![ diff --git a/src/writer/html.rs b/src/writer/html.rs index 19ecd2d18..cececf170 100644 --- a/src/writer/html.rs +++ b/src/writer/html.rs @@ -225,6 +225,19 @@ fn class_declarations(class: TableClass) -> &'static [(&'static str, &'static st ("padding-left", "5px"), ("padding-right", "5px"), ], + TableClass::StubRowGroup => &[ + ("color", "#333333"), + ("background-color", "#FFFFFF"), + ("font-size", "100%"), + ("font-weight", "initial"), + ("text-transform", "inherit"), + ("border-right-style", "solid"), + ("border-right-width", "2px"), + ("border-right-color", "#D3D3D3"), + ("padding-left", "5px"), + ("padding-right", "5px"), + ("vertical-align", "top"), + ], TableClass::ColHeading => &[ ("color", "#333333"), ("background-color", "#FFFFFF"), @@ -366,6 +379,7 @@ const STYLED_CLASSES: &[TableClass] = &[ TableClass::TableBody, TableClass::Row, TableClass::Stub, + TableClass::StubRowGroup, TableClass::ColHeading, TableClass::ColHeadingRow, TableClass::Spanner, From 0084df9499fea8dbea37e5134a0dbbbea366d52b Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 15 Oct 2026 17:40:22 +0200 Subject: [PATCH 09/25] Box Spec::Table and bundle table titles into TableTitles --- src/execute/table/layout.rs | 29 +++++++++++------------------ src/execute/table/mod.rs | 15 +++------------ src/parser/builder.rs | 6 +++++- src/spec.rs | 14 +++++++------- src/table/layout.rs | 27 +++++++++++++++++---------- src/table/mod.rs | 4 ++-- 6 files changed, 45 insertions(+), 50 deletions(-) diff --git a/src/execute/table/layout.rs b/src/execute/table/layout.rs index dfed040de..31d26e05b 100644 --- a/src/execute/table/layout.rs +++ b/src/execute/table/layout.rs @@ -13,7 +13,7 @@ use crate::plot::Labels; use crate::table::{ compose_header, create_caption, create_column_labels, create_heading, create_spanners, groups_are_woven, move_stub_columns, reorder_table_columns, TableCell, TableClass, TableColumn, - TableLayout, TableRow, + TableLayout, TableRow, TableTitles, }; use crate::{ColumnSection, DataFrame, Format, GgsqlError, Result, Spanner}; @@ -83,9 +83,7 @@ pub(super) fn build_cells( columns: Vec, rows: Vec, spans: &[Spanner], - title: Option<&str>, - subtitle: Option<&str>, - caption: Option<&str>, + titles: &TableTitles, facet: Option<&Facet>, ) -> Result { let spanners = create_spanners(&columns, spans); @@ -107,7 +105,11 @@ pub(super) fn build_cells( ) }; let header = compose_header(spanners, column_labels); - let heading = create_heading(title, subtitle, &columns); + let heading = create_heading( + titles.title.as_deref(), + titles.subtitle.as_deref(), + &columns, + ); let header = heading.rowbind(header); let mut body_cells = create_row_labels(df, &columns); body_cells.extend(create_body(df, &columns)); @@ -124,7 +126,7 @@ pub(super) fn build_cells( }; let table_body = TableLayout::new(columns.clone(), body_rows, body_cells); let layout = header.rowbind(table_body); - let caption_layout = create_caption(caption, &columns); + let caption_layout = create_caption(titles.caption.as_deref(), &columns); let layout = layout.rowbind(caption_layout); layout.validate()?; @@ -330,7 +332,7 @@ mod tests { use super::*; use crate::df; use crate::plot::{ParameterValue, Parameters}; - use crate::table::{extract_heading_labels, TableCellKind}; + use crate::table::{extract_table_titles, TableCellKind}; use crate::{ColumnSection, GgsqlError, Spanner, Table}; /// Runs the same steps `resolve_table_with_reader` does, minus the @@ -338,7 +340,7 @@ mod tests { /// directly against a `df!()`-built `DataFrame`. fn resolve_layout(df: &DataFrame, table: &Table) -> Result { let mut labels = table.labels.clone(); - let (title, subtitle, caption) = extract_heading_labels(&mut labels); + let titles = extract_table_titles(&mut labels); let column_names = df.get_column_names(); let spans = table .resolve_spanners(&labels, Some(&column_names)) @@ -347,16 +349,7 @@ mod tests { let columns = setup_columns(df, &spans, &labels, &formats, None); let df = apply_formats(df, &formats)?; let rows = vec![TableRow::body(); df.height()]; - build_cells( - &df, - columns, - rows, - &spans, - title.as_deref(), - subtitle.as_deref(), - caption.as_deref(), - None, - ) + build_cells(&df, columns, rows, &spans, &titles, None) } /// `resolve_layout`, for the (common) tests that only care about cells. diff --git a/src/execute/table/mod.rs b/src/execute/table/mod.rs index 612dc8ba0..527080a74 100644 --- a/src/execute/table/mod.rs +++ b/src/execute/table/mod.rs @@ -15,7 +15,7 @@ mod stub; use format::{apply_formats, setup_formats}; use layout::{build_cells, setup_columns, setup_rows}; -use crate::table::extract_heading_labels; +use crate::table::extract_table_titles; use super::cte::split_with_query; use crate::naming; @@ -46,7 +46,7 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result Result Result> { } "tabulate_statement" => { let table = build_tabulate_statement(&stmt_node, source)?; - (table.source.is_some(), "TABULATE", Spec::Table(table)) + ( + table.source.is_some(), + "TABULATE", + Spec::Table(Box::new(table)), + ) } other => { return Err(GgsqlError::InternalError(format!( diff --git a/src/spec.rs b/src/spec.rs index f37edea76..86ed19a43 100644 --- a/src/spec.rs +++ b/src/spec.rs @@ -10,10 +10,10 @@ use crate::{Plot, Table}; /// (`parser::parse_query` returns one `Spec` per statement, in source order). #[derive(Debug, Clone, Serialize, Deserialize)] pub enum Spec { - // Boxed: `Plot` is far larger than `Table`, and clippy flags the - // resulting size gap (`large_enum_variant`) otherwise. + // Both boxed: whichever of `Plot`/`Table` is larger, an unboxed size gap + // between them trips clippy's `large_enum_variant` lint. Plot(Box), - Table(Table), + Table(Box), } impl Spec { @@ -45,7 +45,7 @@ impl Spec { pub fn into_table(self) -> Option
{ match self { Spec::Plot(_) => None, - Spec::Table(table) => Some(table), + Spec::Table(table) => Some(*table), } } } @@ -60,7 +60,7 @@ mod tests { assert!(plot.as_plot().is_some()); assert!(plot.as_table().is_none()); - let table = Spec::Table(Table::default()); + let table = Spec::Table(Box::default()); assert!(table.as_plot().is_none()); assert!(table.as_table().is_some()); } @@ -70,10 +70,10 @@ mod tests { let plot = Spec::Plot(Box::default()); assert!(plot.into_plot().is_some()); - let table = Spec::Table(Table::default()); + let table = Spec::Table(Box::default()); assert!(table.into_table().is_some()); assert!(Spec::Plot(Box::default()).into_table().is_none()); - assert!(Spec::Table(Table::default()).into_plot().is_none()); + assert!(Spec::Table(Box::default()).into_plot().is_none()); } } diff --git a/src/table/layout.rs b/src/table/layout.rs index 3da843d14..54e9d7344 100644 --- a/src/table/layout.rs +++ b/src/table/layout.rs @@ -926,17 +926,24 @@ pub(crate) fn create_column_labels(columns: &[TableColumn]) -> Vec { .collect() } +/// A table's resolved `LABEL title`/`subtitle`/`caption` texts. +#[derive(Debug, Clone, Default)] +pub(crate) struct TableTitles { + pub title: Option, + pub subtitle: Option, + pub caption: Option, +} + /// Pop the reserved `title`/`subtitle`/`caption` keys out of a `TABULATE` -/// `LABEL` clause's resolved map, leaving only genuine column-name -/// overrides behind. `.flatten()` treats "key absent" and "key present but -/// NULL" the same: neither produces a title/subtitle/caption. -pub(crate) fn extract_heading_labels( - labels: &mut Labels, -) -> (Option, Option, Option) { - let title = labels.labels.remove("title").flatten(); - let subtitle = labels.labels.remove("subtitle").flatten(); - let caption = labels.labels.remove("caption").flatten(); - (title, subtitle, caption) +/// `LABEL` clause's resolved map into `TableTitles`, leaving only genuine +/// column-name overrides behind. `.flatten()` treats "key absent" and "key +/// present but NULL" the same: neither produces a title/subtitle/caption. +pub(crate) fn extract_table_titles(labels: &mut Labels) -> TableTitles { + TableTitles { + title: labels.labels.remove("title").flatten(), + subtitle: labels.labels.remove("subtitle").flatten(), + caption: labels.labels.remove("caption").flatten(), + } } /// Move every stub column to the front — group stub columns first, in diff --git a/src/table/mod.rs b/src/table/mod.rs index 1f42393fb..3fa9234e3 100644 --- a/src/table/mod.rs +++ b/src/table/mod.rs @@ -13,8 +13,8 @@ mod spanner; pub(crate) use facet::{groups_are_woven, setup_facet}; pub(crate) use layout::{ - compose_header, create_caption, create_column_labels, create_heading, extract_heading_labels, - move_stub_columns, standardise_hjust, Slot, + compose_header, create_caption, create_column_labels, create_heading, extract_table_titles, + move_stub_columns, standardise_hjust, Slot, TableTitles, }; pub use layout::{TableCell, TableCellKind, TableClass, TableColumn, TableLayout, TableRow}; pub use spanner::Spanner; From ef2573d8ea346146747e97e5d40d7d1c1c89ebe6 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 16 Oct 2026 12:17:15 +0200 Subject: [PATCH 10/25] Add missing => 'first' | 'last' | 'drop' to TABULATE FACET Nulls in a facet variable form their own group, sorted first or last, or their rows are dropped from the table; default 'last'. --- src/execute/table/facet.rs | 174 +++++++++++++++++++++++++++--------- src/execute/table/layout.rs | 4 +- src/execute/table/mod.rs | 27 ++++++ src/table/facet.rs | 76 ++++++---------- 4 files changed, 188 insertions(+), 93 deletions(-) diff --git a/src/execute/table/facet.rs b/src/execute/table/facet.rs index 31801bf0e..bf535a1b0 100644 --- a/src/execute/table/facet.rs +++ b/src/execute/table/facet.rs @@ -3,49 +3,85 @@ use std::collections::HashMap; -use arrow::array::UInt32Array; +use arrow::array::{ArrayRef, UInt32Array}; use crate::array_util::value_to_string; +use crate::plot::facet::Facet; use crate::DataFrame; -/// Assign each `df` row its group key: one rank per facet variable, levels -/// ranked by first appearance. Lexicographic comparison of keys gives the -/// nested group order (first variable outermost); a key's prefix identifies -/// the row's group at that nesting depth. Rows that are null in a variable -/// share one rank for it. -pub(super) fn setup_groups( - variables: &[String], - df: &DataFrame, -) -> Result>, String> { - let columns: Vec<_> = variables +/// Assign each `df` row its group key: one rank per facet variable (see +/// `rank_column`). Lexicographic comparison of keys gives the nested group +/// order (first variable outermost); a key's prefix identifies the row's +/// group at that nesting depth. +pub(super) fn setup_groups(facet: &Facet, df: &DataFrame) -> Result>, String> { + let columns = facet_columns(facet, df)?; + + let nulls_first = facet.properties.get("missing") + == Some(&crate::plot::ParameterValue::String("first".to_string())); + let mut keys = vec![Vec::with_capacity(columns.len()); df.height()]; + for column in &columns { + let ranks = rank_column(column, nulls_first); + for (key, rank) in keys.iter_mut().zip(ranks) { + key.push(rank); + } + } + + Ok(keys) +} + +/// Remove rows that are null in any of `facet`'s variables. Returns `df` +/// unchanged unless the facet drops nulls (`missing => 'drop'`). +pub(super) fn drop_null_rows(facet: &Facet, df: &DataFrame) -> Result { + if facet.properties.get("missing") + != Some(&crate::plot::ParameterValue::String("drop".to_string())) + { + return Ok(df.clone()); + } + let columns = facet_columns(facet, df)?; + let keep: Vec = (0..df.height() as u32) + .filter(|&row| columns.iter().all(|c| !c.is_null(row as usize))) + .collect(); + df.take(&UInt32Array::from(keep)).map_err(|e| e.to_string()) +} + +/// Resolve `facet`'s variables to `df` columns. +fn facet_columns<'a>(facet: &Facet, df: &'a DataFrame) -> Result, String> { + facet + .get_variables() .iter() .map(|name| { df.column(name) .map_err(|_| format!("TABULATE FACET: no column '{}' in the query result", name)) }) - .collect::>()?; - - // Rank each variable's values by first appearance. - let mut levels: Vec, usize>> = vec![HashMap::new(); columns.len()]; - let mut keys = Vec::with_capacity(df.height()); - for row in 0..df.height() { - let key: Vec = columns - .iter() - .zip(&mut levels) - .map(|(column, ranks)| { - let value = if column.is_null(row) { - None - } else { - Some(value_to_string(column, row)) - }; - let next = ranks.len(); - *ranks.entry(value).or_insert(next) - }) - .collect(); - keys.push(key); + .collect() +} + +/// Rank one facet variable's rows: non-null levels by first appearance, +/// nulls at a fixed rank (0 with `nulls_first`, otherwise past the non-null +/// levels) so their placement doesn't depend on where they occur. +fn rank_column(column: &ArrayRef, nulls_first: bool) -> Vec { + let mut levels: HashMap = HashMap::new(); + let mut values: Vec> = Vec::with_capacity(column.len()); + for row in 0..column.len() { + if column.is_null(row) { + values.push(None); + continue; + } + let value = value_to_string(column, row); + let next = levels.len(); + levels.entry(value.clone()).or_insert(next); + values.push(Some(value)); } - Ok(keys) + let null_rank = if nulls_first { 0 } else { levels.len() }; + let offset = usize::from(nulls_first); + values + .into_iter() + .map(|value| match value { + Some(value) => levels[&value] + offset, + None => null_rank, + }) + .collect() } /// Reorder `df` so rows with equal group keys are contiguous in @@ -74,9 +110,22 @@ pub(super) fn reorder_rows( mod tests { use super::*; use crate::df; + use crate::plot::facet::FacetLayout; + use crate::plot::{ParameterValue, Parameters}; - fn variables(names: &[&str]) -> Vec { - names.iter().map(|n| n.to_string()).collect() + fn facet(names: &[&str], missing: &str) -> Facet { + let mut properties = Parameters::new(); + properties.insert( + "missing".to_string(), + ParameterValue::String(missing.to_string()), + ); + Facet { + layout: FacetLayout::Wrap { + variables: names.iter().map(|n| n.to_string()).collect(), + }, + properties, + resolved: true, + } } #[test] @@ -87,7 +136,7 @@ mod tests { } .unwrap(); - let keys = setup_groups(&variables(&["region"]), &frame).unwrap(); + let keys = setup_groups(&facet(&["region"], "last"), &frame).unwrap(); assert_eq!(keys, vec![vec![0], vec![1], vec![0]]); } @@ -100,7 +149,7 @@ mod tests { } .unwrap(); - let keys = setup_groups(&variables(&["region", "quarter"]), &frame).unwrap(); + let keys = setup_groups(&facet(&["region", "quarter"], "last"), &frame).unwrap(); assert_eq!(keys, vec![vec![0, 0], vec![0, 1]]); } @@ -115,7 +164,7 @@ mod tests { } .unwrap(); - let keys = setup_groups(&variables(&["a", "b"]), &frame).unwrap(); + let keys = setup_groups(&facet(&["a", "b"], "last"), &frame).unwrap(); assert_eq!(keys, vec![vec![0, 0], vec![1, 0], vec![0, 1], vec![1, 1]]); } @@ -124,7 +173,7 @@ mod tests { fn setup_groups_errors_on_an_unknown_column() { let frame = df! { "sales" => vec![1i32] }.unwrap(); - let err = setup_groups(&variables(&["region"]), &frame).unwrap_err(); + let err = setup_groups(&facet(&["region"], "last"), &frame).unwrap_err(); assert!(err.contains("region")); } @@ -136,7 +185,7 @@ mod tests { "sales" => vec![1i32, 2, 3], } .unwrap(); - let keys = setup_groups(&variables(&["region"]), &frame).unwrap(); + let keys = setup_groups(&facet(&["region"], "last"), &frame).unwrap(); let (frame, keys) = reorder_rows(&frame, keys).unwrap(); @@ -161,8 +210,53 @@ mod tests { )]) .unwrap(); - let keys = setup_groups(&variables(&["region"]), &frame).unwrap(); + let keys = setup_groups(&facet(&["region"], "last"), &frame).unwrap(); assert_eq!(keys, vec![vec![0], vec![1], vec![1]]); } + + #[test] + fn setup_groups_ranks_nulls_first_when_asked() { + let frame = DataFrame::new(vec![( + "region", + crate::dataframe::into_array_ref(vec![Some("north"), None, Some("south")]), + )]) + .unwrap(); + + let keys = setup_groups(&facet(&["region"], "first"), &frame).unwrap(); + + assert_eq!(keys, vec![vec![1], vec![0], vec![2]]); + } + + #[test] + fn setup_groups_ranks_nulls_last_regardless_of_where_they_occur() { + let frame = DataFrame::new(vec![( + "region", + crate::dataframe::into_array_ref(vec![Some("north"), None, Some("south")]), + )]) + .unwrap(); + + let keys = setup_groups(&facet(&["region"], "last"), &frame).unwrap(); + + assert_eq!(keys, vec![vec![0], vec![2], vec![1]]); + } + + #[test] + fn drop_null_rows_removes_rows_null_in_any_variable() { + let frame = DataFrame::new(vec![ + ( + "region", + crate::dataframe::into_array_ref(vec![Some("north"), None, Some("south")]), + ), + ( + "quarter", + crate::dataframe::into_array_ref(vec![Some("q1"), Some("q2"), None]), + ), + ]) + .unwrap(); + + let frame = drop_null_rows(&facet(&["region", "quarter"], "drop"), &frame).unwrap(); + + assert_eq!(frame.height(), 1); + } } diff --git a/src/execute/table/layout.rs b/src/execute/table/layout.rs index 31d26e05b..b47ed3cfc 100644 --- a/src/execute/table/layout.rs +++ b/src/execute/table/layout.rs @@ -57,8 +57,8 @@ pub(super) fn setup_rows( let rows = vec![TableRow::body(); df.height()]; return Ok((df, rows)); }; - let keys = super::facet::setup_groups(&facet.get_variables(), &df) - .map_err(GgsqlError::ValidationError)?; + let df = super::facet::drop_null_rows(facet, &df).map_err(GgsqlError::ValidationError)?; + let keys = super::facet::setup_groups(facet, &df).map_err(GgsqlError::ValidationError)?; let (df, keys) = super::facet::reorder_rows(&df, keys).map_err(GgsqlError::ValidationError)?; let mut rows = Vec::with_capacity(keys.len()); for key in keys { diff --git a/src/execute/table/mod.rs b/src/execute/table/mod.rs index 527080a74..871fd8459 100644 --- a/src/execute/table/mod.rs +++ b/src/execute/table/mod.rs @@ -573,6 +573,33 @@ mod integration_tests { assert_eq!(extents, [("null", 1, 1), ("null", 2, 2)]); } + #[test] + fn test_tabulate_facet_missing_drop_removes_null_rows() { + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + reader + .execute_sql( + "CREATE TABLE t AS SELECT * FROM (VALUES ('a', 1), (NULL, 2), ('b', 3)) \ + AS t(name, sales)", + ) + .unwrap(); + + let resolved = resolve_table_with_reader( + "TABULATE * FROM t FACET name SETTING missing => 'drop'", + &reader, + ) + .unwrap(); + + // Header row, and per surviving group a heading row plus one body row. + assert_eq!(resolved.rows().len(), 5); + let headings: Vec<_> = resolved + .cells() + .iter() + .filter(|c| c.kind == TableCellKind::GroupHeading) + .map(|c| c.content.as_str()) + .collect(); + assert_eq!(headings, ["a", "b"]); + } + #[test] fn test_tabulate_facet_covering_every_column_errors() { let reader = reader_with_sales(); diff --git a/src/table/facet.rs b/src/table/facet.rs index 31a0d6a84..149df1730 100644 --- a/src/table/facet.rs +++ b/src/table/facet.rs @@ -17,9 +17,11 @@ const TABLE_FACET_PARAMS: &[ParamDefinition] = &[ constraint: ParamConstraint::string_option(&["left", "top"]), }, ParamDefinition { + // Where rows with null facet values go: a group sorted first or + // last, or dropped from the table. name: "missing", - default: DefaultParamValue::Null, - constraint: ParamConstraint::string_option(&["repeat", "null"]), + default: DefaultParamValue::String("last"), + constraint: ParamConstraint::string_option(&["first", "last", "drop"]), }, ]; @@ -50,6 +52,12 @@ pub(crate) fn setup_facet(facet: &Option) -> Result, String ParameterValue::String("top".to_string()), ); } + if !resolved.properties.contains_key("missing") { + resolved.properties.insert( + "missing".to_string(), + ParameterValue::String("last".to_string()), + ); + } resolved.resolved = true; Ok(Some(resolved)) } @@ -78,7 +86,7 @@ mod tests { } #[test] - fn setup_defaults_place_to_top() { + fn setup_materializes_default_settings() { let facet = table_facet(Parameters::new()); let resolved = setup_facet(&Some(facet)).unwrap().unwrap(); @@ -87,65 +95,31 @@ mod tests { resolved.properties.get("place"), Some(&ParameterValue::String("top".to_string())) ); - assert!(resolved.resolved); - } - - #[test] - fn setup_accepts_place_left() { - let mut settings = Parameters::new(); - settings.insert( - "place".to_string(), - ParameterValue::String("left".to_string()), - ); - let facet = table_facet(settings); - - let resolved = setup_facet(&Some(facet)).unwrap().unwrap(); - assert_eq!( - resolved.properties.get("place"), - Some(&ParameterValue::String("left".to_string())) + resolved.properties.get("missing"), + Some(&ParameterValue::String("last".to_string())) ); + assert!(resolved.resolved); } #[test] - fn setup_rejects_an_unknown_setting() { - let mut settings = Parameters::new(); - settings.insert("ncol".to_string(), ParameterValue::Number(2.0)); - let facet = table_facet(settings); - - let err = setup_facet(&Some(facet)).unwrap_err(); - - assert!(err.contains("ncol")); + fn setup_passes_none_through() { + assert_eq!(setup_facet(&None).unwrap(), None); } #[test] - fn setup_rejects_an_invalid_place_value() { + fn groups_are_woven_only_when_place_is_top() { + let woven = setup_facet(&Some(table_facet(Parameters::new()))) + .unwrap() + .unwrap(); + assert!(groups_are_woven(&woven)); + let mut settings = Parameters::new(); settings.insert( "place".to_string(), - ParameterValue::String("right".to_string()), + ParameterValue::String("left".to_string()), ); - let facet = table_facet(settings); - - assert!(setup_facet(&Some(facet)).is_err()); - } - - #[test] - fn setup_accepts_missing_repeat_and_null() { - for value in ["repeat", "null"] { - let mut settings = Parameters::new(); - settings.insert( - "missing".to_string(), - ParameterValue::String(value.to_string()), - ); - let facet = table_facet(settings); - - setup_facet(&Some(facet)).unwrap(); - } - } - - #[test] - fn setup_passes_none_through() { - assert_eq!(setup_facet(&None).unwrap(), None); + let left = setup_facet(&Some(table_facet(settings))).unwrap().unwrap(); + assert!(!groups_are_woven(&left)); } } From f50f2a65360fbe1c26fe9340e3b7700e65a1507e Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 16 Oct 2026 13:53:38 +0200 Subject: [PATCH 11/25] Add docs --- CHANGELOG.md | 1 + doc/_quarto.yml | 4 ++ doc/syntax/clause/facet-tabulate.qmd | 97 ++++++++++++++++++++++++++++ doc/syntax/clause/facet.qmd | 5 ++ doc/syntax/index.qmd | 1 + 5 files changed, 108 insertions(+) create mode 100644 doc/syntax/clause/facet-tabulate.qmd diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b3ccda8e..ba6a5685e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ labels. - `SPAN` groups columns under a shared header cell (spanner). - `FORMAT` configures how a column's cells are displayed. + - `FACET` sets row groups. - `Writer::write_table` renders a resolved table, taking the new public `TableLayout` — the table's cells bundled with their column and row metadata. diff --git a/doc/_quarto.yml b/doc/_quarto.yml index 948f1083d..fb298d88c 100644 --- a/doc/_quarto.yml +++ b/doc/_quarto.yml @@ -62,6 +62,8 @@ website: href: syntax/clause/span.qmd - text: "`FORMAT`" href: syntax/clause/format.qmd + - text: "`FACET`" + href: syntax/clause/facet-tabulate.qmd - text: Gallery href: gallery/index.qmd - href: faq.qmd @@ -127,6 +129,8 @@ website: href: syntax/clause/span.qmd - text: "`FORMAT`" href: syntax/clause/format.qmd + - text: "`FACET`" + href: syntax/clause/facet-tabulate.qmd - section: Layers contents: - section: Types diff --git a/doc/syntax/clause/facet-tabulate.qmd b/doc/syntax/clause/facet-tabulate.qmd new file mode 100644 index 000000000..f9aaaa825 --- /dev/null +++ b/doc/syntax/clause/facet-tabulate.qmd @@ -0,0 +1,97 @@ +--- +title: "Row-grouping using `FACET`" +--- + +The `FACET` clause allows you to split your data and display each group alongside a group label. +This technique is great for making structure in the data explicit. + +::: {.callout-note} +This page is about the `TABULATE` usage of the `FACET` clause. +For the `VISUALISE` usage, see [this page](facet.qmd). +::: + +## Clause syntax +The `FACET` syntax contains a number of subclauses. + +```ggsql +FACET , ... + SETTING => , ... +``` + +## Column selection + +```ggsql +, ... +``` + +In `FACET`, one or more comma separated columns can be used to split the data. +The first `column` is mandatory, and subsequent columns add nesting levels. +If the columns contain missing `null` values, these are handled by the `missing` setting. + +## Settings + +```ggsql +SETTING => , ... +``` + +Facets take a typical `SETTING` clause with key-value pairs. +It currently takes the following parameters: + +* `missing`: Determines what happens with `null` values in the grouping columns. Can be one of: + * `'first'` Keep the rows with missing group values and display these on top of regular groups. + * `'last'` (default) Keep the rows with missing group values and display these underneath regular groups. + * `'drop'` Removes the rows with missing group values from the output table. +* `place`: Placement options of the group labels. One of the following: + * `'top'` (default) Interleaves table body rows with group label rows. Group labels are placed above its member rows spanning all columns. + * `'left'` Places group labels in the stub on the left. Group labels are vertically merged across member rows. + + +## Examples + +Declare a facet column. + +```{ggsql} +CREATE TEMP TABLE my_data AS +SELECT DISTINCT ON (species, island, sex) * FROM ggsql:penguins + +TABULATE * FROM my_data + FACET sex +``` + +Omit missing group variables from the table. + +```{ggsql} +TABULATE * FROM my_data + FACET sex SETTING missing => 'drop' +``` + +Alternatively, missing group variables can be given a proper label using `FORMAT`. +Note that `missing => 'drop'` has priority over any relabeling in `FORMAT`. + +```{ggsql} +TABULATE * FROM my_data + FACET sex + FORMAT sex RENAMING null => 'unknown' +``` + +Place grouping labels in the stub. + +```{ggsql} +TABULATE * FROM my_data + FACET sex SETTING place => 'left' +``` + +Using multiple group columns to group gives nested labels. + +```{ggsql} +TABULATE * FROM my_data + FACET species, island +``` + +When group labels are placed in the stub, the labels can be controlled by `LABEL`. + +```{ggsql} +TABULATE * FROM my_data + FACET species, island SETTING place => 'left' + LABEL island => 'Place', species => 'Type' +``` diff --git a/doc/syntax/clause/facet.qmd b/doc/syntax/clause/facet.qmd index be7827f73..4db512700 100644 --- a/doc/syntax/clause/facet.qmd +++ b/doc/syntax/clause/facet.qmd @@ -6,6 +6,11 @@ The `FACET` clause allows you to split your data, by one or two variables, and p The technique is great for preventing overplotting. Because each small plot shares the same position scales by default, visual comparisons between groups become easy. +::: {.callout-note} +This page is about the `VISUALISE` usage of the `FACET` clause. +For the `TABULATE` usage, see [this page](facet-tabulate.qmd). +::: + ## Clause syntax The `FACET` syntax contains a number of subclauses that are all optional diff --git a/doc/syntax/index.qmd b/doc/syntax/index.qmd index 9d66a2c17..f00a94101 100644 --- a/doc/syntax/index.qmd +++ b/doc/syntax/index.qmd @@ -19,6 +19,7 @@ ggsql augments the standard SQL syntax with a number of new clauses to describe - [`LABEL`](clause/label-tabulate.qmd) is used to manually change column labels and titles - [`SPAN`](clause/span.qmd) can be used to group columns and display a label across them - [`FORMAT`](clause/format.qmd) declares formatting for columns +- [`FACET`](clause/facet-tabulate.qmd) enables row-grouping operations on tables ## Layers There are many different layers to choose from when visualising your data. Some are straightforward translations of your data into visual marks such as a point layer, while others perform more or less complicated calculations like e.g. the histogram layer. A layer is selected by providing the layer name after the `DRAW` clause From f5628d29da669681f71366bebe0cc5fc8f54150c Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 16 Oct 2026 15:43:33 +0200 Subject: [PATCH 12/25] Add separator setting to TABULATE FACET Joins nested group labels into one ('north - q1'), collapsing label rows or group stub columns to a single level. --- src/execute/table/facet.rs | 88 ++++++++++++++++++++++++++++++++++++- src/execute/table/layout.rs | 9 ++-- src/execute/table/mod.rs | 76 ++++++++++++++++++++++++++++++++ src/execute/table/stub.rs | 4 +- src/table/facet.rs | 7 +++ src/table/layout.rs | 5 +++ src/table/mod.rs | 5 +++ 7 files changed, 184 insertions(+), 10 deletions(-) diff --git a/src/execute/table/facet.rs b/src/execute/table/facet.rs index bf535a1b0..e780fe7c5 100644 --- a/src/execute/table/facet.rs +++ b/src/execute/table/facet.rs @@ -6,8 +6,10 @@ use std::collections::HashMap; use arrow::array::{ArrayRef, UInt32Array}; use crate::array_util::value_to_string; -use crate::plot::facet::Facet; -use crate::DataFrame; +use crate::dataframe::into_array_ref; +use crate::plot::facet::{Facet, FacetLayout}; +use crate::table::{TableColumn, TableRow}; +use crate::{ColumnSection, DataFrame}; /// Assign each `df` row its group key: one rank per facet variable (see /// `rank_column`). Lexicographic comparison of keys gives the nested group @@ -56,6 +58,88 @@ fn facet_columns<'a>(facet: &Facet, df: &'a DataFrame) -> Result, Vec, Option); + +pub(super) fn collapse_group_columns( + df: DataFrame, + mut columns: Vec, + mut rows: Vec, + facet: Option<&Facet>, +) -> Result { + let Some(facet) = facet else { + return Ok((df, columns, rows, None)); + }; + let separator = match facet.properties.get("separator") { + Some(crate::plot::ParameterValue::String(separator)) => separator.clone(), + _ => return Ok((df, columns, rows, Some(facet.clone()))), + }; + + // One variable has nothing to join. + let variables = facet.get_variables(); + if variables.len() == 1 { + return Ok((df, columns, rows, Some(facet.clone()))); + } + // The joined column takes the first variable's name. + let name = variables[0].clone(); + + let arrays: Vec<&ArrayRef> = variables + .iter() + .map(|v| df.column(v).map_err(|e| e.to_string())) + .collect::>()?; + let joined: Vec = (0..df.height()) + .map(|row| { + arrays + .iter() + .map(|array| value_to_string(array, row)) + .collect::>() + .join(&separator) + }) + .collect(); + let df = df + .drop_many(&variables) + .and_then(|df| df.with_column(&name, into_array_ref(joined))) + .map_err(|e| e.to_string())?; + + // Single-id keys: rows are group-sorted, so the id is the chunk index. + let mut id = 0; + let mut previous: Option> = None; + for row in &mut rows { + if previous.as_ref().is_some_and(|key| *key != row.group) { + id += 1; + } + previous = Some(std::mem::take(&mut row.group)); + row.group = vec![id]; + } + + // Replace the group stub columns with one, at the first's position. + if let Some(position) = columns.iter().position(|c| c.is_group_stub()) { + let mut joined_column = columns[position].clone(); + joined_column.target = ColumnSection::GroupStub(0); + columns.retain(|c| !c.is_group_stub()); + columns.insert(position, joined_column); + } + + let mut properties = facet.properties.clone(); + properties.remove("separator"); + let facet = Facet { + layout: FacetLayout::Wrap { + variables: vec![name], + }, + properties, + resolved: facet.resolved, + }; + + Ok((df, columns, rows, Some(facet))) +} + /// Rank one facet variable's rows: non-null levels by first appearance, /// nulls at a fixed rank (0 with `nulls_first`, otherwise past the non-null /// levels) so their placement doesn't depend on where they occur. diff --git a/src/execute/table/layout.rs b/src/execute/table/layout.rs index b47ed3cfc..61aec2db4 100644 --- a/src/execute/table/layout.rs +++ b/src/execute/table/layout.rs @@ -217,8 +217,8 @@ fn create_body(df: &DataFrame, columns: &[TableColumn]) -> Vec { /// Interleave group-label rows into the body. At each group boundary, emit /// one `GroupHeading` row per facet-variable level that changed, labels read /// from the group's first row (so an unchanged outer variable isn't -/// restated). Body cells keep their data-row numbering, shifted down by the -/// label rows before them. +/// restated). Body cells keep their data-row numbering, shifted down by +/// the label rows before them. fn weave_group_labels( df: &DataFrame, columns: &[TableColumn], @@ -298,10 +298,7 @@ fn merge_group_stub_cells( columns: &[TableColumn], rows: &[TableRow], ) -> Vec { - if !columns - .iter() - .any(|c| matches!(c.target, ColumnSection::GroupStub(_))) - { + if !columns.iter().any(|c| c.is_group_stub()) { return cells; } diff --git a/src/execute/table/mod.rs b/src/execute/table/mod.rs index 871fd8459..64d5fa9e9 100644 --- a/src/execute/table/mod.rs +++ b/src/execute/table/mod.rs @@ -12,6 +12,7 @@ mod format; mod layout; mod stub; +use facet::collapse_group_columns; use format::{apply_formats, setup_formats}; use layout::{build_cells, setup_columns, setup_rows}; @@ -83,6 +84,10 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result ' - '", + &reader, + ) + .unwrap(); + + let mut headings: Vec<_> = resolved + .cells() + .iter() + .filter(|c| c.kind == TableCellKind::GroupHeading) + .collect(); + headings.sort_by_key(|c| c.top); + let texts: Vec<&str> = headings.iter().map(|c| c.content.as_str()).collect(); + // One joined heading per group, full key every time. + assert_eq!(texts, ["north - q1", "north - q2", "south - q1"]); + } + + #[test] + fn test_tabulate_facet_place_left_separator_collapses_group_columns() { + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + reader + .execute_sql( + "CREATE TABLE t AS SELECT * FROM (VALUES \ + ('north', 'q1', 1), ('north', 'q1', 2), ('north', 'q2', 3), ('south', 'q1', 4)) \ + AS t(region, quarter, sales)", + ) + .unwrap(); + + let resolved = resolve_table_with_reader( + "TABULATE * FROM t FACET region, quarter SETTING place => 'left', separator => ' - '", + &reader, + ) + .unwrap(); + + // The facet collapses to one stub column; quarter is consumed. + let names: Vec<&str> = resolved.columns().iter().map(|c| c.name.as_str()).collect(); + assert_eq!(names, ["region", "sales"]); + + let mut headings: Vec<_> = resolved + .cells() + .iter() + .filter(|c| c.kind == TableCellKind::GroupHeading) + .collect(); + headings.sort_by_key(|c| c.top); + let extents: Vec<(&str, usize, usize)> = headings + .iter() + .map(|c| (c.content.as_str(), c.top, c.bottom)) + .collect(); + // Cells merge on the full key: north's two rows share one cell, but + // north - q2 and south - q1 stand alone. + assert_eq!( + extents, + [ + ("north - q1", 1, 2), + ("north - q2", 3, 3), + ("south - q1", 4, 4) + ] + ); + } + #[test] fn test_tabulate_facet_place_left_splits_groups_with_equal_labels() { let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); diff --git a/src/execute/table/stub.rs b/src/execute/table/stub.rs index fbcbcbf42..8c2245773 100644 --- a/src/execute/table/stub.rs +++ b/src/execute/table/stub.rs @@ -3,7 +3,7 @@ use crate::array_util::value_to_string; use crate::table::{TableCell, TableColumn}; -use crate::{ColumnSection, DataFrame}; +use crate::DataFrame; /// Build one `StubHead` cell per stub column, numbered from `top == 0`. /// Skips every non-stub column. @@ -36,7 +36,7 @@ pub(crate) fn create_row_labels(df: &DataFrame, columns: &[TableColumn]) -> Vec< .expect("TableColumn.name always names a column of df"); for row in 0..df.height() { - let cell = if matches!(column.target, ColumnSection::GroupStub(_)) { + let cell = if column.is_group_stub() { TableCell::new_stub_row_group(row, index, value_to_string(array, row)) } else { TableCell::new_stub_row_label(row, index, value_to_string(array, row)) diff --git a/src/table/facet.rs b/src/table/facet.rs index 149df1730..f6669b563 100644 --- a/src/table/facet.rs +++ b/src/table/facet.rs @@ -23,6 +23,13 @@ const TABLE_FACET_PARAMS: &[ParamDefinition] = &[ default: DefaultParamValue::String("last"), constraint: ParamConstraint::string_option(&["first", "last", "drop"]), }, + ParamDefinition { + // Joins nested group labels into one, e.g. 'north - q1'. Unset + // keeps one label row/column per nesting level. + name: "separator", + default: DefaultParamValue::Null, + constraint: ParamConstraint::string(), + }, ]; /// Validate a table facet's settings, apply defaults, and return the diff --git a/src/table/layout.rs b/src/table/layout.rs index 54e9d7344..1bddf6d14 100644 --- a/src/table/layout.rs +++ b/src/table/layout.rs @@ -413,6 +413,11 @@ impl TableColumn { pub fn is_stub(&self) -> bool { self.target.is_stub() } + + /// Whether this column is a `TABULATE FACET` group stub column. + pub fn is_group_stub(&self) -> bool { + self.target.is_group_stub() + } } /// One row's resolved properties within a table layout. `classes` carries diff --git a/src/table/mod.rs b/src/table/mod.rs index 3fa9234e3..f244aada6 100644 --- a/src/table/mod.rs +++ b/src/table/mod.rs @@ -234,6 +234,11 @@ impl ColumnSection { matches!(self, ColumnSection::Stub | ColumnSection::GroupStub(_)) } + /// Whether this is a facet group stub section. + pub fn is_group_stub(self) -> bool { + matches!(self, ColumnSection::GroupStub(_)) + } + /// Section ordering for column placement: group stub columns first (by /// nesting depth), then STUB, then body. pub(crate) fn sort_order(self) -> (usize, usize) { From e0445d2ede58817e58acd15c93d2df3614e1fb72 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 16 Oct 2026 16:10:03 +0200 Subject: [PATCH 13/25] Add column_to_strings helper and use it in table cell building --- src/array_util.rs | 7 +++++++ src/execute/table/format.rs | 7 ++----- src/execute/table/layout.rs | 14 +++++++------- src/execute/table/stub.rs | 15 ++++++++------- 4 files changed, 24 insertions(+), 19 deletions(-) diff --git a/src/array_util.rs b/src/array_util.rs index bc7220234..f300ccff8 100644 --- a/src/array_util.rs +++ b/src/array_util.rs @@ -211,6 +211,13 @@ pub fn value_to_string(array: &ArrayRef, idx: usize) -> String { } } +/// Get a string representation of every value in an array. +pub fn column_to_strings(array: &ArrayRef) -> Vec { + (0..array.len()) + .map(|idx| value_to_string(array, idx)) + .collect() +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/execute/table/format.rs b/src/execute/table/format.rs index 511fc1b56..953e73130 100644 --- a/src/execute/table/format.rs +++ b/src/execute/table/format.rs @@ -120,7 +120,7 @@ pub(super) fn resolve_column_properties(dtype: &DataType, format: Option<&Format #[cfg(test)] mod tests { use super::*; - use crate::array_util::value_to_string; + use crate::array_util::column_to_strings; use crate::df; fn format_with(template: &str, mapping: Option>>) -> Format { @@ -134,10 +134,7 @@ mod tests { } 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() + column_to_strings(df.column(name).unwrap()) } #[test] diff --git a/src/execute/table/layout.rs b/src/execute/table/layout.rs index 61aec2db4..2cf3f8920 100644 --- a/src/execute/table/layout.rs +++ b/src/execute/table/layout.rs @@ -7,7 +7,7 @@ use std::collections::HashMap; use super::format::resolve_column_properties; use super::stub::{create_row_labels, create_stubhead}; -use crate::array_util::value_to_string; +use crate::array_util::{column_to_strings, value_to_string}; use crate::plot::facet::Facet; use crate::plot::Labels; use crate::table::{ @@ -199,14 +199,14 @@ fn create_body(df: &DataFrame, columns: &[TableColumn]) -> Vec { // Looked up once per column, outside the row loop: `DataFrame::column` // is an `O(ncol)` scan over the schema, so doing this per row instead // would cost `O(nrow * ncol)` lookups rather than `O(ncol)`. - let array = df - .column(&column.name) - .expect("TableColumn.name always names a column of df"); + let values = column_to_strings( + df.column(&column.name) + .expect("TableColumn.name always names a column of df"), + ); - for row in 0..df.height() { + for (row, value) in values.into_iter().enumerate() { cells.push( - TableCell::new_body(row, index, value_to_string(array, row)) - .with_properties(column.properties.clone()), + TableCell::new_body(row, index, value).with_properties(column.properties.clone()), ); } } diff --git a/src/execute/table/stub.rs b/src/execute/table/stub.rs index 8c2245773..30b04f416 100644 --- a/src/execute/table/stub.rs +++ b/src/execute/table/stub.rs @@ -1,7 +1,7 @@ //! `TABULATE FORMAT STUB` resolution: building the `StubHead`/`StubRowLabel` //! cells. -use crate::array_util::value_to_string; +use crate::array_util::column_to_strings; use crate::table::{TableCell, TableColumn}; use crate::DataFrame; @@ -31,15 +31,16 @@ pub(crate) fn create_row_labels(df: &DataFrame, columns: &[TableColumn]) -> Vec< continue; } - let array = df - .column(&column.name) - .expect("TableColumn.name always names a column of df"); + let values = column_to_strings( + df.column(&column.name) + .expect("TableColumn.name always names a column of df"), + ); - for row in 0..df.height() { + for (row, value) in values.into_iter().enumerate() { let cell = if column.is_group_stub() { - TableCell::new_stub_row_group(row, index, value_to_string(array, row)) + TableCell::new_stub_row_group(row, index, value) } else { - TableCell::new_stub_row_label(row, index, value_to_string(array, row)) + TableCell::new_stub_row_label(row, index, value) }; cells.push(cell.with_properties(column.properties.clone())); } From 8ff041474a4ee3ca48fe31c4baecc251fe166776 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 16 Oct 2026 16:35:45 +0200 Subject: [PATCH 14/25] document `separator` setting --- doc/syntax/clause/facet-tabulate.qmd | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/doc/syntax/clause/facet-tabulate.qmd b/doc/syntax/clause/facet-tabulate.qmd index f9aaaa825..2151d273a 100644 --- a/doc/syntax/clause/facet-tabulate.qmd +++ b/doc/syntax/clause/facet-tabulate.qmd @@ -42,9 +42,11 @@ It currently takes the following parameters: * `'last'` (default) Keep the rows with missing group values and display these underneath regular groups. * `'drop'` Removes the rows with missing group values from the output table. * `place`: Placement options of the group labels. One of the following: - * `'top'` (default) Interleaves table body rows with group label rows. Group labels are placed above its member rows spanning all columns. + * `'top'` (default) Interleaves table body rows with group label rows. + Group labels are placed above its member rows spanning all columns. * `'left'` Places group labels in the stub on the left. Group labels are vertically merged across member rows. - +* `separator`: Optional join string to collapse group labels across nesting levels. +The default (`null`) keeps levels in separate label rows or stub columns. ## Examples @@ -88,7 +90,17 @@ TABULATE * FROM my_data FACET species, island ``` +The labels from multiple groups can be collapsed to single rows (or single stub column) by using the `separator` setting to join the labels. +Labels are joined after `FORMAT RENAMING` is applied. + +```{ggsql} +TABULATE * FROM my_data + FACET species, island + SETTING separator => ' - ' +``` + When group labels are placed in the stub, the labels can be controlled by `LABEL`. +In case of `separator`-joined labels, the first variable (`species` in the example) displays the label. ```{ggsql} TABULATE * FROM my_data From faadb7597e5bf44c5e0940becfd2b092cc0a6935 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 10 Nov 2026 14:22:44 +0100 Subject: [PATCH 15/25] flip default to `place => 'left'` --- doc/syntax/clause/facet-tabulate.qmd | 14 +++++------ src/execute/table/mod.rs | 23 +++++++++++------- src/table/facet.rs | 36 +++++++++++++++++----------- src/table/layout.rs | 5 ++-- 4 files changed, 47 insertions(+), 31 deletions(-) diff --git a/doc/syntax/clause/facet-tabulate.qmd b/doc/syntax/clause/facet-tabulate.qmd index 2151d273a..63b1a463d 100644 --- a/doc/syntax/clause/facet-tabulate.qmd +++ b/doc/syntax/clause/facet-tabulate.qmd @@ -42,15 +42,15 @@ It currently takes the following parameters: * `'last'` (default) Keep the rows with missing group values and display these underneath regular groups. * `'drop'` Removes the rows with missing group values from the output table. * `place`: Placement options of the group labels. One of the following: - * `'top'` (default) Interleaves table body rows with group label rows. + * `'left'` (default) Places group labels in the stub on the left. Group labels are vertically merged across member rows. + * `'top'` Interleaves table body rows with group label rows. Group labels are placed above its member rows spanning all columns. - * `'left'` Places group labels in the stub on the left. Group labels are vertically merged across member rows. * `separator`: Optional join string to collapse group labels across nesting levels. The default (`null`) keeps levels in separate label rows or stub columns. ## Examples -Declare a facet column. +Declare a facet column. By default, these are placed on the left as a stub. ```{ggsql} CREATE TEMP TABLE my_data AS @@ -76,11 +76,11 @@ TABULATE * FROM my_data FORMAT sex RENAMING null => 'unknown' ``` -Place grouping labels in the stub. +Place grouping labels in rows above their members. ```{ggsql} TABULATE * FROM my_data - FACET sex SETTING place => 'left' + FACET sex SETTING place => 'top' ``` Using multiple group columns to group gives nested labels. @@ -90,7 +90,7 @@ TABULATE * FROM my_data FACET species, island ``` -The labels from multiple groups can be collapsed to single rows (or single stub column) by using the `separator` setting to join the labels. +The labels from multiple groups can be collapsed to a single stub column (or single rows) by using the `separator` setting to join the labels. Labels are joined after `FORMAT RENAMING` is applied. ```{ggsql} @@ -104,6 +104,6 @@ In case of `separator`-joined labels, the first variable (`species` in the examp ```{ggsql} TABULATE * FROM my_data - FACET species, island SETTING place => 'left' + FACET species, island LABEL island => 'Place', species => 'Type' ``` diff --git a/src/execute/table/mod.rs b/src/execute/table/mod.rs index 2f235c719..5fcb20db2 100644 --- a/src/execute/table/mod.rs +++ b/src/execute/table/mod.rs @@ -77,8 +77,9 @@ pub fn resolve_table_with_reader(query: &str, reader: &dyn Reader) -> Result 'left'` facet keeps its columns as group stubs, so check + // for body columns rather than emptiness. + if columns.iter().all(|c| c.is_group_stub()) { return Err(GgsqlError::ValidationError( "TABULATE FACET covers every column: no body columns left to show".to_string(), )); @@ -523,8 +524,11 @@ mod integration_tests { ) .unwrap(); - let resolved = - resolve_table_with_reader("TABULATE * FROM t FACET region", &reader).unwrap(); + let resolved = resolve_table_with_reader( + "TABULATE * FROM t FACET region SETTING place => 'top'", + &reader, + ) + .unwrap(); // FACET consumes its column: only quarter and sales remain. let names: Vec<&str> = resolved.columns().iter().map(|c| c.name.as_str()).collect(); @@ -568,8 +572,11 @@ mod integration_tests { ) .unwrap(); - let resolved = - resolve_table_with_reader("TABULATE * FROM t FACET region, quarter", &reader).unwrap(); + let resolved = resolve_table_with_reader( + "TABULATE * FROM t FACET region, quarter SETTING place => 'top'", + &reader, + ) + .unwrap(); let mut headings: Vec<_> = resolved .cells() @@ -597,7 +604,7 @@ mod integration_tests { .unwrap(); let resolved = resolve_table_with_reader( - "TABULATE * FROM t FACET region, quarter SETTING separator => ' - '", + "TABULATE * FROM t FACET region, quarter SETTING place => 'top', separator => ' - '", &reader, ) .unwrap(); @@ -698,7 +705,7 @@ mod integration_tests { .unwrap(); let resolved = resolve_table_with_reader( - "TABULATE * FROM t FACET name SETTING missing => 'drop'", + "TABULATE * FROM t FACET name SETTING place => 'top', missing => 'drop'", &reader, ) .unwrap(); diff --git a/src/table/facet.rs b/src/table/facet.rs index f6669b563..327f74f5b 100644 --- a/src/table/facet.rs +++ b/src/table/facet.rs @@ -10,10 +10,10 @@ use crate::plot::ParameterValue; /// `SETTING` parameters a `TABULATE FACET` accepts. const TABLE_FACET_PARAMS: &[ParamDefinition] = &[ ParamDefinition { - // Where group labels go: interleaved rows above each group, or a - // stub column on the left. + // Where group labels go: a stub column on the left, or interleaved + // rows above each group. name: "place", - default: DefaultParamValue::String("top"), + default: DefaultParamValue::String("left"), constraint: ParamConstraint::string_option(&["left", "top"]), }, ParamDefinition { @@ -56,7 +56,7 @@ pub(crate) fn setup_facet(facet: &Option) -> Result, String if !resolved.properties.contains_key("place") { resolved.properties.insert( "place".to_string(), - ParameterValue::String("top".to_string()), + ParameterValue::String("left".to_string()), ); } if !resolved.properties.contains_key("missing") { @@ -71,9 +71,12 @@ pub(crate) fn setup_facet(facet: &Option) -> Result, String /// Whether a resolved table facet weaves its group labels in as interleaved /// rows (`place => 'top'`) rather than group stub columns (`place => -/// 'left'`). +/// 'left'`, the default). pub(crate) fn groups_are_woven(facet: &Facet) -> bool { - facet.properties.get("place") == Some(&ParameterValue::String("top".to_string())) + match facet.properties.get("place") { + Some(ParameterValue::String(place)) => place == "top", + _ => false, // unresolved facet: 'left' is the default + } } #[cfg(test)] @@ -100,7 +103,7 @@ mod tests { assert_eq!( resolved.properties.get("place"), - Some(&ParameterValue::String("top".to_string())) + Some(&ParameterValue::String("left".to_string())) ); assert_eq!( resolved.properties.get("missing"), @@ -116,17 +119,22 @@ mod tests { #[test] fn groups_are_woven_only_when_place_is_top() { - let woven = setup_facet(&Some(table_facet(Parameters::new()))) - .unwrap() - .unwrap(); - assert!(groups_are_woven(&woven)); - let mut settings = Parameters::new(); settings.insert( "place".to_string(), - ParameterValue::String("left".to_string()), + ParameterValue::String("top".to_string()), ); - let left = setup_facet(&Some(table_facet(settings))).unwrap().unwrap(); + let woven = setup_facet(&Some(table_facet(settings))).unwrap().unwrap(); + assert!(groups_are_woven(&woven)); + + let left = setup_facet(&Some(table_facet(Parameters::new()))) + .unwrap() + .unwrap(); assert!(!groups_are_woven(&left)); } + + #[test] + fn groups_are_woven_defaults_to_left_before_resolution() { + assert!(!groups_are_woven(&table_facet(Parameters::new()))); + } } diff --git a/src/table/layout.rs b/src/table/layout.rs index 98b70cf6f..439177831 100644 --- a/src/table/layout.rs +++ b/src/table/layout.rs @@ -272,10 +272,11 @@ impl TableCell { .with_classes(vec![TableClass::Stub]) } - /// A group-stub group-label cell (`TableClass::StubRowGroup`). + /// A group-stub group-label cell (`TableClass::StubRowGroup`). StubRowGroup + /// comes last so its `vertical-align: top` overrides Row's `middle`. pub(crate) fn new_stub_row_group(row: usize, index: usize, content: String) -> Self { Self::new(TableCellKind::GroupHeading, row, row, index, index, content) - .with_classes(vec![TableClass::StubRowGroup, TableClass::Row]) + .with_classes(vec![TableClass::Row, TableClass::StubRowGroup]) } /// A spanner cell (`TableClass::Spanner`, `SpannerLabel`, centered). From 428f42de6906fffd16526aa21264ea14f46856a9 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 10 Nov 2026 15:50:42 +0100 Subject: [PATCH 16/25] add invalid parameter rejection test --- src/table/facet.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/table/facet.rs b/src/table/facet.rs index 327f74f5b..bb48b4c0d 100644 --- a/src/table/facet.rs +++ b/src/table/facet.rs @@ -117,6 +117,16 @@ mod tests { assert_eq!(setup_facet(&None).unwrap(), None); } + #[test] + fn setup_rejects_an_invalid_place_value() { + let mut settings = Parameters::new(); + settings.insert( + "place".to_string(), + ParameterValue::String("bottom".to_string()), + ); + assert!(setup_facet(&Some(table_facet(settings))).is_err()); + } + #[test] fn groups_are_woven_only_when_place_is_top() { let mut settings = Parameters::new(); From 281ae3c97e25c94fac31bceb491a67c34c08ba3a Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 10 Nov 2026 15:55:31 +0100 Subject: [PATCH 17/25] TABULATE and VISUALISE reject multiple facets --- src/parser/builder.rs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/parser/builder.rs b/src/parser/builder.rs index b198b3c94..c73e005eb 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -459,6 +459,11 @@ fn process_tab_clause(node: &Node, source: &SourceTree, table: &mut Table) -> Re "TABULATE FACET does not support BY".to_string(), )); } + if table.facet.is_some() { + return Err(GgsqlError::ParseError( + "TABULATE supports a single FACET clause".to_string(), + )); + } table.facet = Some(facet); } _ => {} @@ -608,6 +613,11 @@ fn process_viz_clause(node: &Node, source: &SourceTree, spec: &mut Plot) -> Resu .to_string(), )); } + if spec.facet.is_some() { + return Err(GgsqlError::ParseError( + "VISUALISE supports a single FACET clause".to_string(), + )); + } spec.facet = Some(facet); } "project_clause" => { @@ -1642,6 +1652,13 @@ mod tests { assert!(result.is_err()); } + #[test] + fn test_tabulate_a_second_facet_clause_is_rejected() { + let result = parse_test_specs("TABULATE * FROM sales FACET region FACET category"); + + assert!(result.is_err()); + } + #[test] fn test_tabulate_repeated_label_clauses_merge_rather_than_overwrite() { let specs = parse_test_specs("TABULATE * FROM sales LABEL id => 'ID' LABEL name => 'Name'") @@ -1791,6 +1808,18 @@ mod tests { ); } + #[test] + fn test_visualise_a_second_facet_clause_is_rejected() { + let query = r#" + VISUALISE + DRAW point MAPPING x AS x, y AS y + FACET region + FACET category + "#; + + assert!(parse_test_query(query).is_err()); + } + #[test] fn test_project_explicit_aesthetics() { let query = r#" From 0ccd32a312aa36dd328a4f85acb07ffb114b02c6 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 10 Nov 2026 16:17:56 +0100 Subject: [PATCH 18/25] prevent duplicate facetting variables --- src/parser/builder.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/parser/builder.rs b/src/parser/builder.rs index c73e005eb..2d8290d5f 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -1213,6 +1213,19 @@ fn build_facet(node: &Node, source: &SourceTree) -> Result { } } + for (i, var) in row_vars.iter().chain(&column_vars).enumerate() { + if row_vars + .iter() + .chain(&column_vars) + .skip(i + 1) + .any(|v| v == var) + { + return Err(GgsqlError::ParseError(format!( + "FACET lists '{var}' more than once" + ))); + } + } + // Determine layout variant: if column_vars is empty, it's a wrap layout let layout = if column_vars.is_empty() { FacetLayout::Wrap { @@ -1820,6 +1833,17 @@ mod tests { assert!(parse_test_query(query).is_err()); } + #[test] + fn test_visualise_facet_rejects_a_variable_repeated_across_by() { + let query = r#" + VISUALISE + DRAW point MAPPING x AS x, y AS y + FACET region BY region + "#; + + assert!(parse_test_query(query).is_err()); + } + #[test] fn test_project_explicit_aesthetics() { let query = r#" From d0450bf0abe906396e10a0b33d35b4fa25391ba5 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 10 Nov 2026 16:18:51 +0100 Subject: [PATCH 19/25] reindent chunks --- doc/syntax/clause/facet-tabulate.qmd | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/doc/syntax/clause/facet-tabulate.qmd b/doc/syntax/clause/facet-tabulate.qmd index 63b1a463d..841eaa427 100644 --- a/doc/syntax/clause/facet-tabulate.qmd +++ b/doc/syntax/clause/facet-tabulate.qmd @@ -57,14 +57,14 @@ CREATE TEMP TABLE my_data AS SELECT DISTINCT ON (species, island, sex) * FROM ggsql:penguins TABULATE * FROM my_data - FACET sex +FACET sex ``` Omit missing group variables from the table. ```{ggsql} TABULATE * FROM my_data - FACET sex SETTING missing => 'drop' +FACET sex SETTING missing => 'drop' ``` Alternatively, missing group variables can be given a proper label using `FORMAT`. @@ -72,22 +72,22 @@ Note that `missing => 'drop'` has priority over any relabeling in `FORMAT`. ```{ggsql} TABULATE * FROM my_data - FACET sex - FORMAT sex RENAMING null => 'unknown' +FACET sex +FORMAT sex RENAMING null => 'unknown' ``` Place grouping labels in rows above their members. ```{ggsql} TABULATE * FROM my_data - FACET sex SETTING place => 'top' +FACET sex SETTING place => 'top' ``` Using multiple group columns to group gives nested labels. ```{ggsql} TABULATE * FROM my_data - FACET species, island +FACET species, island ``` The labels from multiple groups can be collapsed to a single stub column (or single rows) by using the `separator` setting to join the labels. @@ -95,8 +95,8 @@ Labels are joined after `FORMAT RENAMING` is applied. ```{ggsql} TABULATE * FROM my_data - FACET species, island - SETTING separator => ' - ' +FACET species, island + SETTING separator => ' - ' ``` When group labels are placed in the stub, the labels can be controlled by `LABEL`. @@ -104,6 +104,6 @@ In case of `separator`-joined labels, the first variable (`species` in the examp ```{ggsql} TABULATE * FROM my_data - FACET species, island - LABEL island => 'Place', species => 'Type' +FACET species, island +LABEL island => 'Place', species => 'Type' ``` From ae01764e304cbd70f28100bc6ef8d424581ae9eb Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 10 Nov 2026 17:05:45 +0100 Subject: [PATCH 20/25] hardcode class precedence to prevent drift --- src/table/layout.rs | 42 +++++++++++++++++++++--- src/writer/html.rs | 79 ++++++++++++++++++++++++++------------------- 2 files changed, 83 insertions(+), 38 deletions(-) diff --git a/src/table/layout.rs b/src/table/layout.rs index 439177831..548c18854 100644 --- a/src/table/layout.rs +++ b/src/table/layout.rs @@ -164,6 +164,38 @@ pub enum TableClass { AlignRight, } +impl TableClass { + /// The canonical style-conflict order for every writer: when two classes + /// on the same element declare the same property, the one listed later + /// wins. This is the CSS cascade's source-order rule made + /// writer-neutral: a writer emitting a stylesheet uses this as rule + /// order; a writer folding classes into inline styles sorts by this + /// before folding. Classes not listed carry no declarations, so their + /// position is irrelevant. + pub(crate) const PRECEDENCE: &[TableClass] = &[ + TableClass::Table, + TableClass::TableBody, + TableClass::Row, + TableClass::Stub, + TableClass::StubRowGroup, + TableClass::ColHeading, + TableClass::ColHeadingRow, + TableClass::Spanner, + TableClass::SpannerLabel, + TableClass::SpannerRow, + TableClass::Title, + TableClass::Subtitle, + TableClass::Caption, + TableClass::Heading, + TableClass::GroupHeading, + TableClass::GroupHeadingRow, + TableClass::RowGroupFirst, + TableClass::AlignLeft, + TableClass::AlignCenter, + TableClass::AlignRight, + ]; +} + impl std::fmt::Display for TableClass { /// The class-name suffix a writer hangs its own prefix on, e.g. /// `ggsql_row` for `Row` in the HTML writer. @@ -219,8 +251,9 @@ pub struct TableCell { /// their column's properties; a `Spanner` cell covers several columns /// at once, so it has none of its own. pub properties: Parameters, - /// Style classes recorded at build time (see `TableClass`). Ordered; - /// a writer renders them in order. + /// Style classes recorded at build time (see `TableClass`). A set, not + /// a sequence: conflicts resolve by `TableClass::PRECEDENCE`, + /// never by position in this list. pub classes: Vec, } @@ -272,8 +305,7 @@ impl TableCell { .with_classes(vec![TableClass::Stub]) } - /// A group-stub group-label cell (`TableClass::StubRowGroup`). StubRowGroup - /// comes last so its `vertical-align: top` overrides Row's `middle`. + /// A group-stub group-label cell (`TableClass::StubRowGroup`). pub(crate) fn new_stub_row_group(row: usize, index: usize, content: String) -> Self { Self::new(TableCellKind::GroupHeading, row, row, index, index, content) .with_classes(vec![TableClass::Row, TableClass::StubRowGroup]) @@ -436,7 +468,7 @@ pub struct TableRow { /// Resolved properties for this row's cells. pub properties: Parameters, /// Style classes recorded at build time (see `TableClass`) — row-scoped - /// ones, e.g. `Heading`. Ordered, like `TableCell::classes`. + /// ones, e.g. `Heading`. A set, like `TableCell::classes`. pub classes: Vec, /// Whether this row belongs in a table's header rather than its body. /// `false` by default; set `true` for a spanner, column-label, title or diff --git a/src/writer/html.rs b/src/writer/html.rs index 724a834f5..d78e4a279 100644 --- a/src/writer/html.rs +++ b/src/writer/html.rs @@ -374,33 +374,6 @@ fn class_declarations(class: TableClass) -> &'static [(&'static str, &'static st } } -/// The styled classes (those carrying declarations), in `