From 76503b763c57f1284616415598d10158d4adc344 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 16 Mar 2026 11:28:46 +0100 Subject: [PATCH 01/12] replace validation function --- CLAUDE.md | 2 +- src/execute/mod.rs | 2 +- src/plot/layer/mod.rs | 34 +++++++++++++++++++++++++++------- src/plot/main.rs | 10 +++++----- src/validate.rs | 2 +- src/writer/vegalite/mod.rs | 4 ++-- 6 files changed, 37 insertions(+), 17 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b1a43363..5e8d46cc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -433,7 +433,7 @@ pub struct Theme { - `Layer::with_parameter(parameter, value)` - Add a geom parameter (builder pattern) - `Layer::get_column(aesthetic)` - Get column name for an aesthetic (if mapped to column) - `Layer::get_literal(aesthetic)` - Get literal value for an aesthetic (if literal) -- `Layer::validate_required_aesthetics()` - Validate that required aesthetics are present for the geom type +- `Layer::validate_mapping()` - Validate that required aesthetics are present for the geom type **Type conversions:** diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 625a17b4..dc4c8d0d 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -55,7 +55,7 @@ fn validate(layers: &[Layer], layer_schemas: &[Schema]) -> Result<()> { // Validate required aesthetics for this geom layer - .validate_required_aesthetics() + .validate_mapping() .map_err(|e| GgsqlError::ValidationError(format!("Layer {}: {}", idx + 1, e)))?; // Validate SETTING parameters are valid for this geom diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index eb6f189c..ce71104f 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -157,17 +157,37 @@ impl Layer { } } - /// Check if this layer has the required aesthetics for its geom - pub fn validate_required_aesthetics(&self) -> std::result::Result<(), String> { + /// Check if this layer has the required aesthetics, and no exotic aesthetics. + pub fn validate_mapping(&self) -> std::result::Result<(), String> { + // Check if all required aesthetics exist. + let mut missing: Vec<&str> = Vec::new(); for aesthetic in self.geom.aesthetics().required() { if !self.mappings.contains_key(aesthetic) { - return Err(format!( - "Geom '{}' requires aesthetic '{}' but it was not provided", - self.geom, aesthetic - )); + missing.push(aesthetic); } } - + if !missing.is_empty() { + return Err(format!( + "Layer '{}' mapping requires the '{}' aesthetic(s).", + self.geom, + missing.join(", ") + )); + } + // Check if any unsupported mappings are present + let mut extra: Vec<&str> = Vec::new(); + let supported = self.geom.aesthetics().supported(); + for aesthetic in self.mappings.aesthetics.keys() { + if !supported.contains(&aesthetic.as_str()) { + extra.push(aesthetic); + } + } + if !extra.is_empty() { + return Err(format!( + "Layer '{}' does not support the '{}' mapping(s).", + self.geom, + extra.join(", ") + )); + } Ok(()) } diff --git a/src/plot/main.rs b/src/plot/main.rs index 08eb3d53..7c228d7c 100644 --- a/src/plot/main.rs +++ b/src/plot/main.rs @@ -363,12 +363,12 @@ mod tests { .with_aesthetic("pos1".to_string(), AestheticValue::standard_column("x")) .with_aesthetic("pos2".to_string(), AestheticValue::standard_column("y")); - assert!(valid_point.validate_required_aesthetics().is_ok()); + assert!(valid_point.validate_mapping().is_ok()); let invalid_point = Layer::new(Geom::point()) .with_aesthetic("pos1".to_string(), AestheticValue::standard_column("x")); - assert!(invalid_point.validate_required_aesthetics().is_err()); + assert!(invalid_point.validate_mapping().is_err()); let valid_ribbon = Layer::new(Geom::ribbon()) .with_aesthetic("pos1".to_string(), AestheticValue::standard_column("x")) @@ -381,7 +381,7 @@ mod tests { AestheticValue::standard_column("ymax"), ); - assert!(valid_ribbon.validate_required_aesthetics().is_ok()); + assert!(valid_ribbon.validate_mapping().is_ok()); } #[test] @@ -759,8 +759,8 @@ mod tests { let labels = spec.labels.as_ref().unwrap(); assert_eq!(labels.labels.get("pos1"), Some(&"X Axis".to_string())); assert_eq!(labels.labels.get("pos2"), Some(&"Y Axis".to_string())); - assert!(labels.labels.get("x").is_none()); - assert!(labels.labels.get("y").is_none()); + assert!(!labels.labels.contains_key("x")); + assert!(!labels.labels.contains_key("y")); } #[test] diff --git a/src/validate.rs b/src/validate.rs index b07e8585..aa5029d2 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -178,7 +178,7 @@ pub fn validate(query: &str) -> Result { // Note: Without schema data, we can only check if mappings exist, // not if the columns are valid. We skip this check for wildcards. if !layer.mappings.wildcard { - if let Err(e) = layer.validate_required_aesthetics() { + if let Err(e) = layer.validate_mapping() { errors.push(ValidationError { message: format!("{}: {}", context, e), location: None, diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index 9d775dbf..321075a2 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -1139,8 +1139,8 @@ impl Writer for VegaLiteWriter { // Validate each layer for layer in &spec.layers { - // Check required aesthetics - layer.validate_required_aesthetics().map_err(|e| { + // Check aesthetics + layer.validate_mapping().map_err(|e| { GgsqlError::ValidationError(format!("Layer validation failed: {}", e)) })?; From ba8e3799bcef8315e01c3ecbdbd03c763c197979 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 16 Mar 2026 11:32:51 +0100 Subject: [PATCH 02/12] facet aesthetics are valid --- src/plot/layer/mod.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index ce71104f..53dbfd9e 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -20,7 +20,10 @@ pub use geom::{ // Re-export position types for convenience pub use position::{Position, PositionTrait, PositionType}; -use crate::plot::types::{AestheticValue, DataSource, Mappings, ParameterValue, SqlExpression}; +use crate::plot::{ + is_facet_aesthetic, + types::{AestheticValue, DataSource, Mappings, ParameterValue, SqlExpression}, +}; /// A single visualization layer (from DRAW clause) #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -177,6 +180,9 @@ impl Layer { let mut extra: Vec<&str> = Vec::new(); let supported = self.geom.aesthetics().supported(); for aesthetic in self.mappings.aesthetics.keys() { + if is_facet_aesthetic(aesthetic) { + continue; + } if !supported.contains(&aesthetic.as_str()) { extra.push(aesthetic); } From 0db700879b91348f1b5233145bbf72378e68c95f Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 16 Mar 2026 11:59:11 +0100 Subject: [PATCH 03/12] use user-facing names for aesthetics --- src/execute/mod.rs | 10 +++++++--- src/plot/aesthetic.rs | 30 ++++++++++++++++++++++++++++++ src/plot/layer/mod.rs | 30 ++++++++++++++++++++++-------- src/plot/main.rs | 6 +++--- src/validate.rs | 2 +- src/writer/vegalite/mod.rs | 8 +++++--- 6 files changed, 68 insertions(+), 18 deletions(-) diff --git a/src/execute/mod.rs b/src/execute/mod.rs index dc4c8d0d..6360d703 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -48,14 +48,18 @@ use crate::reader::DuckDBReader; /// - Partition_by columns exist in schema /// - Remapping target aesthetics are supported by geom /// - Remapping source columns are valid stat columns for geom -fn validate(layers: &[Layer], layer_schemas: &[Schema]) -> Result<()> { +fn validate( + layers: &[Layer], + layer_schemas: &[Schema], + aesthetic_context: &Option, +) -> Result<()> { for (idx, (layer, schema)) in layers.iter().zip(layer_schemas.iter()).enumerate() { let schema_columns: HashSet<&str> = schema.iter().map(|c| c.name.as_str()).collect(); let supported = layer.geom.aesthetics().supported(); // Validate required aesthetics for this geom layer - .validate_mapping() + .validate_mapping(aesthetic_context) .map_err(|e| GgsqlError::ValidationError(format!("Layer {}: {}", idx + 1, e)))?; // Validate SETTING parameters are valid for this geom @@ -1018,7 +1022,7 @@ pub fn prepare_data_with_reader(query: &str, reader: &R) -> Result String { + // Check internal facet (facet1, facet2) + if let Some(idx) = self + .internal_facet + .iter() + .position(|i| i == internal_aesthetic) + { + return self.user_facet[idx].to_string(); + } + + // Check internal positional (pos1, pos1min, pos2, etc.) + // Iterate through user_to_internal to find reverse mapping + for (user, internal) in &self.user_to_internal { + if internal == internal_aesthetic { + return user.to_string(); + } + } + + // Non-positional aesthetics (color, size, etc.) + // Internal is the same as external + internal_aesthetic.to_string() + } + // === Checking (O(1) HashMap lookups) === /// Check if internal aesthetic is primary positional (pos1, pos2, ...) diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index 53dbfd9e..45ea12e8 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -20,9 +20,12 @@ pub use geom::{ // Re-export position types for convenience pub use position::{Position, PositionTrait, PositionType}; -use crate::plot::{ - is_facet_aesthetic, - types::{AestheticValue, DataSource, Mappings, ParameterValue, SqlExpression}, +use crate::{ + plot::{ + is_facet_aesthetic, + types::{AestheticValue, DataSource, Mappings, ParameterValue, SqlExpression}, + }, + AestheticContext, }; /// A single visualization layer (from DRAW clause) @@ -161,12 +164,19 @@ impl Layer { } /// Check if this layer has the required aesthetics, and no exotic aesthetics. - pub fn validate_mapping(&self) -> std::result::Result<(), String> { + pub fn validate_mapping( + &self, + context: &Option, + ) -> std::result::Result<(), String> { // Check if all required aesthetics exist. - let mut missing: Vec<&str> = Vec::new(); + let mut missing = Vec::new(); for aesthetic in self.geom.aesthetics().required() { if !self.mappings.contains_key(aesthetic) { - missing.push(aesthetic); + let name = match context { + Some(ctx) => ctx.map_internal_to_user(aesthetic), + _ => aesthetic.to_string(), + }; + missing.push(name); } } if !missing.is_empty() { @@ -177,14 +187,18 @@ impl Layer { )); } // Check if any unsupported mappings are present - let mut extra: Vec<&str> = Vec::new(); + let mut extra = Vec::new(); let supported = self.geom.aesthetics().supported(); for aesthetic in self.mappings.aesthetics.keys() { if is_facet_aesthetic(aesthetic) { continue; } if !supported.contains(&aesthetic.as_str()) { - extra.push(aesthetic); + let name = match context { + Some(ctx) => ctx.map_internal_to_user(aesthetic), + _ => aesthetic.to_string(), + }; + extra.push(name); } } if !extra.is_empty() { diff --git a/src/plot/main.rs b/src/plot/main.rs index 7c228d7c..2a824301 100644 --- a/src/plot/main.rs +++ b/src/plot/main.rs @@ -363,12 +363,12 @@ mod tests { .with_aesthetic("pos1".to_string(), AestheticValue::standard_column("x")) .with_aesthetic("pos2".to_string(), AestheticValue::standard_column("y")); - assert!(valid_point.validate_mapping().is_ok()); + assert!(valid_point.validate_mapping(&None).is_ok()); let invalid_point = Layer::new(Geom::point()) .with_aesthetic("pos1".to_string(), AestheticValue::standard_column("x")); - assert!(invalid_point.validate_mapping().is_err()); + assert!(invalid_point.validate_mapping(&None).is_err()); let valid_ribbon = Layer::new(Geom::ribbon()) .with_aesthetic("pos1".to_string(), AestheticValue::standard_column("x")) @@ -381,7 +381,7 @@ mod tests { AestheticValue::standard_column("ymax"), ); - assert!(valid_ribbon.validate_mapping().is_ok()); + assert!(valid_ribbon.validate_mapping(&None).is_ok()); } #[test] diff --git a/src/validate.rs b/src/validate.rs index aa5029d2..10284632 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -178,7 +178,7 @@ pub fn validate(query: &str) -> Result { // Note: Without schema data, we can only check if mappings exist, // not if the columns are valid. We skip this check for wildcards. if !layer.mappings.wildcard { - if let Err(e) = layer.validate_mapping() { + if let Err(e) = layer.validate_mapping(&plot.aesthetic_context) { errors.push(ValidationError { message: format!("{}: {}", context, e), location: None, diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index 321075a2..f9dd4449 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -1140,9 +1140,11 @@ impl Writer for VegaLiteWriter { // Validate each layer for layer in &spec.layers { // Check aesthetics - layer.validate_mapping().map_err(|e| { - GgsqlError::ValidationError(format!("Layer validation failed: {}", e)) - })?; + layer + .validate_mapping(&spec.aesthetic_context) + .map_err(|e| { + GgsqlError::ValidationError(format!("Layer validation failed: {}", e)) + })?; // Check SETTING parameters are valid for this geom layer.validate_settings().map_err(|e| { From 8fbada72d088a0952157a84c1005af3dda7f6452 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 16 Mar 2026 12:31:26 +0100 Subject: [PATCH 04/12] add tests for user-facing aesthetic name translation Adds comprehensive tests for map_internal_to_user() method and validation error messages to ensure internal aesthetic names (pos1, pos2, facet1) are correctly translated to user-facing names (x, y, panel) in error messages. Tests cover: - Cartesian coordinates (x, y and variants) - Polar coordinates (theta, radius) - Facet aesthetics (panel, row, column) - Roundtrip conversion (user -> internal -> user) - Validation error messages for missing and unsupported aesthetics Co-Authored-By: Claude Sonnet 4.5 --- src/execute/mod.rs | 84 +++++++++++++++++++++++++++++++++++++++++++ src/plot/aesthetic.rs | 65 +++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+) diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 6360d703..f2753675 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -2297,4 +2297,88 @@ mod tests { "line layer should not have fill due to null AS fill" ); } + + // ======================================================================== + // Validation Error Message Tests (User-facing aesthetic names) + // ======================================================================== + + #[cfg(feature = "duckdb")] + #[test] + fn test_validation_error_shows_user_facing_names_for_missing_aesthetics() { + // Test that validation errors show user-facing names (x, y) instead of internal (pos1, pos2) + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + + reader + .connection() + .execute( + "CREATE TABLE test_data AS SELECT * FROM (VALUES (1, 2)) AS t(a, b)", + duckdb::params![], + ) + .unwrap(); + + // Query missing required aesthetic 'y' - should show 'y' not 'pos2' + let query = r#" + SELECT * FROM test_data + VISUALISE + DRAW point MAPPING a AS x + "#; + + let result = prepare_data_with_reader(query, &reader); + assert!(result.is_err(), "Expected validation error"); + + let err_msg = match result { + Err(e) => e.to_string(), + Ok(_) => panic!("Expected error"), + }; + assert!( + err_msg.contains("'y'"), + "Error should mention user-facing name 'y', got: {}", + err_msg + ); + assert!( + !err_msg.contains("pos2"), + "Error should not mention internal name 'pos2', got: {}", + err_msg + ); + } + + #[cfg(feature = "duckdb")] + #[test] + fn test_validation_error_shows_user_facing_names_for_unsupported_aesthetics() { + // Test that validation errors show user-facing names for unsupported aesthetics + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + + reader + .connection() + .execute( + "CREATE TABLE test_data AS SELECT * FROM (VALUES (1, 2, 3)) AS t(a, b, c)", + duckdb::params![], + ) + .unwrap(); + + // Query with unsupported aesthetic 'xmin' for point - should show 'xmin' not 'pos1min' + let query = r#" + SELECT * FROM test_data + VISUALISE + DRAW point MAPPING a AS x, b AS y, c AS xmin + "#; + + let result = prepare_data_with_reader(query, &reader); + assert!(result.is_err(), "Expected validation error"); + + let err_msg = match result { + Err(e) => e.to_string(), + Ok(_) => panic!("Expected error"), + }; + assert!( + err_msg.contains("'xmin'"), + "Error should mention user-facing name 'xmin', got: {}", + err_msg + ); + assert!( + !err_msg.contains("pos1min"), + "Error should not mention internal name 'pos1min', got: {}", + err_msg + ); + } } diff --git a/src/plot/aesthetic.rs b/src/plot/aesthetic.rs index 8937d5e1..98c16010 100644 --- a/src/plot/aesthetic.rs +++ b/src/plot/aesthetic.rs @@ -603,4 +603,69 @@ mod tests { assert_eq!(ctx.primary_internal_positional("pos2end"), Some("pos2")); assert_eq!(ctx.primary_internal_positional("color"), Some("color")); } + + #[test] + fn test_aesthetic_context_internal_to_user_cartesian() { + let ctx = AestheticContext::from_static(&["x", "y"], &[]); + + // Primary aesthetics + assert_eq!(ctx.map_internal_to_user("pos1"), "x"); + assert_eq!(ctx.map_internal_to_user("pos2"), "y"); + + // Variants + assert_eq!(ctx.map_internal_to_user("pos1min"), "xmin"); + assert_eq!(ctx.map_internal_to_user("pos1max"), "xmax"); + assert_eq!(ctx.map_internal_to_user("pos1end"), "xend"); + assert_eq!(ctx.map_internal_to_user("pos2min"), "ymin"); + assert_eq!(ctx.map_internal_to_user("pos2max"), "ymax"); + assert_eq!(ctx.map_internal_to_user("pos2end"), "yend"); + + // Non-positional aesthetics remain unchanged + assert_eq!(ctx.map_internal_to_user("color"), "color"); + assert_eq!(ctx.map_internal_to_user("size"), "size"); + assert_eq!(ctx.map_internal_to_user("fill"), "fill"); + } + + #[test] + fn test_aesthetic_context_internal_to_user_polar() { + let ctx = AestheticContext::from_static(&["theta", "radius"], &[]); + + // Primary aesthetics map to polar names + assert_eq!(ctx.map_internal_to_user("pos1"), "theta"); + assert_eq!(ctx.map_internal_to_user("pos2"), "radius"); + + // Variants + assert_eq!(ctx.map_internal_to_user("pos1end"), "thetaend"); + assert_eq!(ctx.map_internal_to_user("pos2min"), "radiusmin"); + assert_eq!(ctx.map_internal_to_user("pos2max"), "radiusmax"); + } + + #[test] + fn test_aesthetic_context_internal_to_user_facets() { + // Wrap facet (panel) + let ctx_wrap = AestheticContext::from_static(&["x", "y"], &["panel"]); + assert_eq!(ctx_wrap.map_internal_to_user("facet1"), "panel"); + + // Grid facet (row, column) + let ctx_grid = AestheticContext::from_static(&["x", "y"], &["row", "column"]); + assert_eq!(ctx_grid.map_internal_to_user("facet1"), "row"); + assert_eq!(ctx_grid.map_internal_to_user("facet2"), "column"); + } + + #[test] + fn test_aesthetic_context_roundtrip() { + // Test that user -> internal -> user roundtrips correctly + let ctx = AestheticContext::from_static(&["x", "y"], &["panel"]); + + // Positional + let internal = ctx.map_user_to_internal("x").unwrap(); + assert_eq!(ctx.map_internal_to_user(internal), "x"); + + let internal = ctx.map_user_to_internal("ymin").unwrap(); + assert_eq!(ctx.map_internal_to_user(internal), "ymin"); + + // Facet + let internal = ctx.map_user_to_internal("panel").unwrap(); + assert_eq!(ctx.map_internal_to_user(internal), "panel"); + } } From 12f8e4bacefa8891f4e45bd2228ce4a0370aba43 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 16 Mar 2026 13:30:56 +0100 Subject: [PATCH 05/12] bit of polish --- src/execute/mod.rs | 4 ++-- src/plot/layer/mod.rs | 31 +++++++++++++++++-------------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/execute/mod.rs b/src/execute/mod.rs index f2753675..d57d0e76 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -2331,7 +2331,7 @@ mod tests { Ok(_) => panic!("Expected error"), }; assert!( - err_msg.contains("'y'"), + err_msg.contains("`y`"), "Error should mention user-facing name 'y', got: {}", err_msg ); @@ -2371,7 +2371,7 @@ mod tests { Ok(_) => panic!("Expected error"), }; assert!( - err_msg.contains("'xmin'"), + err_msg.contains("`xmin`"), "Error should mention user-facing name 'xmin', got: {}", err_msg ); diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index 45ea12e8..3b3be5c1 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -168,22 +168,28 @@ impl Layer { &self, context: &Option, ) -> std::result::Result<(), String> { + // If there is aesthetic context, translate to user-facing form + let translate = |aes: &str| -> String { + let name = match context { + Some(ctx) => ctx.map_internal_to_user(aes), + None => aes.to_string(), + }; + format!("`{}`", name) + }; + // Check if all required aesthetics exist. let mut missing = Vec::new(); for aesthetic in self.geom.aesthetics().required() { if !self.mappings.contains_key(aesthetic) { - let name = match context { - Some(ctx) => ctx.map_internal_to_user(aesthetic), - _ => aesthetic.to_string(), - }; - missing.push(name); + missing.push(translate(aesthetic)); } } if !missing.is_empty() { return Err(format!( - "Layer '{}' mapping requires the '{}' aesthetic(s).", + "Layer '{}' mapping requires the {} aesthetic{s}.", self.geom, - missing.join(", ") + missing.join(", "), + s = if missing.len() > 1 { "s" } else { "" } )); } // Check if any unsupported mappings are present @@ -194,18 +200,15 @@ impl Layer { continue; } if !supported.contains(&aesthetic.as_str()) { - let name = match context { - Some(ctx) => ctx.map_internal_to_user(aesthetic), - _ => aesthetic.to_string(), - }; - extra.push(name); + extra.push(translate(aesthetic)); } } if !extra.is_empty() { return Err(format!( - "Layer '{}' does not support the '{}' mapping(s).", + "Layer '{}' does not support the {} mapping{s}.", self.geom, - extra.join(", ") + extra.join(", "), + s = if extra.len() > 1 { "s" } else { "" } )); } Ok(()) From 4eab5e9f7cb889538601d0640901a94214109256 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 18 Mar 2026 15:52:15 +0100 Subject: [PATCH 06/12] skip supported check in writer --- src/execute/mod.rs | 8 ++++++-- src/plot/layer/mod.rs | 4 ++++ src/plot/main.rs | 6 +++--- src/validate.rs | 2 +- src/writer/vegalite/mod.rs | 4 +++- 5 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 33971751..5753f2a5 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -60,7 +60,7 @@ fn validate( // Validate required aesthetics for this geom layer - .validate_mapping(aesthetic_context) + .validate_mapping(aesthetic_context, false) .map_err(|e| GgsqlError::ValidationError(format!("Layer {}: {}", idx + 1, e)))?; // Validate SETTING parameters are valid for this geom @@ -1033,7 +1033,11 @@ pub fn prepare_data_with_reader(query: &str, reader: &R) -> Result, + skip_supported: bool, ) -> std::result::Result<(), String> { // If there is aesthetic context, translate to user-facing form let translate = |aes: &str| -> String { @@ -228,6 +229,9 @@ impl Layer { )); } } + if skip_supported { + return Ok(()); + } // Check if any unsupported mappings are present let mut extra = Vec::new(); diff --git a/src/plot/main.rs b/src/plot/main.rs index 6f2c5003..523f9c96 100644 --- a/src/plot/main.rs +++ b/src/plot/main.rs @@ -379,12 +379,12 @@ mod tests { .with_aesthetic("pos1".to_string(), AestheticValue::standard_column("x")) .with_aesthetic("pos2".to_string(), AestheticValue::standard_column("y")); - assert!(valid_point.validate_mapping(&None).is_ok()); + assert!(valid_point.validate_mapping(&None, false).is_ok()); let invalid_point = Layer::new(Geom::point()) .with_aesthetic("pos1".to_string(), AestheticValue::standard_column("x")); - assert!(invalid_point.validate_mapping(&None).is_err()); + assert!(invalid_point.validate_mapping(&None, false).is_err()); let valid_ribbon = Layer::new(Geom::ribbon()) .with_aesthetic("pos1".to_string(), AestheticValue::standard_column("x")) @@ -397,7 +397,7 @@ mod tests { AestheticValue::standard_column("ymax"), ); - assert!(valid_ribbon.validate_mapping(&None).is_ok()); + assert!(valid_ribbon.validate_mapping(&None, false).is_ok()); } #[test] diff --git a/src/validate.rs b/src/validate.rs index 10284632..79c6c36e 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -178,7 +178,7 @@ pub fn validate(query: &str) -> Result { // Note: Without schema data, we can only check if mappings exist, // not if the columns are valid. We skip this check for wildcards. if !layer.mappings.wildcard { - if let Err(e) = layer.validate_mapping(&plot.aesthetic_context) { + if let Err(e) = layer.validate_mapping(&plot.aesthetic_context, false) { errors.push(ValidationError { message: format!("{}: {}", context, e), location: None, diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index b9bf9fed..94d105d6 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -1130,8 +1130,10 @@ impl Writer for VegaLiteWriter { // Validate each layer for layer in &spec.layers { // Check aesthetics + // We already checked the supported aesthetics during execution. + // They'd fail now because delayed have to layer - .validate_mapping(&spec.aesthetic_context) + .validate_mapping(&spec.aesthetic_context, true) .map_err(|e| { GgsqlError::ValidationError(format!("Layer validation failed: {}", e)) })?; From 4616b4bcbd94e948cc769b194a07b4c01301a648 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 18 Mar 2026 17:35:37 +0100 Subject: [PATCH 07/12] ensure layers list all aesthetics --- src/plot/layer/geom/area.rs | 1 + src/plot/layer/geom/bar.rs | 1 + src/plot/layer/geom/density.rs | 1 + src/plot/layer/geom/histogram.rs | 1 + src/plot/layer/geom/mod.rs | 73 ++++++++++++++++++++++++++++++++ 5 files changed, 77 insertions(+) diff --git a/src/plot/layer/geom/area.rs b/src/plot/layer/geom/area.rs index b8bc38af..ee660892 100644 --- a/src/plot/layer/geom/area.rs +++ b/src/plot/layer/geom/area.rs @@ -25,6 +25,7 @@ impl GeomTrait for Area { ("opacity", DefaultAestheticValue::Number(0.8)), ("linewidth", DefaultAestheticValue::Number(1.0)), ("linetype", DefaultAestheticValue::String("solid")), + ("pos2end", DefaultAestheticValue::Delayed), ], } } diff --git a/src/plot/layer/geom/bar.rs b/src/plot/layer/geom/bar.rs index 9922d4e6..5f246687 100644 --- a/src/plot/layer/geom/bar.rs +++ b/src/plot/layer/geom/bar.rs @@ -33,6 +33,7 @@ impl GeomTrait for Bar { defaults: &[ ("pos1", DefaultAestheticValue::Null), // Optional - stat may provide ("pos2", DefaultAestheticValue::Null), // Optional - stat may compute + ("pos2end", DefaultAestheticValue::Delayed), ("weight", DefaultAestheticValue::Null), ("fill", DefaultAestheticValue::String("black")), ("stroke", DefaultAestheticValue::String("black")), diff --git a/src/plot/layer/geom/density.rs b/src/plot/layer/geom/density.rs index c83d2ee4..b236652b 100644 --- a/src/plot/layer/geom/density.rs +++ b/src/plot/layer/geom/density.rs @@ -36,6 +36,7 @@ impl GeomTrait for Density { ("linewidth", DefaultAestheticValue::Number(1.0)), ("linetype", DefaultAestheticValue::String("solid")), ("pos2", DefaultAestheticValue::Delayed), // Computed by stat + ("pos2end", DefaultAestheticValue::Delayed), ], } } diff --git a/src/plot/layer/geom/histogram.rs b/src/plot/layer/geom/histogram.rs index b372d79e..b979b68c 100644 --- a/src/plot/layer/geom/histogram.rs +++ b/src/plot/layer/geom/histogram.rs @@ -31,6 +31,7 @@ impl GeomTrait for Histogram { // pos2 and pos1end are produced by stat_histogram but not valid for manual MAPPING ("pos2", DefaultAestheticValue::Delayed), ("pos1end", DefaultAestheticValue::Delayed), + ("pos2end", DefaultAestheticValue::Delayed), // baseline value ], } } diff --git a/src/plot/layer/geom/mod.rs b/src/plot/layer/geom/mod.rs index b009e521..ec37038d 100644 --- a/src/plot/layer/geom/mod.rs +++ b/src/plot/layer/geom/mod.rs @@ -527,4 +527,77 @@ mod tests { let deserialized: Geom = serde_json::from_str(&json).unwrap(); assert_eq!(deserialized.geom_type(), GeomType::Point); } + + #[test] + fn test_default_remappings_are_in_aesthetics() { + // Test that every aesthetic in default_remappings() exists in aesthetics().defaults + // This ensures that remapped aesthetics are properly declared (usually as Delayed) + + let all_geom_types = [ + GeomType::Point, + GeomType::Line, + GeomType::Path, + GeomType::Bar, + GeomType::Area, + GeomType::Rect, + GeomType::Polygon, + GeomType::Ribbon, + GeomType::Histogram, + GeomType::Density, + GeomType::Smooth, + GeomType::Boxplot, + GeomType::Violin, + GeomType::Text, + GeomType::Segment, + GeomType::Arrow, + GeomType::Rule, + GeomType::Linear, + GeomType::ErrorBar, + ]; + + // This test is rigged to trigger a compiler error when new variants are added. + // Add the new layer to both the array above and as match arm below. + let _exhaustive_check = |t: GeomType| match t { + GeomType::Point + | GeomType::Line + | GeomType::Path + | GeomType::Bar + | GeomType::Area + | GeomType::Rect + | GeomType::Polygon + | GeomType::Ribbon + | GeomType::Histogram + | GeomType::Density + | GeomType::Smooth + | GeomType::Boxplot + | GeomType::Violin + | GeomType::Text + | GeomType::Segment + | GeomType::Arrow + | GeomType::Rule + | GeomType::Linear + | GeomType::ErrorBar => {} + }; + + for geom_type in all_geom_types { + let geom = Geom::from_type(geom_type); + let remappings = geom.default_remappings(); + let aesthetics = geom.aesthetics(); + + // Collect all aesthetic names from aesthetics().defaults + let aesthetic_names: std::collections::HashSet<&str> = + aesthetics.defaults.iter().map(|(name, _)| *name).collect(); + + // Check each remapping name exists in aesthetics + for (name, _) in remappings { + assert!( + aesthetic_names.contains(name), + "Geom '{}' has '{}' in default_remappings() but not in aesthetics().defaults. \ + Add it as DefaultAestheticValue::Delayed if it's a stat-produced aesthetic.", + geom_type, + name + ); + } + } + } } From af1cacee28444637f06b7508e139f3092d46cabf Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 18 Mar 2026 17:54:55 +0100 Subject: [PATCH 08/12] rewire so that writer includes delayed aesthetics as possibilities --- src/plot/layer/mod.rs | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index 83e7b71f..6c92911b 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -4,7 +4,7 @@ //! a single visualization layer (from DRAW clause) in a ggsql specification. use serde::{Deserialize, Serialize}; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; // Geom is a submodule of layer pub mod geom; @@ -173,7 +173,7 @@ impl Layer { pub fn validate_mapping( &self, context: &Option, - skip_supported: bool, + include_delayed: bool, ) -> std::result::Result<(), String> { // If there is aesthetic context, translate to user-facing form let translate = |aes: &str| -> String { @@ -229,18 +229,34 @@ impl Layer { )); } } - if skip_supported { - return Ok(()); + + let mut supported: HashSet = if include_delayed { + self.geom.aesthetics().names() + } else { + self.geom.aesthetics().supported() + } + .into_iter() + .map(|s| s.to_string()) + .collect(); + + // At this point in execution we don't know orientation yet, + // so we'll approve both flipped and upflipped aesthetics. + if let Some(ctx) = context { + let flipped: Vec = supported + .iter() + .map(|aes| ctx.flip_positional(aes)) + .collect(); + supported.extend(flipped); } // Check if any unsupported mappings are present let mut extra = Vec::new(); - let supported = self.geom.aesthetics().supported(); + for aesthetic in self.mappings.aesthetics.keys() { if is_facet_aesthetic(aesthetic) { continue; } - if !supported.contains(&aesthetic.as_str()) { + if !supported.contains(aesthetic.as_str()) { extra.push(translate(aesthetic)); } } From 5c23591bd01680de8d0f2a57a1887298c63797ec Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 18 Mar 2026 17:55:12 +0100 Subject: [PATCH 09/12] adapt recalcitrant tests --- src/execute/mod.rs | 4 ++-- src/writer/vegalite/mod.rs | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 5753f2a5..fc10d651 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -2511,8 +2511,8 @@ mod tests { match result { Err(GgsqlError::ValidationError(msg)) => { assert!( - msg.contains("pos2"), - "Error should mention missing pos2 aesthetic: {}", + msg.contains("y"), + "Error should mention missing y aesthetic: {}", msg ); } diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index 94d105d6..dcff0244 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -1602,7 +1602,7 @@ mod tests { AestheticValue::standard_column("y".to_string()), ) .with_aesthetic( - "color".to_string(), + "stroke".to_string(), AestheticValue::Literal(ParameterValue::String("red".to_string())), ); spec.layers.push(layer); @@ -1618,7 +1618,7 @@ mod tests { assert_valid_vegalite(&json_str); let vl_spec: Value = serde_json::from_str(&json_str).unwrap(); - assert_eq!(vl_spec["layer"][0]["encoding"]["color"]["value"], "red"); + assert_eq!(vl_spec["layer"][0]["encoding"]["stroke"]["value"], "red"); } #[test] @@ -1820,13 +1820,13 @@ mod tests { AestheticValue::standard_column("y".to_string()), ) .with_aesthetic( - "color".to_string(), + "size".to_string(), AestheticValue::standard_column("value".to_string()), ); spec.layers.push(layer); - // Add binned color scale (symbol legend case) - let mut scale = Scale::new("color"); + // Add binned size scale (symbol legend case) + let mut scale = Scale::new("size"); scale.scale_type = Some(ScaleType::binned()); scale.properties.insert( "breaks".to_string(), @@ -1860,7 +1860,7 @@ mod tests { let vl_spec: Value = serde_json::from_str(&json_str).unwrap(); // Check that labelExpr contains VL's range-style format - let label_expr = &vl_spec["layer"][0]["encoding"]["color"]["legend"]["labelExpr"]; + let label_expr = &vl_spec["layer"][0]["encoding"]["size"]["legend"]["labelExpr"]; assert!(label_expr.is_string()); let expr = label_expr.as_str().unwrap(); From 1c58fbf36aef6a036d692245447d2f241571fcef Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 18 Mar 2026 18:13:33 +0100 Subject: [PATCH 10/12] some polish --- src/plot/layer/mod.rs | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index 6c92911b..5288424f 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -169,7 +169,20 @@ impl Layer { } } - /// Check if this layer has the required aesthetics, and no exotic aesthetics. + /// Validate layer aesthetic mappings. + /// + /// Performs three checks: + /// 1. All required aesthetics are present + /// 2. Positional requirements allow bidirectional satisfaction (handles orientation flipping) + /// 3. No unsupported/exotic aesthetics are mapped + /// + /// # Parameters + /// - `context`: Optional aesthetic context for translating internal → user-facing names + /// - `include_delayed`: If true, allows delayed aesthetics (stat-produced). Use `true` for + /// writer validation, `false` for execution validation. + /// + /// # Returns + /// `Ok(())` if validation passes, or `Err(message)` with a user-friendly error message. pub fn validate_mapping( &self, context: &Option, @@ -219,13 +232,17 @@ impl Layer { }); if !identity_ok && !swapped_ok { - let (missing, _, _) = positional_reqs + let (missing, slot, suffix) = positional_reqs .iter() .find(|(name, _, _)| !self.mappings.contains_key(name)) .unwrap(); + let other_slot = if *slot == 1 { 2 } else { 1 }; + let flipped = format!("pos{}{}", other_slot, suffix); return Err(format!( - "Geom '{}' requires aesthetic '{}' (or its bidirectional equivalent) but it was not provided", - self.geom, translate(missing) + "Layer '{}' mapping requires the aesthetic '{}' (or '{}').", + self.geom, + translate(missing), + translate(&flipped) )); } } @@ -256,7 +273,7 @@ impl Layer { if is_facet_aesthetic(aesthetic) { continue; } - if !supported.contains(aesthetic.as_str()) { + if !supported.contains(aesthetic) { extra.push(translate(aesthetic)); } } From 0654725040e278dde90f6f4c2dd7a5f570b0cce2 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 18 Mar 2026 19:07:01 +0100 Subject: [PATCH 11/12] be more specific about mixed orientation --- src/plot/layer/mod.rs | 157 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 140 insertions(+), 17 deletions(-) diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index 5288424f..7477d6c6 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -221,28 +221,45 @@ impl Layer { // Validate positional requirements bidirectionally // Try both slot assignments: (1→1, 2→2) and (1→2, 2→1) if !positional_reqs.is_empty() { - let identity_ok = positional_reqs + // Pre-compute flipped versions to avoid repeated calculation + let pairs: Vec<_> = positional_reqs .iter() - .all(|(name, _, _)| self.mappings.contains_key(name)); - - let swapped_ok = positional_reqs.iter().all(|(_, slot, suffix)| { - let new_slot = if *slot == 1 { 2 } else { 1 }; - let remapped = format!("pos{}{}", new_slot, suffix); - self.mappings.contains_key(&remapped) - }); - - if !identity_ok && !swapped_ok { - let (missing, slot, suffix) = positional_reqs - .iter() - .find(|(name, _, _)| !self.mappings.contains_key(name)) - .unwrap(); - let other_slot = if *slot == 1 { 2 } else { 1 }; - let flipped = format!("pos{}{}", other_slot, suffix); + .map(|(name, slot, suffix)| { + let flipped_slot = if *slot == 1 { 2 } else { 1 }; + let flipped = format!("pos{}{}", flipped_slot, suffix); + (*name, flipped) + }) + .collect(); + + // Find first missing aesthetic in each orientation + let identity_missing = pairs + .iter() + .find(|(name, _)| !self.mappings.contains_key(name)); + + let flipped_missing = pairs + .iter() + .find(|(_, flipped)| !self.mappings.contains_key(flipped)); + + if identity_missing.is_some() && flipped_missing.is_some() { + let (missing, flipped) = identity_missing.unwrap(); + + // Check if flipped version is present (mixed orientation case) + if self.mappings.contains_key(flipped) { + return Err(format!( + "Layer '{}' has mixed positional aesthetic orientations. \ + Found '{}' but expected '{}' to match the orientation of other aesthetics.", + self.geom, + translate(flipped), + translate(missing) + )); + } + + // Truly missing aesthetic return Err(format!( "Layer '{}' mapping requires the aesthetic '{}' (or '{}').", self.geom, translate(missing), - translate(&flipped) + translate(flipped) )); } } @@ -571,4 +588,110 @@ mod tests { ))) ); } + + #[test] + fn test_validate_mapping_bidirectional_missing() { + // Test error message when aesthetic is completely missing (neither identity nor flipped form) + use crate::AestheticContext; + + let mut layer = Layer::new(Geom::ribbon()); + layer.mappings.insert( + "pos1".to_string(), + AestheticValue::standard_column("x".to_string()), + ); + // Missing both pos2min and pos1min (required by ribbon) + + let ctx = AestheticContext::from_static(&["x", "y"], &[]); + let result = layer.validate_mapping(&Some(ctx), false); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!( + err.contains("ymin") && err.contains("xmin"), + "Expected error to mention both alternatives (ymin/xmin), got: {}", + err + ); + } + + #[test] + fn test_validate_mapping_bidirectional_mixed_orientation() { + // Test error message when aesthetics are present but in mixed orientations + use crate::AestheticContext; + + let mut layer = Layer::new(Geom::ribbon()); + layer.mappings.insert( + "pos1".to_string(), + AestheticValue::standard_column("x".to_string()), + ); + layer.mappings.insert( + "pos2min".to_string(), + AestheticValue::standard_column("ymin".to_string()), + ); + layer.mappings.insert( + "pos1max".to_string(), // This should be pos2max to match pos2min's orientation + AestheticValue::standard_column("xmax".to_string()), + ); + + let ctx = AestheticContext::from_static(&["x", "y"], &[]); + let result = layer.validate_mapping(&Some(ctx), false); + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!( + err.contains("mixed") && err.contains("orientation"), + "Expected error about mixed orientation, got: {}", + err + ); + assert!( + err.contains("xmax") && err.contains("ymax"), + "Expected error to mention the conflicting aesthetics (xmax/ymax), got: {}", + err + ); + } + + #[test] + fn test_validate_mapping_bidirectional_identity_ok() { + // Test that validation passes when all requirements are in identity form + use crate::AestheticContext; + + let mut layer = Layer::new(Geom::ribbon()); + layer.mappings.insert( + "pos1".to_string(), + AestheticValue::standard_column("x".to_string()), + ); + layer.mappings.insert( + "pos2min".to_string(), + AestheticValue::standard_column("ymin".to_string()), + ); + layer.mappings.insert( + "pos2max".to_string(), + AestheticValue::standard_column("ymax".to_string()), + ); + + let ctx = AestheticContext::from_static(&["x", "y"], &[]); + let result = layer.validate_mapping(&Some(ctx), false); + assert!(result.is_ok()); + } + + #[test] + fn test_validate_mapping_bidirectional_flipped_ok() { + // Test that validation passes when all requirements are in flipped form + use crate::AestheticContext; + + let mut layer = Layer::new(Geom::ribbon()); + layer.mappings.insert( + "pos2".to_string(), + AestheticValue::standard_column("y".to_string()), + ); + layer.mappings.insert( + "pos1min".to_string(), + AestheticValue::standard_column("xmin".to_string()), + ); + layer.mappings.insert( + "pos1max".to_string(), + AestheticValue::standard_column("xmax".to_string()), + ); + + let ctx = AestheticContext::from_static(&["x", "y"], &[]); + let result = layer.validate_mapping(&Some(ctx), false); + assert!(result.is_ok()); + } } From 28e7959d99635090a91f8f32ed2644c266abefba Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 19 Mar 2026 09:17:23 +0100 Subject: [PATCH 12/12] dance to clippy's whims --- src/plot/layer/mod.rs | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index 7477d6c6..d539e195 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -240,27 +240,26 @@ impl Layer { .iter() .find(|(_, flipped)| !self.mappings.contains_key(flipped)); - if identity_missing.is_some() && flipped_missing.is_some() { - let (missing, flipped) = identity_missing.unwrap(); - - // Check if flipped version is present (mixed orientation case) - if self.mappings.contains_key(flipped) { - return Err(format!( + if let Some((missing, flipped)) = identity_missing { + if flipped_missing.is_some() { + // Check if flipped version is present (mixed orientation case) + if self.mappings.contains_key(flipped) { + return Err(format!( "Layer '{}' has mixed positional aesthetic orientations. \ Found '{}' but expected '{}' to match the orientation of other aesthetics.", self.geom, translate(flipped), translate(missing) )); + } + // Truly missing aesthetic + return Err(format!( + "Layer '{}' mapping requires the aesthetic '{}' (or '{}').", + self.geom, + translate(missing), + translate(flipped) + )); } - - // Truly missing aesthetic - return Err(format!( - "Layer '{}' mapping requires the aesthetic '{}' (or '{}').", - self.geom, - translate(missing), - translate(flipped) - )); } }