From 358b2d05c724416a1edfc9c8dc83c437eedc2b4a Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 16 Jun 2026 10:20:40 +0200 Subject: [PATCH 1/4] Fix partition columns lost during map projection transforms project_position_columns and the spatial materialization path built explicit SELECT lists from only aesthetic columns, dropping any PARTITION BY columns from the output. Include partition_by in the column set via a new util::set_union helper. Co-Authored-By: Claude Opus 4.6 --- src/plot/layer/geom/line.rs | 2 +- src/plot/layer/geom/path.rs | 2 +- src/plot/layer/geom/point.rs | 4 ++-- src/plot/layer/geom/polygon.rs | 2 +- src/plot/layer/geom/ribbon.rs | 2 ++ src/plot/layer/geom/rule.rs | 4 +++- src/plot/layer/geom/segment.rs | 2 ++ src/plot/layer/geom/text.rs | 4 ++-- src/plot/layer/geom/tile.rs | 4 +++- src/plot/projection/coord/map.rs | 3 ++- src/util.rs | 10 ++++++++++ 11 files changed, 29 insertions(+), 10 deletions(-) diff --git a/src/plot/layer/geom/line.rs b/src/plot/layer/geom/line.rs index dfb56efea..623770bd7 100644 --- a/src/plot/layer/geom/line.rs +++ b/src/plot/layer/geom/line.rs @@ -93,7 +93,7 @@ impl GeomTrait for Line { if !needs_projection(projection) { return Ok(query.to_string()); } - let columns = mappings.column_names(); + let columns = crate::util::set_union(mappings.column_names(), partition_by); let pos1_col = naming::aesthetic_column("pos1"); let densified = densify_edges( query, diff --git a/src/plot/layer/geom/path.rs b/src/plot/layer/geom/path.rs index f8bc2c4ca..bdb26c5fe 100644 --- a/src/plot/layer/geom/path.rs +++ b/src/plot/layer/geom/path.rs @@ -53,7 +53,7 @@ impl GeomTrait for Path { if !needs_projection(projection) { return Ok(query.to_string()); } - let columns = mappings.column_names(); + let columns = crate::util::set_union(mappings.column_names(), partition_by); let densified = densify_edges( query, dialect, diff --git a/src/plot/layer/geom/point.rs b/src/plot/layer/geom/point.rs index 1df12d2e7..265c88ca1 100644 --- a/src/plot/layer/geom/point.rs +++ b/src/plot/layer/geom/point.rs @@ -61,10 +61,10 @@ impl GeomTrait for Point { projection: &Projection, dialect: &dyn SqlDialect, mappings: &mut Mappings, - _partition_by: &mut Vec, + partition_by: &mut Vec, _parameters: &mut std::collections::HashMap, ) -> Result { - let columns = mappings.column_names(); + let columns = crate::util::set_union(mappings.column_names(), partition_by); project_position_columns(query, projection, dialect, &columns) } } diff --git a/src/plot/layer/geom/polygon.rs b/src/plot/layer/geom/polygon.rs index 7ed7da94b..a1ff3e1f2 100644 --- a/src/plot/layer/geom/polygon.rs +++ b/src/plot/layer/geom/polygon.rs @@ -54,7 +54,7 @@ impl GeomTrait for Polygon { if !needs_projection(projection) { return Ok(query.to_string()); } - let columns = mappings.column_names(); + let columns = crate::util::set_union(mappings.column_names(), partition_by); let densified = densify_edges(query, dialect, &columns, partition_by, None, true, 1.0, 360); project_position_columns(&densified, projection, dialect, &columns) } diff --git a/src/plot/layer/geom/ribbon.rs b/src/plot/layer/geom/ribbon.rs index 802d80145..647d5df22 100644 --- a/src/plot/layer/geom/ribbon.rs +++ b/src/plot/layer/geom/ribbon.rs @@ -71,6 +71,8 @@ impl GeomTrait for Ribbon { partition_by.push(naming::DENSIFY_ID_COLUMN.to_string()); parameters.insert("densified".to_string(), ParameterValue::Boolean(true)); + let expanded_columns = crate::util::set_union(expanded_columns, partition_by); + let densified = densify_edges( &expanded, dialect, diff --git a/src/plot/layer/geom/rule.rs b/src/plot/layer/geom/rule.rs index ada1cbd91..06ae40f03 100644 --- a/src/plot/layer/geom/rule.rs +++ b/src/plot/layer/geom/rule.rs @@ -58,7 +58,7 @@ impl GeomTrait for Rule { // The rule input always has one position column named __ggsql_aes_pos1__ // regardless of whether the aesthetic key is "pos1" or "pos2" — the executor // normalizes it to the pos1 slot. - let columns = vec![naming::aesthetic_column("pos1")]; + let columns = crate::util::set_union(vec![naming::aesthetic_column("pos1")], partition_by); let has_pos1 = mappings.contains_key("pos1"); let bbox_expr = match projection.coord.coord_kind() { @@ -80,6 +80,8 @@ impl GeomTrait for Rule { partition_by.push(naming::DENSIFY_ID_COLUMN.to_string()); parameters.insert("densified".to_string(), ParameterValue::Boolean(true)); + let expanded_columns = crate::util::set_union(expanded_columns, partition_by); + let densified = densify_edges( &expanded, dialect, diff --git a/src/plot/layer/geom/segment.rs b/src/plot/layer/geom/segment.rs index 76126fa04..ccb82de1d 100644 --- a/src/plot/layer/geom/segment.rs +++ b/src/plot/layer/geom/segment.rs @@ -69,6 +69,8 @@ impl GeomTrait for Segment { partition_by.push(naming::DENSIFY_ID_COLUMN.to_string()); parameters.insert("densified".to_string(), ParameterValue::Boolean(true)); + let expanded_columns = crate::util::set_union(expanded_columns, partition_by); + let densified = densify_edges( &expanded, dialect, diff --git a/src/plot/layer/geom/text.rs b/src/plot/layer/geom/text.rs index f262af30c..6c1256d44 100644 --- a/src/plot/layer/geom/text.rs +++ b/src/plot/layer/geom/text.rs @@ -76,10 +76,10 @@ impl GeomTrait for Text { projection: &Projection, dialect: &dyn SqlDialect, mappings: &mut Mappings, - _partition_by: &mut Vec, + partition_by: &mut Vec, _parameters: &mut std::collections::HashMap, ) -> Result { - let columns = mappings.column_names(); + let columns = crate::util::set_union(mappings.column_names(), partition_by); project_position_columns(query, projection, dialect, &columns) } diff --git a/src/plot/layer/geom/tile.rs b/src/plot/layer/geom/tile.rs index 5bc571824..3bc00943e 100644 --- a/src/plot/layer/geom/tile.rs +++ b/src/plot/layer/geom/tile.rs @@ -222,7 +222,7 @@ impl GeomTrait for Tile { return Ok(query.to_string()); } - let columns = mappings.column_names(); + let columns = crate::util::set_union(mappings.column_names(), partition_by); // Only densify continuous tiles (those parameterized by pos1min/pos1max/pos2min/pos2max). // Discrete tiles use categorical positions and don't appear on maps. @@ -240,6 +240,8 @@ impl GeomTrait for Tile { partition_by.push(naming::DENSIFY_ID_COLUMN.to_string()); parameters.insert("densified".to_string(), ParameterValue::Boolean(true)); + let expanded_columns = crate::util::set_union(expanded_columns, partition_by); + let densified = densify_edges( &expanded, dialect, diff --git a/src/plot/projection/coord/map.rs b/src/plot/projection/coord/map.rs index 108e06264..fc7122b06 100644 --- a/src/plot/projection/coord/map.rs +++ b/src/plot/projection/coord/map.rs @@ -98,7 +98,8 @@ pub(crate) fn apply_map_transforms( } layer_queries[idx] = if is_spatial { - let columns = layer.mappings.column_names(); + let columns = + crate::util::set_union(layer.mappings.column_names(), &layer.partition_by); let geom_col_quoted = naming::quote_ident(&naming::aesthetic_column("geometry")); let wkb_expr = dialect.sql_geometry_to_wkb(&geom_col_quoted); dialect.sql_select_replace( diff --git a/src/util.rs b/src/util.rs index b1ac3d369..9c7b5a073 100644 --- a/src/util.rs +++ b/src/util.rs @@ -77,6 +77,16 @@ fn format_quoted_list(items: &[T], quote: char, conjunction: &str) - format_list("ed, conjunction) } +/// Return the set union of `old` and `new`, preserving order and deduplicating. +pub fn set_union(mut old: Vec, new: &[String]) -> Vec { + for item in new { + if !old.contains(item) { + old.push(item.clone()); + } + } + old +} + #[cfg(test)] mod tests { use super::*; From 271e9ef06cfa76aa7230e9898b491c08385c1324 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 16 Jun 2026 11:35:25 +0200 Subject: [PATCH 2/4] Use set_union for group_by construction in layer transforms Co-Authored-By: Claude Opus 4.6 --- src/execute/layer.rs | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/src/execute/layer.rs b/src/execute/layer.rs index c78480444..f410365aa 100644 --- a/src/execute/layer.rs +++ b/src/execute/layer.rs @@ -530,19 +530,7 @@ where // Note: Facet aesthetics are already in partition_by via add_discrete_columns_to_partition_by, // so we don't add facet.get_variables() here (which would add original column names // instead of aesthetic column names, breaking pre-stat transforms like domain censoring). - let mut group_by: Vec = Vec::new(); - for col in &layer.partition_by { - group_by.push(col.clone()); - } - - // Add literal aesthetic columns to group_by so they survive stat transforms. - // Since literal columns contain constant values (same for every row), adding them - // to GROUP BY doesn't affect aggregation results - they're simply preserved. - for col in &literal_columns { - if !group_by.contains(col) { - group_by.push(col.clone()); - } - } + let group_by = crate::util::set_union(layer.partition_by.clone(), &literal_columns); // Apply statistical transformation (uses aesthetic names) let stat_result = layer.geom.apply_stat_transform( From 9a02b0c41d8e3071df0ea0304eefe5385795218a Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 16 Jun 2026 11:50:36 +0200 Subject: [PATCH 3/4] Add test for partition columns preserved through map projection Regression test: verifies that PARTITION BY columns survive the densification and projection transform pipeline for non-spatial layers with map projections. Co-Authored-By: Claude Opus 4.6 --- src/execute/mod.rs | 47 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 66774ad30..bd65660c3 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -3312,6 +3312,53 @@ mod tests { ); } + #[cfg(all(feature = "duckdb", feature = "spatial"))] + #[test] + fn test_partition_by_preserved_through_map_projection() { + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + reader.connection().execute_batch("LOAD spatial").unwrap(); + reader + .connection() + .execute( + "CREATE TABLE routes AS \ + SELECT 10.0 AS lon, 50.0 AS lat, 'A' AS direction, 1 AS grp \ + UNION ALL SELECT 20.0, 55.0, 'A', 1 \ + UNION ALL SELECT 30.0, 52.0, 'R' , 2", + duckdb::params![], + ) + .unwrap(); + + let query = r#" + SELECT lon, lat, direction, grp FROM routes + VISUALISE lon AS x, lat AS y + DRAW path PARTITION BY direction, grp + PROJECT x, y TO orthographic + "#; + + let result = prepare_data_with_reader(query, &reader); + assert!( + result.is_ok(), + "PARTITION BY columns should survive map projection: {:?}", + result.err() + ); + + let prepared = result.unwrap(); + let layer = &prepared.specs[0].layers[0]; + let data_key = layer.data_key.as_ref().unwrap(); + let df = prepared.data.get(data_key).unwrap(); + let col_names = df.get_column_names(); + assert!( + col_names.iter().any(|c| c == "direction"), + "partition column 'direction' missing from output data; columns: {:?}", + col_names + ); + assert!( + col_names.iter().any(|c| c == "grp"), + "partition column 'grp' missing from output data; columns: {:?}", + col_names + ); + } + #[cfg(feature = "duckdb")] #[test] fn test_case_insensitive_facet() { From d7e34d922c77b42d483115b2c9161df4ec656c0d Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 16 Jun 2026 12:03:37 +0200 Subject: [PATCH 4/4] install spatial for ci --- src/execute/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/execute/mod.rs b/src/execute/mod.rs index bd65660c3..59be3b807 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -3316,7 +3316,10 @@ mod tests { #[test] fn test_partition_by_preserved_through_map_projection() { let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); - reader.connection().execute_batch("LOAD spatial").unwrap(); + reader + .connection() + .execute_batch("INSTALL spatial; LOAD spatial") + .unwrap(); reader .connection() .execute(