diff --git a/CLAUDE.md b/CLAUDE.md index d408cc55..cf082c6a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -449,7 +449,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 c8c7475f..fc10d651 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -49,14 +49,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_required_aesthetics() + .validate_mapping(aesthetic_context, false) .map_err(|e| GgsqlError::ValidationError(format!("Layer {}: {}", idx + 1, e)))?; // Validate SETTING parameters are valid for this geom @@ -1029,7 +1033,11 @@ pub fn prepare_data_with_reader(query: &str, reader: &R) -> Result { assert!( - msg.contains("pos2"), - "Error should mention missing pos2 aesthetic: {}", + msg.contains("y"), + "Error should mention missing y aesthetic: {}", msg ); } @@ -2645,4 +2653,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 c0837a73..161d9cec 100644 --- a/src/plot/aesthetic.rs +++ b/src/plot/aesthetic.rs @@ -240,6 +240,36 @@ impl AestheticContext { } } + /// Map internal aesthetic to user-facing name (reverse of map_user_to_internal). + /// + /// Positional: "pos1" → "x", "pos2min" → "ymin", "pos1" → "theta" (for polar) + /// Facet: "facet1" → "panel" (wrap), "facet1" → "row" (grid), "facet2" → "column" (grid) + /// Non-positional: "color" → "color" (unchanged) + /// + /// Returns None if the internal aesthetic is not recognized. + pub fn map_internal_to_user(&self, internal_aesthetic: &str) -> 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, ...) @@ -633,6 +663,70 @@ mod tests { 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"); + } #[test] fn test_parse_positional() { // Primary positional 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 + ); + } + } + } } diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index d3fe70d5..d539e195 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; @@ -26,7 +26,13 @@ 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, parse_positional, + types::{AestheticValue, DataSource, Mappings, ParameterValue, SqlExpression}, + }, + AestheticContext, +}; /// A single visualization layer (from DRAW clause) #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -163,65 +169,138 @@ impl Layer { } } - /// Check if this layer has the required aesthetics for its geom + /// Validate layer aesthetic mappings. /// - /// Positional aesthetics (pos1*, pos2*) are validated bidirectionally: - /// requirements using pos1/pos2 slots can be satisfied by either axis assignment. - /// For example, requiring `pos1`, `pos2min`, `pos2max` accepts either: - /// - `x`, `ymin`, `ymax` (slot1→x-axis, slot2→y-axis) - /// - `y`, `xmin`, `xmax` (slot1→y-axis, slot2→x-axis) - pub fn validate_required_aesthetics(&self) -> std::result::Result<(), String> { - use crate::plot::aesthetic::parse_positional; - - let required = self.geom.aesthetics().required(); + /// 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, + include_delayed: bool, + ) -> 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) + }; - // Separate positional (with parsed slot/suffix) and non-positional requirements - let mut positional_reqs: Vec<(&str, u8, &str)> = Vec::new(); // (name, slot, suffix) - let mut other_reqs: Vec<&str> = Vec::new(); + // Check if all required aesthetics exist. + let mut missing = Vec::new(); + let mut positional_reqs: Vec<(&str, u8, &str)> = Vec::new(); - for aesthetic in &required { + for aesthetic in self.geom.aesthetics().required() { if let Some((slot, suffix)) = parse_positional(aesthetic) { - positional_reqs.push((aesthetic, slot, suffix)); - } else { - other_reqs.push(aesthetic); + positional_reqs.push((aesthetic, slot, suffix)) + } else if !self.mappings.contains_key(aesthetic) { + missing.push(translate(aesthetic)); } } - // Validate non-positional requirements directly - for aesthetic in &other_reqs { - if !self.mappings.contains_key(aesthetic) { - return Err(format!( - "Geom '{}' requires aesthetic '{}' but it was not provided", - self.geom, aesthetic - )); - } + if !missing.is_empty() { + return Err(format!( + "Layer '{}' mapping requires the {} aesthetic{s}.", + self.geom, + missing.join(", "), + s = if missing.len() > 1 { "s" } else { "" } + )); } // 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, _, _) = positional_reqs - .iter() - .find(|(name, _, _)| !self.mappings.contains_key(name)) - .unwrap(); - return Err(format!( - "Geom '{}' requires aesthetic '{}' (or its bidirectional equivalent) but it was not provided", - self.geom, missing - )); + .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 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) + )); + } } } + 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(); + + for aesthetic in self.mappings.aesthetics.keys() { + if is_facet_aesthetic(aesthetic) { + continue; + } + if !supported.contains(aesthetic) { + extra.push(translate(aesthetic)); + } + } + if !extra.is_empty() { + return Err(format!( + "Layer '{}' does not support the {} mapping{s}.", + self.geom, + extra.join(", "), + s = if extra.len() > 1 { "s" } else { "" } + )); + } Ok(()) } @@ -508,4 +587,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()); + } } diff --git a/src/plot/main.rs b/src/plot/main.rs index 031fe957..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_required_aesthetics().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_required_aesthetics().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_required_aesthetics().is_ok()); + assert!(valid_ribbon.validate_mapping(&None, false).is_ok()); } #[test] diff --git a/src/validate.rs b/src/validate.rs index b07e8585..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_required_aesthetics() { + 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 b4684602..dcff0244 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -1129,10 +1129,14 @@ impl Writer for VegaLiteWriter { // Validate each layer for layer in &spec.layers { - // Check required aesthetics - layer.validate_required_aesthetics().map_err(|e| { - GgsqlError::ValidationError(format!("Layer validation failed: {}", e)) - })?; + // Check aesthetics + // We already checked the supported aesthetics during execution. + // They'd fail now because delayed have to + layer + .validate_mapping(&spec.aesthetic_context, true) + .map_err(|e| { + GgsqlError::ValidationError(format!("Layer validation failed: {}", e)) + })?; // Note: validate_settings() is already called during execution, before // internal parameters like "orientation" are set, so we don't call it here. } @@ -1598,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); @@ -1614,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] @@ -1816,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(), @@ -1856,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();