From a0c70929306995344dad330ac3fd1ac6b9962f1c Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 27 Feb 2026 13:51:14 +0100 Subject: [PATCH 01/15] add resolution helper --- src/writer/vegalite/layer.rs | 77 ++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index ee2a225a..7ea9b58c 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -796,10 +796,87 @@ pub fn get_renderer(geom: &Geom) -> Box { } } +// ============================================================================= +// General Helper Functions +// ============================================================================= + +/// Calculate data resolution (minimum spacing between consecutive unique values). +/// +/// For continuous numeric data: returns the minimum spacing between consecutive unique values. +/// For a single unique value or empty input: returns 1.0 as a default. +/// Uses a small tolerance (epsilon) to avoid floating point precision issues. +/// +/// # Arguments +/// * `values` - Numeric values from the data (will be sorted internally) +/// +/// # Returns +/// The minimum distance between consecutive sorted values, or 1.0 if there are fewer than 2 values. +/// +/// # Example +/// ```ignore +/// let values = vec![1.0, 5.0, 2.0, 6.0]; +/// let resolution = calculate_resolution(&values); // Returns 1.0 (minimum spacing after sorting: 1-2 or 5-6) +/// ``` +fn calculate_resolution(values: &[f64]) -> f64 { + const EPSILON: f64 = 1e-10; + + if values.len() < 2 { + return 1.0; + } + + // Sort and deduplicate values + let mut sorted = values.to_vec(); + sorted.sort_by(|a, b| a.partial_cmp(b).unwrap_or(std::cmp::Ordering::Equal)); + sorted.dedup(); + + if sorted.len() < 2 { + return 1.0; // All values are identical + } + + let mut min_dist = f64::MAX; + for i in 1..sorted.len() { + let dist = sorted[i] - sorted[i - 1]; + // Only consider distances above epsilon to avoid floating point noise + if dist > EPSILON && dist < min_dist { + min_dist = dist; + } + } + + if min_dist == f64::MAX { + 1.0 // All values are within epsilon of each other + } else { + min_dist + } +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn test_calculate_resolution() { + // Normal case: minimum spacing + assert_eq!(calculate_resolution(&[1.0, 2.0, 5.0, 6.0]), 1.0); + + // Unsorted input (should sort internally) + assert_eq!(calculate_resolution(&[5.0, 1.0, 6.0, 2.0]), 1.0); + + // Single value + assert_eq!(calculate_resolution(&[5.0]), 1.0); + + // Empty + assert_eq!(calculate_resolution(&[]), 1.0); + + // All identical values + assert_eq!(calculate_resolution(&[5.0, 5.0, 5.0]), 1.0); + + // Larger spacing + assert_eq!(calculate_resolution(&[0.0, 10.0, 20.0]), 10.0); + + // Fractional values + assert_eq!(calculate_resolution(&[0.0, 0.5, 1.5, 2.0]), 0.5); + } + #[test] fn test_violin_detail_encoding() { let renderer = ViolinRenderer; From 961489a13c604afa037c8b340e6efd9583ab2f39 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 3 Mar 2026 13:51:01 +0100 Subject: [PATCH 02/15] Add rect geom with flexible parameter specification Implements a new rect geom that supports flexible rectangle specification: - X-direction: any 2 of {x (center), width, xmin, xmax} - Y-direction: any 2 of {y (center), height, ymin, ymax} Key features: - Stat-based parameter consolidation via SQL generation - Automatic discrete vs continuous scale detection using Schema - Validates exactly 2 params per direction - Generates appropriate SQL for all 6 parameter combinations per axis - Returns different stat columns based on scale type: - Continuous: pos1min, pos1max, pos2min, pos2max - Discrete: pos1, pos2 (for band-based rendering) Implementation follows existing patterns from histogram and bar geoms. Co-Authored-By: Claude Sonnet 4.5 --- src/plot/layer/geom/mod.rs | 10 ++ src/plot/layer/geom/rect.rs | 318 ++++++++++++++++++++++++++++++++++++ 2 files changed, 328 insertions(+) create mode 100644 src/plot/layer/geom/rect.rs diff --git a/src/plot/layer/geom/mod.rs b/src/plot/layer/geom/mod.rs index c953c9f7..d562ad63 100644 --- a/src/plot/layer/geom/mod.rs +++ b/src/plot/layer/geom/mod.rs @@ -42,6 +42,7 @@ mod line; mod path; mod point; mod polygon; +mod rect; mod ribbon; mod segment; mod smooth; @@ -68,6 +69,7 @@ pub use line::Line; pub use path::Path; pub use point::Point; pub use polygon::Polygon; +pub use rect::Rect; pub use ribbon::Ribbon; pub use segment::Segment; pub use smooth::Smooth; @@ -88,6 +90,7 @@ pub enum GeomType { Bar, Area, Tile, + Rect, Polygon, Ribbon, Histogram, @@ -114,6 +117,7 @@ impl std::fmt::Display for GeomType { GeomType::Bar => "bar", GeomType::Area => "area", GeomType::Tile => "tile", + GeomType::Rect => "rect", GeomType::Polygon => "polygon", GeomType::Ribbon => "ribbon", GeomType::Histogram => "histogram", @@ -256,6 +260,11 @@ impl Geom { Self(Arc::new(Tile)) } + /// Create a Rect geom + pub fn rect() -> Self { + Self(Arc::new(Rect)) + } + /// Create a Polygon geom pub fn polygon() -> Self { Self(Arc::new(Polygon)) @@ -340,6 +349,7 @@ impl Geom { GeomType::Bar => Self::bar(), GeomType::Area => Self::area(), GeomType::Tile => Self::tile(), + GeomType::Rect => Self::rect(), GeomType::Polygon => Self::polygon(), GeomType::Ribbon => Self::ribbon(), GeomType::Histogram => Self::histogram(), diff --git a/src/plot/layer/geom/rect.rs b/src/plot/layer/geom/rect.rs new file mode 100644 index 00000000..b7e15da9 --- /dev/null +++ b/src/plot/layer/geom/rect.rs @@ -0,0 +1,318 @@ +//! Rect geom implementation with flexible parameter specification + +use std::collections::HashMap; + +use super::types::get_column_name; +use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType, StatResult}; +use crate::naming; +use crate::plot::types::{DefaultAestheticValue, ParameterValue}; +use crate::{DataFrame, GgsqlError, Mappings, Result}; + +use super::types::Schema; + +/// Rect geom - rectangles with flexible parameter specification +/// +/// Supports multiple ways to specify rectangles: +/// - X-direction: any 2 of {x (center), width, xmin, xmax} +/// - Y-direction: any 2 of {y (center), height, ymin, ymax} +/// +/// For continuous scales, computes xmin/xmax and ymin/ymax +/// For discrete scales, uses x/y with width/height as band fractions +#[derive(Debug, Clone, Copy)] +pub struct Rect; + +impl GeomTrait for Rect { + fn geom_type(&self) -> GeomType { + GeomType::Rect + } + + fn aesthetics(&self) -> DefaultAesthetics { + DefaultAesthetics { + defaults: &[ + // All positional aesthetics are optional inputs (Null) + // They become Delayed after stat transform + ("pos1", DefaultAestheticValue::Null), // x (center) + ("pos1min", DefaultAestheticValue::Null), // xmin + ("pos1max", DefaultAestheticValue::Null), // xmax + ("width", DefaultAestheticValue::Null), // width (aesthetic, can map to column) + ("pos2", DefaultAestheticValue::Null), // y (center) + ("pos2min", DefaultAestheticValue::Null), // ymin + ("pos2max", DefaultAestheticValue::Null), // ymax + ("height", DefaultAestheticValue::Null), // height (aesthetic, can map to column) + // Visual aesthetics + ("fill", DefaultAestheticValue::String("black")), + ("stroke", DefaultAestheticValue::String("black")), + ("opacity", DefaultAestheticValue::Number(1.0)), + ("linewidth", DefaultAestheticValue::Number(1.0)), + ("linetype", DefaultAestheticValue::String("solid")), + ], + } + } + + fn default_remappings(&self) -> &'static [(&'static str, DefaultAestheticValue)] { + &[ + // For continuous scales: remap to min/max + ("pos1min", DefaultAestheticValue::Column("pos1min")), + ("pos1max", DefaultAestheticValue::Column("pos1max")), + ("pos2min", DefaultAestheticValue::Column("pos2min")), + ("pos2max", DefaultAestheticValue::Column("pos2max")), + // For discrete scales: remap to center + ("pos1", DefaultAestheticValue::Column("pos1")), + ("pos2", DefaultAestheticValue::Column("pos2")), + ] + } + + fn valid_stat_columns(&self) -> &'static [&'static str] { + &["pos1", "pos2", "pos1min", "pos1max", "pos2min", "pos2max"] + } + + fn default_params(&self) -> &'static [DefaultParam] { + &[ + DefaultParam { + name: "width", + default: DefaultParamValue::Number(0.9), + }, + DefaultParam { + name: "height", + default: DefaultParamValue::Number(0.9), + }, + ] + } + + fn stat_consumed_aesthetics(&self) -> &'static [&'static str] { + &[ + "pos1", "pos1min", "pos1max", "width", "pos2", "pos2min", "pos2max", "height", + ] + } + + fn needs_stat_transform(&self, _aesthetics: &Mappings) -> bool { + // Always apply stat transform to validate and consolidate parameters + true + } + + fn apply_stat_transform( + &self, + query: &str, + schema: &Schema, + aesthetics: &Mappings, + group_by: &[String], + parameters: &HashMap, + _execute_query: &dyn Fn(&str) -> Result, + ) -> Result { + stat_rect(query, schema, aesthetics, group_by, parameters) + } +} + +impl std::fmt::Display for Rect { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "rect") + } +} + +/// Statistical transformation for rect: consolidate parameters and compute min/max +fn stat_rect( + query: &str, + schema: &Schema, + aesthetics: &Mappings, + group_by: &[String], + _parameters: &HashMap, +) -> Result { + // Extract x-direction aesthetics + let x = get_column_name(aesthetics, "pos1"); + let xmin = get_column_name(aesthetics, "pos1min"); + let xmax = get_column_name(aesthetics, "pos1max"); + let width = get_column_name(aesthetics, "width"); + + // Extract y-direction aesthetics + let y = get_column_name(aesthetics, "pos2"); + let ymin = get_column_name(aesthetics, "pos2min"); + let ymax = get_column_name(aesthetics, "pos2max"); + let height = get_column_name(aesthetics, "height"); + + // Detect if x and y are discrete by checking schema + let is_x_discrete = x + .as_ref() + .and_then(|col| schema.iter().find(|c| c.name == *col)) + .map(|c| c.is_discrete) + .unwrap_or(false); + let is_y_discrete = y + .as_ref() + .and_then(|col| schema.iter().find(|c| c.name == *col)) + .map(|c| c.is_discrete) + .unwrap_or(false); + + // Generate SQL expressions based on parameter combinations + // Validation (exactly 2 params, discrete + min/max check) happens inside + let (x_expr_min, x_expr_max) = generate_position_expressions( + x.as_deref(), + xmin.as_deref(), + xmax.as_deref(), + width.as_deref(), + is_x_discrete, + "x", + )?; + let (y_expr_min, y_expr_max) = generate_position_expressions( + y.as_deref(), + ymin.as_deref(), + ymax.as_deref(), + height.as_deref(), + is_y_discrete, + "y", + )?; + + // Build stat column names + let stat_x = naming::stat_column("pos1"); + let stat_xmin = naming::stat_column("pos1min"); + let stat_xmax = naming::stat_column("pos1max"); + let stat_y = naming::stat_column("pos2"); + let stat_ymin = naming::stat_column("pos2min"); + let stat_ymax = naming::stat_column("pos2max"); + + // Build SELECT list + let mut select_parts = vec![]; + + // Add group_by columns first + if !group_by.is_empty() { + select_parts.push(group_by.join(", ")); + } + + // Add position columns based on discrete vs continuous + if is_x_discrete { + select_parts.push(format!("{} AS {}", x_expr_min, stat_x)); + } else { + select_parts.push(format!("{} AS {}", x_expr_min, stat_xmin)); + select_parts.push(format!("{} AS {}", x_expr_max, stat_xmax)); + } + + if is_y_discrete { + select_parts.push(format!("{} AS {}", y_expr_min, stat_y)); + } else { + select_parts.push(format!("{} AS {}", y_expr_min, stat_ymin)); + select_parts.push(format!("{} AS {}", y_expr_max, stat_ymax)); + } + + let select_list = select_parts.join(", "); + + // Build transformed query + let transformed_query = format!( + "SELECT {} FROM ({}) AS __ggsql_rect_stat__", + select_list, query + ); + + // Return stat columns based on discrete vs continuous + let stat_columns = match (is_x_discrete, is_y_discrete) { + (true, true) => vec!["pos1".to_string(), "pos2".to_string()], + (true, false) => vec![ + "pos1".to_string(), + "pos2min".to_string(), + "pos2max".to_string(), + ], + (false, true) => vec![ + "pos1min".to_string(), + "pos1max".to_string(), + "pos2".to_string(), + ], + (false, false) => vec![ + "pos1min".to_string(), + "pos1max".to_string(), + "pos2min".to_string(), + "pos2max".to_string(), + ], + }; + + Ok(StatResult::Transformed { + query: transformed_query, + stat_columns, + dummy_columns: vec![], + consumed_aesthetics: vec![ + "pos1".to_string(), + "pos1min".to_string(), + "pos1max".to_string(), + "width".to_string(), + "pos2".to_string(), + "pos2min".to_string(), + "pos2max".to_string(), + "height".to_string(), + ], + }) +} + +/// Generate SQL expressions for position min/max based on parameter combinations +/// +/// Returns (min_expr, max_expr) or (center_expr, center_expr) for discrete +/// +/// Validates: +/// - Discrete scales cannot use min/max aesthetics +/// - Exactly 2 parameters provided (via match statement) +fn generate_position_expressions( + center: Option<&str>, + min: Option<&str>, + max: Option<&str>, + size: Option<&str>, + is_discrete: bool, + axis: &str, +) -> Result<(String, String)> { + // Validate: discrete scales cannot use min/max + if is_discrete && (min.is_some() || max.is_some()) { + return Err(GgsqlError::ValidationError(format!( + "Cannot use {}min/{}max with discrete {} aesthetic. Use {} + {} instead.", + axis, + axis, + axis, + axis, + if axis == "x" { "width" } else { "height" } + ))); + } + + // For discrete, only center + size is valid + if is_discrete { + if let (Some(c), Some(_)) = (center, size) { + return Ok((c.to_string(), c.to_string())); + } + return Err(GgsqlError::ValidationError(format!( + "Discrete {} requires {} and {}.", + axis, + axis, + if axis == "x" { "width" } else { "height" } + ))); + } + + // For continuous, handle all 6 combinations + // The _ arm catches invalid parameter counts (not exactly 2) + match (center, min, max, size) { + // Case 1: min + max + (None, Some(min_col), Some(max_col), None) => { + Ok((min_col.to_string(), max_col.to_string())) + } + // Case 2: center + size + (Some(c), None, None, Some(s)) => Ok(( + format!("({} - {} / 2.0)", c, s), + format!("({} + {} / 2.0)", c, s), + )), + // Case 3: center + min + (Some(c), Some(min_col), None, None) => { + Ok((min_col.to_string(), format!("(2 * {} - {})", c, min_col))) + } + // Case 4: center + max + (Some(c), None, Some(max_col), None) => { + Ok((format!("(2 * {} - {})", c, max_col), max_col.to_string())) + } + // Case 5: min + size + (None, Some(min_col), None, Some(s)) => { + Ok((min_col.to_string(), format!("({} + {})", min_col, s))) + } + // Case 6: max + size + (None, None, Some(max_col), Some(s)) => { + Ok((format!("({} - {})", max_col, s), max_col.to_string())) + } + // Invalid: wrong number of parameters or invalid combination + _ => Err(GgsqlError::ValidationError(format!( + "Rect requires exactly 2 {}-direction parameters from {{{}, {}min, {}max, {}}}.", + axis, + axis, + axis, + axis, + if axis == "x" { "width" } else { "height" } + ))), + } +} From 99b7f25bb1c2d0b24c7cba0cada9f565eb5a64aa Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 3 Mar 2026 13:51:34 +0100 Subject: [PATCH 03/15] Add rect to grammar and parser - Add 'rect' to tree-sitter grammar geom_type rule - Add "rect" case to parser builder geom type mapping Co-Authored-By: Claude Sonnet 4.5 --- src/parser/builder.rs | 1 + tree-sitter-ggsql/grammar.js | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/parser/builder.rs b/src/parser/builder.rs index f7bc9cb1..91818b21 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -584,6 +584,7 @@ fn parse_geom_type(text: &str) -> Result { "bar" => Ok(Geom::bar()), "area" => Ok(Geom::area()), "tile" => Ok(Geom::tile()), + "rect" => Ok(Geom::rect()), "polygon" => Ok(Geom::polygon()), "ribbon" => Ok(Geom::ribbon()), "histogram" => Ok(Geom::histogram()), diff --git a/tree-sitter-ggsql/grammar.js b/tree-sitter-ggsql/grammar.js index 9302f195..d6138c5b 100644 --- a/tree-sitter-ggsql/grammar.js +++ b/tree-sitter-ggsql/grammar.js @@ -470,7 +470,7 @@ module.exports = grammar({ ), geom_type: $ => choice( - 'point', 'line', 'path', 'bar', 'area', 'tile', 'polygon', 'ribbon', + 'point', 'line', 'path', 'bar', 'area', 'tile', 'rect', 'polygon', 'ribbon', 'histogram', 'density', 'smooth', 'boxplot', 'violin', 'text', 'label', 'segment', 'arrow', 'hline', 'vline', 'abline', 'errorbar' ), From 89dcdb60a9267c44f4a0c65d68389719771a98a4 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 3 Mar 2026 14:27:53 +0100 Subject: [PATCH 04/15] Fix rect geom: width/height as aesthetics only Changes: - Remove width/height from default_params() (use trait default) - Treat width/height as aesthetics (columns or literals), not parameters - RectRenderer handles x and y directions independently - For discrete scales: extract literal width/height from encoding or default to 0.9 - Error if width/height are mapped to variable columns on discrete scales - Support mixed continuous/discrete (e.g., continuous x + discrete y) - Extract band size logic into helper function with early returns - Early return from modify_spec when both directions are continuous Co-Authored-By: Claude Sonnet 4.5 --- src/plot/layer/geom/rect.rs | 13 ---- src/writer/vegalite/layer.rs | 112 +++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 13 deletions(-) diff --git a/src/plot/layer/geom/rect.rs b/src/plot/layer/geom/rect.rs index b7e15da9..8b70b1e6 100644 --- a/src/plot/layer/geom/rect.rs +++ b/src/plot/layer/geom/rect.rs @@ -66,19 +66,6 @@ impl GeomTrait for Rect { &["pos1", "pos2", "pos1min", "pos1max", "pos2min", "pos2max"] } - fn default_params(&self) -> &'static [DefaultParam] { - &[ - DefaultParam { - name: "width", - default: DefaultParamValue::Number(0.9), - }, - DefaultParam { - name: "height", - default: DefaultParamValue::Number(0.9), - }, - ] - } - fn stat_consumed_aesthetics(&self) -> &'static [&'static str] { &[ "pos1", "pos1min", "pos1max", "width", "pos2", "pos2min", "pos2max", "height", diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index 65ec4783..651a52ff 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -31,6 +31,7 @@ pub fn geom_to_mark(geom: &Geom) -> Value { GeomType::Bar => "bar", GeomType::Area => "area", GeomType::Tile => "rect", + GeomType::Rect => "rect", GeomType::Ribbon => "area", GeomType::Polygon => "line", GeomType::Histogram => "bar", @@ -241,6 +242,116 @@ impl GeomRenderer for RibbonRenderer { } } +// ============================================================================= +// Rect Renderer +// ============================================================================= + +/// Renderer for rect geom - handles continuous and discrete rectangles +/// +/// For continuous scales: remaps xmin/xmax → x/x2, ymin/ymax → y/y2 +/// For discrete scales: keeps x/y as-is and applies width/height as band fractions +pub struct RectRenderer; + +impl RectRenderer { + /// Extract band size (width or height) from encoding for discrete scales. + /// + /// Returns error if the aesthetic is mapped to a variable column (field), + /// extracts constant value if present, or returns default 0.9. + fn extract_band_size( + encoding_obj: Option<&Map>, + aesthetic: &str, + axis: &str, + ) -> Result { + const DEFAULT_BAND_SIZE: f64 = 0.9; + + // Early return with default if encoding not present + let Some(enc) = encoding_obj else { + return Ok(DEFAULT_BAND_SIZE); + }; + + // Early return with default if aesthetic not in encoding + let Some(size_enc) = enc.get(aesthetic) else { + return Ok(DEFAULT_BAND_SIZE); + }; + + // Check if it's a field (variable) - error + if size_enc.get("field").is_some() { + return Err(GgsqlError::WriterError(format!( + "Discrete {} scale does not support variable {} columns.", + axis, aesthetic + ))); + } + + // Extract constant value or default + Ok(size_enc + .get("value") + .and_then(|v| v.as_f64()) + .unwrap_or(DEFAULT_BAND_SIZE)) + } +} + +impl GeomRenderer for RectRenderer { + fn modify_encoding(&self, encoding: &mut Map, _layer: &Layer) -> Result<()> { + // Handle x-direction: continuous if has xmin/xmax, discrete otherwise + if let Some(xmin) = encoding.remove("xmin") { + encoding.insert("x".to_string(), xmin); + } + if let Some(xmax) = encoding.remove("xmax") { + encoding.insert("x2".to_string(), xmax); + } + + // Handle y-direction: continuous if has ymin/ymax, discrete otherwise + if let Some(ymin) = encoding.remove("ymin") { + encoding.insert("y".to_string(), ymin); + } + if let Some(ymax) = encoding.remove("ymax") { + encoding.insert("y2".to_string(), ymax); + } + + Ok(()) + } + + fn modify_spec(&self, layer_spec: &mut Value, layer: &Layer) -> Result<()> { + // Check each direction independently for discrete vs continuous + let encoding_obj = layer_spec + .get("encoding") + .and_then(|e| e.as_object()); + + let x_is_discrete = encoding_obj + .map(|e| !e.contains_key("x2")) + .unwrap_or(true); + let y_is_discrete = encoding_obj + .map(|e| !e.contains_key("y2")) + .unwrap_or(true); + + // Early return if both continuous (standard rect mark already set by geom_to_mark) + if !x_is_discrete && !y_is_discrete { + return Ok(()); + } + + // At least one direction is discrete - build mark spec with band sizing + let mut mark = json!({ + "type": "rect", + "clip": true + }); + + // Handle discrete x (needs band width) + if x_is_discrete { + let width = Self::extract_band_size(encoding_obj, "width", "x")?; + mark["width"] = json!({"band": width}); + } + + // Handle discrete y (needs band height) + if y_is_discrete { + let height = Self::extract_band_size(encoding_obj, "height", "y")?; + mark["height"] = json!({"band": height}); + } + + layer_spec["mark"] = mark; + Ok(()) + } +} + // ============================================================================= // Area Renderer // ============================================================================= @@ -786,6 +897,7 @@ pub fn get_renderer(geom: &Geom) -> Box { GeomType::Path => Box::new(PathRenderer), GeomType::Bar => Box::new(BarRenderer), GeomType::Area => Box::new(AreaRenderer), + GeomType::Rect => Box::new(RectRenderer), GeomType::Ribbon => Box::new(RibbonRenderer), GeomType::Polygon => Box::new(PolygonRenderer), GeomType::Boxplot => Box::new(BoxplotRenderer), From 8a1422e3e4a1a061945896975ef4d84ccec8627e Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 3 Mar 2026 17:25:35 +0100 Subject: [PATCH 05/15] fix: enable scale training for width/height in rect geom Key changes: - Add width/height to gets_default_scale() so they get proper scales with domains instead of Identity scales (which have scale: null) - Simplify rect.rs stat transform using get_column_name() directly - Merge discrete checking blocks to build SELECT and stat_columns together - Refactor RectRenderer to avoid Result> anti-pattern - Flatten nesting with early exits and error closure for DRY This fixes discrete rect layers with literal width/height (e.g., 0.7 AS width) by ensuring scales are trained and domains are available for constant detection. Co-Authored-By: Claude Sonnet 4.5 --- src/plot/layer/geom/rect.rs | 123 ++++++++++++++++++----------------- src/plot/scale/mod.rs | 2 + src/writer/vegalite/layer.rs | 106 +++++++++++++++++++----------- 3 files changed, 131 insertions(+), 100 deletions(-) diff --git a/src/plot/layer/geom/rect.rs b/src/plot/layer/geom/rect.rs index 8b70b1e6..4bf1c8b1 100644 --- a/src/plot/layer/geom/rect.rs +++ b/src/plot/layer/geom/rect.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use super::types::get_column_name; -use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType, StatResult}; +use super::{DefaultAesthetics, GeomTrait, GeomType, StatResult}; use crate::naming; use crate::plot::types::{DefaultAestheticValue, ParameterValue}; use crate::{DataFrame, GgsqlError, Mappings, Result}; @@ -31,18 +31,18 @@ impl GeomTrait for Rect { defaults: &[ // All positional aesthetics are optional inputs (Null) // They become Delayed after stat transform - ("pos1", DefaultAestheticValue::Null), // x (center) - ("pos1min", DefaultAestheticValue::Null), // xmin - ("pos1max", DefaultAestheticValue::Null), // xmax - ("width", DefaultAestheticValue::Null), // width (aesthetic, can map to column) - ("pos2", DefaultAestheticValue::Null), // y (center) - ("pos2min", DefaultAestheticValue::Null), // ymin - ("pos2max", DefaultAestheticValue::Null), // ymax - ("height", DefaultAestheticValue::Null), // height (aesthetic, can map to column) + ("pos1", DefaultAestheticValue::Null), // x (center) + ("pos1min", DefaultAestheticValue::Null), // xmin + ("pos1max", DefaultAestheticValue::Null), // xmax + ("width", DefaultAestheticValue::Null), // width (aesthetic, can map to column) + ("pos2", DefaultAestheticValue::Null), // y (center) + ("pos2min", DefaultAestheticValue::Null), // ymin + ("pos2max", DefaultAestheticValue::Null), // ymax + ("height", DefaultAestheticValue::Null), // height (aesthetic, can map to column) // Visual aesthetics ("fill", DefaultAestheticValue::String("black")), ("stroke", DefaultAestheticValue::String("black")), - ("opacity", DefaultAestheticValue::Number(1.0)), + ("opacity", DefaultAestheticValue::Number(0.5)), ("linewidth", DefaultAestheticValue::Number(1.0)), ("linetype", DefaultAestheticValue::String("solid")), ], @@ -59,11 +59,14 @@ impl GeomTrait for Rect { // For discrete scales: remap to center ("pos1", DefaultAestheticValue::Column("pos1")), ("pos2", DefaultAestheticValue::Column("pos2")), + // Width/height passed through for discrete (writer validation) + ("width", DefaultAestheticValue::Column("width")), + ("height", DefaultAestheticValue::Column("height")), ] } fn valid_stat_columns(&self) -> &'static [&'static str] { - &["pos1", "pos2", "pos1min", "pos1max", "pos2min", "pos2max"] + &["pos1", "pos2", "pos1min", "pos1max", "pos2min", "pos2max", "width", "height"] } fn stat_consumed_aesthetics(&self) -> &'static [&'static str] { @@ -104,27 +107,36 @@ fn stat_rect( group_by: &[String], _parameters: &HashMap, ) -> Result { - // Extract x-direction aesthetics + // Get aesthetic column names for SQL (at stat time, all aesthetics are columns) let x = get_column_name(aesthetics, "pos1"); let xmin = get_column_name(aesthetics, "pos1min"); let xmax = get_column_name(aesthetics, "pos1max"); let width = get_column_name(aesthetics, "width"); - // Extract y-direction aesthetics let y = get_column_name(aesthetics, "pos2"); let ymin = get_column_name(aesthetics, "pos2min"); let ymax = get_column_name(aesthetics, "pos2max"); let height = get_column_name(aesthetics, "height"); + // Filter out width/height from group_by (they're position aesthetics, not grouping) + let group_by: Vec = group_by + .iter() + .filter(|col| { + !width.as_ref().map_or(false, |w| col == &w) + && !height.as_ref().map_or(false, |h| col == &h) + }) + .cloned() + .collect(); + // Detect if x and y are discrete by checking schema let is_x_discrete = x .as_ref() - .and_then(|col| schema.iter().find(|c| c.name == *col)) + .and_then(|col| schema.iter().find(|c| &c.name == col)) .map(|c| c.is_discrete) .unwrap_or(false); let is_y_discrete = y .as_ref() - .and_then(|col| schema.iter().find(|c| c.name == *col)) + .and_then(|col| schema.iter().find(|c| &c.name == col)) .map(|c| c.is_discrete) .unwrap_or(false); @@ -147,35 +159,45 @@ fn stat_rect( "y", )?; - // Build stat column names - let stat_x = naming::stat_column("pos1"); - let stat_xmin = naming::stat_column("pos1min"); - let stat_xmax = naming::stat_column("pos1max"); - let stat_y = naming::stat_column("pos2"); - let stat_ymin = naming::stat_column("pos2min"); - let stat_ymax = naming::stat_column("pos2max"); - - // Build SELECT list + // Build SELECT list and stat_columns based on discrete vs continuous let mut select_parts = vec![]; + let mut stat_columns = vec![]; // Add group_by columns first if !group_by.is_empty() { select_parts.push(group_by.join(", ")); } - // Add position columns based on discrete vs continuous + // X direction if is_x_discrete { - select_parts.push(format!("{} AS {}", x_expr_min, stat_x)); + select_parts.push(format!("{} AS {}", x_expr_min, naming::stat_column("pos1"))); + stat_columns.push("pos1".to_string()); + // For discrete, pass through width if mapped (for scale training) + if let Some(ref width_col) = width { + select_parts.push(format!("{} AS {}", width_col, naming::stat_column("width"))); + stat_columns.push("width".to_string()); + } } else { - select_parts.push(format!("{} AS {}", x_expr_min, stat_xmin)); - select_parts.push(format!("{} AS {}", x_expr_max, stat_xmax)); + select_parts.push(format!("{} AS {}", x_expr_min, naming::stat_column("pos1min"))); + select_parts.push(format!("{} AS {}", x_expr_max, naming::stat_column("pos1max"))); + stat_columns.push("pos1min".to_string()); + stat_columns.push("pos1max".to_string()); } + // Y direction if is_y_discrete { - select_parts.push(format!("{} AS {}", y_expr_min, stat_y)); + select_parts.push(format!("{} AS {}", y_expr_min, naming::stat_column("pos2"))); + stat_columns.push("pos2".to_string()); + // For discrete, pass through height if mapped (for scale training) + if let Some(ref height_col) = height { + select_parts.push(format!("{} AS {}", height_col, naming::stat_column("height"))); + stat_columns.push("height".to_string()); + } } else { - select_parts.push(format!("{} AS {}", y_expr_min, stat_ymin)); - select_parts.push(format!("{} AS {}", y_expr_max, stat_ymax)); + select_parts.push(format!("{} AS {}", y_expr_min, naming::stat_column("pos2min"))); + select_parts.push(format!("{} AS {}", y_expr_max, naming::stat_column("pos2max"))); + stat_columns.push("pos2min".to_string()); + stat_columns.push("pos2max".to_string()); } let select_list = select_parts.join(", "); @@ -186,41 +208,20 @@ fn stat_rect( select_list, query ); - // Return stat columns based on discrete vs continuous - let stat_columns = match (is_x_discrete, is_y_discrete) { - (true, true) => vec!["pos1".to_string(), "pos2".to_string()], - (true, false) => vec![ - "pos1".to_string(), - "pos2min".to_string(), - "pos2max".to_string(), - ], - (false, true) => vec![ - "pos1min".to_string(), - "pos1max".to_string(), - "pos2".to_string(), - ], - (false, false) => vec![ - "pos1min".to_string(), - "pos1max".to_string(), - "pos2min".to_string(), - "pos2max".to_string(), - ], - }; + // Build consumed aesthetics - all potentially mapped positional aesthetics + let mut consumed = vec!["pos1", "pos1min", "pos1max", "pos2", "pos2min", "pos2max"]; + if width.is_some() { + consumed.push("width"); + } + if height.is_some() { + consumed.push("height"); + } Ok(StatResult::Transformed { query: transformed_query, stat_columns, dummy_columns: vec![], - consumed_aesthetics: vec![ - "pos1".to_string(), - "pos1min".to_string(), - "pos1max".to_string(), - "width".to_string(), - "pos2".to_string(), - "pos2min".to_string(), - "pos2max".to_string(), - "height".to_string(), - ], + consumed_aesthetics: consumed.iter().map(|s| s.to_string()).collect(), }) } diff --git a/src/plot/scale/mod.rs b/src/plot/scale/mod.rs index fb7203c7..31ea9b5e 100644 --- a/src/plot/scale/mod.rs +++ b/src/plot/scale/mod.rs @@ -61,6 +61,8 @@ pub fn gets_default_scale(aesthetic: &str) -> bool { "fill" | "stroke" // Size aesthetics | "size" | "linewidth" + // Dimension aesthetics + | "width" | "height" // Other visual aesthetics | "opacity" | "shape" | "linetype" ) diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index 651a52ff..859d892b 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -253,40 +253,70 @@ impl GeomRenderer for RibbonRenderer { pub struct RectRenderer; impl RectRenderer { - /// Extract band size (width or height) from encoding for discrete scales. - /// - /// Returns error if the aesthetic is mapped to a variable column (field), - /// extracts constant value if present, or returns default 0.9. + /// Extract and remove band size (width/height) from encoding for discrete scales. + /// Should only be called for discrete scales. fn extract_band_size( - encoding_obj: Option<&Map>, + encoding: &mut Map, aesthetic: &str, axis: &str, ) -> Result { const DEFAULT_BAND_SIZE: f64 = 0.9; - // Early return with default if encoding not present - let Some(enc) = encoding_obj else { - return Ok(DEFAULT_BAND_SIZE); - }; + // Extract and remove the aesthetic + let size_enc = encoding.remove(aesthetic); - // Early return with default if aesthetic not in encoding - let Some(size_enc) = enc.get(aesthetic) else { + // If no aesthetic specified, use default + let Some(size_enc) = size_enc else { return Ok(DEFAULT_BAND_SIZE); }; - // Check if it's a field (variable) - error - if size_enc.get("field").is_some() { + // Case 1: value encoding (from SETTING parameter) - extract directly + if let Some(value) = size_enc.get("value").and_then(|v| v.as_f64()) { + return Ok(value); + } + + // Case 2: field encoding (from MAPPING) - check scale domain for constant + if size_enc.get("field").is_none() { + // Neither value nor field - shouldn't happen return Err(GgsqlError::WriterError(format!( - "Discrete {} scale does not support variable {} columns.", - axis, aesthetic + "Invalid {} encoding (expected value or field).", + aesthetic ))); } - // Extract constant value or default - Ok(size_enc - .get("value") - .and_then(|v| v.as_f64()) - .unwrap_or(DEFAULT_BAND_SIZE)) + // Helper closure for repeated error message + let domain_error = || { + GgsqlError::WriterError(format!( + "Could not determine {} value for discrete {} scale.", + aesthetic, axis + )) + }; + + // Extract domain from scale + let domain = size_enc + .get("scale") + .and_then(|s| s.get("domain")) + .and_then(|d| d.as_array()) + .ok_or_else(domain_error)?; + + if domain.len() != 2 { + return Err(domain_error()); + } + + let (Some(min), Some(max)) = (domain[0].as_f64(), domain[1].as_f64()) else { + return Err(domain_error()); + }; + + if (min - max).abs() < 1e-10 { + // Constant value - use it + Ok(min) + } else { + // Variable - error + Err(GgsqlError::WriterError(format!( + "Discrete {} scale does not support variable {} columns.", + axis, aesthetic + ))) + } } } @@ -311,39 +341,37 @@ impl GeomRenderer for RectRenderer { Ok(()) } - fn modify_spec(&self, layer_spec: &mut Value, layer: &Layer) -> Result<()> { - // Check each direction independently for discrete vs continuous - let encoding_obj = layer_spec - .get("encoding") - .and_then(|e| e.as_object()); - - let x_is_discrete = encoding_obj - .map(|e| !e.contains_key("x2")) - .unwrap_or(true); - let y_is_discrete = encoding_obj - .map(|e| !e.contains_key("y2")) - .unwrap_or(true); - - // Early return if both continuous (standard rect mark already set by geom_to_mark) + fn modify_spec(&self, layer_spec: &mut Value, _layer: &Layer) -> Result<()> { + let encoding = layer_spec + .get_mut("encoding") + .and_then(|e| e.as_object_mut()); + + let Some(encoding) = encoding else { + return Ok(()); + }; + + // Check which directions are discrete + let x_is_discrete = !encoding.contains_key("x2"); + let y_is_discrete = !encoding.contains_key("y2"); + + // Early return if both continuous if !x_is_discrete && !y_is_discrete { return Ok(()); } - // At least one direction is discrete - build mark spec with band sizing + // Build mark spec with band sizing for discrete directions let mut mark = json!({ "type": "rect", "clip": true }); - // Handle discrete x (needs band width) if x_is_discrete { - let width = Self::extract_band_size(encoding_obj, "width", "x")?; + let width = Self::extract_band_size(encoding, "width", "x")?; mark["width"] = json!({"band": width}); } - // Handle discrete y (needs band height) if y_is_discrete { - let height = Self::extract_band_size(encoding_obj, "height", "y")?; + let height = Self::extract_band_size(encoding, "height", "y")?; mark["height"] = json!({"band": height}); } From e9045b8c1159be1009aa2c669e176b9d4e3f52ca Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 3 Mar 2026 17:54:04 +0100 Subject: [PATCH 06/15] test: add comprehensive tests for rect geom Add 10 parameterized tests covering: - All 6 x-direction parameter combinations (continuous) - All 6 y-direction parameter combinations (continuous) - Discrete scales with width/height - Validation errors (param count, discrete+min/max) - Group by filtering for width/height Tests use a grid/loop approach to systematically verify all rect parameter combinations and error cases. Co-Authored-By: Claude Sonnet 4.5 --- src/plot/layer/geom/rect.rs | 380 ++++++++++++++++++++++++++++++++++++ 1 file changed, 380 insertions(+) diff --git a/src/plot/layer/geom/rect.rs b/src/plot/layer/geom/rect.rs index 4bf1c8b1..ccf1e9bf 100644 --- a/src/plot/layer/geom/rect.rs +++ b/src/plot/layer/geom/rect.rs @@ -304,3 +304,383 @@ fn generate_position_expressions( ))), } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::plot::types::{AestheticValue, ColumnInfo}; + use polars::prelude::DataType; + + // ==================== Helper Functions ==================== + + fn create_schema(discrete_cols: &[&str]) -> Schema { + vec![ + ColumnInfo { + name: "__ggsql_aes_pos1__".to_string(), + dtype: if discrete_cols.contains(&"pos1") { + DataType::String + } else { + DataType::Float64 + }, + is_discrete: discrete_cols.contains(&"pos1"), + min: None, + max: None, + }, + ColumnInfo { + name: "__ggsql_aes_pos1min__".to_string(), + dtype: DataType::Float64, + is_discrete: false, + min: None, + max: None, + }, + ColumnInfo { + name: "__ggsql_aes_pos1max__".to_string(), + dtype: DataType::Float64, + is_discrete: false, + min: None, + max: None, + }, + ColumnInfo { + name: "__ggsql_aes_width__".to_string(), + dtype: DataType::Float64, + is_discrete: false, + min: None, + max: None, + }, + ColumnInfo { + name: "__ggsql_aes_pos2__".to_string(), + dtype: if discrete_cols.contains(&"pos2") { + DataType::String + } else { + DataType::Float64 + }, + is_discrete: discrete_cols.contains(&"pos2"), + min: None, + max: None, + }, + ColumnInfo { + name: "__ggsql_aes_pos2min__".to_string(), + dtype: DataType::Float64, + is_discrete: false, + min: None, + max: None, + }, + ColumnInfo { + name: "__ggsql_aes_pos2max__".to_string(), + dtype: DataType::Float64, + is_discrete: false, + min: None, + max: None, + }, + ColumnInfo { + name: "__ggsql_aes_height__".to_string(), + dtype: DataType::Float64, + is_discrete: false, + min: None, + max: None, + }, + ] + } + + fn create_aesthetics(mappings: &[&str]) -> Mappings { + let mut aesthetics = Mappings::new(); + for aesthetic in mappings { + // Use aesthetic column naming convention + let col_name = naming::aesthetic_column(aesthetic); + aesthetics.insert( + aesthetic.to_string(), + AestheticValue::standard_column(col_name), + ); + } + aesthetics + } + + // ==================== X-Direction Parameter Combinations (Continuous) ==================== + + #[test] + fn test_continuous_x_all_combinations() { + let test_cases = vec![ + // (name, x_aesthetics, expected_min_expr, expected_max_expr) + ( + "xmin + xmax", + vec!["pos1min", "pos1max"], + "__ggsql_aes_pos1min__", + "__ggsql_aes_pos1max__", + ), + ( + "x + width", + vec!["pos1", "width"], + "(__ggsql_aes_pos1__ - __ggsql_aes_width__ / 2.0)", + "(__ggsql_aes_pos1__ + __ggsql_aes_width__ / 2.0)", + ), + ( + "x + xmin", + vec!["pos1", "pos1min"], + "__ggsql_aes_pos1min__", + "(2 * __ggsql_aes_pos1__ - __ggsql_aes_pos1min__)", + ), + ( + "x + xmax", + vec!["pos1", "pos1max"], + "(2 * __ggsql_aes_pos1__ - __ggsql_aes_pos1max__)", + "__ggsql_aes_pos1max__", + ), + ( + "xmin + width", + vec!["pos1min", "width"], + "__ggsql_aes_pos1min__", + "(__ggsql_aes_pos1min__ + __ggsql_aes_width__)", + ), + ( + "xmax + width", + vec!["pos1max", "width"], + "(__ggsql_aes_pos1max__ - __ggsql_aes_width__)", + "__ggsql_aes_pos1max__", + ), + ]; + + for (name, x_aesthetics, expected_min, expected_max) in test_cases { + // Combine x aesthetics with fixed y mappings (ymin + ymax) + let mut all_mappings = x_aesthetics.clone(); + all_mappings.extend_from_slice(&["pos2min", "pos2max"]); + + let aesthetics = create_aesthetics(&all_mappings); + let schema = create_schema(&[]); + let group_by = vec![]; + let parameters = HashMap::new(); + + let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + + assert!(result.is_ok(), "{}: stat_rect failed: {:?}", name, result.err()); + let stat_result = result.unwrap(); + + if let StatResult::Transformed { query, stat_columns, .. } = stat_result { + let stat_pos1min = naming::stat_column("pos1min"); + let stat_pos1max = naming::stat_column("pos1max"); + assert!(query.contains(&format!("{} AS {}", expected_min, stat_pos1min)), + "{}: Expected '{} AS {}' in query, got: {}", name, expected_min, stat_pos1min, query); + assert!(query.contains(&format!("{} AS {}", expected_max, stat_pos1max)), + "{}: Expected '{} AS {}' in query, got: {}", name, expected_max, stat_pos1max, query); + assert!(stat_columns.contains(&"pos1min".to_string()), "{}: Missing pos1min in stat_columns", name); + assert!(stat_columns.contains(&"pos1max".to_string()), "{}: Missing pos1max in stat_columns", name); + } else { + panic!("{}: Expected Transformed result", name); + } + } + } + + // ==================== Y-Direction Parameter Combinations (Continuous) ==================== + + #[test] + fn test_continuous_y_all_combinations() { + let test_cases = vec![ + // (name, y_aesthetics, expected_min_expr, expected_max_expr) + ( + "ymin + ymax", + vec!["pos2min", "pos2max"], + "__ggsql_aes_pos2min__", + "__ggsql_aes_pos2max__", + ), + ( + "y + height", + vec!["pos2", "height"], + "(__ggsql_aes_pos2__ - __ggsql_aes_height__ / 2.0)", + "(__ggsql_aes_pos2__ + __ggsql_aes_height__ / 2.0)", + ), + ( + "y + ymin", + vec!["pos2", "pos2min"], + "__ggsql_aes_pos2min__", + "(2 * __ggsql_aes_pos2__ - __ggsql_aes_pos2min__)", + ), + ( + "y + ymax", + vec!["pos2", "pos2max"], + "(2 * __ggsql_aes_pos2__ - __ggsql_aes_pos2max__)", + "__ggsql_aes_pos2max__", + ), + ( + "ymin + height", + vec!["pos2min", "height"], + "__ggsql_aes_pos2min__", + "(__ggsql_aes_pos2min__ + __ggsql_aes_height__)", + ), + ( + "ymax + height", + vec!["pos2max", "height"], + "(__ggsql_aes_pos2max__ - __ggsql_aes_height__)", + "__ggsql_aes_pos2max__", + ), + ]; + + for (name, y_aesthetics, expected_min, expected_max) in test_cases { + // Combine y aesthetics with fixed x mappings (xmin + xmax) + let mut all_mappings = vec!["pos1min", "pos1max"]; + all_mappings.extend_from_slice(&y_aesthetics); + + let aesthetics = create_aesthetics(&all_mappings); + let schema = create_schema(&[]); + let group_by = vec![]; + let parameters = HashMap::new(); + + let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + + assert!(result.is_ok(), "{}: stat_rect failed: {:?}", name, result.err()); + let stat_result = result.unwrap(); + + if let StatResult::Transformed { query, stat_columns, .. } = stat_result { + let stat_pos2min = naming::stat_column("pos2min"); + let stat_pos2max = naming::stat_column("pos2max"); + assert!(query.contains(&format!("{} AS {}", expected_min, stat_pos2min)), + "{}: Expected '{} AS {}' in query, got: {}", name, expected_min, stat_pos2min, query); + assert!(query.contains(&format!("{} AS {}", expected_max, stat_pos2max)), + "{}: Expected '{} AS {}' in query, got: {}", name, expected_max, stat_pos2max, query); + assert!(stat_columns.contains(&"pos2min".to_string()), "{}: Missing pos2min in stat_columns", name); + assert!(stat_columns.contains(&"pos2max".to_string()), "{}: Missing pos2max in stat_columns", name); + } else { + panic!("{}: Expected Transformed result", name); + } + } + } + + // ==================== Discrete Scale Tests ==================== + + #[test] + fn test_discrete_x_with_width() { + let aesthetics = create_aesthetics(&["pos1", "width", "pos2min", "pos2max"]); + let schema = create_schema(&["pos1"]); + let group_by = vec![]; + let parameters = HashMap::new(); + + let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + assert!(result.is_ok()); + + if let Ok(StatResult::Transformed { query, stat_columns, .. }) = result { + assert!(query.contains("__ggsql_aes_pos1__ AS __ggsql_stat_pos1")); + assert!(query.contains("__ggsql_aes_width__ AS __ggsql_stat_width")); + assert!(stat_columns.contains(&"pos1".to_string())); + assert!(stat_columns.contains(&"width".to_string())); + assert!(stat_columns.contains(&"pos2min".to_string())); + assert!(stat_columns.contains(&"pos2max".to_string())); + } + } + + #[test] + fn test_discrete_y_with_height() { + let aesthetics = create_aesthetics(&["pos1min", "pos1max", "pos2", "height"]); + let schema = create_schema(&["pos2"]); + let group_by = vec![]; + let parameters = HashMap::new(); + + let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + assert!(result.is_ok()); + + if let Ok(StatResult::Transformed { query, stat_columns, .. }) = result { + assert!(query.contains("__ggsql_aes_pos2__ AS __ggsql_stat_pos2")); + assert!(query.contains("__ggsql_aes_height__ AS __ggsql_stat_height")); + assert!(stat_columns.contains(&"pos1min".to_string())); + assert!(stat_columns.contains(&"pos1max".to_string())); + assert!(stat_columns.contains(&"pos2".to_string())); + assert!(stat_columns.contains(&"height".to_string())); + } + } + + #[test] + fn test_discrete_both_directions() { + let aesthetics = create_aesthetics(&["pos1", "width", "pos2", "height"]); + let schema = create_schema(&["pos1", "pos2"]); + let group_by = vec![]; + let parameters = HashMap::new(); + + let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + assert!(result.is_ok()); + + if let Ok(StatResult::Transformed { query, stat_columns, .. }) = result { + assert!(query.contains("__ggsql_aes_pos1__ AS __ggsql_stat_pos1")); + assert!(query.contains("__ggsql_aes_width__ AS __ggsql_stat_width")); + assert!(query.contains("__ggsql_aes_pos2__ AS __ggsql_stat_pos2")); + assert!(query.contains("__ggsql_aes_height__ AS __ggsql_stat_height")); + assert_eq!(stat_columns.len(), 4); + } + } + + // ==================== Validation Error Tests ==================== + + #[test] + fn test_error_too_few_x_params() { + let aesthetics = create_aesthetics(&["pos1", "pos2min", "pos2max"]); + let schema = create_schema(&[]); + let group_by = vec![]; + let parameters = HashMap::new(); + + let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + assert!(result.is_err()); + let err_msg = result.unwrap_err().to_string(); + assert!(err_msg.contains("exactly 2 x-direction parameters")); + } + + #[test] + fn test_error_too_many_x_params() { + let aesthetics = create_aesthetics(&["pos1", "pos1min", "pos1max", "pos2min", "pos2max"]); + let schema = create_schema(&[]); + let group_by = vec![]; + let parameters = HashMap::new(); + + let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + assert!(result.is_err()); + let err_msg = result.unwrap_err().to_string(); + assert!(err_msg.contains("exactly 2 x-direction parameters")); + } + + #[test] + fn test_error_discrete_with_min_max() { + let aesthetics = create_aesthetics(&["pos1", "pos1min", "pos2min", "pos2max"]); + let schema = create_schema(&["pos1"]); + let group_by = vec![]; + let parameters = HashMap::new(); + + let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + assert!(result.is_err()); + let err_msg = result.unwrap_err().to_string(); + assert!(err_msg.contains("Cannot use xmin/xmax with discrete x")); + } + + #[test] + fn test_error_discrete_requires_width() { + let aesthetics = create_aesthetics(&["pos1", "pos2min", "pos2max"]); + let schema = create_schema(&["pos1"]); + let group_by = vec![]; + let parameters = HashMap::new(); + + let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + assert!(result.is_err()); + let err_msg = result.unwrap_err().to_string(); + assert!(err_msg.contains("Discrete x requires x and width")); + } + + // ==================== Group By Tests ==================== + + #[test] + fn test_width_height_filtered_from_group_by() { + let aesthetics = create_aesthetics(&["pos1", "width", "pos2", "height"]); + let schema = create_schema(&["pos1", "pos2"]); + // width and height in group_by should be filtered out + let group_by = vec![ + "__ggsql_aes_width__".to_string(), + "__ggsql_aes_height__".to_string(), + "__ggsql_aes_fill__".to_string(), + ]; + let parameters = HashMap::new(); + + let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + assert!(result.is_ok()); + + if let Ok(StatResult::Transformed { query, .. }) = result { + // Should only have fill in group by, not width or height + assert!(query.contains("SELECT __ggsql_aes_fill__,")); + // width and height should appear as stat columns, not group by + assert!(query.contains("__ggsql_aes_width__ AS __ggsql_stat_width")); + assert!(query.contains("__ggsql_aes_height__ AS __ggsql_stat_height")); + } + } +} From 72351e51c80ed8071787885bbf815ce125057414 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 4 Mar 2026 09:55:42 +0100 Subject: [PATCH 07/15] tests for the rectrenderer --- src/writer/vegalite/layer.rs | 227 ++++++++++++++++++++++++++++++++++- 1 file changed, 226 insertions(+), 1 deletion(-) diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index 859d892b..44d010a4 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -260,7 +260,7 @@ impl RectRenderer { aesthetic: &str, axis: &str, ) -> Result { - const DEFAULT_BAND_SIZE: f64 = 0.9; + const DEFAULT_BAND_SIZE: f64 = 1.0; // Extract and remove the aesthetic let size_enc = encoding.remove(aesthetic); @@ -1110,4 +1110,229 @@ mod tests { ])) ); } + + // ============================================================================= + // RectRenderer Test Helpers + // ============================================================================= + + /// Helper to create a quantitative encoding entry + fn quant(field: &str) -> Value { + json!({"field": field, "type": "quantitative"}) + } + + /// Helper to create a nominal encoding entry + fn nominal(field: &str) -> Value { + json!({"field": field, "type": "nominal"}) + } + + /// Helper to create a literal value encoding + fn literal(val: f64) -> Value { + json!({"value": val}) + } + + /// Helper to create a scale encoding with explicit domain + /// Use same min/max for constant scales, different values for variable scales + fn scale(field: &str, min: f64, max: f64) -> Value { + json!({ + "field": field, + "type": "quantitative", + "scale": { + "domain": [min, max] + } + }) + } + + /// Helper to run rect rendering pipeline (modify_encoding + modify_spec) + fn render_rect( + encoding: &mut Map, + ) -> Result { + let renderer = RectRenderer; + let layer = Layer::new(crate::plot::Geom::rect()); + + renderer.modify_encoding(encoding, &layer)?; + + let mut layer_spec = json!({ + "mark": {"type": "rect", "clip": true}, + "encoding": encoding + }); + + renderer.modify_spec(&mut layer_spec, &layer)?; + Ok(layer_spec) + } + + // ============================================================================= + // RectRenderer Tests + // ============================================================================= + + #[test] + fn test_rect_continuous_both_axes() { + // Test rect with continuous scales on both axes (xmin/xmax, ymin/ymax) + // Should remap xmin->x, xmax->x2, ymin->y, ymax->y2 + let mut encoding = serde_json::Map::new(); + encoding.insert("xmin".to_string(), quant("xmin_col")); + encoding.insert("xmax".to_string(), quant("xmax_col")); + encoding.insert("ymin".to_string(), quant("ymin_col")); + encoding.insert("ymax".to_string(), quant("ymax_col")); + + let spec = render_rect(&mut encoding).unwrap(); + + // Should remap to x/x2/y/y2 + let enc = spec["encoding"].as_object().unwrap(); + assert_eq!(enc.get("x"), Some(&quant("xmin_col"))); + assert_eq!(enc.get("x2"), Some(&quant("xmax_col"))); + assert_eq!(enc.get("y"), Some(&quant("ymin_col"))); + assert_eq!(enc.get("y2"), Some(&quant("ymax_col"))); + + // Original min/max should be removed + assert!(enc.get("xmin").is_none()); + assert!(enc.get("xmax").is_none()); + assert!(enc.get("ymin").is_none()); + assert!(enc.get("ymax").is_none()); + + // Should not have band sizing (both continuous) + assert!(spec["mark"].get("width").is_none()); + assert!(spec["mark"].get("height").is_none()); + } + + #[test] + fn test_rect_discrete_x_continuous_y() { + // Test rect with discrete x scale and continuous y scale + // x/width (discrete) and ymin/ymax (continuous) + let mut encoding = serde_json::Map::new(); + encoding.insert("x".to_string(), nominal("day")); + encoding.insert("width".to_string(), literal(0.8)); + encoding.insert("ymin".to_string(), quant("ymin_col")); + encoding.insert("ymax".to_string(), quant("ymax_col")); + + let spec = render_rect(&mut encoding).unwrap(); + let enc = spec["encoding"].as_object().unwrap(); + + // x should remain as x (discrete) + assert_eq!(enc.get("x"), Some(&nominal("day"))); + + // y should be remapped from ymin/ymax + assert_eq!(enc.get("y"), Some(&quant("ymin_col"))); + assert_eq!(enc.get("y2"), Some(&quant("ymax_col"))); + + // width should be removed + assert!(enc.get("width").is_none()); + + // Should have width band sizing for discrete x + assert_eq!(spec["mark"]["width"], json!({"band": 0.8})); + assert!(spec["mark"].get("height").is_none()); // y is continuous, no band height + } + + #[test] + fn test_rect_discrete_both_axes_literal_width() { + // Test rect with discrete scales on both axes with literal width/height + let mut encoding = serde_json::Map::new(); + encoding.insert("x".to_string(), nominal("day")); + encoding.insert("width".to_string(), literal(0.7)); + encoding.insert("y".to_string(), nominal("hour")); + encoding.insert("height".to_string(), literal(0.9)); + + let spec = render_rect(&mut encoding).unwrap(); + let enc = spec["encoding"].as_object().unwrap(); + + // x and y should remain + assert_eq!(enc.get("x"), Some(&nominal("day"))); + assert_eq!(enc.get("y"), Some(&nominal("hour"))); + + // width/height should be removed + assert!(enc.get("width").is_none()); + assert!(enc.get("height").is_none()); + + // Should have both width and height band sizing + assert_eq!(spec["mark"]["width"], json!({"band": 0.7})); + assert_eq!(spec["mark"]["height"], json!({"band": 0.9})); + } + + #[test] + fn test_rect_discrete_both_axes_default_width() { + // Test rect with discrete scales on both axes without explicit width/height + // Should use default band size (1.0) + let mut encoding = serde_json::Map::new(); + encoding.insert("x".to_string(), nominal("day")); + encoding.insert("y".to_string(), nominal("hour")); + + let spec = render_rect(&mut encoding).unwrap(); + + // Should have default band sizing (1.0) for both + assert_eq!(spec["mark"]["width"], json!({"band": 1.0})); + assert_eq!(spec["mark"]["height"], json!({"band": 1.0})); + } + + #[test] + fn test_rect_discrete_with_constant_width_column() { + // Test rect with discrete x scale where width is a constant-valued column + // This should work by detecting the constant in the scale domain + let mut encoding = serde_json::Map::new(); + encoding.insert("x".to_string(), nominal("day")); + encoding.insert("width".to_string(), scale("width_col", 0.85, 0.85)); // constant + encoding.insert("ymin".to_string(), quant("ymin_col")); + encoding.insert("ymax".to_string(), quant("ymax_col")); + + let spec = render_rect(&mut encoding).unwrap(); + + // Should extract the constant value 0.85 + assert_eq!(spec["mark"]["width"], json!({"band": 0.85})); + } + + #[test] + fn test_rect_discrete_with_variable_width_column_error() { + // Test that variable width columns on discrete scales produce an error + let mut encoding = serde_json::Map::new(); + encoding.insert("x".to_string(), nominal("day")); + encoding.insert("width".to_string(), scale("width_col", 0.5, 0.9)); // variable + + let result = render_rect(&mut encoding); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!(err.to_string().contains("Discrete x scale")); + assert!(err.to_string().contains("does not support variable width columns")); + } + + #[test] + fn test_rect_extract_band_size_missing_domain_error() { + // Test that missing domain in scale produces an error + let mut encoding = serde_json::Map::new(); + encoding.insert("x".to_string(), nominal("day")); + encoding.insert("width".to_string(), json!({ + "field": "width_col", + "type": "quantitative", + "scale": {} // missing domain + })); + + let result = render_rect(&mut encoding); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!(err.to_string().contains("Could not determine width value")); + } + + #[test] + fn test_rect_continuous_x_discrete_y() { + // Test rect with continuous x (xmin/xmax) and discrete y (y/height) + let mut encoding = serde_json::Map::new(); + encoding.insert("xmin".to_string(), quant("xmin_col")); + encoding.insert("xmax".to_string(), quant("xmax_col")); + encoding.insert("y".to_string(), nominal("category")); + encoding.insert("height".to_string(), literal(0.6)); + + let spec = render_rect(&mut encoding).unwrap(); + let enc = spec["encoding"].as_object().unwrap(); + + // x should be remapped from xmin/xmax + assert_eq!(enc.get("x"), Some(&quant("xmin_col"))); + assert_eq!(enc.get("x2"), Some(&quant("xmax_col"))); + + // y should remain as y (discrete) + assert_eq!(enc.get("y"), Some(&nominal("category"))); + + // height should be removed + assert!(enc.get("height").is_none()); + + // Should have height band sizing for discrete y + assert!(spec["mark"].get("width").is_none()); // x is continuous, no band width + assert_eq!(spec["mark"]["height"], json!({"band": 0.6})); + } } From f4d5ca9ee4ce74d1424db31f360203dd11459d13 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 4 Mar 2026 10:58:56 +0100 Subject: [PATCH 08/15] fix(rect): preserve non-positional aesthetics in stat transform The rect stat transform was only including group_by columns in its SELECT list, causing non-positional aesthetics like fill and color to be dropped unless they were literal values. Now uses the schema to determine which columns to pass through: - Defines consumed aesthetics once (positional params that get transformed) - Iterates through schema and includes all non-consumed columns - Eliminates duplicate "consumed columns" logic Also increases default rect opacity from 0.5 to 0.8 for better visibility. Co-Authored-By: Claude Sonnet 4.5 --- src/plot/layer/geom/rect.rs | 314 +++++++++++++++++++++++++++--------- 1 file changed, 240 insertions(+), 74 deletions(-) diff --git a/src/plot/layer/geom/rect.rs b/src/plot/layer/geom/rect.rs index ccf1e9bf..dd6d07c2 100644 --- a/src/plot/layer/geom/rect.rs +++ b/src/plot/layer/geom/rect.rs @@ -42,7 +42,7 @@ impl GeomTrait for Rect { // Visual aesthetics ("fill", DefaultAestheticValue::String("black")), ("stroke", DefaultAestheticValue::String("black")), - ("opacity", DefaultAestheticValue::Number(0.5)), + ("opacity", DefaultAestheticValue::Number(0.8)), ("linewidth", DefaultAestheticValue::Number(1.0)), ("linetype", DefaultAestheticValue::String("solid")), ], @@ -66,7 +66,9 @@ impl GeomTrait for Rect { } fn valid_stat_columns(&self) -> &'static [&'static str] { - &["pos1", "pos2", "pos1min", "pos1max", "pos2min", "pos2max", "width", "height"] + &[ + "pos1", "pos2", "pos1min", "pos1max", "pos2min", "pos2max", "width", "height", + ] } fn stat_consumed_aesthetics(&self) -> &'static [&'static str] { @@ -104,7 +106,7 @@ fn stat_rect( query: &str, schema: &Schema, aesthetics: &Mappings, - group_by: &[String], + _group_by: &[String], _parameters: &HashMap, ) -> Result { // Get aesthetic column names for SQL (at stat time, all aesthetics are columns) @@ -118,16 +120,6 @@ fn stat_rect( let ymax = get_column_name(aesthetics, "pos2max"); let height = get_column_name(aesthetics, "height"); - // Filter out width/height from group_by (they're position aesthetics, not grouping) - let group_by: Vec = group_by - .iter() - .filter(|col| { - !width.as_ref().map_or(false, |w| col == &w) - && !height.as_ref().map_or(false, |h| col == &h) - }) - .cloned() - .collect(); - // Detect if x and y are discrete by checking schema let is_x_discrete = x .as_ref() @@ -159,13 +151,26 @@ fn stat_rect( "y", )?; + // Define consumed aesthetics (these will be transformed, not passed through) + // This list determines both what columns to exclude from pass-through + // and what to report in StatResult.consumed_aesthetics + let consumed_aesthetic_names = ["pos1", "pos1min", "pos1max", "width", "pos2", "pos2min", "pos2max", "height"]; + + // Convert aesthetic names to column names for filtering + let consumed_columns: Vec = consumed_aesthetic_names + .iter() + .filter_map(|aes| get_column_name(aesthetics, aes)) + .collect(); + // Build SELECT list and stat_columns based on discrete vs continuous let mut select_parts = vec![]; let mut stat_columns = vec![]; - // Add group_by columns first - if !group_by.is_empty() { - select_parts.push(group_by.join(", ")); + // Add non-consumed columns from schema (pass through all non-positional aesthetics) + for col_info in schema { + if !consumed_columns.contains(&col_info.name) { + select_parts.push(col_info.name.clone()); + } } // X direction @@ -178,8 +183,16 @@ fn stat_rect( stat_columns.push("width".to_string()); } } else { - select_parts.push(format!("{} AS {}", x_expr_min, naming::stat_column("pos1min"))); - select_parts.push(format!("{} AS {}", x_expr_max, naming::stat_column("pos1max"))); + select_parts.push(format!( + "{} AS {}", + x_expr_min, + naming::stat_column("pos1min") + )); + select_parts.push(format!( + "{} AS {}", + x_expr_max, + naming::stat_column("pos1max") + )); stat_columns.push("pos1min".to_string()); stat_columns.push("pos1max".to_string()); } @@ -190,12 +203,24 @@ fn stat_rect( stat_columns.push("pos2".to_string()); // For discrete, pass through height if mapped (for scale training) if let Some(ref height_col) = height { - select_parts.push(format!("{} AS {}", height_col, naming::stat_column("height"))); + select_parts.push(format!( + "{} AS {}", + height_col, + naming::stat_column("height") + )); stat_columns.push("height".to_string()); } } else { - select_parts.push(format!("{} AS {}", y_expr_min, naming::stat_column("pos2min"))); - select_parts.push(format!("{} AS {}", y_expr_max, naming::stat_column("pos2max"))); + select_parts.push(format!( + "{} AS {}", + y_expr_min, + naming::stat_column("pos2min") + )); + select_parts.push(format!( + "{} AS {}", + y_expr_max, + naming::stat_column("pos2max") + )); stat_columns.push("pos2min".to_string()); stat_columns.push("pos2max".to_string()); } @@ -208,20 +233,12 @@ fn stat_rect( select_list, query ); - // Build consumed aesthetics - all potentially mapped positional aesthetics - let mut consumed = vec!["pos1", "pos1min", "pos1max", "pos2", "pos2min", "pos2max"]; - if width.is_some() { - consumed.push("width"); - } - if height.is_some() { - consumed.push("height"); - } - + // Use the same consumed aesthetic names for StatResult Ok(StatResult::Transformed { query: transformed_query, stat_columns, dummy_columns: vec![], - consumed_aesthetics: consumed.iter().map(|s| s.to_string()).collect(), + consumed_aesthetics: consumed_aesthetic_names.iter().map(|s| s.to_string()).collect(), }) } @@ -314,7 +331,11 @@ mod tests { // ==================== Helper Functions ==================== fn create_schema(discrete_cols: &[&str]) -> Schema { - vec![ + create_schema_with_extra(discrete_cols, &[]) + } + + fn create_schema_with_extra(discrete_cols: &[&str], extra_cols: &[&str]) -> Schema { + let mut schema = vec![ ColumnInfo { name: "__ggsql_aes_pos1__".to_string(), dtype: if discrete_cols.contains(&"pos1") { @@ -379,7 +400,20 @@ mod tests { min: None, max: None, }, - ] + ]; + + // Add extra columns (e.g., fill, color, etc.) + for col_name in extra_cols { + schema.push(ColumnInfo { + name: col_name.to_string(), + dtype: DataType::String, + is_discrete: true, + min: None, + max: None, + }); + } + + schema } fn create_aesthetics(mappings: &[&str]) -> Mappings { @@ -449,20 +483,56 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + let result = stat_rect( + "SELECT * FROM data", + &schema, + &aesthetics, + &group_by, + ¶meters, + ); - assert!(result.is_ok(), "{}: stat_rect failed: {:?}", name, result.err()); + assert!( + result.is_ok(), + "{}: stat_rect failed: {:?}", + name, + result.err() + ); let stat_result = result.unwrap(); - if let StatResult::Transformed { query, stat_columns, .. } = stat_result { + if let StatResult::Transformed { + query, + stat_columns, + .. + } = stat_result + { let stat_pos1min = naming::stat_column("pos1min"); let stat_pos1max = naming::stat_column("pos1max"); - assert!(query.contains(&format!("{} AS {}", expected_min, stat_pos1min)), - "{}: Expected '{} AS {}' in query, got: {}", name, expected_min, stat_pos1min, query); - assert!(query.contains(&format!("{} AS {}", expected_max, stat_pos1max)), - "{}: Expected '{} AS {}' in query, got: {}", name, expected_max, stat_pos1max, query); - assert!(stat_columns.contains(&"pos1min".to_string()), "{}: Missing pos1min in stat_columns", name); - assert!(stat_columns.contains(&"pos1max".to_string()), "{}: Missing pos1max in stat_columns", name); + assert!( + query.contains(&format!("{} AS {}", expected_min, stat_pos1min)), + "{}: Expected '{} AS {}' in query, got: {}", + name, + expected_min, + stat_pos1min, + query + ); + assert!( + query.contains(&format!("{} AS {}", expected_max, stat_pos1max)), + "{}: Expected '{} AS {}' in query, got: {}", + name, + expected_max, + stat_pos1max, + query + ); + assert!( + stat_columns.contains(&"pos1min".to_string()), + "{}: Missing pos1min in stat_columns", + name + ); + assert!( + stat_columns.contains(&"pos1max".to_string()), + "{}: Missing pos1max in stat_columns", + name + ); } else { panic!("{}: Expected Transformed result", name); } @@ -523,20 +593,56 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + let result = stat_rect( + "SELECT * FROM data", + &schema, + &aesthetics, + &group_by, + ¶meters, + ); - assert!(result.is_ok(), "{}: stat_rect failed: {:?}", name, result.err()); + assert!( + result.is_ok(), + "{}: stat_rect failed: {:?}", + name, + result.err() + ); let stat_result = result.unwrap(); - if let StatResult::Transformed { query, stat_columns, .. } = stat_result { + if let StatResult::Transformed { + query, + stat_columns, + .. + } = stat_result + { let stat_pos2min = naming::stat_column("pos2min"); let stat_pos2max = naming::stat_column("pos2max"); - assert!(query.contains(&format!("{} AS {}", expected_min, stat_pos2min)), - "{}: Expected '{} AS {}' in query, got: {}", name, expected_min, stat_pos2min, query); - assert!(query.contains(&format!("{} AS {}", expected_max, stat_pos2max)), - "{}: Expected '{} AS {}' in query, got: {}", name, expected_max, stat_pos2max, query); - assert!(stat_columns.contains(&"pos2min".to_string()), "{}: Missing pos2min in stat_columns", name); - assert!(stat_columns.contains(&"pos2max".to_string()), "{}: Missing pos2max in stat_columns", name); + assert!( + query.contains(&format!("{} AS {}", expected_min, stat_pos2min)), + "{}: Expected '{} AS {}' in query, got: {}", + name, + expected_min, + stat_pos2min, + query + ); + assert!( + query.contains(&format!("{} AS {}", expected_max, stat_pos2max)), + "{}: Expected '{} AS {}' in query, got: {}", + name, + expected_max, + stat_pos2max, + query + ); + assert!( + stat_columns.contains(&"pos2min".to_string()), + "{}: Missing pos2min in stat_columns", + name + ); + assert!( + stat_columns.contains(&"pos2max".to_string()), + "{}: Missing pos2max in stat_columns", + name + ); } else { panic!("{}: Expected Transformed result", name); } @@ -552,10 +658,21 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + let result = stat_rect( + "SELECT * FROM data", + &schema, + &aesthetics, + &group_by, + ¶meters, + ); assert!(result.is_ok()); - if let Ok(StatResult::Transformed { query, stat_columns, .. }) = result { + if let Ok(StatResult::Transformed { + query, + stat_columns, + .. + }) = result + { assert!(query.contains("__ggsql_aes_pos1__ AS __ggsql_stat_pos1")); assert!(query.contains("__ggsql_aes_width__ AS __ggsql_stat_width")); assert!(stat_columns.contains(&"pos1".to_string())); @@ -572,10 +689,21 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + let result = stat_rect( + "SELECT * FROM data", + &schema, + &aesthetics, + &group_by, + ¶meters, + ); assert!(result.is_ok()); - if let Ok(StatResult::Transformed { query, stat_columns, .. }) = result { + if let Ok(StatResult::Transformed { + query, + stat_columns, + .. + }) = result + { assert!(query.contains("__ggsql_aes_pos2__ AS __ggsql_stat_pos2")); assert!(query.contains("__ggsql_aes_height__ AS __ggsql_stat_height")); assert!(stat_columns.contains(&"pos1min".to_string())); @@ -592,10 +720,21 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + let result = stat_rect( + "SELECT * FROM data", + &schema, + &aesthetics, + &group_by, + ¶meters, + ); assert!(result.is_ok()); - if let Ok(StatResult::Transformed { query, stat_columns, .. }) = result { + if let Ok(StatResult::Transformed { + query, + stat_columns, + .. + }) = result + { assert!(query.contains("__ggsql_aes_pos1__ AS __ggsql_stat_pos1")); assert!(query.contains("__ggsql_aes_width__ AS __ggsql_stat_width")); assert!(query.contains("__ggsql_aes_pos2__ AS __ggsql_stat_pos2")); @@ -613,7 +752,13 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + let result = stat_rect( + "SELECT * FROM data", + &schema, + &aesthetics, + &group_by, + ¶meters, + ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); assert!(err_msg.contains("exactly 2 x-direction parameters")); @@ -626,7 +771,13 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + let result = stat_rect( + "SELECT * FROM data", + &schema, + &aesthetics, + &group_by, + ¶meters, + ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); assert!(err_msg.contains("exactly 2 x-direction parameters")); @@ -639,7 +790,13 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + let result = stat_rect( + "SELECT * FROM data", + &schema, + &aesthetics, + &group_by, + ¶meters, + ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); assert!(err_msg.contains("Cannot use xmin/xmax with discrete x")); @@ -652,33 +809,42 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + let result = stat_rect( + "SELECT * FROM data", + &schema, + &aesthetics, + &group_by, + ¶meters, + ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); assert!(err_msg.contains("Discrete x requires x and width")); } - // ==================== Group By Tests ==================== + // ==================== Non-Consumed Aesthetic Tests ==================== #[test] - fn test_width_height_filtered_from_group_by() { + fn test_non_consumed_aesthetics_passed_through() { let aesthetics = create_aesthetics(&["pos1", "width", "pos2", "height"]); - let schema = create_schema(&["pos1", "pos2"]); - // width and height in group_by should be filtered out - let group_by = vec![ - "__ggsql_aes_width__".to_string(), - "__ggsql_aes_height__".to_string(), - "__ggsql_aes_fill__".to_string(), - ]; + // Include fill in schema (it's a non-consumed aesthetic) + let schema = create_schema_with_extra(&["pos1", "pos2"], &["__ggsql_aes_fill__"]); + let group_by = vec![]; let parameters = HashMap::new(); - let result = stat_rect("SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters); + let result = stat_rect( + "SELECT * FROM data", + &schema, + &aesthetics, + &group_by, + ¶meters, + ); assert!(result.is_ok()); if let Ok(StatResult::Transformed { query, .. }) = result { - // Should only have fill in group by, not width or height - assert!(query.contains("SELECT __ggsql_aes_fill__,")); - // width and height should appear as stat columns, not group by + // Should include fill column (non-consumed aesthetic from schema) + assert!(query.contains("__ggsql_aes_fill__")); + // Should NOT include width/height as pass-through (they're consumed) + // They should only appear as stat columns assert!(query.contains("__ggsql_aes_width__ AS __ggsql_stat_width")); assert!(query.contains("__ggsql_aes_height__ AS __ggsql_stat_height")); } From c4e0e7091727522dfc41e821ec6495848d9f305a Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 4 Mar 2026 11:13:07 +0100 Subject: [PATCH 09/15] Revert "add resolution helper" This reverts commit a0c70929306995344dad330ac3fd1ac6b9962f1c. --- src/writer/vegalite/layer.rs | 77 ------------------------------------ 1 file changed, 77 deletions(-) diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index 44d010a4..31dbb335 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -936,87 +936,10 @@ pub fn get_renderer(geom: &Geom) -> Box { } } -// ============================================================================= -// General Helper Functions -// ============================================================================= - -/// Calculate data resolution (minimum spacing between consecutive unique values). -/// -/// For continuous numeric data: returns the minimum spacing between consecutive unique values. -/// For a single unique value or empty input: returns 1.0 as a default. -/// Uses a small tolerance (epsilon) to avoid floating point precision issues. -/// -/// # Arguments -/// * `values` - Numeric values from the data (will be sorted internally) -/// -/// # Returns -/// The minimum distance between consecutive sorted values, or 1.0 if there are fewer than 2 values. -/// -/// # Example -/// ```ignore -/// let values = vec![1.0, 5.0, 2.0, 6.0]; -/// let resolution = calculate_resolution(&values); // Returns 1.0 (minimum spacing after sorting: 1-2 or 5-6) -/// ``` -fn calculate_resolution(values: &[f64]) -> f64 { - const EPSILON: f64 = 1e-10; - - if values.len() < 2 { - return 1.0; - } - - // Sort and deduplicate values - let mut sorted = values.to_vec(); - sorted.sort_by(|a, b| a.partial_cmp(b).unwrap_or(std::cmp::Ordering::Equal)); - sorted.dedup(); - - if sorted.len() < 2 { - return 1.0; // All values are identical - } - - let mut min_dist = f64::MAX; - for i in 1..sorted.len() { - let dist = sorted[i] - sorted[i - 1]; - // Only consider distances above epsilon to avoid floating point noise - if dist > EPSILON && dist < min_dist { - min_dist = dist; - } - } - - if min_dist == f64::MAX { - 1.0 // All values are within epsilon of each other - } else { - min_dist - } -} - #[cfg(test)] mod tests { use super::*; - #[test] - fn test_calculate_resolution() { - // Normal case: minimum spacing - assert_eq!(calculate_resolution(&[1.0, 2.0, 5.0, 6.0]), 1.0); - - // Unsorted input (should sort internally) - assert_eq!(calculate_resolution(&[5.0, 1.0, 6.0, 2.0]), 1.0); - - // Single value - assert_eq!(calculate_resolution(&[5.0]), 1.0); - - // Empty - assert_eq!(calculate_resolution(&[]), 1.0); - - // All identical values - assert_eq!(calculate_resolution(&[5.0, 5.0, 5.0]), 1.0); - - // Larger spacing - assert_eq!(calculate_resolution(&[0.0, 10.0, 20.0]), 10.0); - - // Fractional values - assert_eq!(calculate_resolution(&[0.0, 0.5, 1.5, 2.0]), 0.5); - } - #[test] fn test_violin_detail_encoding() { let renderer = ViolinRenderer; From bf0e2403e26260787c33c2fc62741d1748d29ba0 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 4 Mar 2026 11:44:17 +0100 Subject: [PATCH 10/15] delete tile layer --- CLAUDE.md | 4 +-- doc/ggsql.xml | 2 +- ggsql-vscode/syntaxes/ggsql.tmLanguage.json | 2 +- src/parser/builder.rs | 1 - src/parser/mod.rs | 12 ++++---- src/plot/layer/geom/mod.rs | 10 ------ src/plot/layer/geom/tile.rs | 34 --------------------- src/reader/mod.rs | 2 +- src/writer/vegalite/layer.rs | 24 ++++++++------- src/writer/vegalite/mod.rs | 4 --- tree-sitter-ggsql/grammar.js | 2 +- tree-sitter-ggsql/queries/highlights.scm | 2 +- 12 files changed, 26 insertions(+), 73 deletions(-) delete mode 100644 src/plot/layer/geom/tile.rs diff --git a/CLAUDE.md b/CLAUDE.md index 07ea0b0c..e01a901a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -330,7 +330,7 @@ pub struct Layer { pub enum Geom { // Basic geoms - Point, Line, Path, Bar, Col, Area, Tile, Polygon, Ribbon, + Point, Line, Path, Bar, Col, Area, Rect, Polygon, Ribbon, // Statistical geoms Histogram, Density, Smooth, Boxplot, Violin, // Annotation geoms @@ -1200,7 +1200,7 @@ All clauses (MAPPING, SETTING, PARTITION BY, FILTER) are optional. **Geom Types**: -- **Basic**: `point`, `line`, `path`, `bar`, `col`, `area`, `tile`, `polygon`, `ribbon` +- **Basic**: `point`, `line`, `path`, `bar`, `col`, `area`, `rect`, `polygon`, `ribbon` - **Statistical**: `histogram`, `density`, `smooth`, `boxplot`, `violin` - **Annotation**: `text`, `label`, `segment`, `arrow`, `hline`, `vline`, `abline`, `errorbar` diff --git a/doc/ggsql.xml b/doc/ggsql.xml index d244edd8..d6b14316 100644 --- a/doc/ggsql.xml +++ b/doc/ggsql.xml @@ -129,7 +129,7 @@ bar col area - tile + rect polygon ribbon histogram diff --git a/ggsql-vscode/syntaxes/ggsql.tmLanguage.json b/ggsql-vscode/syntaxes/ggsql.tmLanguage.json index b8966255..81b92106 100644 --- a/ggsql-vscode/syntaxes/ggsql.tmLanguage.json +++ b/ggsql-vscode/syntaxes/ggsql.tmLanguage.json @@ -294,7 +294,7 @@ "patterns": [ { "name": "support.type.geom.ggsql", - "match": "\\b(point|line|path|bar|col|area|tile|polygon|ribbon|histogram|density|smooth|boxplot|violin|text|label|segment|arrow|hline|vline|abline|errorbar)\\b" + "match": "\\b(point|line|path|bar|col|area|rect|polygon|ribbon|histogram|density|smooth|boxplot|violin|text|label|segment|arrow|hline|vline|abline|errorbar)\\b" }, { "include": "#common-clause-patterns" } ] diff --git a/src/parser/builder.rs b/src/parser/builder.rs index 91818b21..c01912bc 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -583,7 +583,6 @@ fn parse_geom_type(text: &str) -> Result { "path" => Ok(Geom::path()), "bar" => Ok(Geom::bar()), "area" => Ok(Geom::area()), - "tile" => Ok(Geom::tile()), "rect" => Ok(Geom::rect()), "polygon" => Ok(Geom::polygon()), "ribbon" => Ok(Geom::ribbon()), diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 6c80d63a..a3d8676c 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -146,12 +146,12 @@ mod tests { let query = r#" SELECT x, y FROM data VISUALIZE x, y - DRAW tile + DRAW point "#; let specs = parse_query(query).unwrap(); assert_eq!(specs.len(), 1); - assert_eq!(specs[0].layers[0].geom, Geom::tile()); + assert_eq!(specs[0].layers[0].geom, Geom::point()); } #[test] @@ -163,7 +163,7 @@ mod tests { VISUALIZE DRAW bar MAPPING x AS x, y AS y VISUALISE z AS x, y AS y - DRAW tile + DRAW point "#; let specs = parse_query(query).unwrap(); @@ -219,7 +219,7 @@ mod tests { VISUALISE x, y DRAW line VISUALIZE - DRAW tile MAPPING x AS x, y AS y + DRAW point MAPPING x AS x, y AS y VISUALISE DRAW bar MAPPING x AS x, y AS y "#; @@ -227,7 +227,7 @@ mod tests { let specs = parse_query(query).unwrap(); assert_eq!(specs.len(), 3); assert_eq!(specs[0].layers[0].geom, Geom::line()); - assert_eq!(specs[1].layers[0].geom, Geom::tile()); + assert_eq!(specs[1].layers[0].geom, Geom::point()); assert_eq!(specs[2].layers[0].geom, Geom::bar()); } @@ -245,7 +245,7 @@ mod tests { VISUALIZE DRAW bar MAPPING date AS x, revenue AS y VISUALISE - DRAW tile MAPPING date AS x, revenue AS y + DRAW point MAPPING date AS x, revenue AS y "#; let specs = parse_query(query).unwrap(); diff --git a/src/plot/layer/geom/mod.rs b/src/plot/layer/geom/mod.rs index d562ad63..30ba7c8a 100644 --- a/src/plot/layer/geom/mod.rs +++ b/src/plot/layer/geom/mod.rs @@ -47,7 +47,6 @@ mod ribbon; mod segment; mod smooth; mod text; -mod tile; mod violin; mod vline; @@ -74,7 +73,6 @@ pub use ribbon::Ribbon; pub use segment::Segment; pub use smooth::Smooth; pub use text::Text; -pub use tile::Tile; pub use violin::Violin; pub use vline::VLine; @@ -89,7 +87,6 @@ pub enum GeomType { Path, Bar, Area, - Tile, Rect, Polygon, Ribbon, @@ -116,7 +113,6 @@ impl std::fmt::Display for GeomType { GeomType::Path => "path", GeomType::Bar => "bar", GeomType::Area => "area", - GeomType::Tile => "tile", GeomType::Rect => "rect", GeomType::Polygon => "polygon", GeomType::Ribbon => "ribbon", @@ -255,11 +251,6 @@ impl Geom { Self(Arc::new(Area)) } - /// Create a Tile geom - pub fn tile() -> Self { - Self(Arc::new(Tile)) - } - /// Create a Rect geom pub fn rect() -> Self { Self(Arc::new(Rect)) @@ -348,7 +339,6 @@ impl Geom { GeomType::Path => Self::path(), GeomType::Bar => Self::bar(), GeomType::Area => Self::area(), - GeomType::Tile => Self::tile(), GeomType::Rect => Self::rect(), GeomType::Polygon => Self::polygon(), GeomType::Ribbon => Self::ribbon(), diff --git a/src/plot/layer/geom/tile.rs b/src/plot/layer/geom/tile.rs deleted file mode 100644 index 870721d3..00000000 --- a/src/plot/layer/geom/tile.rs +++ /dev/null @@ -1,34 +0,0 @@ -//! Tile geom implementation - -use super::{DefaultAesthetics, GeomTrait, GeomType}; -use crate::plot::types::DefaultAestheticValue; - -/// Tile geom - heatmaps and tile-based visualizations -#[derive(Debug, Clone, Copy)] -pub struct Tile; - -impl GeomTrait for Tile { - fn geom_type(&self) -> GeomType { - GeomType::Tile - } - - fn aesthetics(&self) -> DefaultAesthetics { - DefaultAesthetics { - defaults: &[ - ("pos1", DefaultAestheticValue::Required), - ("pos2", DefaultAestheticValue::Required), - ("fill", DefaultAestheticValue::String("black")), - ("stroke", DefaultAestheticValue::String("black")), - ("width", DefaultAestheticValue::Null), - ("height", DefaultAestheticValue::Null), - ("opacity", DefaultAestheticValue::Number(1.0)), - ], - } - } -} - -impl std::fmt::Display for Tile { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "tile") - } -} diff --git a/src/reader/mod.rs b/src/reader/mod.rs index 25771b79..c9d50e7c 100644 --- a/src/reader/mod.rs +++ b/src/reader/mod.rs @@ -733,7 +733,7 @@ mod tests { (4, 40, 85.0) ) AS t(x, y, value) VISUALISE - DRAW tile MAPPING x AS x, y AS y, value AS fill + DRAW point MAPPING x AS x, y AS y, value AS fill SCALE BINNED fill FROM [0, 100] TO viridis SETTING breaks => [0, 25, 50, 75, 100] "#; diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index 31dbb335..249bf373 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -30,7 +30,6 @@ pub fn geom_to_mark(geom: &Geom) -> Value { GeomType::Path => "line", GeomType::Bar => "bar", GeomType::Area => "area", - GeomType::Tile => "rect", GeomType::Rect => "rect", GeomType::Ribbon => "area", GeomType::Polygon => "line", @@ -931,7 +930,7 @@ pub fn get_renderer(geom: &Geom) -> Box { GeomType::Boxplot => Box::new(BoxplotRenderer), GeomType::Density => Box::new(AreaRenderer), GeomType::Violin => Box::new(ViolinRenderer), - // All other geoms (Point, Line, Tile, etc.) use the default renderer + // All other geoms (Point, Line, etc.) use the default renderer _ => Box::new(DefaultRenderer), } } @@ -1066,9 +1065,7 @@ mod tests { } /// Helper to run rect rendering pipeline (modify_encoding + modify_spec) - fn render_rect( - encoding: &mut Map, - ) -> Result { + fn render_rect(encoding: &mut Map) -> Result { let renderer = RectRenderer; let layer = Layer::new(crate::plot::Geom::rect()); @@ -1212,7 +1209,9 @@ mod tests { assert!(result.is_err()); let err = result.unwrap_err(); assert!(err.to_string().contains("Discrete x scale")); - assert!(err.to_string().contains("does not support variable width columns")); + assert!(err + .to_string() + .contains("does not support variable width columns")); } #[test] @@ -1220,11 +1219,14 @@ mod tests { // Test that missing domain in scale produces an error let mut encoding = serde_json::Map::new(); encoding.insert("x".to_string(), nominal("day")); - encoding.insert("width".to_string(), json!({ - "field": "width_col", - "type": "quantitative", - "scale": {} // missing domain - })); + encoding.insert( + "width".to_string(), + json!({ + "field": "width_col", + "type": "quantitative", + "scale": {} // missing domain + }), + ); let result = render_rect(&mut encoding); assert!(result.is_err()); diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index 7e5c7ef6..86e9b683 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -1155,10 +1155,6 @@ mod tests { geom_to_mark(&Geom::area()), json!({"type": "area", "clip": true}) ); - assert_eq!( - geom_to_mark(&Geom::tile()), - json!({"type": "rect", "clip": true}) - ); } #[test] diff --git a/tree-sitter-ggsql/grammar.js b/tree-sitter-ggsql/grammar.js index d6138c5b..8d188206 100644 --- a/tree-sitter-ggsql/grammar.js +++ b/tree-sitter-ggsql/grammar.js @@ -470,7 +470,7 @@ module.exports = grammar({ ), geom_type: $ => choice( - 'point', 'line', 'path', 'bar', 'area', 'tile', 'rect', 'polygon', 'ribbon', + 'point', 'line', 'path', 'bar', 'area', 'rect', 'polygon', 'ribbon', 'histogram', 'density', 'smooth', 'boxplot', 'violin', 'text', 'label', 'segment', 'arrow', 'hline', 'vline', 'abline', 'errorbar' ), diff --git a/tree-sitter-ggsql/queries/highlights.scm b/tree-sitter-ggsql/queries/highlights.scm index 7066685d..110c2250 100644 --- a/tree-sitter-ggsql/queries/highlights.scm +++ b/tree-sitter-ggsql/queries/highlights.scm @@ -12,7 +12,7 @@ "path" "bar" "area" - "tile" + "rect" "polygon" "ribbon" "histogram" From 5e930243f03f8e4ea3ce9f7f77c9d81db979ca14 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 4 Mar 2026 12:33:29 +0100 Subject: [PATCH 11/15] feat: default width/height to 1.0 for rect geom Allow rect to work with just x (or y) specified, defaulting width (or height) to 1.0 for both discrete and continuous scales. Changes: - Split position expression generation into discrete and continuous variants - generate_discrete_position_expressions: returns (center, size) with size defaulting to "1.0" when not provided - generate_continuous_position_expressions: returns (min_expr, max_expr) with 7 valid parameter combinations including center-only - Discrete scales: center-only specification defaults to 1.0 bandwidth - Continuous scales: center-only specification defaults to width=1.0 (xmin = x - 0.5, xmax = x + 0.5) - Improved variable naming: x_expr_1/x_expr_2 instead of x_expr_min/x_expr_max to reflect dual usage (center/size for discrete, min/max for continuous) - Updated tests to verify default behavior All 18 rect tests passing. --- src/plot/layer/geom/rect.rs | 178 +++++++++++++++++++++++------------- test.txt | 0 2 files changed, 114 insertions(+), 64 deletions(-) create mode 100644 test.txt diff --git a/src/plot/layer/geom/rect.rs b/src/plot/layer/geom/rect.rs index dd6d07c2..c1d532fe 100644 --- a/src/plot/layer/geom/rect.rs +++ b/src/plot/layer/geom/rect.rs @@ -133,23 +133,42 @@ fn stat_rect( .unwrap_or(false); // Generate SQL expressions based on parameter combinations - // Validation (exactly 2 params, discrete + min/max check) happens inside - let (x_expr_min, x_expr_max) = generate_position_expressions( - x.as_deref(), - xmin.as_deref(), - xmax.as_deref(), - width.as_deref(), - is_x_discrete, - "x", - )?; - let (y_expr_min, y_expr_max) = generate_position_expressions( - y.as_deref(), - ymin.as_deref(), - ymax.as_deref(), - height.as_deref(), - is_y_discrete, - "y", - )?; + // For discrete: returns (center, size) where size defaults to 1.0 + // For continuous: returns (min_expr, max_expr) + let (x_expr_1, x_expr_2) = if is_x_discrete { + generate_discrete_position_expressions( + x.as_deref(), + xmin.as_deref(), + xmax.as_deref(), + width.as_deref(), + "x", + )? + } else { + generate_continuous_position_expressions( + x.as_deref(), + xmin.as_deref(), + xmax.as_deref(), + width.as_deref(), + "x", + )? + }; + let (y_expr_1, y_expr_2) = if is_y_discrete { + generate_discrete_position_expressions( + y.as_deref(), + ymin.as_deref(), + ymax.as_deref(), + height.as_deref(), + "y", + )? + } else { + generate_continuous_position_expressions( + y.as_deref(), + ymin.as_deref(), + ymax.as_deref(), + height.as_deref(), + "y", + )? + }; // Define consumed aesthetics (these will be transformed, not passed through) // This list determines both what columns to exclude from pass-through @@ -175,22 +194,21 @@ fn stat_rect( // X direction if is_x_discrete { - select_parts.push(format!("{} AS {}", x_expr_min, naming::stat_column("pos1"))); + // x_expr_1 is center, x_expr_2 is width (defaults to 1.0) + select_parts.push(format!("{} AS {}", x_expr_1, naming::stat_column("pos1"))); + select_parts.push(format!("{} AS {}", x_expr_2, naming::stat_column("width"))); stat_columns.push("pos1".to_string()); - // For discrete, pass through width if mapped (for scale training) - if let Some(ref width_col) = width { - select_parts.push(format!("{} AS {}", width_col, naming::stat_column("width"))); - stat_columns.push("width".to_string()); - } + stat_columns.push("width".to_string()); } else { + // x_expr_1 is min, x_expr_2 is max select_parts.push(format!( "{} AS {}", - x_expr_min, + x_expr_1, naming::stat_column("pos1min") )); select_parts.push(format!( "{} AS {}", - x_expr_max, + x_expr_2, naming::stat_column("pos1max") )); stat_columns.push("pos1min".to_string()); @@ -199,26 +217,21 @@ fn stat_rect( // Y direction if is_y_discrete { - select_parts.push(format!("{} AS {}", y_expr_min, naming::stat_column("pos2"))); + // y_expr_1 is center, y_expr_2 is height (defaults to 1.0) + select_parts.push(format!("{} AS {}", y_expr_1, naming::stat_column("pos2"))); + select_parts.push(format!("{} AS {}", y_expr_2, naming::stat_column("height"))); stat_columns.push("pos2".to_string()); - // For discrete, pass through height if mapped (for scale training) - if let Some(ref height_col) = height { - select_parts.push(format!( - "{} AS {}", - height_col, - naming::stat_column("height") - )); - stat_columns.push("height".to_string()); - } + stat_columns.push("height".to_string()); } else { + // y_expr_1 is min, y_expr_2 is max select_parts.push(format!( "{} AS {}", - y_expr_min, + y_expr_1, naming::stat_column("pos2min") )); select_parts.push(format!( "{} AS {}", - y_expr_max, + y_expr_2, naming::stat_column("pos2max") )); stat_columns.push("pos2min".to_string()); @@ -242,23 +255,21 @@ fn stat_rect( }) } -/// Generate SQL expressions for position min/max based on parameter combinations -/// -/// Returns (min_expr, max_expr) or (center_expr, center_expr) for discrete +/// Generate SQL expressions for discrete position (returns center, size) /// /// Validates: /// - Discrete scales cannot use min/max aesthetics -/// - Exactly 2 parameters provided (via match statement) -fn generate_position_expressions( +/// - Center is required +/// - Size defaults to "1.0" if not provided +fn generate_discrete_position_expressions( center: Option<&str>, min: Option<&str>, max: Option<&str>, size: Option<&str>, - is_discrete: bool, axis: &str, ) -> Result<(String, String)> { // Validate: discrete scales cannot use min/max - if is_discrete && (min.is_some() || max.is_some()) { + if min.is_some() || max.is_some() { return Err(GgsqlError::ValidationError(format!( "Cannot use {}min/{}max with discrete {} aesthetic. Use {} + {} instead.", axis, @@ -269,21 +280,33 @@ fn generate_position_expressions( ))); } - // For discrete, only center + size is valid - if is_discrete { - if let (Some(c), Some(_)) = (center, size) { - return Ok((c.to_string(), c.to_string())); - } - return Err(GgsqlError::ValidationError(format!( - "Discrete {} requires {} and {}.", + match center { + Some(c) => Ok((c.to_string(), size.unwrap_or("1.0").to_string())), + None => Err(GgsqlError::ValidationError(format!( + "Discrete {} requires {}.", axis, - axis, - if axis == "x" { "width" } else { "height" } - ))); + axis + ))), } +} - // For continuous, handle all 6 combinations - // The _ arm catches invalid parameter counts (not exactly 2) +/// Generate SQL expressions for continuous position (returns min_expr, max_expr) +/// +/// Handles all 7 valid parameter combinations: +/// - min + max +/// - center + size +/// - center only (defaults size to 1.0) +/// - center + min +/// - center + max +/// - min + size +/// - max + size +fn generate_continuous_position_expressions( + center: Option<&str>, + min: Option<&str>, + max: Option<&str>, + size: Option<&str>, + axis: &str, +) -> Result<(String, String)> { match (center, min, max, size) { // Case 1: min + max (None, Some(min_col), Some(max_col), None) => { @@ -294,6 +317,11 @@ fn generate_position_expressions( format!("({} - {} / 2.0)", c, s), format!("({} + {} / 2.0)", c, s), )), + // Case 2b: center only (default size to 1.0) + (Some(c), None, None, None) => Ok(( + format!("({} - 0.5)", c), + format!("({} + 0.5)", c), + )), // Case 3: center + min (Some(c), Some(min_col), None, None) => { Ok((min_col.to_string(), format!("(2 * {} - {})", c, min_col))) @@ -447,6 +475,12 @@ mod tests { "(__ggsql_aes_pos1__ - __ggsql_aes_width__ / 2.0)", "(__ggsql_aes_pos1__ + __ggsql_aes_width__ / 2.0)", ), + ( + "x only (default width 1.0)", + vec!["pos1"], + "(__ggsql_aes_pos1__ - 0.5)", + "(__ggsql_aes_pos1__ + 0.5)", + ), ( "x + xmin", vec!["pos1", "pos1min"], @@ -746,7 +780,8 @@ mod tests { // ==================== Validation Error Tests ==================== #[test] - fn test_error_too_few_x_params() { + fn test_continuous_x_defaults_width() { + // Test that continuous x without explicit width defaults to 1.0 let aesthetics = create_aesthetics(&["pos1", "pos2min", "pos2max"]); let schema = create_schema(&[]); let group_by = vec![]; @@ -759,9 +794,17 @@ mod tests { &group_by, ¶meters, ); - assert!(result.is_err()); - let err_msg = result.unwrap_err().to_string(); - assert!(err_msg.contains("exactly 2 x-direction parameters")); + assert!(result.is_ok()); + let stat_result = result.unwrap(); + match stat_result { + StatResult::Transformed { query, stat_columns, .. } => { + assert!(query.contains("(__ggsql_aes_pos1__ - 0.5)")); + assert!(query.contains("(__ggsql_aes_pos1__ + 0.5)")); + assert!(stat_columns.contains(&"pos1min".to_string())); + assert!(stat_columns.contains(&"pos1max".to_string())); + } + _ => panic!("Expected Transformed"), + } } #[test] @@ -803,7 +846,8 @@ mod tests { } #[test] - fn test_error_discrete_requires_width() { + fn test_discrete_x_defaults_width() { + // Test that discrete x without explicit width defaults to 1.0 let aesthetics = create_aesthetics(&["pos1", "pos2min", "pos2max"]); let schema = create_schema(&["pos1"]); let group_by = vec![]; @@ -816,9 +860,15 @@ mod tests { &group_by, ¶meters, ); - assert!(result.is_err()); - let err_msg = result.unwrap_err().to_string(); - assert!(err_msg.contains("Discrete x requires x and width")); + assert!(result.is_ok()); + let stat_result = result.unwrap(); + match stat_result { + StatResult::Transformed { query, stat_columns, .. } => { + assert!(query.contains("1.0 AS __ggsql_stat_width")); + assert!(stat_columns.contains(&"width".to_string())); + } + _ => panic!("Expected Transformed"), + } } // ==================== Non-Consumed Aesthetic Tests ==================== diff --git a/test.txt b/test.txt new file mode 100644 index 00000000..e69de29b From 892fd78d37416e48f456150c70b22187b68eafe6 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 4 Mar 2026 12:45:53 +0100 Subject: [PATCH 12/15] feat: support SETTING width/height for rect geom Add support for specifying width/height via SETTING clause in addition to MAPPING. Implements precedence: MAPPING > SETTING > default 1.0. Changes: - stat_rect now checks both aesthetics (MAPPING) and parameters (SETTING) for width/height values - Uses ParameterValue::to_string() to convert SETTING values to SQL literals - Precedence order ensures MAPPING columns take priority over SETTING literals, which take priority over default 1.0 - Stat transform happens before resolve_aesthetics(), so we check parameters directly rather than relying on aesthetic resolution Example usage: DRAW rect MAPPING x AS x, ymin AS ymin, ymax AS ymax SETTING width => 0.8 All 19 rect tests passing. --- src/plot/layer/geom/rect.rs | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/src/plot/layer/geom/rect.rs b/src/plot/layer/geom/rect.rs index c1d532fe..7367fc69 100644 --- a/src/plot/layer/geom/rect.rs +++ b/src/plot/layer/geom/rect.rs @@ -107,18 +107,23 @@ fn stat_rect( schema: &Schema, aesthetics: &Mappings, _group_by: &[String], - _parameters: &HashMap, + parameters: &HashMap, ) -> Result { // Get aesthetic column names for SQL (at stat time, all aesthetics are columns) let x = get_column_name(aesthetics, "pos1"); let xmin = get_column_name(aesthetics, "pos1min"); let xmax = get_column_name(aesthetics, "pos1max"); - let width = get_column_name(aesthetics, "width"); + // Prefer MAPPING width, fallback to SETTING width (as SQL literal) + // Precedence: MAPPING > SETTING > default (default handled in position expressions) + let width = get_column_name(aesthetics, "width") + .or_else(|| parameters.get("width").map(|v| v.to_string())); let y = get_column_name(aesthetics, "pos2"); let ymin = get_column_name(aesthetics, "pos2min"); let ymax = get_column_name(aesthetics, "pos2max"); - let height = get_column_name(aesthetics, "height"); + // Prefer MAPPING height, fallback to SETTING height (as SQL literal) + let height = get_column_name(aesthetics, "height") + .or_else(|| parameters.get("height").map(|v| v.to_string())); // Detect if x and y are discrete by checking schema let is_x_discrete = x @@ -899,4 +904,30 @@ mod tests { assert!(query.contains("__ggsql_aes_height__ AS __ggsql_stat_height")); } } + + #[test] + fn test_setting_width_as_fallback() { + // Test that SETTING width/height are used when no MAPPING is provided + let aesthetics = create_aesthetics(&["pos1", "pos2"]); + let schema = create_schema(&["pos1", "pos2"]); + let group_by = vec![]; + let mut parameters = HashMap::new(); + parameters.insert("width".to_string(), ParameterValue::Number(0.7)); + parameters.insert("height".to_string(), ParameterValue::Number(0.9)); + + let result = stat_rect( + "SELECT * FROM data", + &schema, + &aesthetics, + &group_by, + ¶meters, + ); + assert!(result.is_ok()); + + if let Ok(StatResult::Transformed { query, .. }) = result { + // Should use SETTING values as SQL literals + assert!(query.contains("0.7 AS __ggsql_stat_width")); + assert!(query.contains("0.9 AS __ggsql_stat_height")); + } + } } From f50b92cf348bc8e8481002efdee4771232b4cd8e Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 4 Mar 2026 13:54:24 +0100 Subject: [PATCH 13/15] refactor: unify x/y direction logic in rect stat transform Extract duplicated x and y direction logic into a single process_direction helper function that handles both axes. Changes: - New process_direction() function processes a single direction - Takes only axis ("x" or "y"), derives all aesthetic names from it - Returns SELECT parts and stat column names - Handles both discrete and continuous cases - Determine stat_cols first, then format SELECT parts outside if-block to eliminate duplication of format! calls - stat_rect() now calls process_direction() twice (once for x, once for y) - Eliminates ~100 lines of duplicated logic Call sites simplified from: process_direction("pos1", "pos1min", "pos1max", "width", "width", "x", ...) to: process_direction("x", aesthetics, parameters, schema) Net reduction: 26 lines (114 deletions, 88 additions) All 19 rect tests passing. --- src/plot/layer/geom/rect.rs | 188 ++++++++++++++---------------------- 1 file changed, 74 insertions(+), 114 deletions(-) diff --git a/src/plot/layer/geom/rect.rs b/src/plot/layer/geom/rect.rs index 7367fc69..0d8bc108 100644 --- a/src/plot/layer/geom/rect.rs +++ b/src/plot/layer/geom/rect.rs @@ -101,83 +101,85 @@ impl std::fmt::Display for Rect { } } -/// Statistical transformation for rect: consolidate parameters and compute min/max -fn stat_rect( - query: &str, - schema: &Schema, +/// Process a single direction (x or y) for rect stat transform +/// Returns (select_parts, stat_column_names) +fn process_direction( + axis: &str, aesthetics: &Mappings, - _group_by: &[String], parameters: &HashMap, -) -> Result { - // Get aesthetic column names for SQL (at stat time, all aesthetics are columns) - let x = get_column_name(aesthetics, "pos1"); - let xmin = get_column_name(aesthetics, "pos1min"); - let xmax = get_column_name(aesthetics, "pos1max"); - // Prefer MAPPING width, fallback to SETTING width (as SQL literal) - // Precedence: MAPPING > SETTING > default (default handled in position expressions) - let width = get_column_name(aesthetics, "width") - .or_else(|| parameters.get("width").map(|v| v.to_string())); - - let y = get_column_name(aesthetics, "pos2"); - let ymin = get_column_name(aesthetics, "pos2min"); - let ymax = get_column_name(aesthetics, "pos2max"); - // Prefer MAPPING height, fallback to SETTING height (as SQL literal) - let height = get_column_name(aesthetics, "height") - .or_else(|| parameters.get("height").map(|v| v.to_string())); - - // Detect if x and y are discrete by checking schema - let is_x_discrete = x - .as_ref() - .and_then(|col| schema.iter().find(|c| &c.name == col)) - .map(|c| c.is_discrete) - .unwrap_or(false); - let is_y_discrete = y + schema: &Schema, +) -> Result<(Vec, Vec)> { + // Derive aesthetic names from axis + let (center_aes, min_aes, max_aes, size_aes) = match axis { + "x" => ("pos1", "pos1min", "pos1max", "width"), + "y" => ("pos2", "pos2min", "pos2max", "height"), + _ => unreachable!("axis must be 'x' or 'y'"), + }; + + // Get column names from MAPPING, with SETTING fallback for size + let center = get_column_name(aesthetics, center_aes); + let min = get_column_name(aesthetics, min_aes); + let max = get_column_name(aesthetics, max_aes); + let size = get_column_name(aesthetics, size_aes) + .or_else(|| parameters.get(size_aes).map(|v| v.to_string())); + + // Detect if discrete by checking schema + let is_discrete = center .as_ref() .and_then(|col| schema.iter().find(|c| &c.name == col)) .map(|c| c.is_discrete) .unwrap_or(false); - // Generate SQL expressions based on parameter combinations - // For discrete: returns (center, size) where size defaults to 1.0 - // For continuous: returns (min_expr, max_expr) - let (x_expr_1, x_expr_2) = if is_x_discrete { + // Generate position expressions + let (expr_1, expr_2) = if is_discrete { generate_discrete_position_expressions( - x.as_deref(), - xmin.as_deref(), - xmax.as_deref(), - width.as_deref(), - "x", + center.as_deref(), + min.as_deref(), + max.as_deref(), + size.as_deref(), + axis, )? } else { generate_continuous_position_expressions( - x.as_deref(), - xmin.as_deref(), - xmax.as_deref(), - width.as_deref(), - "x", + center.as_deref(), + min.as_deref(), + max.as_deref(), + size.as_deref(), + axis, )? }; - let (y_expr_1, y_expr_2) = if is_y_discrete { - generate_discrete_position_expressions( - y.as_deref(), - ymin.as_deref(), - ymax.as_deref(), - height.as_deref(), - "y", - )? + + // Determine stat column names based on discrete vs continuous + let stat_cols = if is_discrete { + vec![center_aes.to_string(), size_aes.to_string()] } else { - generate_continuous_position_expressions( - y.as_deref(), - ymin.as_deref(), - ymax.as_deref(), - height.as_deref(), - "y", - )? + vec![min_aes.to_string(), max_aes.to_string()] }; + // Build SELECT parts using the stat columns + let select_parts = vec![ + format!("{} AS {}", expr_1, naming::stat_column(&stat_cols[0])), + format!("{} AS {}", expr_2, naming::stat_column(&stat_cols[1])), + ]; + + Ok((select_parts, stat_cols)) +} + +/// Statistical transformation for rect: consolidate parameters and compute min/max +fn stat_rect( + query: &str, + schema: &Schema, + aesthetics: &Mappings, + _group_by: &[String], + parameters: &HashMap, +) -> Result { + // Process X direction + let (x_select, x_stat_cols) = process_direction("x", aesthetics, parameters, schema)?; + + // Process Y direction + let (y_select, y_stat_cols) = process_direction("y", aesthetics, parameters, schema)?; + // Define consumed aesthetics (these will be transformed, not passed through) - // This list determines both what columns to exclude from pass-through - // and what to report in StatResult.consumed_aesthetics let consumed_aesthetic_names = ["pos1", "pos1min", "pos1max", "width", "pos2", "pos2min", "pos2max", "height"]; // Convert aesthetic names to column names for filtering @@ -186,62 +188,20 @@ fn stat_rect( .filter_map(|aes| get_column_name(aesthetics, aes)) .collect(); - // Build SELECT list and stat_columns based on discrete vs continuous - let mut select_parts = vec![]; - let mut stat_columns = vec![]; - - // Add non-consumed columns from schema (pass through all non-positional aesthetics) - for col_info in schema { - if !consumed_columns.contains(&col_info.name) { - select_parts.push(col_info.name.clone()); - } - } + // Build SELECT list starting with non-consumed columns + let mut select_parts: Vec = schema + .iter() + .filter(|col| !consumed_columns.contains(&col.name)) + .map(|col| col.name.clone()) + .collect(); - // X direction - if is_x_discrete { - // x_expr_1 is center, x_expr_2 is width (defaults to 1.0) - select_parts.push(format!("{} AS {}", x_expr_1, naming::stat_column("pos1"))); - select_parts.push(format!("{} AS {}", x_expr_2, naming::stat_column("width"))); - stat_columns.push("pos1".to_string()); - stat_columns.push("width".to_string()); - } else { - // x_expr_1 is min, x_expr_2 is max - select_parts.push(format!( - "{} AS {}", - x_expr_1, - naming::stat_column("pos1min") - )); - select_parts.push(format!( - "{} AS {}", - x_expr_2, - naming::stat_column("pos1max") - )); - stat_columns.push("pos1min".to_string()); - stat_columns.push("pos1max".to_string()); - } + // Add X direction SELECT parts and collect stat columns + select_parts.extend(x_select); + let mut stat_columns = x_stat_cols; - // Y direction - if is_y_discrete { - // y_expr_1 is center, y_expr_2 is height (defaults to 1.0) - select_parts.push(format!("{} AS {}", y_expr_1, naming::stat_column("pos2"))); - select_parts.push(format!("{} AS {}", y_expr_2, naming::stat_column("height"))); - stat_columns.push("pos2".to_string()); - stat_columns.push("height".to_string()); - } else { - // y_expr_1 is min, y_expr_2 is max - select_parts.push(format!( - "{} AS {}", - y_expr_1, - naming::stat_column("pos2min") - )); - select_parts.push(format!( - "{} AS {}", - y_expr_2, - naming::stat_column("pos2max") - )); - stat_columns.push("pos2min".to_string()); - stat_columns.push("pos2max".to_string()); - } + // Add Y direction SELECT parts and collect stat columns + select_parts.extend(y_select); + stat_columns.extend(y_stat_cols); let select_list = select_parts.join(", "); From 5031116887082d411a4e3da9f8d02a8e6445327f Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 4 Mar 2026 15:00:31 +0100 Subject: [PATCH 14/15] add docs --- doc/syntax/layer/rect.qmd | 103 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 doc/syntax/layer/rect.qmd diff --git a/doc/syntax/layer/rect.qmd b/doc/syntax/layer/rect.qmd new file mode 100644 index 00000000..a6dc3e5f --- /dev/null +++ b/doc/syntax/layer/rect.qmd @@ -0,0 +1,103 @@ +--- +title: "Rectangle" +--- + +> Layers are declared with the [`DRAW` clause](../clause/draw.qmd). Read the documentation for this clause for a thorough description of how to use it. + +Rectangles can be used to draw heatmaps or indicate ranges. + +## Aesthetics +The following aesthetics are recognised by the rectangle layer. + +### Required + +* Pick two of the following: + * `x`: The center along the x-axis. + * `width`: The size of the rectangle in the horizontal dimension. + * `xmin`: The left position along the x-axis. Unavailable when `x` is discrete. + * `xmax`: The right position laong the x-axis. Unavailable when `x` is discrete. + +Alternatively, use only `x`, which will set `width` to 1 by default. + +* Pick two of the following: + * `y`: The center along the y-axis. + * `height`: The size of the rectangle in the vertical dimension. + * `ymin`: The bottom position along the y-axis. Unavailable when `y` is discrete. + * `ymax`: The top position along the y-axis. Unailable when `y` is discrete. + +Alternatively, use only `y`, which will set `height` to 1 by default. + +### Optional +* `stroke`: The colour of the contour lines. +* `fill`: The colour of the inner area. +* `colour`: Shorthand for setting `stroke` and `fill` simultaneously. +* `opacity`: The opacity of colours. +* `linewidth`: The width of the contour lines. +* `linetype`: The dash pattern of the contour line. + +## Settings +The rectangle layer has no additional settings. + +## Data transformation. +When the x-aesthetics are continuous, x-data is reparameterised to `xmin` and `xmax`. +When the y-aesthetics are continuous, y-data is reparameterised to `ymin` and `ymax`. + +## Examples + +Just using `x` and `y`. Note that `width` and `height` are set to 1. + +```{ggsql} +VISUALISE Day AS x, Month AS y, Temp AS colour FROM ggsql:airquality + DRAW rect +``` + +Customising `width` and `height` with either the `MAPPING` or `SETTING` clauses. + +```{ggsql} +VISUALISE Day AS x, Month AS y, Temp AS colour FROM ggsql:airquality + DRAW rect MAPPING 0.5 AS width SETTING height => 0.8 +``` + +If `x` is continuous, then `width` can be variable. Likewise for `y` and `height`. + +```{ggsql} +SELECT *, Temp / (SELECT MAX(Temp) FROM ggsql:airquality) AS norm_temp +FROM ggsql:airquality +VISUALISE + Day AS x, + Month AS y, + Temp AS colour + DRAW rect + MAPPING + norm_temp AS width, + norm_temp AS height +``` + +Using top, right, bottom, left parameterisation instead. + +```{ggsql} +SELECT + MIN(Date) AS start, + MAX(Date) AS end, + MIN(Temp) AS min, + MAX(Temp) AS max +FROM ggsql:airquality +GROUP BY + WEEKOFYEAR(Date) + +VISUALISE start AS xmin, end AS xmax, min AS ymin, max AS ymax + DRAW rect +``` + +Using a rectangle as an annotation. + +```{ggsql} +VISUALISE FROM ggsql:airquality + DRAW rect MAPPING + '1973-06-01' AS xmin, + '1973-06-30' AS xmax, + 50 AS ymin, + 100 AS ymax, + 'June' AS colour + DRAW line MAPPING Date AS x, Temp AS y +``` \ No newline at end of file From b21cb7cfe3922b6c207f678af5948c1d9581825a Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 4 Mar 2026 15:30:52 +0100 Subject: [PATCH 15/15] cargo fmt --- src/plot/layer/geom/rect.rs | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/plot/layer/geom/rect.rs b/src/plot/layer/geom/rect.rs index 0d8bc108..5f7a8822 100644 --- a/src/plot/layer/geom/rect.rs +++ b/src/plot/layer/geom/rect.rs @@ -180,7 +180,9 @@ fn stat_rect( let (y_select, y_stat_cols) = process_direction("y", aesthetics, parameters, schema)?; // Define consumed aesthetics (these will be transformed, not passed through) - let consumed_aesthetic_names = ["pos1", "pos1min", "pos1max", "width", "pos2", "pos2min", "pos2max", "height"]; + let consumed_aesthetic_names = [ + "pos1", "pos1min", "pos1max", "width", "pos2", "pos2min", "pos2max", "height", + ]; // Convert aesthetic names to column names for filtering let consumed_columns: Vec = consumed_aesthetic_names @@ -216,7 +218,10 @@ fn stat_rect( query: transformed_query, stat_columns, dummy_columns: vec![], - consumed_aesthetics: consumed_aesthetic_names.iter().map(|s| s.to_string()).collect(), + consumed_aesthetics: consumed_aesthetic_names + .iter() + .map(|s| s.to_string()) + .collect(), }) } @@ -249,8 +254,7 @@ fn generate_discrete_position_expressions( Some(c) => Ok((c.to_string(), size.unwrap_or("1.0").to_string())), None => Err(GgsqlError::ValidationError(format!( "Discrete {} requires {}.", - axis, - axis + axis, axis ))), } } @@ -283,10 +287,7 @@ fn generate_continuous_position_expressions( format!("({} + {} / 2.0)", c, s), )), // Case 2b: center only (default size to 1.0) - (Some(c), None, None, None) => Ok(( - format!("({} - 0.5)", c), - format!("({} + 0.5)", c), - )), + (Some(c), None, None, None) => Ok((format!("({} - 0.5)", c), format!("({} + 0.5)", c))), // Case 3: center + min (Some(c), Some(min_col), None, None) => { Ok((min_col.to_string(), format!("(2 * {} - {})", c, min_col))) @@ -762,7 +763,11 @@ mod tests { assert!(result.is_ok()); let stat_result = result.unwrap(); match stat_result { - StatResult::Transformed { query, stat_columns, .. } => { + StatResult::Transformed { + query, + stat_columns, + .. + } => { assert!(query.contains("(__ggsql_aes_pos1__ - 0.5)")); assert!(query.contains("(__ggsql_aes_pos1__ + 0.5)")); assert!(stat_columns.contains(&"pos1min".to_string())); @@ -828,7 +833,11 @@ mod tests { assert!(result.is_ok()); let stat_result = result.unwrap(); match stat_result { - StatResult::Transformed { query, stat_columns, .. } => { + StatResult::Transformed { + query, + stat_columns, + .. + } => { assert!(query.contains("1.0 AS __ggsql_stat_width")); assert!(stat_columns.contains(&"width".to_string())); }