From 1e2c652b91320556add5ca71c09041f7558ca2c7 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 23 Sep 2026 13:46:53 +0200 Subject: [PATCH 1/4] Fix bad transpose logic in aggregate --- src/plot/layer/geom/stat_aggregate.rs | 61 ++++++++++++++++++++++----- src/plot/layer/mod.rs | 3 ++ 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/src/plot/layer/geom/stat_aggregate.rs b/src/plot/layer/geom/stat_aggregate.rs index 17cf67f90..9c19eb88b 100644 --- a/src/plot/layer/geom/stat_aggregate.rs +++ b/src/plot/layer/geom/stat_aggregate.rs @@ -533,17 +533,38 @@ 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. +/// Read the layer's resolved orientation out of its parameters. The executor +/// stores the `resolve_orientations` verdict on the layer before any stat +/// runs; the standalone validate path has no such entry (and its mappings are +/// still in user orientation), so absence means "not transposed". +fn is_transposed_param(parameters: &Parameters) -> bool { + parameters.get("orientation").and_then(|v| v.as_str()) + == Some(crate::plot::layer::orientation::TRANSPOSED) +} + fn resolve_target_aesthetic( user_aes: &str, aesthetics: &Mappings, aesthetic_ctx: &AestheticContext, + transposed: bool, ) -> 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 aesthetics.aesthetics.contains_key(internal) { - out.push(internal.to_string()); - return out; + // Transposed layers have their position mappings flipped to aligned + // orientation before the stat runs (e.g. user `xmin` lives at + // `pos2min`), so the flipped internal name is the primary candidate. + let flipped = aesthetic_ctx.flip_position(internal); + let candidates: [&str; 2] = if transposed { + [&flipped, internal] + } else { + [internal, &flipped] + }; + for candidate in candidates { + if aesthetics.aesthetics.contains_key(candidate) { + out.push(candidate.to_string()); + return out; + } } } for (alias, targets) in AESTHETIC_ALIASES { @@ -589,10 +610,11 @@ pub(crate) fn resolve_aggregate_targets( spec: &AggregateSpec, aesthetics: &Mappings, aesthetic_ctx: &AestheticContext, + transposed: bool, ) -> 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, aesthetic_ctx, transposed); if resolved.is_empty() { return Err(format!( "aggregate target '{}' is not mapped on this layer", @@ -629,9 +651,10 @@ pub fn targeted_aesthetics( Some(s) => s, None => return HashSet::new(), }; + let transposed = is_transposed_param(parameters); 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, aesthetic_ctx, transposed) { targeted.insert(internal); } } @@ -664,9 +687,10 @@ pub fn aggregated_aesthetics( } let spec = parse_aggregate_param(raw).ok()??; + let transposed = is_transposed_param(parameters); 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, aesthetic_ctx, transposed) { targeted.insert(internal); } } @@ -740,8 +764,13 @@ 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, + aesthetic_ctx, + is_transposed_param(parameters), + ) + .map_err(GgsqlError::ValidationError)?; // Walk mappings. Three buckets: // - aggregated: (internal_aes, raw_col, fns of length n) — each emits one column per row @@ -804,8 +833,16 @@ pub fn apply( } } + let transposed = is_transposed_param(parameters); for d in &dropped { - let user_aes = aesthetic_ctx.map_internal_to_user(d); + // On transposed layers the internal name is flipped relative to the + // user's axes, so flip it back before translating for display. + let display_internal = if transposed { + aesthetic_ctx.flip_position(d) + } else { + d.clone() + }; + let user_aes = aesthetic_ctx.map_internal_to_user(&display_internal); eprintln!( "Warning: aggregate dropped numeric mapping for aesthetic '{}' \ (no applicable default and no targeted function). \ @@ -851,7 +888,11 @@ pub fn apply( }; let mut stat_columns: Vec = aggregated.iter().map(|(a, _, _)| a.clone()).collect(); - let consumed_aesthetics: Vec = stat_columns.clone(); + // Dropped mappings are removed alongside consumed ones: their columns + // won't exist in the stat output, so leaving the mapping in place would + // produce a dangling column reference at write time. + let mut consumed_aesthetics: Vec = stat_columns.clone(); + consumed_aesthetics.extend(dropped.iter().cloned()); // The synthetic `aggregate` column is only emitted for the multi-row // (explosion) case, where it differentiates rows that share the same // group key. diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index 06a206575..f178f8ecf 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -481,6 +481,9 @@ impl Layer { &spec, &self.mappings, ctx, + // The validate path runs before orientation resolution, so + // mappings are still in user orientation. + false, )?; } Ok(()) From 6936671f69af65eb5b8c8b791e67f5ceac056582 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 23 Sep 2026 13:50:26 +0200 Subject: [PATCH 2/4] add changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d70d8036..0672bc4e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ - Fixed a parser bug that interpreted comment characters inside string literals as initializing a comment (#555). +- Fixed a bug in stat_aggregate prevented transposed layers from properly + aggregating in certain situations (#561) ## 0.5.2 - 2026-09-11 From e96a6374376833f63f58dd6fb59371ebf105a65e Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 23 Sep 2026 13:54:40 +0200 Subject: [PATCH 3/4] Add tests --- src/plot/layer/geom/stat_aggregate.rs | 157 ++++++++++++++++++++++++++ 1 file changed, 157 insertions(+) diff --git a/src/plot/layer/geom/stat_aggregate.rs b/src/plot/layer/geom/stat_aggregate.rs index 9c19eb88b..e7255353c 100644 --- a/src/plot/layer/geom/stat_aggregate.rs +++ b/src/plot/layer/geom/stat_aggregate.rs @@ -2324,6 +2324,163 @@ mod tests { } } + fn run_transposed( + params: ParameterValue, + aes: &Mappings, + schema: &Schema, + group_by: &[String], + dialect: &dyn SqlDialect, + ) -> Result { + let mut p = Parameters::new(); + p.insert("aggregate".to_string(), params); + p.insert( + "orientation".to_string(), + ParameterValue::String(crate::plot::layer::orientation::TRANSPOSED.to_string()), + ); + let ctx = cartesian_ctx(); + apply( + "SELECT * FROM t", + schema, + aes, + group_by, + &p, + dialect, + &ctx, + &[], + ) + } + + /// Regression test: on a transposed layer the executor flips position + /// mappings to aligned orientation before the stat runs, so user-facing + /// aggregate targets (`xmin`, `y`) must resolve to the *flipped* internal + /// names. Previously this failed with "aggregate target 'xmin' is not + /// mapped on this layer", or silently aggregated the wrong column. + #[test] + fn transposed_targets_resolve_to_flipped_aesthetics() { + // Flipped mappings, as apply() sees them on a transposed layer: + // user y → pos1, user xmin/xmax → pos2min/pos2max. + let mut aes = Mappings::new(); + aes.insert("pos1", col("__ggsql_aes_pos1__")); + aes.insert("pos2min", col("__ggsql_aes_pos2min__")); + aes.insert("pos2max", col("__ggsql_aes_pos2max__")); + let schema = schema_for(&[ + ("__ggsql_aes_pos1__", false), + ("__ggsql_aes_pos2min__", false), + ("__ggsql_aes_pos2max__", false), + ]); + let result = run_transposed( + arr(&["y:mean", "xmin:min", "xmax:max"]), + &aes, + &schema, + &[], + &InlineQuantileDialect, + ) + .unwrap(); + match result { + StatResult::Transformed { + query, + stat_columns, + .. + } => { + // `y:mean` aggregates the column behind pos1 (user's y data), + // `xmin`/`xmax` hit pos2min/pos2max (the flipped slots). + assert!(query.contains("AVG(\"__ggsql_aes_pos1__\")"), "{}", query); + assert!( + query.contains("MIN(\"__ggsql_aes_pos2min__\")"), + "{}", + query + ); + assert!( + query.contains("MAX(\"__ggsql_aes_pos2max__\")"), + "{}", + query + ); + assert_eq!( + stat_columns, + vec![ + "pos1".to_string(), + "pos2max".to_string(), + "pos2min".to_string() + ] + ); + } + _ => panic!("expected Transformed"), + } + } + + /// On an aligned layer the same user-facing targets resolve to the + /// unflipped internal names — transposition must not leak across layers. + #[test] + fn aligned_targets_resolve_to_unflipped_aesthetics() { + let mut aes = Mappings::new(); + aes.insert("pos2", col("__ggsql_aes_pos2__")); + aes.insert("pos1min", col("__ggsql_aes_pos1min__")); + aes.insert("pos1max", col("__ggsql_aes_pos1max__")); + let schema = schema_for(&[ + ("__ggsql_aes_pos2__", false), + ("__ggsql_aes_pos1min__", false), + ("__ggsql_aes_pos1max__", false), + ]); + let result = run( + arr(&["y:mean", "xmin:min", "xmax:max"]), + &aes, + &schema, + &[], + &InlineQuantileDialect, + ) + .unwrap(); + match result { + StatResult::Transformed { query, .. } => { + assert!(query.contains("AVG(\"__ggsql_aes_pos2__\")"), "{}", query); + assert!( + query.contains("MIN(\"__ggsql_aes_pos1min__\")"), + "{}", + query + ); + assert!( + query.contains("MAX(\"__ggsql_aes_pos1max__\")"), + "{}", + query + ); + } + _ => panic!("expected Transformed"), + } + } + + /// A numeric mapping that no aggregate function applies to is dropped from + /// the stat output; its mapping must also be marked as consumed, otherwise + /// the writer sees a dangling reference to a column that no longer exists. + #[test] + fn dropped_numeric_mapping_is_consumed() { + let mut aes = Mappings::new(); + 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)]); + // Only `y` targeted, no default → x (pos1) is dropped. + let result = run( + ParameterValue::String("y:mean".to_string()), + &aes, + &schema, + &[], + &InlineQuantileDialect, + ) + .unwrap(); + match result { + StatResult::Transformed { + consumed_aesthetics, + .. + } => { + assert!(consumed_aesthetics.contains(&"pos2".to_string())); + assert!( + consumed_aesthetics.contains(&"pos1".to_string()), + "dropped aesthetic pos1 should be consumed, got: {:?}", + consumed_aesthetics + ); + } + _ => panic!("expected Transformed"), + } + } + #[test] fn unknown_targeted_aesthetic_is_error() { let mut aes = Mappings::new(); From 3d33c3ee09872b87e828b1069b93a9714afc0621 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Thu, 24 Sep 2026 09:48:02 +0200 Subject: [PATCH 4/4] Unify transpose checking --- src/plot/layer/geom/stat_aggregate.rs | 17 ++++------------- src/plot/layer/orientation.rs | 13 +++++++++++-- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/plot/layer/geom/stat_aggregate.rs b/src/plot/layer/geom/stat_aggregate.rs index e7255353c..9d5b5fda8 100644 --- a/src/plot/layer/geom/stat_aggregate.rs +++ b/src/plot/layer/geom/stat_aggregate.rs @@ -533,15 +533,6 @@ 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. -/// Read the layer's resolved orientation out of its parameters. The executor -/// stores the `resolve_orientations` verdict on the layer before any stat -/// runs; the standalone validate path has no such entry (and its mappings are -/// still in user orientation), so absence means "not transposed". -fn is_transposed_param(parameters: &Parameters) -> bool { - parameters.get("orientation").and_then(|v| v.as_str()) - == Some(crate::plot::layer::orientation::TRANSPOSED) -} - fn resolve_target_aesthetic( user_aes: &str, aesthetics: &Mappings, @@ -651,7 +642,7 @@ pub fn targeted_aesthetics( Some(s) => s, None => return HashSet::new(), }; - let transposed = is_transposed_param(parameters); + let transposed = crate::plot::layer::orientation::is_transposed_params(parameters); let mut targeted: HashSet = HashSet::new(); for (user_aes, _fns) in &spec.targets { for internal in resolve_target_aesthetic(user_aes, aesthetics, aesthetic_ctx, transposed) { @@ -687,7 +678,7 @@ pub fn aggregated_aesthetics( } let spec = parse_aggregate_param(raw).ok()??; - let transposed = is_transposed_param(parameters); + let transposed = crate::plot::layer::orientation::is_transposed_params(parameters); let mut targeted: HashSet = HashSet::new(); for (user_aes, _fns) in &spec.targets { for internal in resolve_target_aesthetic(user_aes, aesthetics, aesthetic_ctx, transposed) { @@ -768,7 +759,7 @@ pub fn apply( &spec, aesthetics, aesthetic_ctx, - is_transposed_param(parameters), + crate::plot::layer::orientation::is_transposed_params(parameters), ) .map_err(GgsqlError::ValidationError)?; @@ -833,7 +824,7 @@ pub fn apply( } } - let transposed = is_transposed_param(parameters); + let transposed = crate::plot::layer::orientation::is_transposed_params(parameters); for d in &dropped { // On transposed layers the internal name is flipped relative to the // user's axes, so flip it back before translating for display. diff --git a/src/plot/layer/orientation.rs b/src/plot/layer/orientation.rs index 7a39124ea..4a13a4c31 100644 --- a/src/plot/layer/orientation.rs +++ b/src/plot/layer/orientation.rs @@ -72,8 +72,17 @@ pub fn resolve_orientation(layer: &Layer, scales: &[Scale]) -> &'static str { /// Reads the orientation from the layer's parameters, which must have been /// set by `resolve_orientations()` during execution. pub fn is_transposed(layer: &Layer) -> bool { - layer - .parameters + is_transposed_params(&layer.parameters) +} + +/// Check transposition directly from a parameter set. +/// +/// This is the single place that knows the orientation is stored as a +/// `"transposed"` string parameter; callers that have a `Layer` should prefer +/// [`is_transposed`]. Absence of the parameter means "aligned" — e.g. the +/// standalone validate path, which runs before `resolve_orientations()`. +pub fn is_transposed_params(parameters: &crate::plot::Parameters) -> bool { + parameters .get("orientation") .and_then(|v| v.as_str()) .map(|s| s == TRANSPOSED)