diff --git a/src/execute/layer.rs b/src/execute/layer.rs index c7848044..4fe34322 100644 --- a/src/execute/layer.rs +++ b/src/execute/layer.rs @@ -3,7 +3,7 @@ //! This module handles building SQL queries for layers, applying pre-stat //! transformations, stat transforms, and post-query operations. -use crate::plot::aesthetic::{self, AestheticContext}; +use crate::plot::aesthetic; use crate::plot::layer::is_transposed; use crate::plot::layer::orientation::{flip_position_aesthetics, resolve_orientation}; use crate::plot::{ @@ -276,7 +276,6 @@ pub fn apply_pre_stat_transform( aesthetic_schema: &Schema, scales: &[Scale], dialect: &dyn SqlDialect, - aesthetic_ctx: &AestheticContext, ) -> String { let mut transform_exprs: Vec<(String, String)> = vec![]; let mut transformed_columns: HashSet = HashSet::new(); @@ -290,7 +289,6 @@ pub fn apply_pre_stat_transform( &layer.parameters, &layer.mappings, aesthetic_schema, - aesthetic_ctx, layer.geom.aggregate_domain_aesthetics().unwrap_or(&[]), ); @@ -476,7 +474,6 @@ pub fn apply_layer_transforms( scales: &[Scale], dialect: &dyn SqlDialect, execute_query: &F, - aesthetic_ctx: &AestheticContext, ) -> Result where F: Fn(&str) -> Result, @@ -523,7 +520,6 @@ where &aesthetic_schema, scales, dialect, - aesthetic_ctx, ); // Build group_by columns from partition_by @@ -553,7 +549,6 @@ where &layer.parameters, execute_query, dialect, - aesthetic_ctx, )?; // Flip user remappings BEFORE merging defaults for Transposed orientation. @@ -964,7 +959,6 @@ pub fn resolve_orientations( layers: &mut [Layer], scales: &[Scale], layer_type_info: &mut [Vec], - aesthetic_ctx: &AestheticContext, ) { for (layer_idx, layer) in layers.iter_mut().enumerate() { let orientation = resolve_orientation(layer, scales); @@ -979,7 +973,11 @@ pub fn resolve_orientations( if layer_idx < layer_type_info.len() { for (name, _, _) in &mut layer_type_info[layer_idx] { if let Some(aesthetic) = naming::extract_aesthetic_name(name) { - let flipped = aesthetic_ctx.flip_position(aesthetic); + let flipped = layer + .mappings + .context() + .map(|ctx| ctx.flip_position(aesthetic)) + .unwrap_or_else(|| aesthetic.to_string()); if flipped != aesthetic { *name = naming::aesthetic_column(&flipped); } diff --git a/src/execute/mod.rs b/src/execute/mod.rs index c26f7cd0..78c94091 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -24,7 +24,7 @@ pub use schema::TypeInfo; use crate::naming; use crate::parser; -use crate::plot::aesthetic::{is_position_aesthetic, AestheticContext}; +use crate::plot::aesthetic::is_position_aesthetic; use crate::plot::facet::{resolve_properties as resolve_facet_properties, FacetDataContext}; use crate::plot::layer::is_transposed; use crate::plot::projection::resolve_projection_properties; @@ -50,25 +50,14 @@ 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], - aesthetic_context: &Option, -) -> Result<()> { - let translate = |aes: &str| -> String { - match aesthetic_context { - Some(ctx) => ctx.map_internal_to_user(aes), - None => aes.to_string(), - } - }; - +fn validate(layers: &[Layer], layer_schemas: &[Schema]) -> 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(aesthetic_context, false) + .validate_mapping(false) .map_err(|e| GgsqlError::ValidationError(format!("Layer {}: {}", idx + 1, e)))?; // Validate SETTING parameters are valid for this geom @@ -92,7 +81,7 @@ fn validate( return Err(GgsqlError::ValidationError(format!( "Layer {}: aesthetic '{}' references non-existent column '{}'", idx + 1, - translate(aesthetic), + layer.mappings.display_name(aesthetic), col_name ))); } @@ -118,7 +107,7 @@ fn validate( return Err(GgsqlError::ValidationError(format!( "Layer {}: REMAPPING targets unsupported aesthetic '{}' for geom '{}'", idx + 1, - translate(target_aesthetic), + layer.mappings.display_name(target_aesthetic), layer.geom ))); } @@ -839,7 +828,6 @@ fn add_discrete_columns_to_partition_by( layers: &mut [Layer], layer_schemas: &[Schema], scales: &[Scale], - aesthetic_ctx: &AestheticContext, ) { // Build a map of aesthetic -> scale for quick lookup let scale_map: HashMap<&str, &Scale> = @@ -875,7 +863,6 @@ fn add_discrete_columns_to_partition_by( &layer.parameters, &layer.mappings, schema, - aesthetic_ctx, layer.geom.aggregate_domain_aesthetics().unwrap_or(&[]), ) .map(|(t, _)| t) @@ -906,8 +893,10 @@ fn add_discrete_columns_to_partition_by( // // Discrete and Binned scales produce categorical groupings. // Continuous scales don't group. Identity defers to column type. - let primary_aes = aesthetic_ctx - .primary_internal_position(aesthetic) + let primary_aes = layer + .mappings + .context() + .and_then(|ctx| ctx.primary_internal_position(aesthetic)) .unwrap_or(aesthetic); let is_discrete = if let Some(scale) = scale_map.get(primary_aes) { if let Some(ref scale_type) = scale.scale_type { @@ -1252,11 +1241,7 @@ pub fn prepare_data_with_reader(query: &str, reader: &dyn Reader) -> Result Result Result Result Result ColumnInfo { @@ -3410,6 +3383,7 @@ mod tests { fn point_layer_with_mapping(aesthetic: &str, column: &str) -> Layer { let mut layer = Layer::new(Geom::point()); + layer.mappings = Mappings::new().with_cartesian_context(); // Point geom requires both pos1 and pos2; map both, but only the // one we're testing points at a missing column. layer.mappings.aesthetics.insert( @@ -3430,6 +3404,7 @@ mod tests { #[test] fn aesthetic_column_missing_translates_pos1_to_x_under_cartesian() { let mut layer = Layer::new(Geom::point()); + layer.mappings = Mappings::new().with_cartesian_context(); layer.mappings.aesthetics.insert( "pos1".to_string(), AestheticValue::standard_column("missing"), @@ -3439,9 +3414,8 @@ mod tests { AestheticValue::standard_column("present_y"), ); let schema: Schema = vec![col("present_y")]; - let ctx = Some(AestheticContext::from_static(&["x", "y"], &[])); - let err = validate(&[layer], &[schema], &ctx).unwrap_err().to_string(); + let err = validate(&[layer], &[schema]).unwrap_err().to_string(); assert_eq!( err, "Validation error: Layer 1: aesthetic 'x' references non-existent column 'missing'" @@ -3451,6 +3425,9 @@ mod tests { #[test] fn aesthetic_column_missing_translates_pos1_to_angle_under_polar() { let mut layer = Layer::new(Geom::point()); + layer + .mappings + .set_context(AestheticContext::from_static(&["angle", "radius"], &[])); layer.mappings.aesthetics.insert( "pos1".to_string(), AestheticValue::standard_column("missing"), @@ -3460,9 +3437,8 @@ mod tests { AestheticValue::standard_column("present_radius"), ); let schema: Schema = vec![col("present_radius")]; - let ctx = Some(AestheticContext::from_static(&["angle", "radius"], &[])); - let err = validate(&[layer], &[schema], &ctx).unwrap_err().to_string(); + let err = validate(&[layer], &[schema]).unwrap_err().to_string(); assert_eq!( err, "Validation error: Layer 1: aesthetic 'angle' references non-existent column 'missing'" @@ -3473,10 +3449,9 @@ mod tests { fn aesthetic_column_missing_translates_pos2_to_y_under_cartesian() { let layer = point_layer_with_mapping("pos2", "missing"); let schema: Schema = vec![col("present_x"), col("present_y")]; - let ctx = Some(AestheticContext::from_static(&["x", "y"], &[])); // pos2 is overridden to point at "missing" by point_layer_with_mapping - let err = validate(&[layer], &[schema], &ctx).unwrap_err().to_string(); + let err = validate(&[layer], &[schema]).unwrap_err().to_string(); assert_eq!( err, "Validation error: Layer 1: aesthetic 'y' references non-existent column 'missing'" @@ -3488,9 +3463,8 @@ mod tests { // Material aesthetics (color, size, etc.) should round-trip unchanged. let layer = point_layer_with_mapping("color", "missing"); let schema: Schema = vec![col("present_x"), col("present_y")]; - let ctx = Some(AestheticContext::from_static(&["x", "y"], &[])); - let err = validate(&[layer], &[schema], &ctx).unwrap_err().to_string(); + let err = validate(&[layer], &[schema]).unwrap_err().to_string(); assert_eq!( err, "Validation error: Layer 1: aesthetic 'color' references non-existent column 'missing'" @@ -3513,6 +3487,7 @@ mod tests { // (only pos1 and the delayed pos2). The test here builds the // post-transform state directly. let mut layer = Layer::new(Geom::histogram()); + layer.mappings = Mappings::new().with_cartesian_context(); layer.mappings.aesthetics.insert( "pos1".to_string(), AestheticValue::standard_column("present_x"), @@ -3522,9 +3497,8 @@ mod tests { AestheticValue::standard_column("count"), ); let schema: Schema = vec![col("present_x")]; - let ctx = Some(AestheticContext::from_static(&["x", "y"], &[])); - let err = validate(&[layer], &[schema], &ctx).unwrap_err().to_string(); + let err = validate(&[layer], &[schema]).unwrap_err().to_string(); assert_eq!( err, "Validation error: Layer 1: REMAPPING targets unsupported aesthetic 'xmax' for geom 'histogram'" @@ -3534,6 +3508,9 @@ mod tests { #[test] fn remapping_unsupported_target_translates_pos1max_to_anglemax_under_polar() { let mut layer = Layer::new(Geom::histogram()); + layer + .mappings + .set_context(AestheticContext::from_static(&["angle", "radius"], &[])); layer.mappings.aesthetics.insert( "pos1".to_string(), AestheticValue::standard_column("present_x"), @@ -3543,9 +3520,8 @@ mod tests { AestheticValue::standard_column("count"), ); let schema: Schema = vec![col("present_x")]; - let ctx = Some(AestheticContext::from_static(&["angle", "radius"], &[])); - let err = validate(&[layer], &[schema], &ctx).unwrap_err().to_string(); + let err = validate(&[layer], &[schema]).unwrap_err().to_string(); assert_eq!( err, "Validation error: Layer 1: REMAPPING targets unsupported aesthetic 'anglemax' for geom 'histogram'" diff --git a/src/execute/scale.rs b/src/execute/scale.rs index 7d1b4614..52789426 100644 --- a/src/execute/scale.rs +++ b/src/execute/scale.rs @@ -165,7 +165,6 @@ pub fn apply_post_stat_binning( crate::plot::layer::geom::stat_aggregate::targeted_aesthetics( &layer.parameters, &layer.mappings, - &aesthetic_ctx, ) }) .collect(); @@ -535,7 +534,6 @@ pub fn apply_pre_stat_resolve(spec: &mut Plot, layer_schemas: &[Schema]) -> Resu for aes in crate::plot::layer::geom::stat_aggregate::targeted_aesthetics( &layer.parameters, &layer.mappings, - &aesthetic_ctx, ) { targeted_in_any_layer.insert(aes); } diff --git a/src/plot/layer/geom/area.rs b/src/plot/layer/geom/area.rs index 266add13..7700205e 100644 --- a/src/plot/layer/geom/area.rs +++ b/src/plot/layer/geom/area.rs @@ -69,7 +69,6 @@ impl GeomTrait for Area { parameters: &std::collections::HashMap, _execute_query: &dyn Fn(&str) -> crate::Result, dialect: &dyn crate::reader::SqlDialect, - aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> crate::Result { let result = if has_aggregate_param(parameters) { stat_aggregate::apply( @@ -79,7 +78,6 @@ impl GeomTrait for Area { group_by, parameters, dialect, - aesthetic_ctx, self.aggregate_domain_aesthetics().unwrap_or(&[]), )? } else { diff --git a/src/plot/layer/geom/bar.rs b/src/plot/layer/geom/bar.rs index b1441cbb..205f9b16 100644 --- a/src/plot/layer/geom/bar.rs +++ b/src/plot/layer/geom/bar.rs @@ -93,7 +93,6 @@ impl GeomTrait for Bar { parameters: &HashMap, _execute_query: &dyn Fn(&str) -> Result, dialect: &dyn SqlDialect, - aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { let inner = if has_aggregate_param(parameters) { stat_aggregate::apply( @@ -103,7 +102,6 @@ impl GeomTrait for Bar { group_by, parameters, dialect, - aesthetic_ctx, self.aggregate_domain_aesthetics().unwrap_or(&[]), )? } else { diff --git a/src/plot/layer/geom/boxplot.rs b/src/plot/layer/geom/boxplot.rs index e49599ed..ab8d48f9 100644 --- a/src/plot/layer/geom/boxplot.rs +++ b/src/plot/layer/geom/boxplot.rs @@ -99,7 +99,6 @@ impl GeomTrait for Boxplot { parameters: &HashMap, _execute_query: &dyn Fn(&str) -> Result, dialect: &dyn SqlDialect, - _aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { stat_boxplot(query, aesthetics, group_by, parameters, dialect) } @@ -119,7 +118,10 @@ fn stat_boxplot( dialect: &dyn SqlDialect, ) -> Result { let y = get_column_name(aesthetics, "pos2").ok_or_else(|| { - GgsqlError::ValidationError("Boxplot requires 'y' aesthetic mapping".to_string()) + GgsqlError::ValidationError(format!( + "Boxplot requires '{}' aesthetic mapping", + aesthetics.display_name("pos2") + )) })?; // pos1 is optional. When the user omits it, wrap the input query with a diff --git a/src/plot/layer/geom/density.rs b/src/plot/layer/geom/density.rs index 8032f5f4..04aa08a0 100644 --- a/src/plot/layer/geom/density.rs +++ b/src/plot/layer/geom/density.rs @@ -107,9 +107,7 @@ impl GeomTrait for Density { parameters: &std::collections::HashMap, _execute_query: &dyn Fn(&str) -> crate::Result, dialect: &dyn SqlDialect, - _aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> crate::Result { - // Density geom: no tails limit (don't set tails parameter, defaults to None) stat_density( query, aesthetics, "pos1", None, group_by, parameters, dialect, ) @@ -140,6 +138,7 @@ fn with_leading_comma(s: &str) -> String { } } +#[allow(clippy::too_many_arguments)] pub(crate) fn stat_density( query: &str, aesthetics: &Mappings, @@ -150,7 +149,10 @@ pub(crate) fn stat_density( dialect: &dyn SqlDialect, ) -> Result { let value = get_column_name(aesthetics, value_aesthetic).ok_or_else(|| { - GgsqlError::ValidationError("Density requires a position aesthetic mapping".to_string()) + GgsqlError::ValidationError(format!( + "Density requires '{}' aesthetic mapping", + aesthetics.display_name(value_aesthetic) + )) })?; let smooth = smooth_aesthetic.and_then(|smth| get_column_name(aesthetics, smth)); let weight = get_column_name(aesthetics, "weight"); @@ -1185,8 +1187,8 @@ mod tests { // ========================================================================= #[test] - fn stat_density_missing_value_aesthetic_emits_coord_agnostic_message() { - let mappings = crate::Mappings::new(); + fn stat_density_missing_value_aesthetic_emits_user_facing_name() { + let mappings = crate::Mappings::new().with_cartesian_context(); let parameters = std::collections::HashMap::new(); let dialect = AnsiDialect; @@ -1203,7 +1205,7 @@ mod tests { .to_string(); assert_eq!( err, - "Validation error: Density requires a position aesthetic mapping" + "Validation error: Density requires 'x' aesthetic mapping" ); } } diff --git a/src/plot/layer/geom/histogram.rs b/src/plot/layer/geom/histogram.rs index fc37c10f..e0db9e6b 100644 --- a/src/plot/layer/geom/histogram.rs +++ b/src/plot/layer/geom/histogram.rs @@ -93,7 +93,6 @@ impl GeomTrait for Histogram { parameters: &HashMap, execute_query: &dyn Fn(&str) -> Result, dialect: &dyn SqlDialect, - _aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { stat_histogram( query, @@ -123,7 +122,10 @@ fn stat_histogram( ) -> Result { // Get x column name from aesthetics let x_col = get_quoted_column_name(aesthetics, "pos1").ok_or_else(|| { - GgsqlError::ValidationError("Histogram requires 'x' aesthetic mapping".to_string()) + GgsqlError::ValidationError(format!( + "Histogram requires '{}' aesthetic mapping", + aesthetics.display_name("pos1") + )) })?; // Get bins from parameters (default: 30, validated by constraint) diff --git a/src/plot/layer/geom/line.rs b/src/plot/layer/geom/line.rs index b40b975f..66a1d6c0 100644 --- a/src/plot/layer/geom/line.rs +++ b/src/plot/layer/geom/line.rs @@ -57,7 +57,6 @@ impl GeomTrait for Line { parameters: &std::collections::HashMap, _execute_query: &dyn Fn(&str) -> crate::Result, dialect: &dyn crate::reader::SqlDialect, - aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> crate::Result { let result = if has_aggregate_param(parameters) { stat_aggregate::apply( @@ -67,7 +66,6 @@ impl GeomTrait for Line { group_by, parameters, dialect, - aesthetic_ctx, self.aggregate_domain_aesthetics().unwrap_or(&[]), )? } else { diff --git a/src/plot/layer/geom/mod.rs b/src/plot/layer/geom/mod.rs index 8a0ab55d..2e7d2a73 100644 --- a/src/plot/layer/geom/mod.rs +++ b/src/plot/layer/geom/mod.rs @@ -76,7 +76,6 @@ pub use text::Text; pub use tile::Tile; pub use violin::Violin; -use crate::plot::aesthetic::AestheticContext; use crate::plot::types::{ParameterValue, Schema}; use crate::reader::SqlDialect; @@ -240,21 +239,13 @@ pub trait GeomTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { parameters: &HashMap, _execute_query: &dyn Fn(&str) -> Result, dialect: &dyn SqlDialect, - aesthetic_ctx: &AestheticContext, ) -> Result { let mut result = if let (Some(domain), true) = ( self.aggregate_domain_aesthetics(), has_aggregate_param(parameters), ) { stat_aggregate::apply( - query, - schema, - aesthetics, - group_by, - parameters, - dialect, - aesthetic_ctx, - domain, + query, schema, aesthetics, group_by, parameters, dialect, domain, )? } else { StatResult::Identity @@ -522,7 +513,6 @@ impl Geom { parameters: &HashMap, execute_query: &dyn Fn(&str) -> Result, dialect: &dyn SqlDialect, - aesthetic_ctx: &AestheticContext, ) -> Result { self.0.apply_stat_transform( query, @@ -532,7 +522,6 @@ impl Geom { parameters, execute_query, dialect, - aesthetic_ctx, ) } diff --git a/src/plot/layer/geom/range.rs b/src/plot/layer/geom/range.rs index 0dd23c20..ca4fe481 100644 --- a/src/plot/layer/geom/range.rs +++ b/src/plot/layer/geom/range.rs @@ -62,19 +62,18 @@ impl std::fmt::Display for Range { #[cfg(test)] mod tests { - use crate::plot::{AestheticContext, AestheticValue, Geom, Layer}; + use crate::plot::{AestheticValue, Geom, Layer, Mappings}; - /// Helper function to create a layer with given mappings and validate it fn validate_range(mappings: &[(&str, &str)]) -> Result<(), String> { let mut layer = Layer::new(Geom::range()); + layer.mappings = Mappings::new().with_cartesian_context(); for (aesthetic, column) in mappings { layer.mappings.insert( aesthetic.to_string(), AestheticValue::standard_column(column.to_string()), ); } - let ctx = AestheticContext::from_static(&["x", "y"], &[]); - layer.validate_mapping(&Some(ctx), false) + layer.validate_mapping(false) } #[test] diff --git a/src/plot/layer/geom/ribbon.rs b/src/plot/layer/geom/ribbon.rs index 47f9bc26..ac44117d 100644 --- a/src/plot/layer/geom/ribbon.rs +++ b/src/plot/layer/geom/ribbon.rs @@ -56,7 +56,6 @@ impl GeomTrait for Ribbon { parameters: &std::collections::HashMap, _execute_query: &dyn Fn(&str) -> crate::Result, dialect: &dyn crate::reader::SqlDialect, - aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> crate::Result { let result = if has_aggregate_param(parameters) { stat_aggregate::apply( @@ -66,7 +65,6 @@ impl GeomTrait for Ribbon { group_by, parameters, dialect, - aesthetic_ctx, self.aggregate_domain_aesthetics().unwrap_or(&[]), )? } else { diff --git a/src/plot/layer/geom/rule.rs b/src/plot/layer/geom/rule.rs index 8549808e..123620f7 100644 --- a/src/plot/layer/geom/rule.rs +++ b/src/plot/layer/geom/rule.rs @@ -40,7 +40,11 @@ impl GeomTrait for Rule { let has_pos2 = mappings.contains_key("pos2"); if has_pos1 && has_pos2 { - return Err("Layer 'rule' requires exactly one of `x` or `y`, not both.".to_string()); + return Err(format!( + "Layer 'rule' requires exactly one of `{}` or `{}`, not both.", + mappings.display_name("pos1"), + mappings.display_name("pos2") + )); } Ok(()) @@ -112,19 +116,18 @@ impl std::fmt::Display for Rule { #[cfg(test)] mod tests { - use crate::plot::{AestheticContext, AestheticValue, Geom, Layer}; + use crate::plot::{AestheticValue, Geom, Layer, Mappings}; - /// Helper function to create a layer with given mappings and validate it fn validate_rule(mappings: &[(&str, &str)]) -> Result<(), String> { let mut layer = Layer::new(Geom::rule()); + layer.mappings = Mappings::new().with_cartesian_context(); for (aesthetic, column) in mappings { layer.mappings.insert( aesthetic.to_string(), AestheticValue::standard_column(column.to_string()), ); } - let ctx = AestheticContext::from_static(&["x", "y"], &[]); - layer.validate_mapping(&Some(ctx), false) + layer.validate_mapping(false) } #[test] diff --git a/src/plot/layer/geom/segment.rs b/src/plot/layer/geom/segment.rs index 3a76dc97..a19b9e1a 100644 --- a/src/plot/layer/geom/segment.rs +++ b/src/plot/layer/geom/segment.rs @@ -55,19 +55,18 @@ impl std::fmt::Display for Segment { #[cfg(test)] mod tests { - use crate::plot::{AestheticContext, AestheticValue, Geom, Layer}; + use crate::plot::{AestheticValue, Geom, Layer, Mappings}; - /// Helper function to create a layer with given mappings and validate it fn validate_segment(mappings: &[(&str, &str)]) -> Result<(), String> { let mut layer = Layer::new(Geom::segment()); + layer.mappings = Mappings::new().with_cartesian_context(); for (aesthetic, column) in mappings { layer.mappings.insert( aesthetic.to_string(), AestheticValue::standard_column(column.to_string()), ); } - let ctx = AestheticContext::from_static(&["x", "y"], &[]); - layer.validate_mapping(&Some(ctx), false) + layer.validate_mapping(false) } #[test] diff --git a/src/plot/layer/geom/smooth.rs b/src/plot/layer/geom/smooth.rs index d032ec09..6568e1be 100644 --- a/src/plot/layer/geom/smooth.rs +++ b/src/plot/layer/geom/smooth.rs @@ -96,7 +96,6 @@ impl GeomTrait for Smooth { parameters: &std::collections::HashMap, _execute_query: &dyn Fn(&str) -> crate::Result, dialect: &dyn SqlDialect, - _aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> crate::Result { // Get method from parameters (validated by ParamConstraint::string_option) let ParameterValue::String(method) = parameters.get("method").unwrap() else { @@ -134,14 +133,16 @@ impl std::fmt::Display for Smooth { fn stat_ols(query: &str, aesthetics: &Mappings, group_by: &[String]) -> Result { let x_col = get_quoted_column_name(aesthetics, "pos1").ok_or_else(|| { - GgsqlError::ValidationError( - "Smooth requires both position aesthetics to be mapped".to_string(), - ) + GgsqlError::ValidationError(format!( + "Smooth requires '{}' aesthetic mapping", + aesthetics.display_name("pos1") + )) })?; let y_col = get_quoted_column_name(aesthetics, "pos2").ok_or_else(|| { - GgsqlError::ValidationError( - "Smooth requires both position aesthetics to be mapped".to_string(), - ) + GgsqlError::ValidationError(format!( + "Smooth requires '{}' aesthetic mapping", + aesthetics.display_name("pos2") + )) })?; // Build group-related SQL fragments @@ -200,14 +201,16 @@ fn stat_ols(query: &str, aesthetics: &Mappings, group_by: &[String]) -> Result Result { let x_col = get_quoted_column_name(aesthetics, "pos1").ok_or_else(|| { - GgsqlError::ValidationError( - "Smooth requires both position aesthetics to be mapped".to_string(), - ) + GgsqlError::ValidationError(format!( + "Smooth requires '{}' aesthetic mapping", + aesthetics.display_name("pos1") + )) })?; let y_col = get_quoted_column_name(aesthetics, "pos2").ok_or_else(|| { - GgsqlError::ValidationError( - "Smooth requires both position aesthetics to be mapped".to_string(), - ) + GgsqlError::ValidationError(format!( + "Smooth requires '{}' aesthetic mapping", + aesthetics.display_name("pos2") + )) })?; // Build group-related SQL fragments @@ -485,22 +488,24 @@ mod tests { // ========================================================================= #[test] - fn stat_ols_missing_pos_aesthetics_emits_coord_agnostic_message() { - let mapping = crate::Mappings::new(); + fn stat_ols_missing_pos_aesthetics_emits_user_facing_name() { + let mapping = crate::Mappings::new().with_cartesian_context(); + let err = stat_ols("SELECT 1", &mapping, &[]).unwrap_err().to_string(); assert_eq!( err, - "Validation error: Smooth requires both position aesthetics to be mapped" + "Validation error: Smooth requires 'x' aesthetic mapping" ); } #[test] - fn stat_tls_missing_pos_aesthetics_emits_coord_agnostic_message() { - let mapping = crate::Mappings::new(); + fn stat_tls_missing_pos_aesthetics_emits_user_facing_name() { + let mapping = crate::Mappings::new().with_cartesian_context(); + let err = stat_tls("SELECT 1", &mapping, &[]).unwrap_err().to_string(); assert_eq!( err, - "Validation error: Smooth requires both position aesthetics to be mapped" + "Validation error: Smooth requires 'x' aesthetic mapping" ); } } diff --git a/src/plot/layer/geom/spatial.rs b/src/plot/layer/geom/spatial.rs index c73b8beb..57b381d0 100644 --- a/src/plot/layer/geom/spatial.rs +++ b/src/plot/layer/geom/spatial.rs @@ -32,7 +32,6 @@ impl GeomTrait for Spatial { _parameters: &std::collections::HashMap, execute_query: &dyn Fn(&str) -> crate::Result, dialect: &dyn crate::reader::SqlDialect, - _aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> crate::Result { for stmt in dialect.sql_spatial_setup() { execute_query(&stmt)?; diff --git a/src/plot/layer/geom/stat_aggregate.rs b/src/plot/layer/geom/stat_aggregate.rs index 2db7240c..f21301e8 100644 --- a/src/plot/layer/geom/stat_aggregate.rs +++ b/src/plot/layer/geom/stat_aggregate.rs @@ -32,7 +32,6 @@ use regex::Regex; use super::types::StatResult; use crate::naming; -use crate::plot::aesthetic::AestheticContext; use crate::plot::types::{ArrayElement, ParameterValue, Schema}; use crate::reader::SqlDialect; use crate::{GgsqlError, Mappings, Result}; @@ -533,14 +532,10 @@ fn unquote(qcol: &str) -> String { /// 3. The name is a material aesthetic with the same internal name (e.g. `size`). /// /// Returns the empty vector if no resolution finds a mapped aesthetic. -fn resolve_target_aesthetic( - user_aes: &str, - aesthetics: &Mappings, - aesthetic_ctx: &AestheticContext, -) -> Vec { +fn resolve_target_aesthetic(user_aes: &str, aesthetics: &Mappings) -> Vec { use crate::plot::layer::geom::types::AESTHETIC_ALIASES; let mut out = Vec::new(); - if let Some(internal) = aesthetic_ctx.map_user_to_internal(user_aes) { + if let Some(internal) = aesthetics.internal_name(user_aes) { if aesthetics.aesthetics.contains_key(internal) { out.push(internal.to_string()); return out; @@ -549,8 +544,8 @@ fn resolve_target_aesthetic( for (alias, targets) in AESTHETIC_ALIASES { if *alias == user_aes { for t in *targets { - let internal = aesthetic_ctx - .map_user_to_internal(t) + let internal = aesthetics + .internal_name(t) .map(|s| s.to_string()) .unwrap_or_else(|| (*t).to_string()); if aesthetics.aesthetics.contains_key(&internal) && !out.contains(&internal) { @@ -588,11 +583,10 @@ fn is_upper_half(internal_aes: &str) -> bool { pub(crate) fn resolve_aggregate_targets( spec: &AggregateSpec, aesthetics: &Mappings, - aesthetic_ctx: &AestheticContext, ) -> std::result::Result>, String> { let mut targets_internal: HashMap> = HashMap::new(); for (user_aes, fns) in &spec.targets { - let resolved = resolve_target_aesthetic(user_aes, aesthetics, aesthetic_ctx); + let resolved = resolve_target_aesthetic(user_aes, aesthetics); if resolved.is_empty() { return Err(format!( "aggregate target '{}' is not mapped on this layer", @@ -619,7 +613,6 @@ pub(crate) fn resolve_aggregate_targets( pub fn targeted_aesthetics( parameters: &HashMap, aesthetics: &Mappings, - aesthetic_ctx: &AestheticContext, ) -> HashSet { let raw = match parameters.get("aggregate") { Some(v) if !matches!(v, ParameterValue::Null) => v, @@ -631,7 +624,7 @@ pub fn targeted_aesthetics( }; let mut targeted: HashSet = HashSet::new(); for (user_aes, _fns) in &spec.targets { - for internal in resolve_target_aesthetic(user_aes, aesthetics, aesthetic_ctx) { + for internal in resolve_target_aesthetic(user_aes, aesthetics) { targeted.insert(internal); } } @@ -655,7 +648,6 @@ pub fn aggregated_aesthetics( parameters: &HashMap, aesthetics: &Mappings, schema: &Schema, - aesthetic_ctx: &AestheticContext, domain_aesthetics: &[&'static str], ) -> Option<(HashSet, HashSet)> { let raw = parameters.get("aggregate")?; @@ -666,7 +658,7 @@ pub fn aggregated_aesthetics( let mut targeted: HashSet = HashSet::new(); for (user_aes, _fns) in &spec.targets { - for internal in resolve_target_aesthetic(user_aes, aesthetics, aesthetic_ctx) { + for internal in resolve_target_aesthetic(user_aes, aesthetics) { targeted.insert(internal); } } @@ -714,7 +706,6 @@ pub fn aggregated_aesthetics( /// (the *reduce* path) — or, when at least one target lists multiple functions, /// `N` rows per group with a synthetic `aggregate` column tagging each row /// (the *explode* path). -#[allow(clippy::too_many_arguments)] pub fn apply( query: &str, schema: &Schema, @@ -722,7 +713,6 @@ pub fn apply( group_by: &[String], parameters: &HashMap, dialect: &dyn SqlDialect, - aesthetic_ctx: &AestheticContext, domain_aesthetics: &[&'static str], ) -> Result { let raw = match parameters.get("aggregate") { @@ -740,8 +730,8 @@ pub fn apply( // Resolve target keys (user-facing) → internal aesthetic names. An alias // like `color` expands to whichever of its targets (stroke/fill) is mapped // on the layer; the same function list applies to all of them. - let targets_internal = resolve_aggregate_targets(&spec, aesthetics, aesthetic_ctx) - .map_err(GgsqlError::ValidationError)?; + let targets_internal = + resolve_aggregate_targets(&spec, aesthetics).map_err(GgsqlError::ValidationError)?; // Walk mappings. Three buckets: // - aggregated: (internal_aes, raw_col, fns of length n) — each emits one column per row @@ -805,7 +795,7 @@ pub fn apply( } for d in &dropped { - let user_aes = aesthetic_ctx.map_internal_to_user(d); + let user_aes = aesthetics.display_name(d); eprintln!( "Warning: aggregate dropped numeric mapping for aesthetic '{}' \ (no applicable default and no targeted function). \ @@ -1094,10 +1084,6 @@ mod tests { .collect() } - fn cartesian_ctx() -> AestheticContext { - AestheticContext::from_static(&["x", "y"], &[]) - } - fn run( params: ParameterValue, aes: &Mappings, @@ -1118,7 +1104,6 @@ mod tests { ) -> Result { let mut p = HashMap::new(); p.insert("aggregate".to_string(), params); - let ctx = cartesian_ctx(); apply( "SELECT * FROM t", schema, @@ -1126,7 +1111,6 @@ mod tests { group_by, &p, dialect, - &ctx, domain, ) } @@ -1338,10 +1322,9 @@ mod tests { #[test] fn returns_identity_when_param_unset() { - let aes = Mappings::new(); + let aes = Mappings::new().with_cartesian_context(); let schema: Schema = vec![]; let p: HashMap = HashMap::new(); - let ctx = cartesian_ctx(); let result = apply( "SELECT * FROM t", &schema, @@ -1349,7 +1332,6 @@ mod tests { &[], &p, &InlineQuantileDialect, - &ctx, &[], ) .unwrap(); @@ -1358,7 +1340,7 @@ mod tests { #[test] fn returns_identity_when_param_null() { - let aes = Mappings::new(); + let aes = Mappings::new().with_cartesian_context(); let schema: Schema = vec![]; let result = run( ParameterValue::Null, @@ -1373,7 +1355,7 @@ mod tests { #[test] fn single_default_applies_to_every_numeric_mapping() { - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); let schema = schema_for(&[("__ggsql_aes_pos1__", false), ("__ggsql_aes_pos2__", false)]); @@ -1412,7 +1394,7 @@ mod tests { fn sqlite_dialect_emits_portable_stddev_and_first() { use crate::reader::sqlite::SqliteDialect; - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); let schema = schema_for(&[("__ggsql_aes_pos1__", false), ("__ggsql_aes_pos2__", false)]); @@ -1551,7 +1533,7 @@ mod tests { } } - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); let schema = schema_for(&[("__ggsql_aes_pos1__", false), ("__ggsql_aes_pos2__", false)]); @@ -1572,7 +1554,7 @@ mod tests { #[test] fn mid_emits_min_max_midpoint() { - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); let schema = schema_for(&[("__ggsql_aes_pos1__", false), ("__ggsql_aes_pos2__", false)]); @@ -1600,7 +1582,7 @@ mod tests { #[test] fn diff_uses_row_position_and_subtracts_first_from_last() { - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); let schema = schema_for(&[("__ggsql_aes_pos1__", false), ("__ggsql_aes_pos2__", false)]); @@ -1658,7 +1640,7 @@ mod tests { fn duckdb_first_skips_row_number_cte() { use crate::reader::duckdb::DuckDbDialect; - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); let schema = schema_for(&[("__ggsql_aes_pos1__", false), ("__ggsql_aes_pos2__", false)]); @@ -1692,7 +1674,7 @@ mod tests { // empty group_cols (so windows emit `OVER ()`). A bug that // forgot to thread `PARTITION BY ` through wouldn't // surface in those tests. - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); let schema = schema_for(&[ @@ -1752,7 +1734,7 @@ mod tests { #[test] fn first_and_last_emit_positional_aggregates() { - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2min", col("__ggsql_aes_pos2min__")); aes.insert("pos2max", col("__ggsql_aes_pos2max__")); @@ -1788,7 +1770,7 @@ mod tests { #[test] fn two_defaults_split_lower_and_upper_for_segment() { - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); aes.insert("pos1end", col("__ggsql_aes_pos1end__")); @@ -1831,7 +1813,7 @@ mod tests { #[test] fn two_defaults_split_for_ribbon() { - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2min", col("__ggsql_aes_pos2min__")); aes.insert("pos2max", col("__ggsql_aes_pos2max__")); @@ -1862,7 +1844,7 @@ mod tests { #[test] fn targeted_prefix_overrides_default() { - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); let schema = schema_for(&[("__ggsql_aes_pos1__", false), ("__ggsql_aes_pos2__", false)]); @@ -1886,7 +1868,7 @@ mod tests { #[test] fn material_aesthetic_targeted_by_user_facing_name() { - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); aes.insert("size", col("__ggsql_aes_size__")); @@ -1920,7 +1902,7 @@ mod tests { fn color_alias_targets_stroke_and_fill() { // `color` is an alias that resolves to whichever of `stroke`/`fill` // is actually mapped on the layer. - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); aes.insert("fill", col("__ggsql_aes_fill__")); @@ -1955,7 +1937,7 @@ mod tests { fn explosion_emits_union_all_with_aggregate_label_column() { // ('y:min', 'y:max') on a line-style layer → 2 rows per group, each // tagged with the function name in __ggsql_stat_aggregate__. - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); let schema = schema_for(&[("__ggsql_aes_pos1__", false), ("__ggsql_aes_pos2__", false)]); @@ -1995,7 +1977,7 @@ mod tests { // - default 'mean' applies to non-targeted aesthetics, recycled // - y is exploded into [min, max] → N=2 // - color is targeted with one function → recycled to [median, median] - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); aes.insert("fill", col("__ggsql_aes_fill__")); @@ -2050,7 +2032,7 @@ mod tests { // and expects pos1 (the continuous time-axis column) to be a group // key, not a dropped numeric mapping. The geom declares pos1 as a // domain aesthetic; the stat keeps it as a group column. - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); let schema = schema_for(&[ @@ -2102,7 +2084,7 @@ mod tests { // - color is exploded → N=2 // Result: two rows, with color taking p25 in row 0 and p75 in row 1; // pos1/pos2min always use mean-sdev, pos2max always uses mean+sdev. - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2min", col("__ggsql_aes_pos2min__")); aes.insert("pos2max", col("__ggsql_aes_pos2max__")); @@ -2143,7 +2125,7 @@ mod tests { #[test] fn discrete_mapping_becomes_group_key() { - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); aes.insert("color", col("__ggsql_aes_color__")); @@ -2181,7 +2163,7 @@ mod tests { #[test] fn literal_mapping_passes_through() { - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); aes.insert( @@ -2209,7 +2191,7 @@ mod tests { #[test] fn untargeted_numeric_mapping_dropped_when_no_default() { - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); let schema = schema_for(&[("__ggsql_aes_pos1__", false), ("__ggsql_aes_pos2__", false)]); @@ -2238,7 +2220,7 @@ mod tests { #[test] fn quantile_uses_dialect_inline_when_available() { - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos2", col("__ggsql_aes_pos2__")); let schema = schema_for(&[("__ggsql_aes_pos2__", false)]); let result = run( @@ -2260,7 +2242,7 @@ mod tests { #[test] fn quantile_falls_back_to_correlated_subquery_without_inline() { - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos2", col("__ggsql_aes_pos2__")); let schema = schema_for(&[("__ggsql_aes_pos2__", false)]); let result = run( @@ -2284,7 +2266,7 @@ mod tests { #[test] fn unknown_targeted_aesthetic_is_error() { - let mut aes = Mappings::new(); + let mut aes = Mappings::new().with_cartesian_context(); aes.insert("pos1", col("__ggsql_aes_pos1__")); aes.insert("pos2", col("__ggsql_aes_pos2__")); let schema = schema_for(&[("__ggsql_aes_pos1__", false), ("__ggsql_aes_pos2__", false)]); diff --git a/src/plot/layer/geom/tile.rs b/src/plot/layer/geom/tile.rs index b166a622..a99024ca 100644 --- a/src/plot/layer/geom/tile.rs +++ b/src/plot/layer/geom/tile.rs @@ -116,7 +116,6 @@ impl GeomTrait for Tile { parameters: &HashMap, _execute_query: &dyn Fn(&str) -> Result, dialect: &dyn SqlDialect, - aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { // When `aggregate` is set, collapse rows first, then run the standard // tile parameter consolidation over the aggregated result. The wrapper @@ -133,7 +132,6 @@ impl GeomTrait for Tile { group_by, parameters, dialect, - aesthetic_ctx, self.aggregate_domain_aesthetics().unwrap_or(&[]), )?; match agg { @@ -246,6 +244,7 @@ fn process_direction( aesthetics: &Mappings, parameters: &HashMap, schema: &Schema, + display_name: &str, ) -> Result<(Vec, Vec)> { // Derive aesthetic names from axis let (center_aes, min_aes, max_aes, size_aes) = match axis { @@ -271,13 +270,15 @@ fn process_direction( .unwrap_or(false); // Generate position expressions + let size_name = if axis == "x" { "width" } else { "height" }; let (expr_1, expr_2) = if is_discrete { generate_discrete_position_expressions( center.as_deref(), min.as_deref(), max.as_deref(), size.as_deref(), - axis, + display_name, + size_name, )? } else { generate_continuous_position_expressions( @@ -285,7 +286,8 @@ fn process_direction( min.as_deref(), max.as_deref(), size.as_deref(), - axis, + display_name, + size_name, )? }; @@ -321,11 +323,16 @@ fn stat_tile( _group_by: &[String], parameters: &HashMap, ) -> Result { + let display_x = aesthetics.display_name("pos1"); + let display_y = aesthetics.display_name("pos2"); + // Process X direction - let (x_select, x_stat_cols) = process_direction("x", aesthetics, parameters, schema)?; + let (x_select, x_stat_cols) = + process_direction("x", aesthetics, parameters, schema, &display_x)?; // Process Y direction - let (y_select, y_stat_cols) = process_direction("y", aesthetics, parameters, schema)?; + let (y_select, y_stat_cols) = + process_direction("y", aesthetics, parameters, schema, &display_y)?; // Define consumed aesthetics (these will be transformed, not passed through) let consumed_aesthetic_names = [ @@ -384,17 +391,14 @@ fn generate_discrete_position_expressions( min: Option<&str>, max: Option<&str>, size: Option<&str>, - axis: &str, + display_name: &str, + size_name: &str, ) -> Result<(String, String)> { // Validate: discrete scales cannot use min/max if 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" } + display_name, display_name, display_name, display_name, size_name ))); } @@ -402,7 +406,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 + display_name, display_name ))), } } @@ -422,7 +426,8 @@ fn generate_continuous_position_expressions( min: Option<&str>, max: Option<&str>, size: Option<&str>, - axis: &str, + display_name: &str, + size_name: &str, ) -> Result<(String, String)> { match (center, min, max, size) { // Case 1: min + max @@ -455,11 +460,7 @@ fn generate_continuous_position_expressions( // Invalid: wrong number of parameters or invalid combination _ => Err(GgsqlError::ValidationError(format!( "Tile requires exactly 2 {}-direction parameters from {{{}, {}min, {}max, {}}}.", - axis, - axis, - axis, - axis, - if axis == "x" { "width" } else { "height" } + display_name, display_name, display_name, display_name, size_name ))), } } @@ -559,7 +560,7 @@ mod tests { } fn create_aesthetics(mappings: &[&str]) -> Mappings { - let mut aesthetics = Mappings::new(); + let mut aesthetics = Mappings::new().with_cartesian_context(); for aesthetic in mappings { // Use aesthetic column naming convention let col_name = naming::aesthetic_column(aesthetic); @@ -1043,7 +1044,7 @@ mod tests { min: None, max: None, }); - let ctx = AestheticContext::from_static(&["x", "y"], &[]); + let mut parameters = HashMap::new(); parameters.insert( "aggregate".to_string(), @@ -1059,7 +1060,6 @@ mod tests { ¶meters, &|_| panic!("execute_query should not run during stat building"), &AnsiDialect, - &ctx, ) .unwrap(); @@ -1114,7 +1114,7 @@ mod tests { min: None, max: None, }); - let ctx = AestheticContext::from_static(&["x", "y"], &[]); + let mut parameters = HashMap::new(); parameters.insert( "aggregate".to_string(), @@ -1133,7 +1133,6 @@ mod tests { ¶meters, &|_| panic!("execute_query should not run during stat building"), &AnsiDialect, - &ctx, ) .unwrap(); diff --git a/src/plot/layer/geom/violin.rs b/src/plot/layer/geom/violin.rs index d3ae8c91..8af3fc0f 100644 --- a/src/plot/layer/geom/violin.rs +++ b/src/plot/layer/geom/violin.rs @@ -121,7 +121,6 @@ impl GeomTrait for Violin { parameters: &HashMap, _execute_query: &dyn Fn(&str) -> crate::Result, dialect: &dyn crate::reader::SqlDialect, - _aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { stat_violin(query, aesthetics, group_by, parameters, dialect) } @@ -205,9 +204,10 @@ fn stat_violin( ) -> Result { // Verify y exists if get_column_name(aesthetics, "pos2").is_none() { - return Err(GgsqlError::ValidationError( - "Violin requires 'y' aesthetic mapping (continuous)".to_string(), - )); + return Err(GgsqlError::ValidationError(format!( + "Violin requires '{}' aesthetic mapping (continuous)", + aesthetics.display_name("pos2") + ))); } // pos1 is optional. When the user omits it, wrap the source with a diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index b2a13061..15b069e6 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -26,14 +26,11 @@ pub use geom::{ // Re-export position types for convenience pub use position::{Position, PositionTrait, PositionType}; -use crate::{ - plot::{ - is_facet_aesthetic, parse_position, - types::{ - validate_parameter, AestheticValue, DataSource, Mappings, ParameterValue, SqlExpression, - }, +use crate::plot::{ + is_facet_aesthetic, parse_position, + types::{ + validate_parameter, AestheticValue, DataSource, Mappings, ParameterValue, SqlExpression, }, - AestheticContext, }; /// A single visualization layer (from DRAW clause) @@ -185,19 +182,8 @@ impl Layer { /// /// # 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) - }; + pub fn validate_mapping(&self, include_delayed: bool) -> std::result::Result<(), String> { + let translate = |aes: &str| -> String { format!("`{}`", self.mappings.display_name(aes)) }; // Check if all required aesthetics exist. The Aggregate stat replaces // mapped values in place — it never synthesises new aesthetics — so @@ -278,8 +264,8 @@ impl Layer { .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 { + // so we'll approve both flipped and unflipped aesthetics. + if let Some(ctx) = self.mappings.context() { let flipped: Vec = supported.iter().map(|aes| ctx.flip_position(aes)).collect(); supported.extend(flipped); } @@ -450,10 +436,7 @@ impl Layer { /// Used by the standalone validate path (`ggsql validate`); the execute /// path catches the same errors at execute time inside /// `stat_aggregate::apply` (avoiding a redundant parse). - pub fn validate_aggregate_setting( - &self, - aesthetic_ctx: Option<&AestheticContext>, - ) -> std::result::Result<(), String> { + pub fn validate_aggregate_setting(&self) -> std::result::Result<(), String> { if !self.geom.supports_aggregate() { return Ok(()); } @@ -474,13 +457,7 @@ impl Layer { Some(s) => s, None => return Ok(()), }; - if let Some(ctx) = aesthetic_ctx { - crate::plot::layer::geom::stat_aggregate::resolve_aggregate_targets( - &spec, - &self.mappings, - ctx, - )?; - } + crate::plot::layer::geom::stat_aggregate::resolve_aggregate_targets(&spec, &self.mappings)?; Ok(()) } @@ -673,18 +650,14 @@ 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 = Mappings::new().with_cartesian_context(); 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); + let result = layer.validate_mapping(false); assert!(result.is_err()); let err = result.unwrap_err(); assert!( @@ -696,10 +669,8 @@ mod tests { #[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 = Mappings::new().with_cartesian_context(); layer.mappings.insert( "pos1".to_string(), AestheticValue::standard_column("x".to_string()), @@ -709,12 +680,11 @@ mod tests { AestheticValue::standard_column("ymin".to_string()), ); layer.mappings.insert( - "pos1max".to_string(), // This should be pos2max to match pos2min's orientation + "pos1max".to_string(), AestheticValue::standard_column("xmax".to_string()), ); - let ctx = AestheticContext::from_static(&["x", "y"], &[]); - let result = layer.validate_mapping(&Some(ctx), false); + let result = layer.validate_mapping(false); assert!(result.is_err()); let err = result.unwrap_err(); assert!( @@ -731,10 +701,8 @@ mod tests { #[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 = Mappings::new().with_cartesian_context(); layer.mappings.insert( "pos1".to_string(), AestheticValue::standard_column("x".to_string()), @@ -748,17 +716,14 @@ mod tests { AestheticValue::standard_column("ymax".to_string()), ); - let ctx = AestheticContext::from_static(&["x", "y"], &[]); - let result = layer.validate_mapping(&Some(ctx), false); + let result = layer.validate_mapping(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 = Mappings::new().with_cartesian_context(); layer.mappings.insert( "pos2".to_string(), AestheticValue::standard_column("y".to_string()), @@ -772,8 +737,7 @@ mod tests { AestheticValue::standard_column("xmax".to_string()), ); - let ctx = AestheticContext::from_static(&["x", "y"], &[]); - let result = layer.validate_mapping(&Some(ctx), false); + let result = layer.validate_mapping(false); assert!(result.is_ok()); } } diff --git a/src/plot/layer/orientation.rs b/src/plot/layer/orientation.rs index e38a08ce..329791b5 100644 --- a/src/plot/layer/orientation.rs +++ b/src/plot/layer/orientation.rs @@ -25,7 +25,7 @@ use super::geom::{Geom, GeomType}; use super::Layer; -use crate::plot::aesthetic::{is_position_aesthetic, AestheticContext}; +use crate::plot::aesthetic::is_position_aesthetic; use crate::plot::scale::ScaleTypeKind; use crate::plot::{AestheticValue, DefaultAestheticValue, Mappings, Scale}; use crate::{naming, DataFrame}; @@ -271,10 +271,10 @@ pub fn flip_position_aesthetics( /// /// This is called after query execution for layers with Transposed orientation, /// in coordination with `normalize_mapping_column_names` which updates the mappings. -pub fn flip_dataframe_position_columns( - df: DataFrame, - aesthetic_ctx: &AestheticContext, -) -> DataFrame { +pub fn flip_dataframe_position_columns(df: DataFrame, mappings: &crate::Mappings) -> DataFrame { + let Some(aesthetic_ctx) = mappings.context() else { + return df; + }; // Collect renames needed let renames: Vec<(String, String)> = df .get_column_names() diff --git a/src/plot/main.rs b/src/plot/main.rs index 4535c0bb..a26b4b30 100644 --- a/src/plot/main.rs +++ b/src/plot/main.rs @@ -364,13 +364,13 @@ 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, false).is_ok()); + assert!(valid_point.validate_mapping(false).is_ok()); // Line still requires both pos1 and pos2 - mapping only one fails. let invalid_line = Layer::new(Geom::line()) .with_aesthetic("pos1".to_string(), AestheticValue::standard_column("x")); - assert!(invalid_line.validate_mapping(&None, false).is_err()); + assert!(invalid_line.validate_mapping(false).is_err()); let valid_ribbon = Layer::new(Geom::ribbon()) .with_aesthetic("pos1".to_string(), AestheticValue::standard_column("x")) @@ -383,7 +383,7 @@ mod tests { AestheticValue::standard_column("ymax"), ); - assert!(valid_ribbon.validate_mapping(&None, false).is_ok()); + assert!(valid_ribbon.validate_mapping(false).is_ok()); } #[test] diff --git a/src/plot/types.rs b/src/plot/types.rs index ba69fc8f..144f2d08 100644 --- a/src/plot/types.rs +++ b/src/plot/types.rs @@ -61,12 +61,22 @@ pub type Schema = Vec; /// /// Used for both global mappings (VISUALISE clause) and layer mappings (MAPPING clause). /// Supports wildcards combined with explicit mappings. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Default)] +#[derive(Debug, Clone, Serialize, Deserialize, Default)] pub struct Mappings { /// Whether a wildcard (*) was specified pub wildcard: bool, /// Explicit aesthetic mappings (aesthetic → value) pub aesthetics: HashMap, + /// Aesthetic context for coordinate-aware name resolution. + /// Set once during execution setup; None during parsing. + #[serde(skip)] + context: Option, +} + +impl PartialEq for Mappings { + fn eq(&self, other: &Self) -> bool { + self.wildcard == other.wildcard && self.aesthetics == other.aesthetics + } } impl Mappings { @@ -75,6 +85,7 @@ impl Mappings { Self { wildcard: false, aesthetics: HashMap::new(), + context: None, } } @@ -83,9 +94,47 @@ impl Mappings { Self { wildcard: true, aesthetics: HashMap::new(), + context: None, + } + } + + /// Set the aesthetic context. Must only be called once. + pub fn set_context(&mut self, ctx: super::AestheticContext) { + debug_assert!(self.context.is_none(), "aesthetic context already set"); + self.context = Some(ctx); + } + + /// Get the aesthetic context, if set. + pub fn context(&self) -> Option<&super::AestheticContext> { + self.context.as_ref() + } + + /// Translate an internal aesthetic name to its user-facing name for display. + /// + /// Returns the user-facing name (e.g., "x" for "pos1") when context is available, + /// or the internal name unchanged when it isn't. + pub fn display_name(&self, internal: &str) -> String { + match &self.context { + Some(ctx) => ctx.map_internal_to_user(internal), + None => internal.to_string(), } } + /// Translate a user-facing aesthetic name to its internal name. + /// + /// Returns `None` when the name isn't a position/facet aesthetic + /// (i.e., it's a material aesthetic that doesn't get renamed). + pub fn internal_name(&self, user: &str) -> Option<&str> { + self.context.as_ref()?.map_user_to_internal(user) + } + + /// Set a default cartesian context (x/y). Test-only convenience. + #[cfg(test)] + pub fn with_cartesian_context(mut self) -> Self { + self.set_context(super::AestheticContext::from_static(&["x", "y"], &[])); + self + } + /// Check if the mappings are empty (no wildcard and no aesthetics) pub fn is_empty(&self) -> bool { !self.wildcard && self.aesthetics.is_empty() @@ -116,7 +165,10 @@ impl Mappings { /// Uses the provided AestheticContext to map user-facing position aesthetic names /// (e.g., "x", "y", "angle", "radius") to internal names (e.g., "pos1", "pos2"). /// Material aesthetics (e.g., "color", "size") are left unchanged. + /// + /// Also stores the context for later use by stat transforms and validation. pub fn transform_to_internal(&mut self, ctx: &super::AestheticContext) { + self.set_context(ctx.clone()); let original_aesthetics = std::mem::take(&mut self.aesthetics); for (aesthetic, value) in original_aesthetics { let internal_name = ctx diff --git a/src/validate.rs b/src/validate.rs index d4e2245b..811d0d45 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -235,7 +235,7 @@ pub fn validate(query: &str) -> Result { .or_insert(value.clone()); } } - if let Err(e) = merged.validate_mapping(&plot.aesthetic_context, false) { + if let Err(e) = merged.validate_mapping(false) { errors.push(ValidationError { message: format!("{}: {}", context, e), location: None, @@ -256,7 +256,7 @@ pub fn validate(query: &str) -> Result { // catches malformed `aggregate` values and unmapped/duplicate // targets. The execute path skips this; `stat_aggregate::apply` // parses + reports there. - if let Err(e) = layer.validate_aggregate_setting(plot.aesthetic_context.as_ref()) { + if let Err(e) = layer.validate_aggregate_setting() { errors.push(ValidationError { message: format!("{}: {}", context, e), location: None, diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index 1331a0aa..9b2d7594 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -7,7 +7,6 @@ //! Each geom type can override specific phases of the rendering pipeline while using //! sensible defaults for standard behavior. -use crate::plot::aesthetic::AestheticContext; use crate::plot::layer::geom::GeomType; use crate::plot::layer::is_transposed; use crate::plot::{ArrayElement, ParameterValue}; @@ -69,12 +68,7 @@ fn side_is_positive(side: &str, is_horizontal: bool) -> bool { } /// Validate column references for a single layer against its specific DataFrame -pub fn validate_layer_columns( - layer: &Layer, - data: &DataFrame, - layer_idx: usize, - ctx: &AestheticContext, -) -> Result<()> { +pub fn validate_layer_columns(layer: &Layer, data: &DataFrame, layer_idx: usize) -> Result<()> { let available_columns: Vec = data .get_column_names() .iter() @@ -89,12 +83,9 @@ pub fn validate_layer_columns( } else { " (global data)".to_string() }; - // Translate both the column name (which may be wrapped, e.g. - // `__ggsql_aes_pos1__`) and the aesthetic key (which is in - // internal form, e.g. `pos1`) to the user-facing form. let stripped = naming::extract_aesthetic_name(col).unwrap_or(col.as_str()); - let display_col = ctx.map_internal_to_user(stripped); - let display_aes = ctx.map_internal_to_user(aesthetic); + let display_col = layer.mappings.display_name(stripped); + let display_aes = layer.mappings.display_name(aesthetic); return Err(GgsqlError::ValidationError(format!( "Column '{}' referenced in aesthetic '{}' (layer {}{}) does not exist.\nAvailable columns: {}", display_col, @@ -2365,6 +2356,7 @@ pub fn get_renderer(geom: &Geom) -> Box { #[cfg(test)] mod tests { use super::*; + use crate::plot::aesthetic::AestheticContext; #[test] fn test_violin_detail_encoding() { diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index fb1644fb..acab14ee 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -1133,7 +1133,6 @@ impl Writer for VegaLiteWriter { .collect(); // 4. Validate columns for each layer - let aesthetic_ctx = spec.get_aesthetic_context(); for (layer_idx, (layer, key)) in spec.layers.iter().zip(layer_data_keys.iter()).enumerate() { let df = data.get(key).ok_or_else(|| { @@ -1143,7 +1142,7 @@ impl Writer for VegaLiteWriter { layer_idx + 1 )) })?; - validate_layer_columns(layer, df, layer_idx, &aesthetic_ctx)?; + validate_layer_columns(layer, df, layer_idx)?; } // 5. Build base Vega-Lite spec @@ -1241,11 +1240,9 @@ impl Writer for VegaLiteWriter { // 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)) - })?; + layer.validate_mapping(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. }