From 835c430259e360343f80d5276f8333438c76dda2 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 2 Jun 2026 10:03:03 +0200 Subject: [PATCH 01/11] fix histogram message --- src/plot/layer/geom/histogram.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/plot/layer/geom/histogram.rs b/src/plot/layer/geom/histogram.rs index fc37c10f..6c55c085 100644 --- a/src/plot/layer/geom/histogram.rs +++ b/src/plot/layer/geom/histogram.rs @@ -93,7 +93,7 @@ impl GeomTrait for Histogram { parameters: &HashMap, execute_query: &dyn Fn(&str) -> Result, dialect: &dyn SqlDialect, - _aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, + aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { stat_histogram( query, @@ -102,6 +102,7 @@ impl GeomTrait for Histogram { parameters, execute_query, dialect, + aesthetic_ctx, ) } } @@ -120,10 +121,12 @@ fn stat_histogram( parameters: &HashMap, execute_query: &dyn Fn(&str) -> Result, dialect: &dyn SqlDialect, + aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> 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()) + let name = aesthetic_ctx.map_internal_to_user("pos1"); + GgsqlError::ValidationError(format!("Histogram requires '{}' aesthetic mapping", name)) })?; // Get bins from parameters (default: 30, validated by constraint) From 460c3ff5d6ffac6242c06da45257f655e543c8ee Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 2 Jun 2026 10:09:00 +0200 Subject: [PATCH 02/11] fix boxplot message --- src/plot/layer/geom/boxplot.rs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/plot/layer/geom/boxplot.rs b/src/plot/layer/geom/boxplot.rs index e49599ed..28e98870 100644 --- a/src/plot/layer/geom/boxplot.rs +++ b/src/plot/layer/geom/boxplot.rs @@ -99,9 +99,16 @@ impl GeomTrait for Boxplot { parameters: &HashMap, _execute_query: &dyn Fn(&str) -> Result, dialect: &dyn SqlDialect, - _aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, + aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { - stat_boxplot(query, aesthetics, group_by, parameters, dialect) + stat_boxplot( + query, + aesthetics, + group_by, + parameters, + dialect, + aesthetic_ctx, + ) } } @@ -117,9 +124,11 @@ fn stat_boxplot( group_by: &[String], parameters: &HashMap, dialect: &dyn SqlDialect, + aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { let y = get_column_name(aesthetics, "pos2").ok_or_else(|| { - GgsqlError::ValidationError("Boxplot requires 'y' aesthetic mapping".to_string()) + let name = aesthetic_ctx.map_internal_to_user("pos2"); + GgsqlError::ValidationError(format!("Boxplot requires '{}' aesthetic mapping", name)) })?; // pos1 is optional. When the user omits it, wrap the input query with a @@ -629,12 +638,14 @@ mod tests { parameters.insert("coef".to_string(), ParameterValue::Number(1.5)); parameters.insert("outliers".to_string(), ParameterValue::Boolean(true)); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_boxplot( "SELECT * FROM data", &aesthetics, &[], ¶meters, &AnsiDialect, + &ctx, ) .expect("stat_boxplot should succeed without pos1"); From 9d9ed181aa247d8be20e37a3465cac520d62d260 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 2 Jun 2026 10:12:32 +0200 Subject: [PATCH 03/11] fix violin message --- src/plot/layer/geom/violin.rs | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/plot/layer/geom/violin.rs b/src/plot/layer/geom/violin.rs index d3ae8c91..2aad4ff8 100644 --- a/src/plot/layer/geom/violin.rs +++ b/src/plot/layer/geom/violin.rs @@ -121,9 +121,16 @@ impl GeomTrait for Violin { parameters: &HashMap, _execute_query: &dyn Fn(&str) -> crate::Result, dialect: &dyn crate::reader::SqlDialect, - _aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, + aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { - stat_violin(query, aesthetics, group_by, parameters, dialect) + stat_violin( + query, + aesthetics, + group_by, + parameters, + dialect, + aesthetic_ctx, + ) } /// Post-process the violin DataFrame to scale offset to [0, 0.5 * width]. @@ -202,12 +209,15 @@ fn stat_violin( group_by: &[String], parameters: &HashMap, dialect: &dyn crate::reader::SqlDialect, + aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { // Verify y exists if get_column_name(aesthetics, "pos2").is_none() { - return Err(GgsqlError::ValidationError( - "Violin requires 'y' aesthetic mapping (continuous)".to_string(), - )); + let name = aesthetic_ctx.map_internal_to_user("pos2"); + return Err(GgsqlError::ValidationError(format!( + "Violin requires '{}' aesthetic mapping (continuous)", + name + ))); } // pos1 is optional. When the user omits it, wrap the source with a @@ -342,7 +352,8 @@ mod tests { let execute = |sql: &str| reader.execute_sql(sql); - let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect) + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); + let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect, &ctx) .expect("stat_violin should succeed"); // Verify the result is a transformed stat result @@ -406,7 +417,8 @@ mod tests { let execute = |sql: &str| reader.execute_sql(sql); - let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect) + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); + let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect, &ctx) .expect("stat_violin should succeed"); // Verify the result is a transformed stat result @@ -524,7 +536,8 @@ mod tests { let execute = |sql: &str| reader.execute_sql(sql); - let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect) + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); + let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect, &ctx) .expect("stat_violin with custom tails should succeed"); // Verify the SQL includes the tails constraint @@ -639,7 +652,8 @@ mod tests { reader.execute_sql(setup_sql).unwrap(); let execute = |sql: &str| reader.execute_sql(sql); - let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect) + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); + let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect, &ctx) .expect("stat_violin should succeed without pos1"); match result { From dad6b77bdc7f43c1311242567ffd0477fa1ffb29 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 2 Jun 2026 10:21:17 +0200 Subject: [PATCH 04/11] fix smooth message --- src/plot/layer/geom/smooth.rs | 70 +++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 27 deletions(-) diff --git a/src/plot/layer/geom/smooth.rs b/src/plot/layer/geom/smooth.rs index d032ec09..5fa30f39 100644 --- a/src/plot/layer/geom/smooth.rs +++ b/src/plot/layer/geom/smooth.rs @@ -96,7 +96,7 @@ impl GeomTrait for Smooth { parameters: &std::collections::HashMap, _execute_query: &dyn Fn(&str) -> crate::Result, dialect: &dyn SqlDialect, - _aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, + 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 { @@ -119,8 +119,8 @@ impl GeomTrait for Smooth { dialect, ) } - "ols" => stat_ols(query, aesthetics, group_by), - "tls" => stat_tls(query, aesthetics, group_by), + "ols" => stat_ols(query, aesthetics, group_by, aesthetic_ctx), + "tls" => stat_tls(query, aesthetics, group_by, aesthetic_ctx), _ => unreachable!("method validated by ParamConstraint::string_option"), } } @@ -132,16 +132,19 @@ impl std::fmt::Display for Smooth { } } -fn stat_ols(query: &str, aesthetics: &Mappings, group_by: &[String]) -> Result { +fn stat_ols( + query: &str, + aesthetics: &Mappings, + group_by: &[String], + aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, +) -> 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(), - ) + let name = aesthetic_ctx.map_internal_to_user("pos1"); + GgsqlError::ValidationError(format!("Smooth requires '{}' aesthetic mapping", name)) })?; let y_col = get_quoted_column_name(aesthetics, "pos2").ok_or_else(|| { - GgsqlError::ValidationError( - "Smooth requires both position aesthetics to be mapped".to_string(), - ) + let name = aesthetic_ctx.map_internal_to_user("pos2"); + GgsqlError::ValidationError(format!("Smooth requires '{}' aesthetic mapping", name)) })?; // Build group-related SQL fragments @@ -198,16 +201,19 @@ fn stat_ols(query: &str, aesthetics: &Mappings, group_by: &[String]) -> Result Result { +fn stat_tls( + query: &str, + aesthetics: &Mappings, + group_by: &[String], + aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, +) -> 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(), - ) + let name = aesthetic_ctx.map_internal_to_user("pos1"); + GgsqlError::ValidationError(format!("Smooth requires '{}' aesthetic mapping", name)) })?; let y_col = get_quoted_column_name(aesthetics, "pos2").ok_or_else(|| { - GgsqlError::ValidationError( - "Smooth requires both position aesthetics to be mapped".to_string(), - ) + let name = aesthetic_ctx.map_internal_to_user("pos2"); + GgsqlError::ValidationError(format!("Smooth requires '{}' aesthetic mapping", name)) })?; // Build group-related SQL fragments @@ -307,7 +313,8 @@ mod tests { }, ); - let result = stat_ols(query, &mapping, &groups).expect("stat_ols should succeed"); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); + let result = stat_ols(query, &mapping, &groups, &ctx).expect("stat_ols should succeed"); if let StatResult::Transformed { query: sql, @@ -358,7 +365,8 @@ mod tests { }, ); - let result = stat_ols(query, &mapping, &groups).expect("stat_ols should succeed"); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); + let result = stat_ols(query, &mapping, &groups, &ctx).expect("stat_ols should succeed"); if let StatResult::Transformed { query: sql, @@ -406,7 +414,8 @@ mod tests { }, ); - let result = stat_tls(query, &mapping, &groups).expect("stat_tls should succeed"); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); + let result = stat_tls(query, &mapping, &groups, &ctx).expect("stat_tls should succeed"); if let StatResult::Transformed { query: sql, @@ -457,7 +466,8 @@ mod tests { }, ); - let result = stat_tls(query, &mapping, &groups).expect("stat_tls should succeed"); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); + let result = stat_tls(query, &mapping, &groups, &ctx).expect("stat_tls should succeed"); if let StatResult::Transformed { query: sql, @@ -485,22 +495,28 @@ mod tests { // ========================================================================= #[test] - fn stat_ols_missing_pos_aesthetics_emits_coord_agnostic_message() { + fn stat_ols_missing_pos_aesthetics_emits_user_facing_name() { let mapping = crate::Mappings::new(); - let err = stat_ols("SELECT 1", &mapping, &[]).unwrap_err().to_string(); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); + let err = stat_ols("SELECT 1", &mapping, &[], &ctx) + .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() { + fn stat_tls_missing_pos_aesthetics_emits_user_facing_name() { let mapping = crate::Mappings::new(); - let err = stat_tls("SELECT 1", &mapping, &[]).unwrap_err().to_string(); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); + let err = stat_tls("SELECT 1", &mapping, &[], &ctx) + .unwrap_err() + .to_string(); assert_eq!( err, - "Validation error: Smooth requires both position aesthetics to be mapped" + "Validation error: Smooth requires 'x' aesthetic mapping" ); } } From 51b63a21cab6fa7b4622f350f575800225cbed10 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 2 Jun 2026 10:29:51 +0200 Subject: [PATCH 05/11] fix density message --- src/plot/layer/geom/density.rs | 22 +++++++++++++++++----- src/plot/layer/geom/smooth.rs | 1 + src/plot/layer/geom/violin.rs | 1 + 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/plot/layer/geom/density.rs b/src/plot/layer/geom/density.rs index 8032f5f4..d8f094f0 100644 --- a/src/plot/layer/geom/density.rs +++ b/src/plot/layer/geom/density.rs @@ -107,11 +107,18 @@ impl GeomTrait for Density { parameters: &std::collections::HashMap, _execute_query: &dyn Fn(&str) -> crate::Result, dialect: &dyn SqlDialect, - _aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, + 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, + query, + aesthetics, + "pos1", + None, + group_by, + parameters, + dialect, + aesthetic_ctx, ) } } @@ -140,6 +147,7 @@ fn with_leading_comma(s: &str) -> String { } } +#[allow(clippy::too_many_arguments)] pub(crate) fn stat_density( query: &str, aesthetics: &Mappings, @@ -148,9 +156,11 @@ pub(crate) fn stat_density( group_by: &[String], parameters: &HashMap, dialect: &dyn SqlDialect, + aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { let value = get_column_name(aesthetics, value_aesthetic).ok_or_else(|| { - GgsqlError::ValidationError("Density requires a position aesthetic mapping".to_string()) + let name = aesthetic_ctx.map_internal_to_user(value_aesthetic); + GgsqlError::ValidationError(format!("Density requires '{}' aesthetic mapping", name)) })?; let smooth = smooth_aesthetic.and_then(|smth| get_column_name(aesthetics, smth)); let weight = get_column_name(aesthetics, "weight"); @@ -1185,10 +1195,11 @@ mod tests { // ========================================================================= #[test] - fn stat_density_missing_value_aesthetic_emits_coord_agnostic_message() { + fn stat_density_missing_value_aesthetic_emits_user_facing_name() { let mappings = crate::Mappings::new(); let parameters = std::collections::HashMap::new(); let dialect = AnsiDialect; + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let err = stat_density( "SELECT 1", @@ -1198,12 +1209,13 @@ mod tests { &[], ¶meters, &dialect, + &ctx, ) .unwrap_err() .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/smooth.rs b/src/plot/layer/geom/smooth.rs index 5fa30f39..31a42cfb 100644 --- a/src/plot/layer/geom/smooth.rs +++ b/src/plot/layer/geom/smooth.rs @@ -117,6 +117,7 @@ impl GeomTrait for Smooth { group_by, ¶ms, dialect, + aesthetic_ctx, ) } "ols" => stat_ols(query, aesthetics, group_by, aesthetic_ctx), diff --git a/src/plot/layer/geom/violin.rs b/src/plot/layer/geom/violin.rs index 2aad4ff8..6b80500f 100644 --- a/src/plot/layer/geom/violin.rs +++ b/src/plot/layer/geom/violin.rs @@ -247,6 +247,7 @@ fn stat_violin( group_by.as_slice(), parameters, dialect, + aesthetic_ctx, )?; if !use_dummy { From bef8b376b8589c3fc731496c1812e1fed060cbaf Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 2 Jun 2026 10:42:12 +0200 Subject: [PATCH 06/11] fix rule message --- src/plot/layer/geom/mod.rs | 14 +++++++++++--- src/plot/layer/geom/rule.rs | 16 ++++++++++++++-- src/plot/layer/mod.rs | 2 +- 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/plot/layer/geom/mod.rs b/src/plot/layer/geom/mod.rs index 8a0ab55d..be036819 100644 --- a/src/plot/layer/geom/mod.rs +++ b/src/plot/layer/geom/mod.rs @@ -149,7 +149,11 @@ pub trait GeomTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { /// to allow geoms to implement custom validation logic (e.g., XOR constraints). /// /// Default: no additional validation - fn validate_aesthetics(&self, _mappings: &crate::Mappings) -> std::result::Result<(), String> { + fn validate_aesthetics( + &self, + _mappings: &crate::Mappings, + _aesthetic_ctx: &Option, + ) -> std::result::Result<(), String> { Ok(()) } @@ -572,8 +576,12 @@ impl Geom { } /// Validate aesthetic mappings - pub fn validate_aesthetics(&self, mappings: &Mappings) -> std::result::Result<(), String> { - self.0.validate_aesthetics(mappings) + pub fn validate_aesthetics( + &self, + mappings: &Mappings, + aesthetic_ctx: &Option, + ) -> std::result::Result<(), String> { + self.0.validate_aesthetics(mappings, aesthetic_ctx) } } diff --git a/src/plot/layer/geom/rule.rs b/src/plot/layer/geom/rule.rs index 8549808e..90f7c4e9 100644 --- a/src/plot/layer/geom/rule.rs +++ b/src/plot/layer/geom/rule.rs @@ -34,13 +34,25 @@ impl GeomTrait for Rule { Some(&[]) } - fn validate_aesthetics(&self, mappings: &crate::Mappings) -> std::result::Result<(), String> { + fn validate_aesthetics( + &self, + mappings: &crate::Mappings, + aesthetic_ctx: &Option, + ) -> std::result::Result<(), String> { // Rule requires exactly one of pos1 or pos2 (XOR logic) let has_pos1 = mappings.contains_key("pos1"); 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()); + let translate = |aes: &str| match aesthetic_ctx { + Some(ctx) => ctx.map_internal_to_user(aes), + None => aes.to_string(), + }; + return Err(format!( + "Layer 'rule' requires exactly one of `{}` or `{}`, not both.", + translate("pos1"), + translate("pos2") + )); } Ok(()) diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index b2a13061..47f1e7aa 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -305,7 +305,7 @@ impl Layer { } // Call geom-specific validation (e.g., XOR constraints for Rule) - self.geom.validate_aesthetics(&self.mappings)?; + self.geom.validate_aesthetics(&self.mappings, context)?; Ok(()) } From ac68865675c0224117e132e9858f1f27361cd0de Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 2 Jun 2026 10:54:01 +0200 Subject: [PATCH 07/11] fix tile message --- src/plot/layer/geom/tile.rs | 61 ++++++++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 17 deletions(-) diff --git a/src/plot/layer/geom/tile.rs b/src/plot/layer/geom/tile.rs index b166a622..dac354f8 100644 --- a/src/plot/layer/geom/tile.rs +++ b/src/plot/layer/geom/tile.rs @@ -182,6 +182,7 @@ impl GeomTrait for Tile { aesthetics, group_by, parameters, + aesthetic_ctx, )?; if exploded { @@ -246,6 +247,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 +273,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 +289,8 @@ fn process_direction( min.as_deref(), max.as_deref(), size.as_deref(), - axis, + display_name, + size_name, )? }; @@ -320,12 +325,18 @@ fn stat_tile( aesthetics: &Mappings, _group_by: &[String], parameters: &HashMap, + aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { + let display_x = aesthetic_ctx.map_internal_to_user("pos1"); + let display_y = aesthetic_ctx.map_internal_to_user("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 +395,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 +410,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 +430,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 +464,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 ))), } } @@ -631,12 +636,14 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, + &ctx, ); assert!( @@ -741,12 +748,14 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, + &ctx, ); assert!( @@ -806,12 +815,14 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, + &ctx, ); assert!(result.is_ok()); @@ -837,12 +848,14 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, + &ctx, ); assert!(result.is_ok()); @@ -868,12 +881,14 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, + &ctx, ); assert!(result.is_ok()); @@ -901,12 +916,14 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, + &ctx, ); assert!(result.is_ok()); let stat_result = result.unwrap(); @@ -932,12 +949,14 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, + &ctx, ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); @@ -951,12 +970,14 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, + &ctx, ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); @@ -971,12 +992,14 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, + &ctx, ); assert!(result.is_ok()); let stat_result = result.unwrap(); @@ -1003,12 +1026,14 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, + &ctx, ); assert!(result.is_ok()); @@ -1171,12 +1196,14 @@ mod tests { parameters.insert("width".to_string(), ParameterValue::Number(0.7)); parameters.insert("height".to_string(), ParameterValue::Number(0.9)); + let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, + &ctx, ); assert!(result.is_ok()); From 35eeca50d12dc14cbe1827be0283e3b4369fbce9 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 2 Jun 2026 12:14:15 +0200 Subject: [PATCH 08/11] store AestheticContext on Mappings, remove from function signatures Mappings now carries an optional AestheticContext field (set once during transform_to_internal). This eliminates the need to pass aesthetic_ctx as a separate parameter through ~30 stat/geom/execute function signatures. Added display_name() and internal_name() convenience methods on Mappings for translating between internal and user-facing aesthetic names. Co-Authored-By: Claude Opus 4.6 --- src/execute/layer.rs | 14 ++--- src/execute/mod.rs | 10 +-- src/execute/scale.rs | 2 - src/plot/layer/geom/area.rs | 2 - src/plot/layer/geom/bar.rs | 2 - src/plot/layer/geom/boxplot.rs | 19 ++---- src/plot/layer/geom/density.rs | 22 ++----- src/plot/layer/geom/histogram.rs | 9 ++- src/plot/layer/geom/line.rs | 2 - src/plot/layer/geom/mod.rs | 27 ++------ src/plot/layer/geom/ribbon.rs | 2 - src/plot/layer/geom/rule.rs | 14 +---- src/plot/layer/geom/smooth.rs | 72 +++++++++------------- src/plot/layer/geom/spatial.rs | 1 - src/plot/layer/geom/stat_aggregate.rs | 88 +++++++++++---------------- src/plot/layer/geom/tile.rs | 38 ++---------- src/plot/layer/geom/violin.rs | 27 ++------ src/plot/layer/mod.rs | 15 +---- src/plot/types.rs | 54 +++++++++++++++- src/validate.rs | 2 +- 20 files changed, 162 insertions(+), 260 deletions(-) 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..e236dcc4 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -875,7 +875,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) @@ -1289,13 +1288,7 @@ pub fn prepare_data_with_reader(query: &str, reader: &dyn Reader) -> Result Result 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 28e98870..ab8d48f9 100644 --- a/src/plot/layer/geom/boxplot.rs +++ b/src/plot/layer/geom/boxplot.rs @@ -99,16 +99,8 @@ 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, - aesthetic_ctx, - ) + stat_boxplot(query, aesthetics, group_by, parameters, dialect) } } @@ -124,11 +116,12 @@ fn stat_boxplot( group_by: &[String], parameters: &HashMap, dialect: &dyn SqlDialect, - aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { let y = get_column_name(aesthetics, "pos2").ok_or_else(|| { - let name = aesthetic_ctx.map_internal_to_user("pos2"); - GgsqlError::ValidationError(format!("Boxplot requires '{}' aesthetic mapping", name)) + 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 @@ -638,14 +631,12 @@ mod tests { parameters.insert("coef".to_string(), ParameterValue::Number(1.5)); parameters.insert("outliers".to_string(), ParameterValue::Boolean(true)); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_boxplot( "SELECT * FROM data", &aesthetics, &[], ¶meters, &AnsiDialect, - &ctx, ) .expect("stat_boxplot should succeed without pos1"); diff --git a/src/plot/layer/geom/density.rs b/src/plot/layer/geom/density.rs index d8f094f0..04aa08a0 100644 --- a/src/plot/layer/geom/density.rs +++ b/src/plot/layer/geom/density.rs @@ -107,18 +107,9 @@ 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, - aesthetic_ctx, + query, aesthetics, "pos1", None, group_by, parameters, dialect, ) } } @@ -156,11 +147,12 @@ pub(crate) fn stat_density( group_by: &[String], parameters: &HashMap, dialect: &dyn SqlDialect, - aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { let value = get_column_name(aesthetics, value_aesthetic).ok_or_else(|| { - let name = aesthetic_ctx.map_internal_to_user(value_aesthetic); - GgsqlError::ValidationError(format!("Density requires '{}' aesthetic mapping", name)) + 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"); @@ -1196,10 +1188,9 @@ mod tests { #[test] fn stat_density_missing_value_aesthetic_emits_user_facing_name() { - let mappings = crate::Mappings::new(); + let mappings = crate::Mappings::new().with_cartesian_context(); let parameters = std::collections::HashMap::new(); let dialect = AnsiDialect; - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let err = stat_density( "SELECT 1", @@ -1209,7 +1200,6 @@ mod tests { &[], ¶meters, &dialect, - &ctx, ) .unwrap_err() .to_string(); diff --git a/src/plot/layer/geom/histogram.rs b/src/plot/layer/geom/histogram.rs index 6c55c085..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, @@ -102,7 +101,6 @@ impl GeomTrait for Histogram { parameters, execute_query, dialect, - aesthetic_ctx, ) } } @@ -121,12 +119,13 @@ fn stat_histogram( parameters: &HashMap, execute_query: &dyn Fn(&str) -> Result, dialect: &dyn SqlDialect, - aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { // Get x column name from aesthetics let x_col = get_quoted_column_name(aesthetics, "pos1").ok_or_else(|| { - let name = aesthetic_ctx.map_internal_to_user("pos1"); - GgsqlError::ValidationError(format!("Histogram requires '{}' aesthetic mapping", name)) + 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 be036819..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; @@ -149,11 +148,7 @@ pub trait GeomTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { /// to allow geoms to implement custom validation logic (e.g., XOR constraints). /// /// Default: no additional validation - fn validate_aesthetics( - &self, - _mappings: &crate::Mappings, - _aesthetic_ctx: &Option, - ) -> std::result::Result<(), String> { + fn validate_aesthetics(&self, _mappings: &crate::Mappings) -> std::result::Result<(), String> { Ok(()) } @@ -244,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 @@ -526,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, @@ -536,7 +522,6 @@ impl Geom { parameters, execute_query, dialect, - aesthetic_ctx, ) } @@ -576,12 +561,8 @@ impl Geom { } /// Validate aesthetic mappings - pub fn validate_aesthetics( - &self, - mappings: &Mappings, - aesthetic_ctx: &Option, - ) -> std::result::Result<(), String> { - self.0.validate_aesthetics(mappings, aesthetic_ctx) + pub fn validate_aesthetics(&self, mappings: &Mappings) -> std::result::Result<(), String> { + self.0.validate_aesthetics(mappings) } } 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 90f7c4e9..dd829e6f 100644 --- a/src/plot/layer/geom/rule.rs +++ b/src/plot/layer/geom/rule.rs @@ -34,24 +34,16 @@ impl GeomTrait for Rule { Some(&[]) } - fn validate_aesthetics( - &self, - mappings: &crate::Mappings, - aesthetic_ctx: &Option, - ) -> std::result::Result<(), String> { + fn validate_aesthetics(&self, mappings: &crate::Mappings) -> std::result::Result<(), String> { // Rule requires exactly one of pos1 or pos2 (XOR logic) let has_pos1 = mappings.contains_key("pos1"); let has_pos2 = mappings.contains_key("pos2"); if has_pos1 && has_pos2 { - let translate = |aes: &str| match aesthetic_ctx { - Some(ctx) => ctx.map_internal_to_user(aes), - None => aes.to_string(), - }; return Err(format!( "Layer 'rule' requires exactly one of `{}` or `{}`, not both.", - translate("pos1"), - translate("pos2") + mappings.display_name("pos1"), + mappings.display_name("pos2") )); } diff --git a/src/plot/layer/geom/smooth.rs b/src/plot/layer/geom/smooth.rs index 31a42cfb..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 { @@ -117,11 +116,10 @@ impl GeomTrait for Smooth { group_by, ¶ms, dialect, - aesthetic_ctx, ) } - "ols" => stat_ols(query, aesthetics, group_by, aesthetic_ctx), - "tls" => stat_tls(query, aesthetics, group_by, aesthetic_ctx), + "ols" => stat_ols(query, aesthetics, group_by), + "tls" => stat_tls(query, aesthetics, group_by), _ => unreachable!("method validated by ParamConstraint::string_option"), } } @@ -133,19 +131,18 @@ impl std::fmt::Display for Smooth { } } -fn stat_ols( - query: &str, - aesthetics: &Mappings, - group_by: &[String], - aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, -) -> Result { +fn stat_ols(query: &str, aesthetics: &Mappings, group_by: &[String]) -> Result { let x_col = get_quoted_column_name(aesthetics, "pos1").ok_or_else(|| { - let name = aesthetic_ctx.map_internal_to_user("pos1"); - GgsqlError::ValidationError(format!("Smooth requires '{}' aesthetic mapping", name)) + GgsqlError::ValidationError(format!( + "Smooth requires '{}' aesthetic mapping", + aesthetics.display_name("pos1") + )) })?; let y_col = get_quoted_column_name(aesthetics, "pos2").ok_or_else(|| { - let name = aesthetic_ctx.map_internal_to_user("pos2"); - GgsqlError::ValidationError(format!("Smooth requires '{}' aesthetic mapping", name)) + GgsqlError::ValidationError(format!( + "Smooth requires '{}' aesthetic mapping", + aesthetics.display_name("pos2") + )) })?; // Build group-related SQL fragments @@ -202,19 +199,18 @@ fn stat_ols( }) } -fn stat_tls( - query: &str, - aesthetics: &Mappings, - group_by: &[String], - aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, -) -> Result { +fn stat_tls(query: &str, aesthetics: &Mappings, group_by: &[String]) -> Result { let x_col = get_quoted_column_name(aesthetics, "pos1").ok_or_else(|| { - let name = aesthetic_ctx.map_internal_to_user("pos1"); - GgsqlError::ValidationError(format!("Smooth requires '{}' aesthetic mapping", name)) + GgsqlError::ValidationError(format!( + "Smooth requires '{}' aesthetic mapping", + aesthetics.display_name("pos1") + )) })?; let y_col = get_quoted_column_name(aesthetics, "pos2").ok_or_else(|| { - let name = aesthetic_ctx.map_internal_to_user("pos2"); - GgsqlError::ValidationError(format!("Smooth requires '{}' aesthetic mapping", name)) + GgsqlError::ValidationError(format!( + "Smooth requires '{}' aesthetic mapping", + aesthetics.display_name("pos2") + )) })?; // Build group-related SQL fragments @@ -314,8 +310,7 @@ mod tests { }, ); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); - let result = stat_ols(query, &mapping, &groups, &ctx).expect("stat_ols should succeed"); + let result = stat_ols(query, &mapping, &groups).expect("stat_ols should succeed"); if let StatResult::Transformed { query: sql, @@ -366,8 +361,7 @@ mod tests { }, ); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); - let result = stat_ols(query, &mapping, &groups, &ctx).expect("stat_ols should succeed"); + let result = stat_ols(query, &mapping, &groups).expect("stat_ols should succeed"); if let StatResult::Transformed { query: sql, @@ -415,8 +409,7 @@ mod tests { }, ); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); - let result = stat_tls(query, &mapping, &groups, &ctx).expect("stat_tls should succeed"); + let result = stat_tls(query, &mapping, &groups).expect("stat_tls should succeed"); if let StatResult::Transformed { query: sql, @@ -467,8 +460,7 @@ mod tests { }, ); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); - let result = stat_tls(query, &mapping, &groups, &ctx).expect("stat_tls should succeed"); + let result = stat_tls(query, &mapping, &groups).expect("stat_tls should succeed"); if let StatResult::Transformed { query: sql, @@ -497,11 +489,9 @@ mod tests { #[test] fn stat_ols_missing_pos_aesthetics_emits_user_facing_name() { - let mapping = crate::Mappings::new(); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); - let err = stat_ols("SELECT 1", &mapping, &[], &ctx) - .unwrap_err() - .to_string(); + 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 'x' aesthetic mapping" @@ -510,11 +500,9 @@ mod tests { #[test] fn stat_tls_missing_pos_aesthetics_emits_user_facing_name() { - let mapping = crate::Mappings::new(); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); - let err = stat_tls("SELECT 1", &mapping, &[], &ctx) - .unwrap_err() - .to_string(); + 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 '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 dac354f8..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 { @@ -182,7 +180,6 @@ impl GeomTrait for Tile { aesthetics, group_by, parameters, - aesthetic_ctx, )?; if exploded { @@ -325,10 +322,9 @@ fn stat_tile( aesthetics: &Mappings, _group_by: &[String], parameters: &HashMap, - aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { - let display_x = aesthetic_ctx.map_internal_to_user("pos1"); - let display_y = aesthetic_ctx.map_internal_to_user("pos2"); + let display_x = aesthetics.display_name("pos1"); + let display_y = aesthetics.display_name("pos2"); // Process X direction let (x_select, x_stat_cols) = @@ -564,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); @@ -636,14 +632,12 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, - &ctx, ); assert!( @@ -748,14 +742,12 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, - &ctx, ); assert!( @@ -815,14 +807,12 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, - &ctx, ); assert!(result.is_ok()); @@ -848,14 +838,12 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, - &ctx, ); assert!(result.is_ok()); @@ -881,14 +869,12 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, - &ctx, ); assert!(result.is_ok()); @@ -916,14 +902,12 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, - &ctx, ); assert!(result.is_ok()); let stat_result = result.unwrap(); @@ -949,14 +933,12 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, - &ctx, ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); @@ -970,14 +952,12 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, - &ctx, ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); @@ -992,14 +972,12 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, - &ctx, ); assert!(result.is_ok()); let stat_result = result.unwrap(); @@ -1026,14 +1004,12 @@ mod tests { let group_by = vec![]; let parameters = HashMap::new(); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, - &ctx, ); assert!(result.is_ok()); @@ -1068,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(), @@ -1084,7 +1060,6 @@ mod tests { ¶meters, &|_| panic!("execute_query should not run during stat building"), &AnsiDialect, - &ctx, ) .unwrap(); @@ -1139,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(), @@ -1158,7 +1133,6 @@ mod tests { ¶meters, &|_| panic!("execute_query should not run during stat building"), &AnsiDialect, - &ctx, ) .unwrap(); @@ -1196,14 +1170,12 @@ mod tests { parameters.insert("width".to_string(), ParameterValue::Number(0.7)); parameters.insert("height".to_string(), ParameterValue::Number(0.9)); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); let result = stat_tile( "SELECT * FROM data", &schema, &aesthetics, &group_by, ¶meters, - &ctx, ); assert!(result.is_ok()); diff --git a/src/plot/layer/geom/violin.rs b/src/plot/layer/geom/violin.rs index 6b80500f..8af3fc0f 100644 --- a/src/plot/layer/geom/violin.rs +++ b/src/plot/layer/geom/violin.rs @@ -121,16 +121,8 @@ 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, - aesthetic_ctx, - ) + stat_violin(query, aesthetics, group_by, parameters, dialect) } /// Post-process the violin DataFrame to scale offset to [0, 0.5 * width]. @@ -209,14 +201,12 @@ fn stat_violin( group_by: &[String], parameters: &HashMap, dialect: &dyn crate::reader::SqlDialect, - aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, ) -> Result { // Verify y exists if get_column_name(aesthetics, "pos2").is_none() { - let name = aesthetic_ctx.map_internal_to_user("pos2"); return Err(GgsqlError::ValidationError(format!( "Violin requires '{}' aesthetic mapping (continuous)", - name + aesthetics.display_name("pos2") ))); } @@ -247,7 +237,6 @@ fn stat_violin( group_by.as_slice(), parameters, dialect, - aesthetic_ctx, )?; if !use_dummy { @@ -353,8 +342,7 @@ mod tests { let execute = |sql: &str| reader.execute_sql(sql); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); - let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect, &ctx) + let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect) .expect("stat_violin should succeed"); // Verify the result is a transformed stat result @@ -418,8 +406,7 @@ mod tests { let execute = |sql: &str| reader.execute_sql(sql); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); - let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect, &ctx) + let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect) .expect("stat_violin should succeed"); // Verify the result is a transformed stat result @@ -537,8 +524,7 @@ mod tests { let execute = |sql: &str| reader.execute_sql(sql); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); - let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect, &ctx) + let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect) .expect("stat_violin with custom tails should succeed"); // Verify the SQL includes the tails constraint @@ -653,8 +639,7 @@ mod tests { reader.execute_sql(setup_sql).unwrap(); let execute = |sql: &str| reader.execute_sql(sql); - let ctx = crate::plot::aesthetic::AestheticContext::from_static(&["x", "y"], &[]); - let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect, &ctx) + let result = stat_violin(query, &aesthetics, &groups, ¶meters, &AnsiDialect) .expect("stat_violin should succeed without pos1"); match result { diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index 47f1e7aa..e74201e4 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -305,7 +305,7 @@ impl Layer { } // Call geom-specific validation (e.g., XOR constraints for Rule) - self.geom.validate_aesthetics(&self.mappings, context)?; + self.geom.validate_aesthetics(&self.mappings)?; Ok(()) } @@ -450,10 +450,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 +471,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(()) } 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..292f892c 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -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, From 02f10b7b48f2ddc546006523a638481aaf3ceed8 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 2 Jun 2026 13:18:30 +0200 Subject: [PATCH 09/11] remove aesthetic_ctx parameter from validate_mapping and validate These functions now get context from the layer's Mappings directly, removing another set of explicit AestheticContext threading. Co-Authored-By: Claude Opus 4.6 --- src/execute/mod.rs | 54 ++++++++++++------------------ src/plot/layer/geom/range.rs | 7 ++-- src/plot/layer/geom/rule.rs | 7 ++-- src/plot/layer/geom/segment.rs | 7 ++-- src/plot/layer/mod.rs | 61 ++++++++++------------------------ src/plot/main.rs | 6 ++-- src/validate.rs | 2 +- src/writer/vegalite/mod.rs | 8 ++--- 8 files changed, 54 insertions(+), 98 deletions(-) diff --git a/src/execute/mod.rs b/src/execute/mod.rs index e236dcc4..f0cf07cc 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -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 ))); } @@ -1251,11 +1240,7 @@ pub fn prepare_data_with_reader(query: &str, reader: &dyn Reader) -> Result ColumnInfo { @@ -3402,6 +3387,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( @@ -3422,6 +3408,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"), @@ -3431,9 +3418,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'" @@ -3443,6 +3429,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"), @@ -3452,9 +3441,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'" @@ -3465,10 +3453,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'" @@ -3480,9 +3467,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'" @@ -3505,6 +3491,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"), @@ -3514,9 +3501,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'" @@ -3526,6 +3512,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"), @@ -3535,9 +3524,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/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/rule.rs b/src/plot/layer/geom/rule.rs index dd829e6f..123620f7 100644 --- a/src/plot/layer/geom/rule.rs +++ b/src/plot/layer/geom/rule.rs @@ -116,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/mod.rs b/src/plot/layer/mod.rs index e74201e4..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); } @@ -664,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!( @@ -687,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()), @@ -700,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!( @@ -722,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()), @@ -739,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()), @@ -763,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/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/validate.rs b/src/validate.rs index 292f892c..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, diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index fb1644fb..17019100 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -1241,11 +1241,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. } From e6da3555acf880da3ef4638c667887b0430ccf02 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 2 Jun 2026 13:26:57 +0200 Subject: [PATCH 10/11] use layer mappings context in partition_by and flip_dataframe add_discrete_columns_to_partition_by and flip_dataframe_position_columns now get AestheticContext from the layer's Mappings instead of requiring it as a separate parameter. Co-Authored-By: Claude Opus 4.6 --- src/execute/mod.rs | 20 ++++++++------------ src/plot/layer/orientation.rs | 10 +++++----- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/src/execute/mod.rs b/src/execute/mod.rs index f0cf07cc..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; @@ -828,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> = @@ -894,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 { @@ -1305,13 +1306,7 @@ pub fn prepare_data_with_reader(query: &str, reader: &dyn Reader) -> Result Result 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() From 53c2fa7d9ffb27d927c7031a0fab2955c46e51e6 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 2 Jun 2026 13:41:41 +0200 Subject: [PATCH 11/11] use layer mappings context in validate_layer_columns The writer's validate_layer_columns now uses layer.mappings.display_name() instead of receiving a separate AestheticContext parameter. Co-Authored-By: Claude Opus 4.6 --- src/writer/vegalite/layer.rs | 16 ++++------------ src/writer/vegalite/mod.rs | 3 +-- 2 files changed, 5 insertions(+), 14 deletions(-) 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 17019100..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