From f84b5a96568cb1cfa924e85a7b9f8d4981c9381b Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Tue, 10 Mar 2026 19:10:08 +0100 Subject: [PATCH 01/16] first stab at a rebase --- doc/syntax/layer/type/bar.qmd | 19 +- doc/syntax/layer/type/boxplot.qmd | 17 +- doc/syntax/layer/type/density.qmd | 7 +- doc/syntax/layer/type/histogram.qmd | 17 +- doc/syntax/layer/type/ribbon.qmd | 9 +- doc/syntax/layer/type/violin.qmd | 14 +- src/execute/layer.rs | 89 +++- src/execute/mod.rs | 161 ++++++- src/parser/builder.rs | 17 + src/plot/aesthetic.rs | 109 ++++- src/plot/layer/mod.rs | 61 ++- src/plot/layer/orientation.rs | 706 ++++++++++++++++++++++++++++ src/writer/vegalite/layer.rs | 169 +++++-- src/writer/vegalite/mod.rs | 2 +- 14 files changed, 1320 insertions(+), 77 deletions(-) create mode 100644 src/plot/layer/orientation.rs diff --git a/doc/syntax/layer/type/bar.qmd b/doc/syntax/layer/type/bar.qmd index 0acc5ab7..55e0484a 100644 --- a/doc/syntax/layer/type/bar.qmd +++ b/doc/syntax/layer/type/bar.qmd @@ -13,8 +13,8 @@ The following aesthetics are recognised by the bar layer. The bar layer has no required aesthetics ### Optional -* `x`: Position on the x-axis. If missing all records will be shown in the same bar -* `y`: The height of the plot. If missing, it will be calculated by the layer +* Primary axis (e.g. `x`): The categories to create bars for. If missing all records will be shown in the same bar +* Secondary axis (e.g. `y`): The height of the bars. If missing, it will be calculated by the layer * `colour`: The default colour of each bar * `stroke`: The colour of the stroke around each bar. Overrides `colour` * `fill`: The fill colour of each bar. Overrides `colour` @@ -27,7 +27,7 @@ The bar layer has no required aesthetics * `width`: The width of the bars as a proportion of the available width ## Data transformation -If `y` has not been mapped the layer will calculate it for you. +If the secondary axis has not been mapped the layer will calculate it for you. ### Properties @@ -40,7 +40,10 @@ If `y` has not been mapped the layer will calculate it for you. ### Default remappings -* `count AS y`: By default the barplot will show count as the height of the bars +* `count AS `: By default the barplot will show count as the height of the bars + +## Orientation +Bar plots have categories along their primary axis. The orientation can be deduced purely from the mapping. To create a horizontal bar plot you map the categories to `y` instead of `x` (assuming a default Cartesian coordinate system). ## Examples @@ -100,3 +103,11 @@ SCALE BINNED x And use with a polar coordinate system to create a pie chart **TBD** + +Create a horizontal bar plot by changing the mapping + +```{ggsql} +VISUALISE FROM ggsql:penguins +DRAW bar + MAPPING species AS y +``` diff --git a/doc/syntax/layer/type/boxplot.qmd b/doc/syntax/layer/type/boxplot.qmd index 60ad010f..9527da00 100644 --- a/doc/syntax/layer/type/boxplot.qmd +++ b/doc/syntax/layer/type/boxplot.qmd @@ -9,8 +9,8 @@ Boxplots display a summary of a continuous distribution. In the style of Tukey, The following aesthetics are recognised by the boxplot layer. ### Required -* `x`: Position on the x-axis -* `y`: Position on the y-axis +* Primary axis (e.g. `x`): The categorical variable to group by +* Secondary axis (e.g. `y`): The continuous variable to summarize ### Optional * `stroke`: The colour of the box contours, whiskers, median line and outliers. @@ -43,7 +43,10 @@ Observations are considered outliers when they are more extreme than the whisker ### Default remapping -* `value AS y`: By default the values are displayed along the y-axis. +* `value AS `: By default the values are displayed along the secondary axis. + +## Orientation +The boxplot has its categorical groups along the primary axis and the continuous values along the secondary axis. The orientation can be deduced from the scale types or from the mapping. To create a horizontal boxplot, map the categorical variable to `y` and the continuous variable to `x` (assuming a default Cartesian coordinate system). ### Examples @@ -83,3 +86,11 @@ DRAW boxplot MAPPING species AS x, bill_len AS y SETTING coef => 0.1 ``` + +Create a horizontal boxplot by swapping x and y: + +```{ggsql} +VISUALISE FROM ggsql:penguins +DRAW boxplot + MAPPING species AS y, bill_len AS x +``` diff --git a/doc/syntax/layer/type/density.qmd b/doc/syntax/layer/type/density.qmd index b73bd3a2..2c5f4129 100644 --- a/doc/syntax/layer/type/density.qmd +++ b/doc/syntax/layer/type/density.qmd @@ -10,7 +10,7 @@ Visualise the distribution of a single continuous variable by computing a kernel The following aesthetics are recognised by the density layer. ### Required -* `x`: Position on the x-axis. +* Primary axis (e.g. `x`): The continuous variable to estimate density for. ### Optional * `stroke`: The colour of the contour lines. @@ -62,7 +62,10 @@ $$ ### Default remappings -* `density AS y`: By default the density layer will display the computed density along the y-axis. +* `density AS `: By default the density layer will display the computed density along the secondary axis. + +## Orientation +The density has its primary axis along the variable for which density is computed. The orientation can be deduced from the mapping. To create a horizontal density plot, map the variable to `y` instead of `x` (assuming a default Cartesian coordinate system). ## Examples diff --git a/doc/syntax/layer/type/histogram.qmd b/doc/syntax/layer/type/histogram.qmd index 08ba69f8..954d4ce1 100644 --- a/doc/syntax/layer/type/histogram.qmd +++ b/doc/syntax/layer/type/histogram.qmd @@ -4,13 +4,13 @@ title: "Histogram" > Layers are declared with the [`DRAW` clause](../../clause/draw.qmd). Read the documentation for this clause for a thorough description of how to use it. -Visualise the distribution of a single continuous variable by dividing the x axis into bins and counting the number of observations in each bin. If providing a weight then a weighted histogram is calculated instead. +Visualise the distribution of a single continuous variable by dividing the primary axis into bins and counting the number of observations in each bin. If providing a weight then a weighted histogram is calculated instead. ## Aesthetics The following aesthetics are recognised by the bar layer. ### Required -* `x`: Position on the x-axis +* Primary axis (e.g. `x`): The continuous variable to bin ### Optional * `colour`: The default colour of each bar @@ -40,7 +40,10 @@ The histogram layer will bin the records in each group and count them. By defaul ### Default remappings -* `count AS y`: By default the histogram will show count as the height of the bars +* `count AS `: By default the histogram will show count as the height of the bars + +## Orientation +The histogram has its primary axis along the binned variable. The orientation can be deduced from the mapping. To create a horizontal histogram you map the variable to `y` instead of `x` (assuming a default Cartesian coordinate system). ## Examples @@ -86,3 +89,11 @@ DRAW histogram MAPPING body_mass AS x SETTING binwidth => 100 ``` + +Create a horizontal histogram by changing the mapping + +```{ggsql} +VISUALISE FROM ggsql:penguins +DRAW histogram + MAPPING body_mass AS y +``` diff --git a/doc/syntax/layer/type/ribbon.qmd b/doc/syntax/layer/type/ribbon.qmd index 58c36b1f..23a3d833 100644 --- a/doc/syntax/layer/type/ribbon.qmd +++ b/doc/syntax/layer/type/ribbon.qmd @@ -10,9 +10,9 @@ The ribbon layer is used to display extrema over a sorted x-axis. It can be seen The following aesthetics are recognised by the ribbon layer. ### Required -* `x`: Position along the x-axis -* `ymin`: Lower position along the y-axis. -* `ymax`: Upper position along the y-axis. +* Primary axis (e.g. `x`): Position along the primary (domain) axis +* Secondary axis min (e.g. `ymin`): Lower position along the secondary (value) axis. +* Secondary axis max (e.g. `ymax`): Upper position along the secondary (value) axis. ### Optional * `stroke`: The colour of the contour lines. @@ -27,6 +27,9 @@ The following aesthetics are recognised by the ribbon layer. ## Data transformation The ribbon layer does not transform its data but passes it through unchanged. +## Orientation +The ribbon has its domain axis as the primary axis and the value range on the secondary axis. The orientation can be deduced from the mapping. To create a horizontal ribbon, map the domain to `y` and use `xmin`/`xmax` for the value range (assuming a default Cartesian coordinate system). + ## Examples A ribbon plot with arbitrary values as minima/maxima diff --git a/doc/syntax/layer/type/violin.qmd b/doc/syntax/layer/type/violin.qmd index 88a91d1f..e95675db 100644 --- a/doc/syntax/layer/type/violin.qmd +++ b/doc/syntax/layer/type/violin.qmd @@ -11,8 +11,8 @@ The violins are mirrored kernel density estimates, similar to the [density](dens The following aesthetics are recognised by the violin layer. ### Required -* `x`: Position on the x-axis (categorical). -* `y`: Value on the y-axis for which to compute density. +* Primary axis (e.g. `x`): The categorical variable for grouping. +* Secondary axis (e.g. `y`): The continuous variable to compute density for. ### Optional * `stroke`: The colour of the contour lines. @@ -52,6 +52,9 @@ The major difference between a violin layer and a density layer is just the matt * `density AS offset`: By default the offsets around a centerline reflect the computed density. +## Orientation +The violin plot has its categorical groups along the primary axis and the continuous values along the secondary axis. The orientation can be deduced from the scale types or from the mapping. To create horizontal violins, swap `x` and `y` in the mapping (assuming a default Cartesian coordinate system). + ## Examples A typical violin plot. @@ -83,3 +86,10 @@ VISUALISE species AS x, bill_dep AS y, island AS fill FROM ggsql:penguins DRAW violin ``` +Create horizontal violins by swapping x and y: + +```{ggsql} +VISUALISE species AS y, bill_dep AS x FROM ggsql:penguins + DRAW violin +``` + diff --git a/src/execute/layer.rs b/src/execute/layer.rs index d4d911fb..0419d5af 100644 --- a/src/execute/layer.rs +++ b/src/execute/layer.rs @@ -337,6 +337,7 @@ pub fn build_layer_base_query( /// * `schema` - The layer's schema (with min/max from base_query) /// * `scales` - All resolved scales /// * `type_names` - SQL type names for the database backend +/// * `aesthetic_ctx` - Aesthetic context for name transformations /// * `execute_query` - Function to execute queries (needed for some stat transforms) /// /// # Returns @@ -348,15 +349,23 @@ pub fn apply_layer_transforms( schema: &Schema, scales: &[Scale], type_names: &SqlTypeNames, + aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, execute_query: &F, ) -> Result where F: Fn(&str) -> Result, { + use crate::plot::layer::orientation::{flip_mappings, flip_remappings, Orientation}; + // Clone order_by early to avoid borrow conflicts let order_by = layer.order_by.clone(); + // Orientation detection and initial flip was already done in mod.rs before + // build_layer_base_query. We just check if we need to flip back after stat. + let needs_flip = layer.orientation == Some(Orientation::Transposed); + // Build the aesthetic-named schema for stat transforms + // Note: Mappings were already flipped in mod.rs if needed, so schema reflects normalized orientation let aesthetic_schema: Schema = build_aesthetic_schema(layer, schema); // Collect literal aesthetic column names BEFORE conversion to Column values. @@ -418,8 +427,17 @@ where execute_query, )?; + // Flip user remappings BEFORE merging defaults for Transposed orientation. + // User remappings are in user orientation (e.g., `count AS x` for horizontal histogram). + // We flip them to aligned orientation so they're uniform with defaults. + // At the end, we flip everything back together. + if needs_flip { + flip_remappings(layer); + } + // Apply literal default remappings from geom defaults (e.g., y2 => 0.0 for bar baseline). // These apply regardless of stat transform, but only if user hasn't overridden them. + // Defaults are always in aligned orientation. for (aesthetic, default_value) in layer.geom.default_remappings() { // Only process literal values here (Column values are handled in Transformed branch) if !matches!(default_value, DefaultAestheticValue::Column(_)) { @@ -555,7 +573,22 @@ where StatResult::Identity => query, }; - // Apply ORDER BY + // Flip mappings back after stat transforms if we flipped them earlier + // Now pos1/pos2 map to the user's intended x/y positions + // Note: We only flip mappings here, not remappings. Remappings are flipped + // later in mod.rs after apply_remappings_post_query creates the columns, + // so that Phase 4.5 can flip those columns along with everything else. + if needs_flip { + flip_mappings(layer); + + // Normalize mapping column names to match their aesthetic keys. + // After flip_mappings, pos1 might point to __ggsql_aes_pos2__ (and vice versa). + // We update the column names so pos1 → __ggsql_aes_pos1__, etc. + // The DataFrame columns will be renamed correspondingly in mod.rs. + normalize_mapping_column_names(layer, aesthetic_ctx); + } + + // Apply explicit ORDER BY if provided let final_query = if let Some(ref o) = order_by { format!("{} ORDER BY {}", final_query, o.as_str()) } else { @@ -564,3 +597,57 @@ where Ok(final_query) } + +/// Normalize mapping column names to match their aesthetic keys after flip-back. +/// +/// After `flip_mappings`, the mapping values (column names) may not match the keys. +/// For example, pos1 might point to `__ggsql_aes_pos2__`. +/// This function updates the column names so pos1 → `__ggsql_aes_pos1__`, etc. +/// +/// This should be called after flip_mappings during flip-back. +/// The DataFrame columns should be renamed correspondingly using `flip_dataframe_columns`. +fn normalize_mapping_column_names( + layer: &mut Layer, + aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, +) { + // Collect the aesthetics to update (to avoid borrowing issues) + let aesthetics_to_update: Vec = layer + .mappings + .aesthetics + .keys() + .filter(|aes| crate::plot::aesthetic::is_positional_aesthetic(aes)) + .cloned() + .collect(); + + for aesthetic in aesthetics_to_update { + if let Some(value) = layer.mappings.aesthetics.get_mut(&aesthetic) { + let expected_col = naming::aesthetic_column(&aesthetic); + match value { + AestheticValue::Column { + name, + original_name, + .. + } => { + // The current column name might be wrong (e.g., __ggsql_aes_pos2__ for pos1) + // We need to flip it to match the aesthetic key + let current_col_aesthetic = + naming::extract_aesthetic_name(name).unwrap_or_default(); + let flipped_aesthetic = aesthetic_ctx.flip_positional(current_col_aesthetic); + + // If flipping results in the correct aesthetic, update the column name + if flipped_aesthetic == aesthetic { + *name = expected_col; + } + // Preserve original_name for axis labels + if original_name.is_none() { + *original_name = Some(aesthetic.clone()); + } + } + AestheticValue::Literal(_) => { + // Literals become columns with the expected name + *value = AestheticValue::standard_column(expected_col); + } + } + } + } +} diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 470a55cd..231bf141 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -35,6 +35,84 @@ use crate::reader::Reader; #[cfg(all(feature = "duckdb", test))] use crate::reader::DuckDBReader; +// ============================================================================= +// DataFrame Column Flipping for Orientation +// ============================================================================= + +/// Flip positional column names in a DataFrame for Transposed orientation layers. +/// +/// Swaps column names like `__ggsql_aes_pos1__` ↔ `__ggsql_aes_pos2__` so that +/// the data matches the flipped mapping names. +/// +/// This is called after query execution for layers with Transposed orientation, +/// in coordination with `normalize_mapping_column_names` which updates the mappings. +fn flip_dataframe_positional_columns(df: DataFrame, aesthetic_ctx: &AestheticContext) -> DataFrame { + use polars::prelude::*; + + // Collect all positional column renames needed + let mut renames: Vec<(String, String)> = Vec::new(); + + for col_name in df.get_column_names() { + let col_str = col_name.to_string(); + + // Check if this is an aesthetic column (__ggsql_aes_XXX__) + if let Some(aesthetic) = naming::extract_aesthetic_name(&col_str) { + if is_positional_aesthetic(aesthetic) { + let flipped = aesthetic_ctx.flip_positional(aesthetic); + if flipped != aesthetic { + let new_col_name = naming::aesthetic_column(&flipped); + renames.push((col_str, new_col_name)); + } + } + } + } + + if renames.is_empty() { + return df; + } + + // To swap columns (A ↔ B), we need to: + // 1. Rename A to temp + // 2. Rename B to A + // 3. Rename temp to B + // But if both A and B exist, we need to handle them together + + let df_clone = df.clone(); // For fallback if collect fails + let mut lazy = df.lazy(); + + // Group renames into swap pairs + let mut processed: HashSet = HashSet::new(); + for (from, to) in &renames { + if processed.contains(from) { + continue; + } + + // Check if the reverse rename also exists (it's a swap) + let is_swap = renames.iter().any(|(f, t)| f == to && t == from); + + if is_swap { + // Swap: use intermediate names to avoid collision + let temp_from = format!("{}_temp_swap_", from); + let temp_to = format!("{}_temp_swap_", to); + + lazy = lazy + .rename([from.as_str()], [temp_from.as_str()], true) + .rename([to.as_str()], [temp_to.as_str()], true) + .rename([temp_from.as_str()], [to.as_str()], true) + .rename([temp_to.as_str()], [from.as_str()], true); + + processed.insert(from.clone()); + processed.insert(to.clone()); + } else { + // Simple rename (one direction only) + lazy = lazy.rename([from.as_str()], [to.as_str()], true); + processed.insert(from.clone()); + } + } + + lazy.collect().unwrap_or(df_clone) +} + // ============================================================================= // Validation // ============================================================================= @@ -1024,6 +1102,35 @@ pub fn prepare_data_with_reader(query: &str, reader: &R) -> Result(query: &str, reader: &R) -> Result(query: &str, reader: &R) -> Result = HashMap::new(); + // Key by (query, serialized_remappings, orientation) to detect when layers can share data + // Layers with identical query AND remappings AND orientation share data via data_key + let mut config_to_key: HashMap<(String, String, bool), String> = HashMap::new(); for (idx, q) in layer_queries.iter().enumerate() { let layer = &mut specs[0].layers[idx]; let remappings_key = serde_json::to_string(&layer.remappings).unwrap_or_default(); - let config_key = (q.clone(), remappings_key); + let needs_flip = + layer.orientation == Some(crate::plot::layer::orientation::Orientation::Transposed); + let config_key = (q.clone(), remappings_key, needs_flip); if let Some(existing_key) = config_to_key.get(&config_key) { - // Same query AND same remappings - share data + // Same query AND same remappings AND same orientation - share data layer.data_key = Some(existing_key.clone()); } else { - // Need own data entry (either first occurrence or different remappings) + // Need own data entry (either first occurrence or different config) let layer_key = naming::layer_key(idx); let df = query_to_result.get(q).unwrap().clone(); + data_map.insert(layer_key.clone(), df); config_to_key.insert(config_key, layer_key.clone()); layer.data_key = Some(layer_key); } } - // Phase 4: Apply remappings and post-process - // - Rename stat columns to prefixed aesthetic names (e.g., __ggsql_stat_count → __ggsql_aes_y__) - // - Apply geom post_process (e.g., violin offset scaling) + // Phase 4: Apply remappings (rename stat columns and add literal columns) + // e.g., __ggsql_stat_count → __ggsql_aes_y__, or add __ggsql_aes_pos2end__ = 0.0 // Note: Prefixed aesthetic names persist through the entire pipeline // Track processed keys to avoid duplicate work on shared datasets let mut processed_keys: HashSet = HashSet::new(); @@ -1145,6 +1255,18 @@ pub fn prepare_data_with_reader(query: &str, reader: &R) -> Result(query: &str, reader: &R) -> Result = HashSet::new(); + for layer in specs[0].layers.iter() { + let needs_flip = + layer.orientation == Some(crate::plot::layer::orientation::Orientation::Transposed); + if needs_flip { + if let Some(ref key) = layer.data_key { + if flipped_keys.insert(key.clone()) { + // First time flipping this data key + if let Some(df) = data_map.remove(key) { + let flipped_df = flip_dataframe_positional_columns(df, &aesthetic_ctx); + data_map.insert(key.clone(), flipped_df); + } + } + } + } + } + // Create scales for aesthetics added by stat transforms (e.g., y from histogram) // This must happen after build_layer_query() which applies stat transforms // and modifies layer.mappings with new aesthetics like y → __ggsql_stat_count__ diff --git a/src/parser/builder.rs b/src/parser/builder.rs index 3754eeb6..4647e671 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -493,6 +493,22 @@ fn build_layer(node: &Node, source: &SourceTree) -> Result { .unwrap_or_default() }; + // Extract orientation from parameters if present + let orientation = parameters + .remove("orientation") + .map(|v| { + match v { + ParameterValue::String(s) => s + .parse() + .map_err(|e: String| GgsqlError::ParseError(e)), + _ => Err(GgsqlError::ParseError( + "orientation must be a string ('aligned', 'transposed', 'vertical', or 'horizontal')" + .to_string(), + )), + } + }) + .transpose()?; + let mut layer = Layer::new(geom); layer.position = position; layer.mappings = aesthetics; @@ -502,6 +518,7 @@ fn build_layer(node: &Node, source: &SourceTree) -> Result { layer.filter = filter; layer.order_by = order_by; layer.source = layer_source; + layer.orientation = orientation; Ok(layer) } diff --git a/src/plot/aesthetic.rs b/src/plot/aesthetic.rs index b6253ed8..b10feee0 100644 --- a/src/plot/aesthetic.rs +++ b/src/plot/aesthetic.rs @@ -30,6 +30,10 @@ use std::collections::HashMap; /// Positional aesthetic suffixes - applied to primary names to create variant aesthetics /// e.g., "x" + "min" = "xmin", "pos1" + "end" = "pos1end" +/// +/// Note: "offset" is intentionally NOT included here because it's a positioning +/// adjustment that shouldn't influence scale training or be part of aesthetic families. +/// The `flip_positional` method handles offset correctly via prefix detection. pub const POSITIONAL_SUFFIXES: &[&str] = &["min", "max", "end"]; // ============================================================================= @@ -304,6 +308,40 @@ impl AestheticContext { pub fn user_facet(&self) -> &[&'static str] { &self.user_facet } + + // === Orientation Flipping === + + /// Flip a positional aesthetic to its opposite position. + /// + /// Swaps pos1 ↔ pos2 (and their suffixed variants like pos1min ↔ pos2min). + /// Non-positional aesthetics are returned unchanged. + /// + /// # Examples + /// + /// ```ignore + /// let ctx = AestheticContext::from_static(&["x", "y"], &[]); + /// assert_eq!(ctx.flip_positional("pos1"), "pos2"); + /// assert_eq!(ctx.flip_positional("pos2min"), "pos1min"); + /// assert_eq!(ctx.flip_positional("pos1end"), "pos2end"); + /// assert_eq!(ctx.flip_positional("color"), "color"); // unchanged + /// ``` + pub fn flip_positional(&self, name: &str) -> String { + // Only flip if we have exactly 2 positional aesthetics + if self.internal_primaries.len() != 2 { + return name.to_string(); + } + + // Check if it's a pos1 or pos2 variant + if let Some(rest) = name.strip_prefix("pos1") { + return format!("pos2{}", rest); + } + if let Some(rest) = name.strip_prefix("pos2") { + return format!("pos1{}", rest); + } + + // Not a positional aesthetic, return unchanged + name.to_string() + } } /// Check if aesthetic is a user-facing facet aesthetic (panel, row, column) @@ -364,6 +402,33 @@ pub fn is_positional_aesthetic(name: &str) -> bool { false } +/// Parse a positional aesthetic name to extract its slot number and suffix. +/// +/// Returns `Some((slot, suffix))` for positional aesthetics: +/// - `pos1` → (1, "") +/// - `pos2min` → (2, "min") +/// - `pos1end` → (1, "end") +/// +/// Returns `None` for non-positional aesthetics. +pub fn parse_positional(name: &str) -> Option<(u8, &str)> { + if !name.starts_with("pos") { + return None; + } + let rest = &name[3..]; + let slot_char = rest.chars().next()?; + let slot = slot_char.to_digit(10)? as u8; + let suffix = &rest[1..]; + Some((slot, suffix)) +} + +/// Remap a positional aesthetic to a different slot. +/// +/// E.g., `remap_positional_slot("pos1min", 2)` → "pos2min" +pub fn remap_positional_slot(name: &str, new_slot: u8) -> Option { + let (_, suffix) = parse_positional(name)?; + Some(format!("pos{}{}", new_slot, suffix)) +} + #[cfg(test)] mod tests { use super::*; @@ -562,7 +627,7 @@ mod tests { fn test_aesthetic_context_families() { let ctx = AestheticContext::from_static(&["x", "y"], &[]); - // Get internal family + // Get internal family (offset not included - it's a positioning adjustment) let pos1_family = ctx.internal_positional_family("pos1").unwrap(); let pos1_strs: Vec<&str> = pos1_family.iter().map(|s| s.as_str()).collect(); assert_eq!(pos1_strs, vec!["pos1", "pos1min", "pos1max", "pos1end"]); @@ -573,4 +638,46 @@ mod tests { assert_eq!(ctx.primary_internal_positional("pos2end"), Some("pos2")); assert_eq!(ctx.primary_internal_positional("color"), Some("color")); } + + #[test] + fn test_parse_positional() { + // Primary positional + assert_eq!(parse_positional("pos1"), Some((1, ""))); + assert_eq!(parse_positional("pos2"), Some((2, ""))); + + // Variants with suffixes + assert_eq!(parse_positional("pos1min"), Some((1, "min"))); + assert_eq!(parse_positional("pos2max"), Some((2, "max"))); + assert_eq!(parse_positional("pos1end"), Some((1, "end"))); + + // Non-positional + assert_eq!(parse_positional("color"), None); + assert_eq!(parse_positional("x"), None); + assert_eq!(parse_positional("xmin"), None); + } + + #[test] + fn test_remap_positional_slot() { + // Remap slot 1 → slot 2 + assert_eq!(remap_positional_slot("pos1", 2), Some("pos2".to_string())); + assert_eq!( + remap_positional_slot("pos1min", 2), + Some("pos2min".to_string()) + ); + assert_eq!( + remap_positional_slot("pos1end", 2), + Some("pos2end".to_string()) + ); + + // Remap slot 2 → slot 1 + assert_eq!(remap_positional_slot("pos2", 1), Some("pos1".to_string())); + assert_eq!( + remap_positional_slot("pos2max", 1), + Some("pos1max".to_string()) + ); + + // Non-positional returns None + assert_eq!(remap_positional_slot("color", 1), None); + assert_eq!(remap_positional_slot("x", 2), None); + } } diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index eb6f189c..6f078dc0 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -9,9 +9,15 @@ use std::collections::HashMap; // Geom is a submodule of layer pub mod geom; +// Orientation is a submodule of layer +pub mod orientation; + // Position is a submodule of layer pub mod position; +// Re-export orientation types +pub use orientation::Orientation; + // Re-export geom types for convenience pub use geom::{ DefaultAesthetics, DefaultParam, DefaultParamValue, Geom, GeomTrait, GeomType, StatResult, @@ -63,6 +69,10 @@ pub struct Layer { pub order_by: Option, /// Columns for grouping/partitioning (from PARTITION BY clause) pub partition_by: Vec, + /// Layer orientation (None = auto-detect from scales) + /// Used for geoms with implicit orientation (bar, histogram, boxplot, etc.) + #[serde(skip_serializing_if = "Option::is_none")] + pub orientation: Option, /// Key for this layer's data in the datamap (set during execution). /// Defaults to `None`. Set to `__ggsql_layer___` during execution, /// but may point to another layer's data when queries are deduplicated. @@ -88,6 +98,7 @@ impl Layer { filter: None, order_by: None, partition_by: Vec::new(), + orientation: None, data_key: None, adjusted_width: None, } @@ -158,8 +169,31 @@ impl Layer { } /// Check if this layer has the required aesthetics for its geom + /// + /// Positional aesthetics (pos1*, pos2*) are validated bidirectionally: + /// requirements using pos1/pos2 slots can be satisfied by either axis assignment. + /// For example, requiring `pos1`, `pos2min`, `pos2max` accepts either: + /// - `x`, `ymin`, `ymax` (slot1→x-axis, slot2→y-axis) + /// - `y`, `xmin`, `xmax` (slot1→y-axis, slot2→x-axis) pub fn validate_required_aesthetics(&self) -> std::result::Result<(), String> { - for aesthetic in self.geom.aesthetics().required() { + use crate::plot::aesthetic::parse_positional; + + let required = self.geom.aesthetics().required(); + + // Separate positional (with parsed slot/suffix) and non-positional requirements + let mut positional_reqs: Vec<(&str, u8, &str)> = Vec::new(); // (name, slot, suffix) + let mut other_reqs: Vec<&str> = Vec::new(); + + for aesthetic in &required { + if let Some((slot, suffix)) = parse_positional(aesthetic) { + positional_reqs.push((aesthetic, slot, suffix)); + } else { + other_reqs.push(aesthetic); + } + } + + // Validate non-positional requirements directly + for aesthetic in &other_reqs { if !self.mappings.contains_key(aesthetic) { return Err(format!( "Geom '{}' requires aesthetic '{}' but it was not provided", @@ -168,6 +202,31 @@ impl Layer { } } + // Validate positional requirements bidirectionally + // Try both slot assignments: (1→1, 2→2) and (1→2, 2→1) + if !positional_reqs.is_empty() { + let identity_ok = positional_reqs + .iter() + .all(|(name, _, _)| self.mappings.contains_key(name)); + + let swapped_ok = positional_reqs.iter().all(|(_, slot, suffix)| { + let new_slot = if *slot == 1 { 2 } else { 1 }; + let remapped = format!("pos{}{}", new_slot, suffix); + self.mappings.contains_key(&remapped) + }); + + if !identity_ok && !swapped_ok { + let (missing, _, _) = positional_reqs + .iter() + .find(|(name, _, _)| !self.mappings.contains_key(name)) + .unwrap(); + return Err(format!( + "Geom '{}' requires aesthetic '{}' (or its bidirectional equivalent) but it was not provided", + self.geom, missing + )); + } + } + Ok(()) } diff --git a/src/plot/layer/orientation.rs b/src/plot/layer/orientation.rs new file mode 100644 index 00000000..272ee387 --- /dev/null +++ b/src/plot/layer/orientation.rs @@ -0,0 +1,706 @@ +//! Layer orientation detection and mapping flipping. +//! +//! This module provides orientation detection for geoms with implicit orientation +//! (bar, histogram, boxplot, violin, density, ribbon) and handles flipping positional +//! aesthetic mappings before stat computation. +//! +//! # Orientation +//! +//! Some geoms have a "main axis" (categorical/domain axis) and a "value axis": +//! - Bar: main axis = categories, value axis = bar height +//! - Histogram: main axis = bins, value axis = count +//! - Boxplot: main axis = groups, value axis = distribution +//! - Ribbon: main axis = domain (e.g., time), value axis = range (min/max) +//! +//! Orientation describes how the layer's main axis aligns with the coordinate's +//! primary axis (pos1): +//! - **Aligned**: main axis = pos1 (vertical bars, x-axis bins) +//! - **Transposed**: main axis = pos2 (horizontal bars, y-axis bins) +//! +//! # Auto-Detection +//! +//! Orientation is auto-detected from scale types: +//! - For two-axis geoms (bar, boxplot): if pos1 is continuous and pos2 is discrete → Transposed +//! - For single-axis geoms (histogram, density): if pos2 has a scale but pos1 doesn't → Transposed +//! +//! # Explicit Override +//! +//! Users can set `SETTING orientation => 'transposed'` (or `'horizontal'`) or +//! `orientation => 'aligned'` (or `'vertical'`) to override auto-detection. + +use serde::{Deserialize, Serialize}; + +use super::geom::GeomType; +use super::Layer; +use crate::plot::scale::ScaleTypeKind; +use crate::plot::{Mappings, Scale}; + +/// Layer orientation for geoms with implicit direction. +/// +/// - **Aligned**: Layer's main axis aligns with coord's primary axis (pos1) +/// - **Transposed**: Layer's main axis aligns with coord's secondary axis (pos2) +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum Orientation { + /// Aligned orientation: layer's main axis = pos1 (vertical bars, x-axis bins) + Aligned, + /// Transposed orientation: layer's main axis = pos2 (horizontal bars, y-axis bins) + Transposed, +} + +impl std::fmt::Display for Orientation { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Orientation::Aligned => write!(f, "aligned"), + Orientation::Transposed => write!(f, "transposed"), + } + } +} + +impl std::str::FromStr for Orientation { + type Err = String; + + fn from_str(s: &str) -> Result { + match s.to_lowercase().as_str() { + "aligned" | "vertical" => Ok(Orientation::Aligned), + "transposed" | "horizontal" => Ok(Orientation::Transposed), + _ => Err(format!( + "Invalid orientation '{}'. Valid values: aligned, transposed, vertical, horizontal", + s + )), + } + } +} + +/// Determine effective orientation for a layer. +/// +/// If the layer has an explicit orientation set (from SETTING), use that. +/// Otherwise, auto-detect from scales for geoms with implicit orientation. +/// Geoms without implicit orientation always return Aligned. +pub fn resolve_orientation(layer: &Layer, scales: &[Scale]) -> Orientation { + // Explicit orientation always wins + if let Some(orientation) = layer.orientation { + return orientation; + } + + // Only auto-detect for geoms with implicit orientation + if !geom_has_implicit_orientation(&layer.geom.geom_type()) { + return Orientation::Aligned; + } + + detect_from_scales( + scales, + &layer.geom.geom_type(), + &layer.mappings, + &layer.remappings, + ) +} + +/// Check if a geom type supports orientation auto-detection. +/// +/// Returns true for geoms with inherent orientation assumptions: +/// - Bar, Histogram, Boxplot, Violin, Density +/// +/// Returns false for geoms without inherent orientation: +/// - Point, Line, Path, Area, etc. +pub fn geom_has_implicit_orientation(geom: &GeomType) -> bool { + matches!( + geom, + GeomType::Bar + | GeomType::Histogram + | GeomType::Boxplot + | GeomType::Violin + | GeomType::Density + | GeomType::Ribbon + ) +} + +/// Detect orientation from scales, mappings, and remappings. +/// +/// Applies unified rules in order: +/// +/// 0. **Remapping without mapping**: If no positional mappings exist but remappings +/// target a positional axis, the remapping target is the value axis: +/// - Remapping to pos1 only → Transposed (pos1 is value axis, main axis must be pos2) +/// - Remapping to pos2 only → Aligned (pos2 is value axis, main axis is pos1) +/// +/// 1. **Single scale present**: The present scale defines the primary axis +/// - Only pos1 → Primary +/// - Only pos2 → Secondary +/// +/// 2. **Both continuous**: The axis with range mappings is secondary (value axis) +/// - pos1 has range mappings → Secondary +/// - pos2 has range mappings (or neither) → Primary (default) +/// +/// 3. **Mixed types**: The discrete scale is the primary (domain) axis +/// - pos1 discrete, pos2 continuous → Primary +/// - pos1 continuous, pos2 discrete → Secondary +/// +/// 4. **Default**: Primary +fn detect_from_scales( + scales: &[Scale], + _geom: &GeomType, + mappings: &Mappings, + remappings: &Mappings, +) -> Orientation { + // Check for positional mappings + let has_pos1_mapping = mappings.contains_key("pos1"); + let has_pos2_mapping = mappings.contains_key("pos2"); + + // Rule 0: Remapping without mapping - remapping target is the value axis + if !has_pos1_mapping && !has_pos2_mapping { + let has_pos1_remapping = remappings.contains_key("pos1"); + let has_pos2_remapping = remappings.contains_key("pos2"); + + if has_pos1_remapping && !has_pos2_remapping { + return Orientation::Transposed; + } + if has_pos2_remapping && !has_pos1_remapping { + return Orientation::Aligned; + } + } + + let pos1_scale = scales.iter().find(|s| s.aesthetic == "pos1"); + let pos2_scale = scales.iter().find(|s| s.aesthetic == "pos2"); + + let has_pos1 = pos1_scale.is_some(); + let has_pos2 = pos2_scale.is_some(); + + // Rule 1: Single scale present - that axis is primary + if has_pos2 && !has_pos1 { + return Orientation::Transposed; + } + if has_pos1 && !has_pos2 { + return Orientation::Aligned; + } + + // Both scales present + let pos1_continuous = pos1_scale.is_some_and(is_continuous_scale); + let pos2_continuous = pos2_scale.is_some_and(is_continuous_scale); + + // Rule 2: Both continuous - range mapping axis is secondary + // Range mappings include min/max pairs and primary/end pairs + if pos1_continuous && pos2_continuous { + let has_pos1_range = mappings.contains_key("pos1min") + || mappings.contains_key("pos1max") + || mappings.contains_key("pos1end"); + let has_pos2_range = mappings.contains_key("pos2min") + || mappings.contains_key("pos2max") + || mappings.contains_key("pos2end"); + + if has_pos1_range && !has_pos2_range { + return Orientation::Transposed; + } + return Orientation::Aligned; + } + + // Rule 3: Mixed types - discrete axis is primary + let pos1_discrete = pos1_scale.is_some_and(is_discrete_scale); + let pos2_discrete = pos2_scale.is_some_and(is_discrete_scale); + + if pos1_continuous && pos2_discrete { + return Orientation::Transposed; + } + if pos1_discrete && pos2_continuous { + return Orientation::Aligned; + } + + // Default + Orientation::Aligned +} + +/// Check if a scale is continuous (numeric/temporal). +fn is_continuous_scale(scale: &Scale) -> bool { + scale + .scale_type + .as_ref() + .is_some_and(|st| st.scale_type_kind() == ScaleTypeKind::Continuous) +} + +/// Check if a scale is discrete (categorical/ordinal). +fn is_discrete_scale(scale: &Scale) -> bool { + scale.scale_type.as_ref().is_some_and(|st| { + matches!( + st.scale_type_kind(), + ScaleTypeKind::Discrete | ScaleTypeKind::Ordinal + ) + }) +} + +/// Swap positional aesthetic pairs in layer mappings. +/// +/// Swaps the following pairs: +/// - pos1 ↔ pos2 +/// - pos1min ↔ pos2min +/// - pos1max ↔ pos2max +/// - pos1end ↔ pos2end +/// - pos1offset ↔ pos2offset +/// +/// This is called before stat transforms for Secondary orientation layers, +/// so stats always see "standard" orientation. After stat transforms, +/// this is called again to flip back to the correct output positions. +pub fn flip_mappings(layer: &mut Layer) { + let pairs = [ + ("pos1", "pos2"), + ("pos1min", "pos2min"), + ("pos1max", "pos2max"), + ("pos1end", "pos2end"), + ("pos1offset", "pos2offset"), + ]; + + for (a, b) in pairs { + let val_a = layer.mappings.aesthetics.remove(a); + let val_b = layer.mappings.aesthetics.remove(b); + + if let Some(v) = val_a { + layer.mappings.aesthetics.insert(b.to_string(), v); + } + if let Some(v) = val_b { + layer.mappings.aesthetics.insert(a.to_string(), v); + } + } +} + +/// Swap positional aesthetic pairs in remappings. +/// +/// Same as flip_mappings but for remappings (stat output mappings). +pub fn flip_remappings(layer: &mut Layer) { + let pairs = [ + ("pos1", "pos2"), + ("pos1min", "pos2min"), + ("pos1max", "pos2max"), + ("pos1end", "pos2end"), + ("pos1offset", "pos2offset"), + ]; + + for (a, b) in pairs { + let val_a = layer.remappings.aesthetics.remove(a); + let val_b = layer.remappings.aesthetics.remove(b); + + if let Some(v) = val_a { + layer.remappings.aesthetics.insert(b.to_string(), v); + } + if let Some(v) = val_b { + layer.remappings.aesthetics.insert(a.to_string(), v); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::plot::{AestheticValue, Geom, ScaleType}; + + #[test] + fn test_orientation_from_str() { + assert_eq!( + "aligned".parse::().unwrap(), + Orientation::Aligned + ); + assert_eq!( + "vertical".parse::().unwrap(), + Orientation::Aligned + ); + assert_eq!( + "transposed".parse::().unwrap(), + Orientation::Transposed + ); + assert_eq!( + "horizontal".parse::().unwrap(), + Orientation::Transposed + ); + assert_eq!( + "TRANSPOSED".parse::().unwrap(), + Orientation::Transposed + ); + assert!("invalid".parse::().is_err()); + } + + #[test] + fn test_orientation_display() { + assert_eq!(Orientation::Aligned.to_string(), "aligned"); + assert_eq!(Orientation::Transposed.to_string(), "transposed"); + } + + #[test] + fn test_geom_has_implicit_orientation() { + assert!(geom_has_implicit_orientation(&GeomType::Bar)); + assert!(geom_has_implicit_orientation(&GeomType::Histogram)); + assert!(geom_has_implicit_orientation(&GeomType::Boxplot)); + assert!(geom_has_implicit_orientation(&GeomType::Violin)); + assert!(geom_has_implicit_orientation(&GeomType::Density)); + assert!(geom_has_implicit_orientation(&GeomType::Ribbon)); + + assert!(!geom_has_implicit_orientation(&GeomType::Point)); + assert!(!geom_has_implicit_orientation(&GeomType::Line)); + assert!(!geom_has_implicit_orientation(&GeomType::Path)); + assert!(!geom_has_implicit_orientation(&GeomType::Area)); + } + + #[test] + fn test_resolve_orientation_explicit() { + let mut layer = Layer::new(Geom::bar()); + layer.orientation = Some(Orientation::Transposed); + + let scales = vec![]; + assert_eq!( + resolve_orientation(&layer, &scales), + Orientation::Transposed + ); + } + + #[test] + fn test_resolve_orientation_no_implicit() { + // Point geom has no implicit orientation + let layer = Layer::new(Geom::point()); + let scales = vec![]; + assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + } + + #[test] + fn test_resolve_orientation_histogram_default() { + // Histogram with pos1 scale → Aligned + let layer = Layer::new(Geom::histogram()); + let mut scale = Scale::new("pos1"); + scale.scale_type = Some(ScaleType::continuous()); + let scales = vec![scale]; + + assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + } + + #[test] + fn test_resolve_orientation_histogram_horizontal() { + // Histogram with only pos2 scale → Transposed + let layer = Layer::new(Geom::histogram()); + let mut scale = Scale::new("pos2"); + scale.scale_type = Some(ScaleType::continuous()); + let scales = vec![scale]; + + assert_eq!( + resolve_orientation(&layer, &scales), + Orientation::Transposed + ); + } + + #[test] + fn test_resolve_orientation_bar_horizontal() { + // Bar with pos1 continuous, pos2 discrete → Transposed + let layer = Layer::new(Geom::bar()); + let mut scale1 = Scale::new("pos1"); + scale1.scale_type = Some(ScaleType::continuous()); + let mut scale2 = Scale::new("pos2"); + scale2.scale_type = Some(ScaleType::discrete()); + let scales = vec![scale1, scale2]; + + assert_eq!( + resolve_orientation(&layer, &scales), + Orientation::Transposed + ); + } + + #[test] + fn test_resolve_orientation_bar_vertical() { + // Bar with pos1 discrete, pos2 continuous → Aligned + let layer = Layer::new(Geom::bar()); + let mut scale1 = Scale::new("pos1"); + scale1.scale_type = Some(ScaleType::discrete()); + let mut scale2 = Scale::new("pos2"); + scale2.scale_type = Some(ScaleType::continuous()); + let scales = vec![scale1, scale2]; + + assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + } + + #[test] + fn test_flip_mappings() { + let mut layer = Layer::new(Geom::bar()); + layer.mappings.insert( + "pos1".to_string(), + AestheticValue::standard_column("category".to_string()), + ); + layer.mappings.insert( + "pos2".to_string(), + AestheticValue::standard_column("value".to_string()), + ); + layer.mappings.insert( + "pos1end".to_string(), + AestheticValue::standard_column("x2".to_string()), + ); + + flip_mappings(&mut layer); + + // pos1 ↔ pos2 + assert_eq!( + layer.mappings.get("pos2").unwrap().column_name(), + Some("category") + ); + assert_eq!( + layer.mappings.get("pos1").unwrap().column_name(), + Some("value") + ); + // pos1end → pos2end + assert_eq!( + layer.mappings.get("pos2end").unwrap().column_name(), + Some("x2") + ); + assert!(layer.mappings.get("pos1end").is_none()); + } + + #[test] + fn test_flip_mappings_empty() { + let mut layer = Layer::new(Geom::point()); + // No crash with empty mappings + flip_mappings(&mut layer); + assert!(layer.mappings.aesthetics.is_empty()); + } + + #[test] + fn test_flip_mappings_partial() { + let mut layer = Layer::new(Geom::bar()); + // Only pos1 mapped + layer.mappings.insert( + "pos1".to_string(), + AestheticValue::standard_column("x".to_string()), + ); + + flip_mappings(&mut layer); + + // pos1 moves to pos2 + assert!(layer.mappings.get("pos1").is_none()); + assert_eq!(layer.mappings.get("pos2").unwrap().column_name(), Some("x")); + } + + #[test] + fn test_resolve_orientation_ribbon_both_continuous_pos2_range() { + // Ribbon with both continuous scales and pos2 range → Aligned + let mut layer = Layer::new(Geom::ribbon()); + layer.mappings.insert( + "pos1".to_string(), + AestheticValue::standard_column("x".to_string()), + ); + layer.mappings.insert( + "pos2min".to_string(), + AestheticValue::standard_column("ymin".to_string()), + ); + layer.mappings.insert( + "pos2max".to_string(), + AestheticValue::standard_column("ymax".to_string()), + ); + + let mut scale1 = Scale::new("pos1"); + scale1.scale_type = Some(ScaleType::continuous()); + let mut scale2 = Scale::new("pos2"); + scale2.scale_type = Some(ScaleType::continuous()); + let scales = vec![scale1, scale2]; + + assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + } + + #[test] + fn test_resolve_orientation_ribbon_both_continuous_pos1_range() { + // Ribbon with both continuous scales and pos1 range → Secondary + let mut layer = Layer::new(Geom::ribbon()); + layer.mappings.insert( + "pos2".to_string(), + AestheticValue::standard_column("y".to_string()), + ); + layer.mappings.insert( + "pos1min".to_string(), + AestheticValue::standard_column("xmin".to_string()), + ); + layer.mappings.insert( + "pos1max".to_string(), + AestheticValue::standard_column("xmax".to_string()), + ); + + let mut scale1 = Scale::new("pos1"); + scale1.scale_type = Some(ScaleType::continuous()); + let mut scale2 = Scale::new("pos2"); + scale2.scale_type = Some(ScaleType::continuous()); + let scales = vec![scale1, scale2]; + + assert_eq!( + resolve_orientation(&layer, &scales), + Orientation::Transposed + ); + } + + #[test] + fn test_resolve_orientation_ribbon_pos1_continuous_pos2_discrete() { + // Ribbon with pos1 continuous, pos2 discrete → Secondary + let mut layer = Layer::new(Geom::ribbon()); + layer.mappings.insert( + "pos1".to_string(), + AestheticValue::standard_column("value".to_string()), + ); + layer.mappings.insert( + "pos2".to_string(), + AestheticValue::standard_column("category".to_string()), + ); + + let mut scale1 = Scale::new("pos1"); + scale1.scale_type = Some(ScaleType::continuous()); + let mut scale2 = Scale::new("pos2"); + scale2.scale_type = Some(ScaleType::discrete()); + let scales = vec![scale1, scale2]; + + assert_eq!( + resolve_orientation(&layer, &scales), + Orientation::Transposed + ); + } + + #[test] + fn test_resolve_orientation_ribbon_pos1_discrete_pos2_continuous() { + // Ribbon with pos1 discrete, pos2 continuous → Primary + let mut layer = Layer::new(Geom::ribbon()); + layer.mappings.insert( + "pos1".to_string(), + AestheticValue::standard_column("category".to_string()), + ); + layer.mappings.insert( + "pos2".to_string(), + AestheticValue::standard_column("value".to_string()), + ); + + let mut scale1 = Scale::new("pos1"); + scale1.scale_type = Some(ScaleType::discrete()); + let mut scale2 = Scale::new("pos2"); + scale2.scale_type = Some(ScaleType::continuous()); + let scales = vec![scale1, scale2]; + + assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + } + + #[test] + fn test_resolve_orientation_ribbon_pos1_range_with_scales() { + // Ribbon with pos2 mapping and pos1 range (xmin/xmax) with continuous scales → Transposed + // This covers: DRAW ribbon MAPPING Date AS y, Temp AS xmax, 0.0 AS xmin + // Rule 2: Both continuous, pos1 has range → Transposed + let mut layer = Layer::new(Geom::ribbon()); + layer.mappings.insert( + "pos2".to_string(), + AestheticValue::standard_column("Date".to_string()), + ); + layer.mappings.insert( + "pos1min".to_string(), + AestheticValue::Literal(crate::plot::ParameterValue::Number(0.0)), + ); + layer.mappings.insert( + "pos1max".to_string(), + AestheticValue::standard_column("Temp".to_string()), + ); + + // Scales are created and typed by execute pipeline + let mut scale1 = Scale::new("pos1"); + scale1.scale_type = Some(ScaleType::continuous()); + let mut scale2 = Scale::new("pos2"); + scale2.scale_type = Some(ScaleType::continuous()); + let scales = vec![scale1, scale2]; + + assert_eq!( + resolve_orientation(&layer, &scales), + Orientation::Transposed + ); + } + + #[test] + fn test_resolve_orientation_ribbon_pos2_range_with_scales() { + // Ribbon with pos1 mapping and pos2 range (ymin/ymax) with continuous scales → Aligned + // This covers: DRAW ribbon MAPPING Date AS x, Temp AS ymax, 0.0 AS ymin + // Rule 2: Both continuous, pos2 has range (or neither) → Aligned + let mut layer = Layer::new(Geom::ribbon()); + layer.mappings.insert( + "pos1".to_string(), + AestheticValue::standard_column("Date".to_string()), + ); + layer.mappings.insert( + "pos2min".to_string(), + AestheticValue::Literal(crate::plot::ParameterValue::Number(0.0)), + ); + layer.mappings.insert( + "pos2max".to_string(), + AestheticValue::standard_column("Temp".to_string()), + ); + + // Scales are created and typed by execute pipeline + let mut scale1 = Scale::new("pos1"); + scale1.scale_type = Some(ScaleType::continuous()); + let mut scale2 = Scale::new("pos2"); + scale2.scale_type = Some(ScaleType::continuous()); + let scales = vec![scale1, scale2]; + + assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + } + + #[test] + fn test_resolve_orientation_remapping_to_pos1() { + // Bar with no mappings but remapping to pos1 → Transposed + // This covers: VISUALISE FROM data DRAW bar REMAPPING proportion AS x + let mut layer = Layer::new(Geom::bar()); + layer.remappings.insert( + "pos1".to_string(), + AestheticValue::standard_column("proportion".to_string()), + ); + + let scales = vec![]; + assert_eq!( + resolve_orientation(&layer, &scales), + Orientation::Transposed + ); + } + + #[test] + fn test_resolve_orientation_remapping_to_pos2() { + // Bar with no mappings but remapping to pos2 → Aligned (default) + let mut layer = Layer::new(Geom::bar()); + layer.remappings.insert( + "pos2".to_string(), + AestheticValue::standard_column("count".to_string()), + ); + + let scales = vec![]; + assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + } + + #[test] + fn test_resolve_orientation_remapping_both_axes() { + // Bar with remappings to both axes → falls through to default (Aligned) + let mut layer = Layer::new(Geom::bar()); + layer.remappings.insert( + "pos1".to_string(), + AestheticValue::standard_column("x_val".to_string()), + ); + layer.remappings.insert( + "pos2".to_string(), + AestheticValue::standard_column("y_val".to_string()), + ); + + let scales = vec![]; + assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + } + + #[test] + fn test_resolve_orientation_mapping_overrides_remapping() { + // Bar with pos1 mapping AND pos1 remapping → mapping takes precedence + // The remapping rule only applies when NO positional mappings exist + let mut layer = Layer::new(Geom::bar()); + layer.mappings.insert( + "pos1".to_string(), + AestheticValue::standard_column("category".to_string()), + ); + layer.remappings.insert( + "pos1".to_string(), + AestheticValue::standard_column("proportion".to_string()), + ); + + // With pos1 discrete scale → Aligned (normal rule 3) + let mut scale1 = Scale::new("pos1"); + scale1.scale_type = Some(ScaleType::discrete()); + let mut scale2 = Scale::new("pos2"); + scale2.scale_type = Some(ScaleType::continuous()); + let scales = vec![scale1, scale2]; + + assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + } +} diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index f2bfedd0..eda4e2fc 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -191,6 +191,7 @@ pub trait GeomRenderer: Send + Sync { df: &DataFrame, _data_key: &str, binned_columns: &HashMap>, + _layer: &Layer, _context: &RenderContext, ) -> Result { let values = if binned_columns.is_empty() { @@ -270,26 +271,39 @@ impl GeomRenderer for BarRenderer { layer: &Layer, _context: &RenderContext, ) -> Result<()> { + use crate::plot::layer::Orientation; let width = match layer.parameters.get("width") { Some(ParameterValue::Number(w)) => *w, _ => 0.9, }; - // For dodged bars, use expression-based width with the adjusted width - // For non-dodged bars, use band-relative width - let width_value = if let Some(adjusted) = layer.adjusted_width { + // For horizontal bars, use "height" for band size; for vertical, use "width" + let is_horizontal = layer.orientation == Some(Orientation::Transposed); + + // For dodged bars, use expression-based size with the adjusted width + // For non-dodged bars, use band-relative size + let size_value = if let Some(adjusted) = layer.adjusted_width { // Use bandwidth expression for dodged bars - json!({"expr": format!("bandwidth('x') * {}", adjusted)}) + let axis = if is_horizontal { "y" } else { "x" }; + json!({"expr": format!("bandwidth('{}') * {}", axis, adjusted)}) } else { json!({"band": width}) }; - layer_spec["mark"] = json!({ - "type": "bar", - "width": width_value, - "align": "center", - "clip": true - }); + layer_spec["mark"] = if is_horizontal { + json!({ + "type": "bar", + "height": size_value, + "clip": true + }) + } else { + json!({ + "type": "bar", + "width": size_value, + "align": "center", + "clip": true + }) + }; Ok(()) } } @@ -392,6 +406,7 @@ impl GeomRenderer for LinearRenderer { df: &DataFrame, _data_key: &str, _binned_columns: &HashMap>, + _layer: &Layer, _context: &RenderContext, ) -> Result { // Just convert DataFrame to JSON values @@ -496,22 +511,37 @@ impl GeomRenderer for LinearRenderer { // Ribbon Renderer // ============================================================================= -/// Renderer for ribbon geom - remaps ymin/ymax to y/y2 +/// Renderer for ribbon geom - remaps ymin/ymax to y/y2 and preserves data order pub struct RibbonRenderer; impl GeomRenderer for RibbonRenderer { fn modify_encoding( &self, encoding: &mut Map, - _layer: &Layer, + layer: &Layer, _context: &RenderContext, ) -> Result<()> { - if let Some(ymax) = encoding.remove("ymax") { - encoding.insert("y".to_string(), ymax); + use crate::plot::layer::Orientation; + + let is_horizontal = layer.orientation == Some(Orientation::Transposed); + + // Remap min/max to primary/secondary based on orientation: + // - Aligned (vertical): ymax→y, ymin→y2 + // - Transposed (horizontal): xmax→x, xmin→x2 + let (max_key, min_key, target, target2) = if is_horizontal { + ("xmax", "xmin", "x", "x2") + } else { + ("ymax", "ymin", "y", "y2") + }; + + if let Some(max_val) = encoding.remove(max_key) { + encoding.insert(target.to_string(), max_val); } - if let Some(ymin) = encoding.remove("ymin") { - encoding.insert("y2".to_string(), ymin); + if let Some(min_val) = encoding.remove(min_key) { + encoding.insert(target2.to_string(), min_val); } + + // Note: Don't add order encoding for area marks - it interferes with rendering Ok(()) } } @@ -566,21 +596,37 @@ impl GeomRenderer for ViolinRenderer { fn modify_spec( &self, layer_spec: &mut Value, - _layer: &Layer, + layer: &Layer, _context: &RenderContext, ) -> Result<()> { + use crate::plot::layer::Orientation; layer_spec["mark"] = json!({ "type": "line", "filled": true }); let offset_col = naming::aesthetic_column("offset"); + // It'll be implemented as an offset. + let violin_offset = format!("[datum.{offset}, -datum.{offset}]", offset = offset_col); + + // Read orientation from layer (already resolved during execution) + let is_horizontal = layer.orientation == Some(Orientation::Transposed); + + // Continuous axis column for order calculation: + // - Vertical: pos2 (y-axis has continuous density values) + // - Horizontal: pos1 (x-axis has continuous density values) + let continuous_col = if is_horizontal { + naming::aesthetic_column("pos1") + } else { + naming::aesthetic_column("pos2") + }; + // We use an order calculation to create a proper closed shape. - // Right side (+ offset), sort by -y (top -> bottom) - // Left side (- offset), sort by +y (bottom -> top) + // Right side (+ offset), sort by -continuous (top -> bottom) + // Left side (- offset), sort by +continuous (bottom -> top) let calc_order = format!( - "datum.__violin_offset > 0 ? -datum.{y} : datum.{y}", - y = naming::aesthetic_column("pos2") + "datum.__violin_offset > 0 ? -datum.{} : datum.{}", + continuous_col, continuous_col ); // Filter threshold to trim very low density regions (removes thin tails) @@ -595,9 +641,6 @@ impl GeomRenderer for ViolinRenderer { .cloned() .unwrap_or_default(); - // Mirror the offset on both sides (offset is already scaled by post_process) - let violin_offset = format!("[datum.{offset}, -datum.{offset}]", offset = offset_col); - // Check if pos1offset exists (from dodging) - we'll combine it with violin offset let pos1offset_col = naming::aesthetic_column("pos1offset"); @@ -638,41 +681,51 @@ impl GeomRenderer for ViolinRenderer { fn modify_encoding( &self, encoding: &mut Map, - _layer: &Layer, + layer: &Layer, _context: &RenderContext, ) -> Result<()> { - // Ensure x is in detail encoding to create separate violins per x category + use crate::plot::layer::Orientation; + + // Read orientation from layer (already resolved during execution) + let is_horizontal = layer.orientation == Some(Orientation::Transposed); + + // Categorical axis for detail encoding: + // - Vertical: x channel (categorical groups on x-axis) + // - Horizontal: y channel (categorical groups on y-axis) + let categorical_channel = if is_horizontal { "y" } else { "x" }; + + // Ensure categorical field is in detail encoding to create separate violins per category // This is needed because line marks with filled:true require detail to create separate paths - let x_field = encoding - .get("x") + let categorical_field = encoding + .get(categorical_channel) .and_then(|x| x.get("field")) .and_then(|f| f.as_str()) .map(|s| s.to_string()); - if let Some(x_field) = x_field { + if let Some(cat_field) = categorical_field { match encoding.get_mut("detail") { Some(detail) if detail.is_object() => { - // Single field object - check if it's already x, otherwise convert to array - if detail.get("field").and_then(|f| f.as_str()) != Some(&x_field) { + // Single field object - check if it's already the categorical field, otherwise convert to array + if detail.get("field").and_then(|f| f.as_str()) != Some(&cat_field) { let existing = detail.clone(); - *detail = json!([existing, {"field": x_field, "type": "nominal"}]); + *detail = json!([existing, {"field": cat_field, "type": "nominal"}]); } } Some(detail) if detail.is_array() => { - // Array - check if x already present, add if not + // Array - check if categorical field already present, add if not let arr = detail.as_array_mut().unwrap(); - let has_x = arr + let has_cat = arr .iter() - .any(|d| d.get("field").and_then(|f| f.as_str()) == Some(&x_field)); - if !has_x { - arr.push(json!({"field": x_field, "type": "nominal"})); + .any(|d| d.get("field").and_then(|f| f.as_str()) == Some(&cat_field)); + if !has_cat { + arr.push(json!({"field": cat_field, "type": "nominal"})); } } None => { - // No detail encoding - add it with x field + // No detail encoding - add it with categorical field encoding.insert( "detail".to_string(), - json!({"field": x_field, "type": "nominal"}), + json!({"field": cat_field, "type": "nominal"}), ); } _ => {} @@ -703,8 +756,12 @@ impl GeomRenderer for ViolinRenderer { } } + // Offset channel: + // - Vertical: xOffset (offsets left/right from category) + // - Horizontal: yOffset (offsets up/down from category) + let offset_channel = if is_horizontal { "yOffset" } else { "xOffset" }; encoding.insert( - "xOffset".to_string(), + offset_channel.to_string(), json!({ "field": "__final_offset", "type": "quantitative", @@ -907,19 +964,37 @@ impl BoxplotRenderer { base_key: &str, has_outliers: bool, ) -> Result> { + use crate::plot::layer::Orientation; + let mut layers: Vec = Vec::new(); - let value_col = naming::aesthetic_column("pos2"); - let value2_col = naming::aesthetic_column("pos2end"); + // Read orientation from layer (already resolved during execution) + let is_horizontal = layer.orientation == Some(Orientation::Transposed); + + // Value columns depend on orientation (after DataFrame column flip): + // - Vertical: values in pos2/pos2end (no flip) + // - Horizontal: values in pos1/pos1end (was pos2/pos2end before flip) + let (value_col, value2_col) = if is_horizontal { + ( + naming::aesthetic_column("pos1"), + naming::aesthetic_column("pos1end"), + ) + } else { + ( + naming::aesthetic_column("pos2"), + naming::aesthetic_column("pos2end"), + ) + }; - let x_col = layer + // Validate x aesthetic exists (required for boxplot) + layer .mappings .get("pos1") .and_then(|x| x.column_name()) .ok_or_else(|| { GgsqlError::WriterError("Boxplot requires 'x' aesthetic mapping".to_string()) })?; - // Validate y aesthetic exists (not used directly but required for boxplot) + // Validate y aesthetic exists (required for boxplot) layer .mappings .get("pos2") @@ -928,8 +1003,6 @@ impl BoxplotRenderer { GgsqlError::WriterError("Boxplot requires 'y' aesthetic mapping".to_string()) })?; - // Set orientation - let is_horizontal = x_col == value_col; let value_var1 = if is_horizontal { "x" } else { "y" }; let value_var2 = if is_horizontal { "x2" } else { "y2" }; @@ -945,8 +1018,9 @@ impl BoxplotRenderer { // For dodged boxplots, use expression-based width with adjusted_width // For non-dodged boxplots, use band-relative width + let axis = if is_horizontal { "y" } else { "x" }; let width_value = if let Some(adjusted) = layer.adjusted_width { - json!({"expr": format!("bandwidth('x') * {}", adjusted)}) + json!({"expr": format!("bandwidth('{}') * {}", axis, adjusted)}) } else { json!({"band": base_width}) }; @@ -1087,6 +1161,7 @@ impl GeomRenderer for BoxplotRenderer { df: &DataFrame, _data_key: &str, binned_columns: &HashMap>, + _layer: &Layer, _context: &RenderContext, ) -> Result { let (components, has_outliers) = self.prepare_components(df, binned_columns)?; diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index f635663b..b0931c56 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -100,7 +100,7 @@ fn prepare_layer_data( let renderer = get_renderer(&layer.geom); // Prepare data using the renderer (handles both standard and composite cases) - let prepared = renderer.prepare_data(df, data_key, binned_columns, &context)?; + let prepared = renderer.prepare_data(df, data_key, binned_columns, layer, &context)?; // Add data to individual datasets based on prepared type match &prepared { From b134dbae38cffa459433c7cb6bece88881938dea Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Tue, 10 Mar 2026 21:11:35 +0100 Subject: [PATCH 02/16] apply last'ish changes from old PR --- src/plot/layer/geom/area.rs | 26 ++++++- src/plot/layer/geom/line.rs | 29 ++++++-- src/plot/layer/geom/mod.rs | 3 + src/plot/layer/geom/ribbon.rs | 29 ++++++-- src/plot/layer/geom/types.rs | 36 +++++++--- src/reader/data.rs | 125 ++++++++++++++++++++++++++++++++++ src/writer/vegalite/data.rs | 9 +++ src/writer/vegalite/layer.rs | 43 ++++++++++-- 8 files changed, 272 insertions(+), 28 deletions(-) diff --git a/src/plot/layer/geom/area.rs b/src/plot/layer/geom/area.rs index 91d02cb7..f1a6359b 100644 --- a/src/plot/layer/geom/area.rs +++ b/src/plot/layer/geom/area.rs @@ -1,8 +1,9 @@ //! Area geom implementation use crate::plot::{types::DefaultAestheticValue, DefaultParam, DefaultParamValue}; +use crate::{naming, Mappings}; -use super::{DefaultAesthetics, GeomTrait, GeomType}; +use super::{DefaultAesthetics, GeomTrait, GeomType, StatResult}; /// Area geom - filled area charts #[derive(Debug, Clone, Copy)] @@ -37,6 +38,29 @@ impl GeomTrait for Area { default: DefaultParamValue::String("stack"), }] } + + fn needs_stat_transform(&self, _aesthetics: &Mappings) -> bool { + true + } + + fn apply_stat_transform( + &self, + query: &str, + _schema: &crate::plot::Schema, + _aesthetics: &Mappings, + _group_by: &[String], + _parameters: &std::collections::HashMap, + _execute_query: &dyn Fn(&str) -> crate::Result, + ) -> crate::Result { + // Area geom needs ordering by pos1 (domain axis) for proper rendering + let order_col = naming::aesthetic_column("pos1"); + Ok(StatResult::Transformed { + query: format!("{} ORDER BY \"{}\"", query, order_col), + stat_columns: vec![], + dummy_columns: vec![], + consumed_aesthetics: vec![], + }) + } } impl std::fmt::Display for Area { diff --git a/src/plot/layer/geom/line.rs b/src/plot/layer/geom/line.rs index 7f283676..53dfe962 100644 --- a/src/plot/layer/geom/line.rs +++ b/src/plot/layer/geom/line.rs @@ -1,7 +1,8 @@ //! Line geom implementation -use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType}; +use super::{DefaultAesthetics, GeomTrait, GeomType, StatResult}; use crate::plot::types::DefaultAestheticValue; +use crate::{naming, Mappings}; /// Line geom - line charts with connected points #[derive(Debug, Clone, Copy)] @@ -25,11 +26,27 @@ impl GeomTrait for Line { } } - fn default_params(&self) -> &'static [DefaultParam] { - &[DefaultParam { - name: "position", - default: DefaultParamValue::String("identity"), - }] + fn needs_stat_transform(&self, _aesthetics: &Mappings) -> bool { + true + } + + fn apply_stat_transform( + &self, + query: &str, + _schema: &crate::plot::Schema, + _aesthetics: &Mappings, + _group_by: &[String], + _parameters: &std::collections::HashMap, + _execute_query: &dyn Fn(&str) -> crate::Result, + ) -> crate::Result { + // Line geom needs ordering by pos1 (domain axis) for proper rendering + let order_col = naming::aesthetic_column("pos1"); + Ok(StatResult::Transformed { + query: format!("{} ORDER BY \"{}\"", query, order_col), + stat_columns: vec![], + dummy_columns: vec![], + consumed_aesthetics: vec![], + }) } } diff --git a/src/plot/layer/geom/mod.rs b/src/plot/layer/geom/mod.rs index f24196f8..ced5b978 100644 --- a/src/plot/layer/geom/mod.rs +++ b/src/plot/layer/geom/mod.rs @@ -221,11 +221,14 @@ pub trait GeomTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { /// Returns valid parameter names for SETTING clause. /// /// Combines supported aesthetics with non-aesthetic parameters. + /// Includes "orientation" for all geoms (explicit override for auto-detection). fn valid_settings(&self) -> Vec<&'static str> { let mut valid: Vec<&'static str> = self.aesthetics().supported(); for param in self.default_params() { valid.push(param.name); } + // All geoms accept orientation parameter for explicit override + valid.push("orientation"); valid } } diff --git a/src/plot/layer/geom/ribbon.rs b/src/plot/layer/geom/ribbon.rs index 0bcd4d8b..37ca967a 100644 --- a/src/plot/layer/geom/ribbon.rs +++ b/src/plot/layer/geom/ribbon.rs @@ -1,7 +1,8 @@ //! Ribbon geom implementation -use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType}; +use super::{DefaultAesthetics, GeomTrait, GeomType, StatResult}; use crate::plot::types::DefaultAestheticValue; +use crate::{naming, Mappings}; /// Ribbon geom - confidence bands and ranges #[derive(Debug, Clone, Copy)] @@ -27,11 +28,27 @@ impl GeomTrait for Ribbon { } } - fn default_params(&self) -> &'static [DefaultParam] { - &[DefaultParam { - name: "position", - default: DefaultParamValue::String("identity"), - }] + fn needs_stat_transform(&self, _aesthetics: &Mappings) -> bool { + true + } + + fn apply_stat_transform( + &self, + query: &str, + _schema: &crate::plot::Schema, + _aesthetics: &Mappings, + _group_by: &[String], + _parameters: &std::collections::HashMap, + _execute_query: &dyn Fn(&str) -> crate::Result, + ) -> crate::Result { + // Ribbon geom needs ordering by pos1 (domain axis) for proper rendering + let order_col = naming::aesthetic_column("pos1"); + Ok(StatResult::Transformed { + query: format!("{} ORDER BY \"{}\"", query, order_col), + stat_columns: vec![], + dummy_columns: vec![], + consumed_aesthetics: vec![], + }) } } diff --git a/src/plot/layer/geom/types.rs b/src/plot/layer/geom/types.rs index cb0632f4..5e04f386 100644 --- a/src/plot/layer/geom/types.rs +++ b/src/plot/layer/geom/types.rs @@ -2,6 +2,7 @@ //! //! These types are used by all geom implementations and are shared across the module. +use crate::plot::aesthetic::parse_positional; use crate::{plot::types::DefaultAestheticValue, Mappings}; // Re-export shared types from the central location @@ -27,16 +28,14 @@ impl DefaultAesthetics { } /// Get supported aesthetic names (excludes Delayed, for MAPPING validation) + /// + /// Returns the literal names from defaults. For bidirectional positional checking, + /// use `is_supported()` which handles pos1/pos2 equivalence. pub fn supported(&self) -> Vec<&'static str> { self.defaults .iter() - .filter_map(|(name, value)| { - if !matches!(value, DefaultAestheticValue::Delayed) { - Some(*name) - } else { - None - } - }) + .filter(|(_, value)| !matches!(value, DefaultAestheticValue::Delayed)) + .map(|(name, _)| *name) .collect() } @@ -55,10 +54,29 @@ impl DefaultAesthetics { } /// Check if an aesthetic is supported (not Delayed) + /// + /// Positional aesthetics are bidirectional: if pos1* is supported, pos2* is also + /// considered supported (and vice versa). pub fn is_supported(&self, name: &str) -> bool { - self.defaults + // Check for direct match first + let direct_match = self + .defaults .iter() - .any(|(n, value)| *n == name && !matches!(value, DefaultAestheticValue::Delayed)) + .any(|(n, value)| !matches!(value, DefaultAestheticValue::Delayed) && *n == name); + if direct_match { + return true; + } + + // Check for bidirectional positional match + if let Some((slot, suffix)) = parse_positional(name) { + let other_slot = if slot == 1 { 2 } else { 1 }; + let equivalent = format!("pos{}{}", other_slot, suffix); + return self.defaults.iter().any(|(n, value)| { + !matches!(value, DefaultAestheticValue::Delayed) && *n == equivalent + }); + } + + false } /// Check if an aesthetic exists (including Delayed) diff --git a/src/reader/data.rs b/src/reader/data.rs index fac5639a..cb53df53 100644 --- a/src/reader/data.rs +++ b/src/reader/data.rs @@ -368,6 +368,131 @@ mod duckdb_tests { assert!(dataframe.column("__ggsql_aes_pos1__").is_ok()); assert!(dataframe.column("__ggsql_aes_pos2__").is_ok()); } + + #[test] + fn test_ribbon_transposed_orientation() { + use crate::naming; + use crate::plot::layer::orientation::Orientation; + + let reader = + crate::reader::DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + + // Ribbon with y as domain axis and xmin/xmax as value range (transposed) + let query = + "VISUALISE FROM ggsql:airquality DRAW ribbon MAPPING Day AS y, Temp AS xmax, 0.0 AS xmin"; + let result = crate::execute::prepare_data_with_reader(query, &reader); + + // Debug: print the error if any + if let Err(ref e) = result { + eprintln!("Error: {:?}", e); + } + + let result = result.unwrap(); + + // Debug: print orientation and scales + let layer = &result.specs[0].layers[0]; + eprintln!("Layer orientation: {:?}", layer.orientation); + eprintln!( + "Scales: {:?}", + result.specs[0] + .scales + .iter() + .map(|s| (&s.aesthetic, &s.scale_type)) + .collect::>() + ); + eprintln!( + "Layer mappings: {:?}", + layer.mappings.aesthetics.keys().collect::>() + ); + + // Check orientation was detected correctly + assert_eq!( + layer.orientation, + Some(Orientation::Transposed), + "Should detect Transposed orientation" + ); + + let dataframe = result.data.get(&naming::layer_key(0)).unwrap(); + + // The flip-back restores user's original axis assignment: + // After flip-back: + // - pos2 = y (user's domain axis = Date/Day) + // - pos1min = xmin (user's value range min = 0.0) + // - pos1max = xmax (user's value range max = Temp) + // The orientation flag tells the writer how to map to x/y. + let cols: Vec<_> = dataframe.get_column_names().into_iter().collect(); + eprintln!("Columns: {:?}", cols); + + assert!( + dataframe.column("__ggsql_aes_pos2__").is_ok(), + "Should have pos2 (domain axis), got columns: {:?}", + cols + ); + assert!( + dataframe.column("__ggsql_aes_pos1min__").is_ok(), + "Should have pos1min (value range min), got columns: {:?}", + cols + ); + assert!( + dataframe.column("__ggsql_aes_pos1max__").is_ok(), + "Should have pos1max (value range max), got columns: {:?}", + cols + ); + } + + #[test] + fn test_ribbon_transposed_vegalite_encoding() { + use crate::reader::Reader; + use crate::writer::{VegaLiteWriter, Writer}; + + let reader = + crate::reader::DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + + // Ribbon with y as domain axis and xmin/xmax as value range (transposed) + let query = + "VISUALISE FROM ggsql:airquality DRAW ribbon MAPPING Day AS y, Temp AS xmax, 0.0 AS xmin"; + let spec = reader.execute(query).unwrap(); + + let writer = VegaLiteWriter::new(); + let json_str = writer.render(&spec).unwrap(); + let vl_spec: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + + // For transposed ribbon, the encoding should have: + // - y: domain axis (Day) + // - x: value range max (Temp via xmax) + // - x2: value range min (0.0 via xmin) + // The encoding is inside layer[0] since VegaLite uses layered structure + let encoding = &vl_spec["layer"][0]["encoding"]; + assert!( + encoding.get("y").is_some(), + "Should have y encoding for domain axis" + ); + assert!( + encoding.get("x").is_some(), + "Should have x encoding for value max" + ); + assert!( + encoding.get("x2").is_some(), + "Should have x2 encoding for value min" + ); + // Should NOT have ymax/ymin/xmax/xmin (these should be remapped to x/x2/y/y2) + assert!( + encoding.get("ymax").is_none(), + "Should not have ymax encoding" + ); + assert!( + encoding.get("ymin").is_none(), + "Should not have ymin encoding" + ); + assert!( + encoding.get("xmax").is_none(), + "Should not have xmax encoding" + ); + assert!( + encoding.get("xmin").is_none(), + "Should not have xmin encoding" + ); + } } #[cfg(feature = "builtin-data")] diff --git a/src/writer/vegalite/data.rs b/src/writer/vegalite/data.rs index 4b1c89e3..22f6c77b 100644 --- a/src/writer/vegalite/data.rs +++ b/src/writer/vegalite/data.rs @@ -4,6 +4,9 @@ //! including temporal type handling and binned data transformations. use crate::plot::scale::ScaleTypeKind; + +/// Column name for row index (used to preserve data order in Vega-Lite) +pub(super) const ROW_INDEX_COLUMN: &str = "__ggsql_row_index__"; // ArrayElement is used for temporal parsing #[allow(unused_imports)] use crate::plot::ArrayElement; @@ -389,7 +392,9 @@ pub(super) fn unify_datasets(datasets: &Map) -> Result // 2. For each dataset, for each row: // - Include all columns (null for missing) // - Add __ggsql_source__ field with dataset key + // - Add __ggsql_row_index__ field for order preservation let mut unified = Vec::new(); + let mut row_index: usize = 0; for (key, values) in datasets { if let Some(arr) = values.as_array() { for row in arr { @@ -405,6 +410,10 @@ pub(super) fn unify_datasets(datasets: &Map) -> Result // Add source identifier new_row.insert(naming::SOURCE_COLUMN.to_string(), json!(key)); + // Add row index for order preservation (used by line/path/polygon geoms) + new_row.insert(ROW_INDEX_COLUMN.to_string(), json!(row_index)); + row_index += 1; + unified.push(Value::Object(new_row)); } } diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index eda4e2fc..8d5131c3 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -16,7 +16,7 @@ use serde_json::{json, Map, Value}; use std::any::Any; use std::collections::HashMap; -use super::data::{dataframe_to_values, dataframe_to_values_with_bins}; +use super::data::{dataframe_to_values, dataframe_to_values_with_bins, ROW_INDEX_COLUMN}; // ============================================================================= // Basic Geom Utilities @@ -322,8 +322,35 @@ impl GeomRenderer for PathRenderer { _layer: &Layer, _context: &RenderContext, ) -> Result<()> { - // Use the natural data order - encoding.insert("order".to_string(), json!({"value": Value::Null})); + // Use row index field to preserve natural data order + encoding.insert( + "order".to_string(), + json!({"field": ROW_INDEX_COLUMN, "type": "quantitative"}), + ); + Ok(()) + } +} + +// ============================================================================= +// Line Renderer +// ============================================================================= + +/// Renderer for line geom - preserves data order for correct line rendering +pub struct LineRenderer; + +impl GeomRenderer for LineRenderer { + fn modify_encoding( + &self, + encoding: &mut Map, + _layer: &Layer, + _context: &RenderContext, + ) -> Result<()> { + // Use row index field to preserve natural data order + // (we've already ordered in SQL via apply_stat_transform) + encoding.insert( + "order".to_string(), + json!({"field": ROW_INDEX_COLUMN, "type": "quantitative"}), + ); Ok(()) } } @@ -566,8 +593,11 @@ impl GeomRenderer for PolygonRenderer { if let Some(color) = encoding.remove("color") { encoding.insert("fill".to_string(), color); } - // Use the natural data order - encoding.insert("order".to_string(), json!({"value": Value::Null})); + // Use row index field to preserve natural data order + encoding.insert( + "order".to_string(), + json!({"field": ROW_INDEX_COLUMN, "type": "quantitative"}), + ); Ok(()) } @@ -1206,6 +1236,7 @@ impl GeomRenderer for BoxplotRenderer { pub fn get_renderer(geom: &Geom) -> Box { match geom.geom_type() { GeomType::Path => Box::new(PathRenderer), + GeomType::Line => Box::new(LineRenderer), GeomType::Bar => Box::new(BarRenderer), GeomType::Ribbon => Box::new(RibbonRenderer), GeomType::Polygon => Box::new(PolygonRenderer), @@ -1215,7 +1246,7 @@ pub fn get_renderer(geom: &Geom) -> Box { GeomType::Linear => Box::new(LinearRenderer), GeomType::ErrorBar => Box::new(ErrorBarRenderer), GeomType::Rule => Box::new(RuleRenderer), - // All other geoms (Point, Line, Area, Density, Tile, etc.) use the default renderer + // All other geoms (Point, Area, Density, Tile, etc.) use the default renderer _ => Box::new(DefaultRenderer), } } From b598c52aeb584677ed09103c777564cfccba1ca1 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Tue, 10 Mar 2026 22:02:02 +0100 Subject: [PATCH 03/16] Fix bug in stacking --- src/plot/layer/position/stack.rs | 35 +++++++++++++++++++++++++------- src/reader/mod.rs | 34 +++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/src/plot/layer/position/stack.rs b/src/plot/layer/position/stack.rs index 17925e4f..02eb1f3c 100644 --- a/src/plot/layer/position/stack.rs +++ b/src/plot/layer/position/stack.rs @@ -100,16 +100,20 @@ fn is_axis_stackable(spec: &Plot, layer: &Layer, df: &DataFrame, axis: &str) -> return false; } + // Helper to check if an aesthetic is defined in mappings OR remappings + // (remappings come from default_remappings for stat geoms like bar) + let has_aesthetic = |aes: &str| -> bool { + layer.mappings.contains_key(aes) || layer.remappings.contains_key(aes) + }; + // Check for pos/posend pair (e.g., pos2/pos2end) let end_aesthetic = format!("{}end", axis); - let has_end_pair = - layer.mappings.contains_key(axis) && layer.mappings.contains_key(&end_aesthetic); + let has_end_pair = has_aesthetic(axis) && has_aesthetic(&end_aesthetic); // Check for posmin/posmax pair (e.g., pos2min/pos2max) let min_aesthetic = format!("{}min", axis); let max_aesthetic = format!("{}max", axis); - let has_minmax_pair = - layer.mappings.contains_key(&min_aesthetic) && layer.mappings.contains_key(&max_aesthetic); + let has_minmax_pair = has_aesthetic(&min_aesthetic) && has_aesthetic(&max_aesthetic); // Check that each row has zero baseline in one of the range columns if has_end_pair { @@ -134,13 +138,30 @@ fn has_zero_baseline_per_row(df: &DataFrame, col_a: &str, col_b: &str) -> bool { let (Ok(a), Ok(b)) = (df.column(col_a), df.column(col_b)) else { return false; }; - let (Ok(a_f64), Ok(b_f64)) = (a.f64(), b.f64()) else { + + // Cast columns to f64 for comparison - handle both Int64 and Float64 sources + let Ok(a_casted) = a.cast(&polars::datatypes::DataType::Float64) else { + return false; + }; + let Ok(b_casted) = b.cast(&polars::datatypes::DataType::Float64) else { + return false; + }; + + let Ok(a_vals) = a_casted.f64() else { + return false; + }; + let Ok(b_vals) = b_casted.f64() else { return false; }; + + // Collect values to avoid borrow issues + let a_vec: Vec> = a_vals.into_iter().collect(); + let b_vec: Vec> = b_vals.into_iter().collect(); + // For each row, either a or b must be 0 - a_f64 + a_vec .into_iter() - .zip(b_f64) + .zip(b_vec) .all(|(a_val, b_val)| a_val == Some(0.0) || b_val == Some(0.0)) } diff --git a/src/reader/mod.rs b/src/reader/mod.rs index 186c0fe4..7e9ca14b 100644 --- a/src/reader/mod.rs +++ b/src/reader/mod.rs @@ -969,6 +969,40 @@ mod tests { ); } + #[test] + fn test_stacked_bar_chart_dummy_x() { + // Test stacked bar chart with no x mapping (dummy x column) + // This is the case where only fill is mapped: all bars at same x position should stack + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + let query = r#" + VISUALISE FROM ggsql:penguins + DRAW bar MAPPING species AS fill + "#; + + let spec = reader.execute(query).unwrap(); + let writer = VegaLiteWriter::new(); + let result = writer.render(&spec).unwrap(); + + let json: serde_json::Value = serde_json::from_str(&result).unwrap(); + let layer = json["layer"].as_array().unwrap().first().unwrap(); + + // Verify y and y2 encodings exist (stacked bars use y/y2 for range) + let encoding = &layer["encoding"]; + assert!(encoding["y"].is_object(), "Should have y encoding"); + assert!( + encoding["y2"].is_object(), + "Should have y2 encoding for stacked bars with dummy x. Encoding: {}", + serde_json::to_string_pretty(encoding).unwrap() + ); + + // Verify Vega-Lite stacking is disabled (we handle it ourselves) + assert!( + encoding["y"]["stack"].is_null(), + "y encoding should have stack: null to disable VL stacking. Got: {}", + serde_json::to_string_pretty(&encoding["y"]).unwrap() + ); + } + #[test] fn test_dodged_bar_chart() { // Test dodged bar chart via position => 'dodge' From 4d8239e03f544d06f0753e94ef569ef47a83720d Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 11 Mar 2026 09:34:13 +0100 Subject: [PATCH 04/16] Fixes related to polar and positioning --- doc/syntax/coord/polar.qmd | 6 +- src/execute/scale.rs | 105 ++++++++++++++++++++++++---- src/parser/builder.rs | 4 +- src/plot/layer/orientation.rs | 37 ++++++++-- src/plot/projection/coord/mod.rs | 6 +- src/plot/projection/coord/polar.rs | 2 +- src/plot/projection/resolve.rs | 2 +- src/plot/projection/types.rs | 2 +- src/plot/scale/scale_type/binned.rs | 2 + src/plot/scale/scale_type/mod.rs | 48 ++++++++++++- src/reader/mod.rs | 36 +++++++++- src/writer/vegalite/encoding.rs | 4 +- src/writer/vegalite/mod.rs | 26 +++---- src/writer/vegalite/projection.rs | 63 +++++++++-------- 14 files changed, 265 insertions(+), 78 deletions(-) diff --git a/doc/syntax/coord/polar.qmd b/doc/syntax/coord/polar.qmd index ffa8ae51..75adaab0 100644 --- a/doc/syntax/coord/polar.qmd +++ b/doc/syntax/coord/polar.qmd @@ -7,8 +7,8 @@ The polar coordinate system interprets its primary aesthetic as the angular posi ## Default aesthetics The polar coordinate system has the following default positional aesthetics which will be used if no others have been provided: -* **Primary**: `theta` (angular position) -* **Secondary**: `radius` (distance from center) +* **Primary**: `radius` (distance from center) +* **Secondary**: `theta` (angular position) Users can provide their own aesthetic names if needed. For example, if using `x` and `y` aesthetics: @@ -16,7 +16,7 @@ Users can provide their own aesthetic names if needed. For example, if using `x` PROJECT y, x TO polar ``` -This maps `y` to theta (angle) and `x` to radius. This is useful when converting from a cartesian coordinate system without editing all the mappings. +This maps `y` to radius and `x` to theta (angle). This is useful when converting from a cartesian coordinate system without editing all the mappings. ## Settings * `clip`: Should data be removed if it appears outside the bounds of the coordinate system. Defaults to `true` diff --git a/src/execute/scale.rs b/src/execute/scale.rs index b8c05f6f..6df0bb6f 100644 --- a/src/execute/scale.rs +++ b/src/execute/scale.rs @@ -93,31 +93,41 @@ pub fn create_missing_scales_post_stat( } } - // Find aesthetics that don't have scales yet + // Find aesthetics that don't have scales yet and create them let existing_scales: HashSet = spec.scales.iter().map(|s| s.aesthetic.clone()).collect(); - // Create scales for new aesthetics and infer their types from data for aesthetic in current_aesthetics { if !existing_scales.contains(&aesthetic) { let mut scale = Scale::new(&aesthetic); if !gets_default_scale(&aesthetic) { scale.scale_type = Some(ScaleType::identity()); - } else { - // Infer scale type from column data - let column_refs = - find_columns_for_aesthetic(&spec.layers, &aesthetic, data_map, &aesthetic_ctx); - if !column_refs.is_empty() { - scale.scale_type = Some(ScaleType::infer_for_aesthetic( - column_refs[0].dtype(), - &aesthetic, - )); - } } spec.scales.push(scale); } } + // Infer types for all scales that don't have scale_type set + // This handles both newly created scales and user-specified scales like + // `SCALE y SETTING expand` where the type wasn't explicitly specified. + // Position adjustments (stack, dodge) need scale types to determine axes. + for scale in &mut spec.scales { + if scale.scale_type.is_none() && gets_default_scale(&scale.aesthetic) { + let column_refs = find_columns_for_aesthetic( + &spec.layers, + &scale.aesthetic, + data_map, + &aesthetic_ctx, + ); + if !column_refs.is_empty() { + scale.scale_type = Some(ScaleType::infer_for_aesthetic( + column_refs[0].dtype(), + &scale.aesthetic, + )); + } + } + } + Ok(()) } @@ -944,6 +954,9 @@ pub fn resolve_scales(spec: &mut Plot, data_map: &mut HashMap let aesthetic_ctx = spec.get_aesthetic_context(); + // Get coord_kind from projection (for disabling expansion on polar theta) + let coord_kind = spec.project.as_ref().map(|p| p.coord.coord_kind()); + for idx in 0..spec.scales.len() { // Clone aesthetic to avoid borrow issues with find_columns_for_aesthetic let aesthetic = spec.scales[idx].aesthetic.clone(); @@ -991,7 +1004,8 @@ pub fn resolve_scales(spec: &mut Plot, data_map: &mut HashMap let use_discrete_range = st.uses_discrete_input_range(); // Build context from actual data columns - let context = ScaleDataContext::from_columns(&column_refs, use_discrete_range); + let mut context = ScaleDataContext::from_columns(&column_refs, use_discrete_range); + context.coord_kind = coord_kind; // Use unified resolve method (includes resolve_output_range) st.resolve(&mut spec.scales[idx], &context, &aesthetic) @@ -1651,4 +1665,69 @@ mod tests { _ => panic!("Expected Number elements"), } } + + #[test] + fn test_resolve_scales_polar_theta_no_expansion() { + use crate::plot::projection::{Coord, Projection}; + use polars::prelude::*; + + // Create a Plot with a polar projection + let mut spec = Plot::new(); + let coord = Coord::polar(); + let aesthetics = coord + .positional_aesthetic_names() + .iter() + .map(|s| s.to_string()) + .collect(); + spec.project = Some(Projection { + coord, + aesthetics, + properties: std::collections::HashMap::new(), + }); + + // Create scale for pos2 (theta in polar) without explicit expand + let scale = crate::plot::Scale::new("pos2"); + spec.scales.push(scale); + + // Add a layer with pos2 mapping + let layer = Layer::new(Geom::bar()) + .with_aesthetic("pos2".to_string(), AestheticValue::standard_column("value")); + spec.layers.push(layer); + + // Create data with numeric values + let df = df! { + "value" => &[10.0f64, 20.0, 30.0] + } + .unwrap(); + + let mut data_map = HashMap::new(); + data_map.insert(naming::layer_key(0), df); + + // Verify projection is set correctly + assert!(spec.project.is_some(), "project should be set"); + let coord_kind = spec.project.as_ref().map(|p| p.coord.coord_kind()); + assert_eq!( + coord_kind, + Some(crate::plot::CoordKind::Polar), + "coord_kind should be Polar" + ); + + // Resolve scales + resolve_scales(&mut spec, &mut data_map).unwrap(); + + // Check that no expansion was applied for polar theta + // Without expansion, range should be exactly [10.0, 30.0] + let scale = &spec.scales[0]; + assert!(scale.input_range.is_some()); + + let range = scale.input_range.as_ref().unwrap(); + assert_eq!(range.len(), 2); + match (&range[0], &range[1]) { + (ArrayElement::Number(min), ArrayElement::Number(max)) => { + assert_eq!(*min, 10.0, "min should be 10.0 (no expansion)"); + assert_eq!(*max, 30.0, "max should be 30.0 (no expansion)"); + } + _ => panic!("Expected Number elements"), + } + } } diff --git a/src/parser/builder.rs b/src/parser/builder.rs index 4647e671..1dd58b37 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -1346,7 +1346,7 @@ mod tests { let project = specs[0].project.as_ref().unwrap(); assert_eq!( project.aesthetics, - vec!["theta".to_string(), "radius".to_string()] + vec!["radius".to_string(), "theta".to_string()] ); } @@ -3386,7 +3386,7 @@ mod tests { // Should infer polar projection let project = specs[0].project.as_ref().unwrap(); assert_eq!(project.coord.coord_kind(), CoordKind::Polar); - assert_eq!(project.aesthetics, vec!["theta", "radius"]); + assert_eq!(project.aesthetics, vec!["radius", "theta"]); } #[test] diff --git a/src/plot/layer/orientation.rs b/src/plot/layer/orientation.rs index 272ee387..61597b14 100644 --- a/src/plot/layer/orientation.rs +++ b/src/plot/layer/orientation.rs @@ -167,11 +167,16 @@ fn detect_from_scales( let has_pos2 = pos2_scale.is_some(); // Rule 1: Single scale present - that axis is primary - if has_pos2 && !has_pos1 { - return Orientation::Transposed; - } - if has_pos1 && !has_pos2 { - return Orientation::Aligned; + // Only apply when there are explicit positional mappings; otherwise the user + // is just customizing a scale (e.g., SCALE y SETTING expand) without intending + // to change orientation. The geom's default_remappings will define orientation. + if has_pos1_mapping || has_pos2_mapping { + if has_pos2 && !has_pos1 { + return Orientation::Transposed; + } + if has_pos1 && !has_pos2 { + return Orientation::Aligned; + } } // Both scales present @@ -370,8 +375,12 @@ mod tests { #[test] fn test_resolve_orientation_histogram_horizontal() { - // Histogram with only pos2 scale → Transposed - let layer = Layer::new(Geom::histogram()); + // Histogram with pos2 mapping (y binned) → Transposed + // Real-world: `VISUALISE y AS y DRAW histogram` or `MAPPING y AS pos2` + let mut layer = Layer::new(Geom::histogram()); + layer + .mappings + .insert("pos2", AestheticValue::standard_column("y_col")); let mut scale = Scale::new("pos2"); scale.scale_type = Some(ScaleType::continuous()); let scales = vec![scale]; @@ -382,6 +391,20 @@ mod tests { ); } + #[test] + fn test_resolve_orientation_scale_only_no_flip() { + // Scale specification without positional mapping shouldn't flip orientation + // Real-world: `VISUALISE FROM data DRAW bar SCALE y SETTING expand => [...]` + // The bar stat will produce pos1=category, pos2=count → should stay Aligned + let layer = Layer::new(Geom::bar()); + let mut scale = Scale::new("pos2"); + scale.scale_type = Some(ScaleType::continuous()); + let scales = vec![scale]; + + // Without positional mappings, scale existence doesn't imply orientation + assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + } + #[test] fn test_resolve_orientation_bar_horizontal() { // Bar with pos1 continuous, pos2 discrete → Transposed diff --git a/src/plot/projection/coord/mod.rs b/src/plot/projection/coord/mod.rs index c6ca9cb4..27023036 100644 --- a/src/plot/projection/coord/mod.rs +++ b/src/plot/projection/coord/mod.rs @@ -67,7 +67,7 @@ pub trait CoordTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { /// Primary positional aesthetic names for this coord. /// /// Returns the user-facing positional aesthetic names. - /// e.g., ["x", "y"] for cartesian, ["theta", "radius"] for polar. + /// e.g., ["x", "y"] for cartesian, ["radius", "theta"] for polar. /// /// These names are transformed to internal names (pos1, pos2, etc.) /// early in the pipeline and transformed back for output. @@ -160,7 +160,7 @@ impl Coord { } /// Primary positional aesthetic names for this coord. - /// e.g., ["x", "y"] for cartesian, ["theta", "radius"] for polar. + /// e.g., ["x", "y"] for cartesian, ["radius", "theta"] for polar. pub fn positional_aesthetic_names(&self) -> &'static [&'static str] { self.0.positional_aesthetic_names() } @@ -286,6 +286,6 @@ mod tests { assert_eq!(cartesian.positional_aesthetic_names(), &["x", "y"]); let polar = Coord::polar(); - assert_eq!(polar.positional_aesthetic_names(), &["theta", "radius"]); + assert_eq!(polar.positional_aesthetic_names(), &["radius", "theta"]); } } diff --git a/src/plot/projection/coord/polar.rs b/src/plot/projection/coord/polar.rs index 5781e02d..95ba5000 100644 --- a/src/plot/projection/coord/polar.rs +++ b/src/plot/projection/coord/polar.rs @@ -17,7 +17,7 @@ impl CoordTrait for Polar { } fn positional_aesthetic_names(&self) -> &'static [&'static str] { - &["theta", "radius"] + &["radius", "theta"] } fn default_properties(&self) -> &'static [DefaultParam] { diff --git a/src/plot/projection/resolve.rs b/src/plot/projection/resolve.rs index 54b0b646..24921206 100644 --- a/src/plot/projection/resolve.rs +++ b/src/plot/projection/resolve.rs @@ -221,7 +221,7 @@ mod tests { assert!(inferred.is_some()); let proj = inferred.unwrap(); assert_eq!(proj.coord.coord_kind(), CoordKind::Polar); - assert_eq!(proj.aesthetics, vec!["theta", "radius"]); + assert_eq!(proj.aesthetics, vec!["radius", "theta"]); } #[test] diff --git a/src/plot/projection/types.rs b/src/plot/projection/types.rs index 53644de0..d2cffd1c 100644 --- a/src/plot/projection/types.rs +++ b/src/plot/projection/types.rs @@ -15,7 +15,7 @@ pub struct Projection { pub coord: Coord, /// Positional aesthetic names (resolved: explicit or coord defaults) /// Always populated after building - never empty. - /// e.g., ["x", "y"] for cartesian, ["theta", "radius"] for polar, + /// e.g., ["x", "y"] for cartesian, ["radius", "theta"] for polar, /// or custom names like ["a", "b"] if user specifies them. pub aesthetics: Vec, /// Projection-specific options diff --git a/src/plot/scale/scale_type/binned.rs b/src/plot/scale/scale_type/binned.rs index 40779a90..4d273e66 100644 --- a/src/plot/scale/scale_type/binned.rs +++ b/src/plot/scale/scale_type/binned.rs @@ -1980,6 +1980,7 @@ mod tests { ])), dtype: Some(DataType::Float64), is_discrete: false, + coord_kind: None, }; binned.resolve(&mut scale, &context, "fill").unwrap(); @@ -2036,6 +2037,7 @@ mod tests { ])), dtype: Some(DataType::Float64), is_discrete: false, + coord_kind: None, }; binned.resolve(&mut scale, &context, "fill").unwrap(); diff --git a/src/plot/scale/scale_type/mod.rs b/src/plot/scale/scale_type/mod.rs index 288360a9..99f704eb 100644 --- a/src/plot/scale/scale_type/mod.rs +++ b/src/plot/scale/scale_type/mod.rs @@ -27,6 +27,7 @@ use std::sync::Arc; use super::transform::{Transform, TransformKind}; use crate::plot::aesthetic::{is_facet_aesthetic, is_positional_aesthetic}; +use crate::plot::projection::CoordKind; use crate::plot::types::{DefaultParam, DefaultParamValue}; use crate::plot::{ArrayElement, ColumnInfo, ParameterValue}; @@ -70,6 +71,8 @@ pub struct ScaleDataContext { pub dtype: Option, /// Whether this is discrete data pub is_discrete: bool, + /// Coordinate system type (for disabling expansion on polar theta) + pub coord_kind: Option, } impl ScaleDataContext { @@ -79,6 +82,7 @@ impl ScaleDataContext { range: None, dtype: None, is_discrete: false, + coord_kind: None, } } @@ -122,6 +126,7 @@ impl ScaleDataContext { range, dtype, is_discrete, + coord_kind: None, } } @@ -147,6 +152,7 @@ impl ScaleDataContext { range, dtype, is_discrete, + coord_kind: None, } } @@ -1636,6 +1642,7 @@ pub(crate) fn expand_numeric_range_selective( } /// Get expand factors from properties, using defaults for continuous/temporal scales. +#[allow(dead_code)] pub(crate) fn get_expand_factors(properties: &HashMap) -> (f64, f64) { properties .get("expand") @@ -1643,6 +1650,38 @@ pub(crate) fn get_expand_factors(properties: &HashMap) - .unwrap_or((DEFAULT_EXPAND_MULT, DEFAULT_EXPAND_ADD)) } +/// Get expand factors with aesthetic-aware defaults. +/// +/// For polar coordinates, pos2 (theta) defaults to zero expansion since it's angular/categorical. +/// Users can still explicitly set expand via SETTING if they want expansion. +pub(crate) fn get_expand_factors_for_aesthetic( + properties: &HashMap, + aesthetic: &str, + context: &ScaleDataContext, + user_explicit_expand: bool, +) -> (f64, f64) { + // If user explicitly set expand in their SCALE SETTING, use their value + if user_explicit_expand { + if let Some(expand) = properties.get("expand").and_then(parse_expand_value) { + return expand; + } + } + + // For polar coordinates, pos2 (theta) defaults to zero expansion + // since it's angular/categorical and shouldn't have padding + if context.coord_kind == Some(CoordKind::Polar) && aesthetic == "pos2" { + return (0.0, 0.0); + } + + // Use the resolved (possibly default) expand value for other aesthetics + if let Some(expand) = properties.get("expand").and_then(parse_expand_value) { + return expand; + } + + // Final fallback (should not normally reach here after resolve_properties) + (DEFAULT_EXPAND_MULT, DEFAULT_EXPAND_ADD) +} + /// Clip an input range to a transform's valid domain. /// /// This prevents expansion from producing invalid values for transforms @@ -1704,6 +1743,9 @@ pub(crate) fn resolve_common_steps( context: &ScaleDataContext, aesthetic: &str, ) -> Result { + // Check if user explicitly set expand BEFORE resolve_properties fills in defaults + let user_explicit_expand = scale.properties.contains_key("expand"); + // 1. Resolve properties (fills in defaults, validates) scale.properties = scale_type.resolve_properties(aesthetic, &scale.properties)?; @@ -1739,7 +1781,11 @@ pub(crate) fn resolve_common_steps( // This ensures expansion is calculated on the final range span. // IMPORTANT: Only expand values that were inferred (originally null), not explicit user values. // For example, `FROM [0, null]` should keep min=0 and only expand max. - let (mult, add) = get_expand_factors(&scale.properties); + // + // For polar coordinates, pos2 (theta) defaults to zero expansion since it's angular/categorical. + // Users can still explicitly set expand if they want. + let (mult, add) = + get_expand_factors_for_aesthetic(&scale.properties, aesthetic, context, user_explicit_expand); // Track the original user range to know which values are explicit vs inferred let original_user_range = scale.input_range.clone(); diff --git a/src/reader/mod.rs b/src/reader/mod.rs index 7e9ca14b..728524ea 100644 --- a/src/reader/mod.rs +++ b/src/reader/mod.rs @@ -560,7 +560,7 @@ mod tests { ); } - // Test case 1: PROJECT y, x TO polar (y as pos1→theta, x as pos2→radius) + // Test case 1: PROJECT y, x TO polar (y as pos1→radius, x as pos2→theta) let query1 = r#" SELECT * FROM (VALUES ('A', 10), ('B', 20)) AS t(category, value) VISUALISE value AS y, category AS fill @@ -573,7 +573,7 @@ mod tests { let json1: serde_json::Value = serde_json::from_str(&result1).unwrap(); check_encoding_keys(&json1, "PROJECT y, x TO polar"); - // Test case 2: PROJECT x, y TO polar (x as pos1→theta, y as pos2→radius) + // Test case 2: PROJECT x, y TO polar (x as pos1→radius, y as pos2→theta) let query2 = r#" SELECT * FROM (VALUES ('A', 10), ('B', 20)) AS t(category, value) VISUALISE value AS x, category AS fill @@ -585,7 +585,7 @@ mod tests { let json2: serde_json::Value = serde_json::from_str(&result2).unwrap(); check_encoding_keys(&json2, "PROJECT x, y TO polar"); - // Test case 3: PROJECT TO polar (default theta/radius names) + // Test case 3: PROJECT TO polar (default radius/theta names) let query3 = r#" SELECT * FROM (VALUES ('A', 10), ('B', 20)) AS t(category, value) VISUALISE value AS theta, category AS fill @@ -1003,6 +1003,36 @@ mod tests { ); } + #[test] + fn test_bar_chart_with_expand_setting() { + // Test bar chart with SCALE y SETTING expand - should work even when y is stat-derived + // This tests that: + // 1. Scale type inference works for stat-generated count columns + // 2. Stacking still works (y2 encoding exists) when SCALE y is specified + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + let query = r#" + VISUALISE FROM ggsql:penguins + DRAW bar MAPPING species AS fill + SCALE y SETTING expand => [0.05, 0.05] + "#; + + let spec = reader.execute(query).unwrap(); + let writer = VegaLiteWriter::new(); + let result = writer.render(&spec).unwrap(); + + // Should succeed without "discrete scale does not support SETTING 'expand'" error + let json: serde_json::Value = serde_json::from_str(&result).unwrap(); + let layer = json["layer"].as_array().unwrap().first().unwrap(); + + // Verify stacking works (y2 encoding exists for stacked bars) + let encoding = &layer["encoding"]; + assert!( + encoding["y2"].is_object(), + "Should have y2 encoding for stacked bars. Encoding: {}", + serde_json::to_string_pretty(encoding).unwrap() + ); + } + #[test] fn test_dodged_bar_chart() { // Test dodged bar chart via position => 'dodge' diff --git a/src/writer/vegalite/encoding.rs b/src/writer/vegalite/encoding.rs index e641d5fc..2b82639a 100644 --- a/src/writer/vegalite/encoding.rs +++ b/src/writer/vegalite/encoding.rs @@ -947,7 +947,7 @@ fn build_literal_encoding(aesthetic: &str, lit: &ParameterValue) -> Result Option { let (primary, secondary) = match coord_kind { CoordKind::Cartesian => ("x", "y"), - CoordKind::Polar => ("theta", "radius"), + CoordKind::Polar => ("radius", "theta"), }; // Match internal positional aesthetic patterns diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index b0931c56..a06e863f 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -603,13 +603,13 @@ fn get_free_scales(facet: Option<&crate::plot::Facet>) -> Option<&[crate::plot:: /// /// Maps ggsql free property (boolean array) to Vega-Lite resolve.scale configuration: /// - `[false, false]`: shared scales (Vega-Lite default, no resolve needed) -/// - `[true, false]`: independent pos1 scale (x or theta), shared pos2 scale -/// - `[false, true]`: shared pos1 scale, independent pos2 scale (y or radius) +/// - `[true, false]`: independent pos1 scale (x or radius), shared pos2 scale +/// - `[false, true]`: shared pos1 scale, independent pos2 scale (y or theta) /// - `[true, true]`: independent scales for both axes /// /// The channel names depend on coord_kind: /// - Cartesian: pos1 -> "x", pos2 -> "y" -/// - Polar: pos1 -> "theta", pos2 -> "radius" +/// - Polar: pos1 -> "radius", pos2 -> "theta" fn apply_facet_scale_resolution( vl_spec: &mut Value, properties: &HashMap, @@ -623,7 +623,7 @@ fn apply_facet_scale_resolution( // Determine channel names based on coord kind let (pos1_channel, pos2_channel) = match coord_kind { CoordKind::Cartesian => ("x", "y"), - CoordKind::Polar => ("theta", "radius"), + CoordKind::Polar => ("radius", "theta"), }; // Extract booleans from the array (position-indexed) @@ -1404,36 +1404,36 @@ mod tests { "text" ); - // Test with polar coord kind - internal positional maps to theta/radius + // Test with polar coord kind - internal positional maps to radius/theta // regardless of the context's user-facing names - let polar_ctx = AestheticContext::from_static(&["theta", "radius"], &[]); + let polar_ctx = AestheticContext::from_static(&["radius", "theta"], &[]); assert_eq!( map_aesthetic_name("pos1", &polar_ctx, CoordKind::Polar), - "theta" + "radius" ); assert_eq!( map_aesthetic_name("pos2", &polar_ctx, CoordKind::Polar), - "radius" + "theta" ); assert_eq!( map_aesthetic_name("pos1end", &polar_ctx, CoordKind::Polar), - "theta2" + "radius2" ); assert_eq!( map_aesthetic_name("pos2end", &polar_ctx, CoordKind::Polar), - "radius2" + "theta2" ); // Even with custom positional names (e.g., PROJECT y, x TO polar), - // internal pos1/pos2 should still map to theta/radius for Vega-Lite + // internal pos1/pos2 should still map to radius/theta for Vega-Lite let custom_ctx = AestheticContext::from_static(&["y", "x"], &[]); assert_eq!( map_aesthetic_name("pos1", &custom_ctx, CoordKind::Polar), - "theta" + "radius" ); assert_eq!( map_aesthetic_name("pos2", &custom_ctx, CoordKind::Polar), - "radius" + "theta" ); } diff --git a/src/writer/vegalite/projection.rs b/src/writer/vegalite/projection.rs index cb49722b..4e8e4ee3 100644 --- a/src/writer/vegalite/projection.rs +++ b/src/writer/vegalite/projection.rs @@ -209,13 +209,20 @@ fn apply_polar_angle_range( // Apply angle range to theta encoding if let Some(theta_enc) = enc_obj.get_mut("theta") { if let Some(theta_obj) = theta_enc.as_object_mut() { - // Set the scale range to the specified start/end angles - theta_obj.insert( - "scale".to_string(), - json!({ - "range": [start_radians, end_radians] - }), - ); + // Merge range into existing scale object (preserving domain from expansion) + if let Some(scale_val) = theta_obj.get_mut("scale") { + if let Some(scale_obj) = scale_val.as_object_mut() { + scale_obj.insert("range".to_string(), json!([start_radians, end_radians])); + } + } else { + // No existing scale, create new one with just range + theta_obj.insert( + "scale".to_string(), + json!({ + "range": [start_radians, end_radians] + }), + ); + } } } @@ -237,35 +244,35 @@ fn apply_polar_radius_range(encoding: &mut Value, inner: f64) -> Result<()> { .as_object_mut() .ok_or_else(|| GgsqlError::WriterError("Encoding is not an object".to_string()))?; - // Apply scale range to radius encoding + // Use expressions for proportional sizing + // min(width,height)/2 is the default max radius in Vega-Lite + let inner_expr = format!("min(width,height)/2*{}", inner); + let outer_expr = "min(width,height)/2".to_string(); + let range_value = json!([{"expr": inner_expr}, {"expr": outer_expr}]); + + // Apply scale range to radius encoding (merge with existing scale) if let Some(radius_enc) = enc_obj.get_mut("radius") { if let Some(radius_obj) = radius_enc.as_object_mut() { - // Use expressions for proportional sizing - // min(width,height)/2 is the default max radius in Vega-Lite - let inner_expr = format!("min(width,height)/2*{}", inner); - let outer_expr = "min(width,height)/2".to_string(); - - radius_obj.insert( - "scale".to_string(), - json!({ - "range": [{"expr": inner_expr}, {"expr": outer_expr}] - }), - ); + if let Some(scale_val) = radius_obj.get_mut("scale") { + if let Some(scale_obj) = scale_val.as_object_mut() { + scale_obj.insert("range".to_string(), range_value.clone()); + } + } else { + radius_obj.insert("scale".to_string(), json!({ "range": range_value.clone() })); + } } } // Also apply to radius2 if present (for arc marks) if let Some(radius2_enc) = enc_obj.get_mut("radius2") { if let Some(radius2_obj) = radius2_enc.as_object_mut() { - let inner_expr = format!("min(width,height)/2*{}", inner); - let outer_expr = "min(width,height)/2".to_string(); - - radius2_obj.insert( - "scale".to_string(), - json!({ - "range": [{"expr": inner_expr}, {"expr": outer_expr}] - }), - ); + if let Some(scale_val) = radius2_obj.get_mut("scale") { + if let Some(scale_obj) = scale_val.as_object_mut() { + scale_obj.insert("range".to_string(), range_value.clone()); + } + } else { + radius2_obj.insert("scale".to_string(), json!({ "range": range_value })); + } } } From 54204212835d98620d126aeb3279f8b48b5f0faf Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 11 Mar 2026 10:34:39 +0100 Subject: [PATCH 05/16] make orientation a regular parameter --- src/execute/layer.rs | 9 +- src/execute/mod.rs | 66 ++++++++++-- src/lib.rs | 61 +++++++++++ src/parser/builder.rs | 17 --- src/plot/layer/geom/area.rs | 15 ++- src/plot/layer/geom/line.rs | 10 +- src/plot/layer/geom/linear.rs | 10 +- src/plot/layer/geom/mod.rs | 4 +- src/plot/layer/mod.rs | 9 +- src/plot/layer/orientation.rs | 192 +++++++++++++--------------------- src/reader/data.rs | 8 +- src/writer/vegalite/layer.rs | 43 +++++--- 12 files changed, 262 insertions(+), 182 deletions(-) diff --git a/src/execute/layer.rs b/src/execute/layer.rs index 0419d5af..973d26d5 100644 --- a/src/execute/layer.rs +++ b/src/execute/layer.rs @@ -355,14 +355,19 @@ pub fn apply_layer_transforms( where F: Fn(&str) -> Result, { - use crate::plot::layer::orientation::{flip_mappings, flip_remappings, Orientation}; + use crate::plot::layer::orientation::{flip_mappings, flip_remappings}; // Clone order_by early to avoid borrow conflicts let order_by = layer.order_by.clone(); // Orientation detection and initial flip was already done in mod.rs before // build_layer_base_query. We just check if we need to flip back after stat. - let needs_flip = layer.orientation == Some(Orientation::Transposed); + let needs_flip = layer + .parameters + .get("orientation") + .and_then(|v| v.as_str()) + .map(|s| s == "transposed") + .unwrap_or(false); // Build the aesthetic-named schema for stat transforms // Note: Mappings were already flipped in mod.rs if needed, so schema reflects normalized orientation diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 231bf141..46bd1de3 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -1108,13 +1108,49 @@ pub fn prepare_data_with_reader(query: &str, reader: &R) -> Result(query: &str, reader: &R) -> Result(query: &str, reader: &R) -> Result(query: &str, reader: &R) -> Result = HashSet::new(); for layer in specs[0].layers.iter() { - let needs_flip = - layer.orientation == Some(crate::plot::layer::orientation::Orientation::Transposed); + let needs_flip = layer + .parameters + .get("orientation") + .and_then(|v| v.as_str()) + .map(|s| s == "transposed") + .unwrap_or(false); if needs_flip { if let Some(ref key) = layer.data_key { if flipped_keys.insert(key.clone()) { diff --git a/src/lib.rs b/src/lib.rs index 6e3b8074..61b3ab33 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -861,4 +861,65 @@ mod integration_tests { assert!(!layer0_rows.is_empty(), "Should have layer data rows"); assert_eq!(layer0_rows[0][&stroke_col], "value"); } + + #[test] + fn test_orientation_setting_rejected_with_helpful_error() { + // Test orientation setting behavior for different geom types + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + + // 1. Bar geom (has implicit orientation) - should reject with auto-detection message + let query = r#" + SELECT 'A' as category, 10 as value + VISUALISE + DRAW bar MAPPING category AS x, value AS y SETTING orientation => 'horizontal' + "#; + + let result = execute::prepare_data_with_reader(query, &reader); + match result { + Err(e) => { + let err = e.to_string(); + assert!( + err.contains("orientation is auto-detected"), + "Error should mention auto-detection: {}", + err + ); + assert!(err.contains("bar"), "Error should mention the geom type: {}", err); + } + Ok(_) => panic!("Should reject orientation setting for bar geom"), + } + + // 2. Point geom (symmetrical, no orientation concept) - should reject + let query2 = r#" + SELECT 1 as x, 2 as y + VISUALISE + DRAW point MAPPING x AS x, y AS y SETTING orientation => 'horizontal' + "#; + + let result2 = execute::prepare_data_with_reader(query2, &reader); + match result2 { + Err(e) => { + let err2 = e.to_string(); + assert!( + err2.contains("does not support orientation"), + "Error should mention lack of support: {}", + err2 + ); + } + Ok(_) => panic!("Should reject orientation setting for point geom"), + } + + // 3. Line geom (has orientation in default_params) - should accept + let query3 = r#" + SELECT 1 as x, 2 as y + VISUALISE + DRAW line MAPPING x AS x, y AS y SETTING orientation => 'aligned' + "#; + + let result3 = execute::prepare_data_with_reader(query3, &reader); + assert!( + result3.is_ok(), + "Line geom should accept orientation setting: {:?}", + result3.err() + ); + } } diff --git a/src/parser/builder.rs b/src/parser/builder.rs index 1dd58b37..24b1c4af 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -493,22 +493,6 @@ fn build_layer(node: &Node, source: &SourceTree) -> Result { .unwrap_or_default() }; - // Extract orientation from parameters if present - let orientation = parameters - .remove("orientation") - .map(|v| { - match v { - ParameterValue::String(s) => s - .parse() - .map_err(|e: String| GgsqlError::ParseError(e)), - _ => Err(GgsqlError::ParseError( - "orientation must be a string ('aligned', 'transposed', 'vertical', or 'horizontal')" - .to_string(), - )), - } - }) - .transpose()?; - let mut layer = Layer::new(geom); layer.position = position; layer.mappings = aesthetics; @@ -518,7 +502,6 @@ fn build_layer(node: &Node, source: &SourceTree) -> Result { layer.filter = filter; layer.order_by = order_by; layer.source = layer_source; - layer.orientation = orientation; Ok(layer) } diff --git a/src/plot/layer/geom/area.rs b/src/plot/layer/geom/area.rs index f1a6359b..f7ed555d 100644 --- a/src/plot/layer/geom/area.rs +++ b/src/plot/layer/geom/area.rs @@ -1,5 +1,6 @@ //! Area geom implementation +use crate::plot::layer::orientation::ALIGNED; use crate::plot::{types::DefaultAestheticValue, DefaultParam, DefaultParamValue}; use crate::{naming, Mappings}; @@ -33,10 +34,16 @@ impl GeomTrait for Area { } fn default_params(&self) -> &'static [DefaultParam] { - &[DefaultParam { - name: "position", - default: DefaultParamValue::String("stack"), - }] + &[ + DefaultParam { + name: "position", + default: DefaultParamValue::String("stack"), + }, + DefaultParam { + name: "orientation", + default: DefaultParamValue::String(ALIGNED), + }, + ] } fn needs_stat_transform(&self, _aesthetics: &Mappings) -> bool { diff --git a/src/plot/layer/geom/line.rs b/src/plot/layer/geom/line.rs index 53dfe962..1ce98c20 100644 --- a/src/plot/layer/geom/line.rs +++ b/src/plot/layer/geom/line.rs @@ -1,6 +1,7 @@ //! Line geom implementation -use super::{DefaultAesthetics, GeomTrait, GeomType, StatResult}; +use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType, StatResult}; +use crate::plot::layer::orientation::ALIGNED; use crate::plot::types::DefaultAestheticValue; use crate::{naming, Mappings}; @@ -26,6 +27,13 @@ impl GeomTrait for Line { } } + fn default_params(&self) -> &'static [DefaultParam] { + &[DefaultParam { + name: "orientation", + default: DefaultParamValue::String(ALIGNED), + }] + } + fn needs_stat_transform(&self, _aesthetics: &Mappings) -> bool { true } diff --git a/src/plot/layer/geom/linear.rs b/src/plot/layer/geom/linear.rs index 7569cff1..b56a76b2 100644 --- a/src/plot/layer/geom/linear.rs +++ b/src/plot/layer/geom/linear.rs @@ -1,6 +1,7 @@ //! Linear geom implementation -use super::{DefaultAesthetics, GeomTrait, GeomType}; +use super::{DefaultAesthetics, DefaultParam, DefaultParamValue, GeomTrait, GeomType}; +use crate::plot::layer::orientation::ALIGNED; use crate::plot::types::DefaultAestheticValue; /// Linear geom - lines with coefficient and intercept @@ -24,6 +25,13 @@ impl GeomTrait for Linear { ], } } + + fn default_params(&self) -> &'static [DefaultParam] { + &[DefaultParam { + name: "orientation", + default: DefaultParamValue::String(ALIGNED), + }] + } } impl std::fmt::Display for Linear { diff --git a/src/plot/layer/geom/mod.rs b/src/plot/layer/geom/mod.rs index ced5b978..34d79a88 100644 --- a/src/plot/layer/geom/mod.rs +++ b/src/plot/layer/geom/mod.rs @@ -221,13 +221,13 @@ pub trait GeomTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { /// Returns valid parameter names for SETTING clause. /// /// Combines supported aesthetics with non-aesthetic parameters. - /// Includes "orientation" for all geoms (explicit override for auto-detection). + /// Includes "orientation" which is set internally during execution (not user-settable). fn valid_settings(&self) -> Vec<&'static str> { let mut valid: Vec<&'static str> = self.aesthetics().supported(); for param in self.default_params() { valid.push(param.name); } - // All geoms accept orientation parameter for explicit override + // Internal parameter set during execution (not user-settable via SETTING) valid.push("orientation"); valid } diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index 6f078dc0..b4ed4263 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -15,8 +15,8 @@ pub mod orientation; // Position is a submodule of layer pub mod position; -// Re-export orientation types -pub use orientation::Orientation; +// Re-export orientation functions and constants +pub use orientation::{is_transposed, ALIGNED, TRANSPOSED}; // Re-export geom types for convenience pub use geom::{ @@ -69,10 +69,6 @@ pub struct Layer { pub order_by: Option, /// Columns for grouping/partitioning (from PARTITION BY clause) pub partition_by: Vec, - /// Layer orientation (None = auto-detect from scales) - /// Used for geoms with implicit orientation (bar, histogram, boxplot, etc.) - #[serde(skip_serializing_if = "Option::is_none")] - pub orientation: Option, /// Key for this layer's data in the datamap (set during execution). /// Defaults to `None`. Set to `__ggsql_layer___` during execution, /// but may point to another layer's data when queries are deduplicated. @@ -98,7 +94,6 @@ impl Layer { filter: None, order_by: None, partition_by: Vec::new(), - orientation: None, data_key: None, adjusted_width: None, } diff --git a/src/plot/layer/orientation.rs b/src/plot/layer/orientation.rs index 61597b14..bcd70b81 100644 --- a/src/plot/layer/orientation.rs +++ b/src/plot/layer/orientation.rs @@ -14,78 +14,34 @@ //! //! Orientation describes how the layer's main axis aligns with the coordinate's //! primary axis (pos1): -//! - **Aligned**: main axis = pos1 (vertical bars, x-axis bins) -//! - **Transposed**: main axis = pos2 (horizontal bars, y-axis bins) +//! - **"aligned"**: main axis = pos1 (vertical bars, x-axis bins) +//! - **"transposed"**: main axis = pos2 (horizontal bars, y-axis bins) //! //! # Auto-Detection //! //! Orientation is auto-detected from scale types: -//! - For two-axis geoms (bar, boxplot): if pos1 is continuous and pos2 is discrete → Transposed -//! - For single-axis geoms (histogram, density): if pos2 has a scale but pos1 doesn't → Transposed -//! -//! # Explicit Override -//! -//! Users can set `SETTING orientation => 'transposed'` (or `'horizontal'`) or -//! `orientation => 'aligned'` (or `'vertical'`) to override auto-detection. - -use serde::{Deserialize, Serialize}; +//! - For two-axis geoms (bar, boxplot): if pos1 is continuous and pos2 is discrete → "transposed" +//! - For single-axis geoms (histogram, density): if pos2 has a scale but pos1 doesn't → "transposed" use super::geom::GeomType; use super::Layer; use crate::plot::scale::ScaleTypeKind; use crate::plot::{Mappings, Scale}; -/// Layer orientation for geoms with implicit direction. -/// -/// - **Aligned**: Layer's main axis aligns with coord's primary axis (pos1) -/// - **Transposed**: Layer's main axis aligns with coord's secondary axis (pos2) -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "lowercase")] -pub enum Orientation { - /// Aligned orientation: layer's main axis = pos1 (vertical bars, x-axis bins) - Aligned, - /// Transposed orientation: layer's main axis = pos2 (horizontal bars, y-axis bins) - Transposed, -} - -impl std::fmt::Display for Orientation { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Orientation::Aligned => write!(f, "aligned"), - Orientation::Transposed => write!(f, "transposed"), - } - } -} +/// Orientation value for aligned/vertical orientation. +pub const ALIGNED: &str = "aligned"; -impl std::str::FromStr for Orientation { - type Err = String; - - fn from_str(s: &str) -> Result { - match s.to_lowercase().as_str() { - "aligned" | "vertical" => Ok(Orientation::Aligned), - "transposed" | "horizontal" => Ok(Orientation::Transposed), - _ => Err(format!( - "Invalid orientation '{}'. Valid values: aligned, transposed, vertical, horizontal", - s - )), - } - } -} +/// Orientation value for transposed/horizontal orientation. +pub const TRANSPOSED: &str = "transposed"; /// Determine effective orientation for a layer. /// -/// If the layer has an explicit orientation set (from SETTING), use that. -/// Otherwise, auto-detect from scales for geoms with implicit orientation. -/// Geoms without implicit orientation always return Aligned. -pub fn resolve_orientation(layer: &Layer, scales: &[Scale]) -> Orientation { - // Explicit orientation always wins - if let Some(orientation) = layer.orientation { - return orientation; - } - +/// Auto-detects orientation from scales for geoms with implicit orientation. +/// Geoms without implicit orientation always return "aligned". +pub fn resolve_orientation(layer: &Layer, scales: &[Scale]) -> &'static str { // Only auto-detect for geoms with implicit orientation if !geom_has_implicit_orientation(&layer.geom.geom_type()) { - return Orientation::Aligned; + return ALIGNED; } detect_from_scales( @@ -96,6 +52,13 @@ pub fn resolve_orientation(layer: &Layer, scales: &[Scale]) -> Orientation { ) } +/// Check if a layer is transposed (horizontal orientation). +/// +/// Convenience helper for downstream code that needs to check orientation. +pub fn is_transposed(layer: &Layer, scales: &[Scale]) -> bool { + resolve_orientation(layer, scales) == TRANSPOSED +} + /// Check if a geom type supports orientation auto-detection. /// /// Returns true for geoms with inherent orientation assumptions: @@ -142,7 +105,7 @@ fn detect_from_scales( _geom: &GeomType, mappings: &Mappings, remappings: &Mappings, -) -> Orientation { +) -> &'static str { // Check for positional mappings let has_pos1_mapping = mappings.contains_key("pos1"); let has_pos2_mapping = mappings.contains_key("pos2"); @@ -153,10 +116,10 @@ fn detect_from_scales( let has_pos2_remapping = remappings.contains_key("pos2"); if has_pos1_remapping && !has_pos2_remapping { - return Orientation::Transposed; + return TRANSPOSED; } if has_pos2_remapping && !has_pos1_remapping { - return Orientation::Aligned; + return ALIGNED; } } @@ -172,10 +135,10 @@ fn detect_from_scales( // to change orientation. The geom's default_remappings will define orientation. if has_pos1_mapping || has_pos2_mapping { if has_pos2 && !has_pos1 { - return Orientation::Transposed; + return TRANSPOSED; } if has_pos1 && !has_pos2 { - return Orientation::Aligned; + return ALIGNED; } } @@ -194,9 +157,9 @@ fn detect_from_scales( || mappings.contains_key("pos2end"); if has_pos1_range && !has_pos2_range { - return Orientation::Transposed; + return TRANSPOSED; } - return Orientation::Aligned; + return ALIGNED; } // Rule 3: Mixed types - discrete axis is primary @@ -204,14 +167,14 @@ fn detect_from_scales( let pos2_discrete = pos2_scale.is_some_and(is_discrete_scale); if pos1_continuous && pos2_discrete { - return Orientation::Transposed; + return TRANSPOSED; } if pos1_discrete && pos2_continuous { - return Orientation::Aligned; + return ALIGNED; } // Default - Orientation::Aligned + ALIGNED } /// Check if a scale is continuous (numeric/temporal). @@ -297,34 +260,9 @@ mod tests { use crate::plot::{AestheticValue, Geom, ScaleType}; #[test] - fn test_orientation_from_str() { - assert_eq!( - "aligned".parse::().unwrap(), - Orientation::Aligned - ); - assert_eq!( - "vertical".parse::().unwrap(), - Orientation::Aligned - ); - assert_eq!( - "transposed".parse::().unwrap(), - Orientation::Transposed - ); - assert_eq!( - "horizontal".parse::().unwrap(), - Orientation::Transposed - ); - assert_eq!( - "TRANSPOSED".parse::().unwrap(), - Orientation::Transposed - ); - assert!("invalid".parse::().is_err()); - } - - #[test] - fn test_orientation_display() { - assert_eq!(Orientation::Aligned.to_string(), "aligned"); - assert_eq!(Orientation::Transposed.to_string(), "transposed"); + fn test_orientation_constants() { + assert_eq!(ALIGNED, "aligned"); + assert_eq!(TRANSPOSED, "transposed"); } #[test] @@ -342,24 +280,34 @@ mod tests { assert!(!geom_has_implicit_orientation(&GeomType::Area)); } - #[test] - fn test_resolve_orientation_explicit() { - let mut layer = Layer::new(Geom::bar()); - layer.orientation = Some(Orientation::Transposed); - - let scales = vec![]; - assert_eq!( - resolve_orientation(&layer, &scales), - Orientation::Transposed - ); - } - #[test] fn test_resolve_orientation_no_implicit() { // Point geom has no implicit orientation let layer = Layer::new(Geom::point()); let scales = vec![]; - assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + assert_eq!(resolve_orientation(&layer, &scales), ALIGNED); + } + + #[test] + fn test_is_transposed_helper() { + // Helper function should return true for transposed orientation + let mut layer = Layer::new(Geom::histogram()); + layer + .mappings + .insert("pos2", AestheticValue::standard_column("y_col")); + let mut scale = Scale::new("pos2"); + scale.scale_type = Some(ScaleType::continuous()); + let scales = vec![scale]; + + assert!(is_transposed(&layer, &scales)); + + // Should return false for aligned orientation + let layer2 = Layer::new(Geom::histogram()); + let mut scale2 = Scale::new("pos1"); + scale2.scale_type = Some(ScaleType::continuous()); + let scales2 = vec![scale2]; + + assert!(!is_transposed(&layer2, &scales2)); } #[test] @@ -370,7 +318,7 @@ mod tests { scale.scale_type = Some(ScaleType::continuous()); let scales = vec![scale]; - assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + assert_eq!(resolve_orientation(&layer, &scales), ALIGNED); } #[test] @@ -387,7 +335,7 @@ mod tests { assert_eq!( resolve_orientation(&layer, &scales), - Orientation::Transposed + TRANSPOSED ); } @@ -402,7 +350,7 @@ mod tests { let scales = vec![scale]; // Without positional mappings, scale existence doesn't imply orientation - assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + assert_eq!(resolve_orientation(&layer, &scales), ALIGNED); } #[test] @@ -417,7 +365,7 @@ mod tests { assert_eq!( resolve_orientation(&layer, &scales), - Orientation::Transposed + TRANSPOSED ); } @@ -431,7 +379,7 @@ mod tests { scale2.scale_type = Some(ScaleType::continuous()); let scales = vec![scale1, scale2]; - assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + assert_eq!(resolve_orientation(&layer, &scales), ALIGNED); } #[test] @@ -516,7 +464,7 @@ mod tests { scale2.scale_type = Some(ScaleType::continuous()); let scales = vec![scale1, scale2]; - assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + assert_eq!(resolve_orientation(&layer, &scales), ALIGNED); } #[test] @@ -544,7 +492,7 @@ mod tests { assert_eq!( resolve_orientation(&layer, &scales), - Orientation::Transposed + TRANSPOSED ); } @@ -569,7 +517,7 @@ mod tests { assert_eq!( resolve_orientation(&layer, &scales), - Orientation::Transposed + TRANSPOSED ); } @@ -592,7 +540,7 @@ mod tests { scale2.scale_type = Some(ScaleType::continuous()); let scales = vec![scale1, scale2]; - assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + assert_eq!(resolve_orientation(&layer, &scales), ALIGNED); } #[test] @@ -623,7 +571,7 @@ mod tests { assert_eq!( resolve_orientation(&layer, &scales), - Orientation::Transposed + TRANSPOSED ); } @@ -653,7 +601,7 @@ mod tests { scale2.scale_type = Some(ScaleType::continuous()); let scales = vec![scale1, scale2]; - assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + assert_eq!(resolve_orientation(&layer, &scales), ALIGNED); } #[test] @@ -669,7 +617,7 @@ mod tests { let scales = vec![]; assert_eq!( resolve_orientation(&layer, &scales), - Orientation::Transposed + TRANSPOSED ); } @@ -683,7 +631,7 @@ mod tests { ); let scales = vec![]; - assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + assert_eq!(resolve_orientation(&layer, &scales), ALIGNED); } #[test] @@ -700,7 +648,7 @@ mod tests { ); let scales = vec![]; - assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + assert_eq!(resolve_orientation(&layer, &scales), ALIGNED); } #[test] @@ -724,6 +672,6 @@ mod tests { scale2.scale_type = Some(ScaleType::continuous()); let scales = vec![scale1, scale2]; - assert_eq!(resolve_orientation(&layer, &scales), Orientation::Aligned); + assert_eq!(resolve_orientation(&layer, &scales), ALIGNED); } } diff --git a/src/reader/data.rs b/src/reader/data.rs index cb53df53..df73aced 100644 --- a/src/reader/data.rs +++ b/src/reader/data.rs @@ -372,7 +372,6 @@ mod duckdb_tests { #[test] fn test_ribbon_transposed_orientation() { use crate::naming; - use crate::plot::layer::orientation::Orientation; let reader = crate::reader::DuckDBReader::from_connection_string("duckdb://memory").unwrap(); @@ -391,7 +390,8 @@ mod duckdb_tests { // Debug: print orientation and scales let layer = &result.specs[0].layers[0]; - eprintln!("Layer orientation: {:?}", layer.orientation); + let orientation = layer.parameters.get("orientation"); + eprintln!("Layer orientation: {:?}", orientation); eprintln!( "Scales: {:?}", result.specs[0] @@ -407,8 +407,8 @@ mod duckdb_tests { // Check orientation was detected correctly assert_eq!( - layer.orientation, - Some(Orientation::Transposed), + orientation.and_then(|v| v.as_str()), + Some("transposed"), "Should detect Transposed orientation" ); diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index 8d5131c3..ddb59182 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -271,14 +271,18 @@ impl GeomRenderer for BarRenderer { layer: &Layer, _context: &RenderContext, ) -> Result<()> { - use crate::plot::layer::Orientation; let width = match layer.parameters.get("width") { Some(ParameterValue::Number(w)) => *w, _ => 0.9, }; // For horizontal bars, use "height" for band size; for vertical, use "width" - let is_horizontal = layer.orientation == Some(Orientation::Transposed); + let is_horizontal = layer + .parameters + .get("orientation") + .and_then(|v| v.as_str()) + .map(|s| s == "transposed") + .unwrap_or(false); // For dodged bars, use expression-based size with the adjusted width // For non-dodged bars, use band-relative size @@ -548,9 +552,12 @@ impl GeomRenderer for RibbonRenderer { layer: &Layer, _context: &RenderContext, ) -> Result<()> { - use crate::plot::layer::Orientation; - - let is_horizontal = layer.orientation == Some(Orientation::Transposed); + let is_horizontal = layer + .parameters + .get("orientation") + .and_then(|v| v.as_str()) + .map(|s| s == "transposed") + .unwrap_or(false); // Remap min/max to primary/secondary based on orientation: // - Aligned (vertical): ymax→y, ymin→y2 @@ -629,7 +636,6 @@ impl GeomRenderer for ViolinRenderer { layer: &Layer, _context: &RenderContext, ) -> Result<()> { - use crate::plot::layer::Orientation; layer_spec["mark"] = json!({ "type": "line", "filled": true @@ -640,7 +646,12 @@ impl GeomRenderer for ViolinRenderer { let violin_offset = format!("[datum.{offset}, -datum.{offset}]", offset = offset_col); // Read orientation from layer (already resolved during execution) - let is_horizontal = layer.orientation == Some(Orientation::Transposed); + let is_horizontal = layer + .parameters + .get("orientation") + .and_then(|v| v.as_str()) + .map(|s| s == "transposed") + .unwrap_or(false); // Continuous axis column for order calculation: // - Vertical: pos2 (y-axis has continuous density values) @@ -714,10 +725,13 @@ impl GeomRenderer for ViolinRenderer { layer: &Layer, _context: &RenderContext, ) -> Result<()> { - use crate::plot::layer::Orientation; - // Read orientation from layer (already resolved during execution) - let is_horizontal = layer.orientation == Some(Orientation::Transposed); + let is_horizontal = layer + .parameters + .get("orientation") + .and_then(|v| v.as_str()) + .map(|s| s == "transposed") + .unwrap_or(false); // Categorical axis for detail encoding: // - Vertical: x channel (categorical groups on x-axis) @@ -994,12 +1008,15 @@ impl BoxplotRenderer { base_key: &str, has_outliers: bool, ) -> Result> { - use crate::plot::layer::Orientation; - let mut layers: Vec = Vec::new(); // Read orientation from layer (already resolved during execution) - let is_horizontal = layer.orientation == Some(Orientation::Transposed); + let is_horizontal = layer + .parameters + .get("orientation") + .and_then(|v| v.as_str()) + .map(|s| s == "transposed") + .unwrap_or(false); // Value columns depend on orientation (after DataFrame column flip): // - Vertical: values in pos2/pos2end (no flip) From 47bceb18edd1edf4ace32fa150068afede5ecae5 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 11 Mar 2026 11:20:55 +0100 Subject: [PATCH 06/16] fix linear layer orientation --- src/plot/layer/orientation.rs | 14 +- src/writer/vegalite/layer.rs | 251 ++++++++++++++++++++++++---------- 2 files changed, 194 insertions(+), 71 deletions(-) diff --git a/src/plot/layer/orientation.rs b/src/plot/layer/orientation.rs index bcd70b81..5e6060dc 100644 --- a/src/plot/layer/orientation.rs +++ b/src/plot/layer/orientation.rs @@ -36,9 +36,19 @@ pub const TRANSPOSED: &str = "transposed"; /// Determine effective orientation for a layer. /// -/// Auto-detects orientation from scales for geoms with implicit orientation. -/// Geoms without implicit orientation always return "aligned". +/// Returns explicit orientation if set via SETTING, otherwise auto-detects +/// from scales for geoms with implicit orientation. +/// Geoms without implicit orientation return "aligned" unless explicitly set. pub fn resolve_orientation(layer: &Layer, scales: &[Scale]) -> &'static str { + // Check for explicit orientation setting first + if let Some(orientation) = layer.parameters.get("orientation").and_then(|v| v.as_str()) { + return if orientation == TRANSPOSED { + TRANSPOSED + } else { + ALIGNED + }; + } + // Only auto-detect for geoms with implicit orientation if !geom_has_implicit_orientation(&layer.geom.geom_type()) { return ALIGNED; diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index ddb59182..77c87f2a 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -449,39 +449,56 @@ impl GeomRenderer for LinearRenderer { fn modify_encoding( &self, encoding: &mut Map, - _layer: &Layer, + layer: &Layer, _context: &RenderContext, ) -> Result<()> { // Remove coefficient and intercept from encoding - they're only used in transforms encoding.remove("coef"); encoding.remove("intercept"); - // Add x/x2/y/y2 encodings for rule mark - // All are fields - x_min/x_max are created by transforms, y_min/y_max are computed + // Check orientation + let is_horizontal = layer + .parameters + .get("orientation") + .and_then(|v| v.as_str()) + .map(|s| s == "transposed") + .unwrap_or(false); + + // For aligned (default): x is primary axis, y is computed (secondary) + // For transposed: y is primary axis, x is computed (secondary) + let (primary, primary2, secondary, secondary2) = if is_horizontal { + ("y", "y2", "x", "x2") + } else { + ("x", "x2", "y", "y2") + }; + + // Add encodings for rule mark + // primary_min/primary_max are created by transforms (extent of the axis) + // secondary_min/secondary_max are computed via formula encoding.insert( - "x".to_string(), + primary.to_string(), json!({ - "field": "x_min", + "field": "primary_min", "type": "quantitative" }), ); encoding.insert( - "x2".to_string(), + primary2.to_string(), json!({ - "field": "x_max" + "field": "primary_max" }), ); encoding.insert( - "y".to_string(), + secondary.to_string(), json!({ - "field": "y_min", + "field": "secondary_min", "type": "quantitative" }), ); encoding.insert( - "y2".to_string(), + secondary2.to_string(), json!({ - "field": "y_max" + "field": "secondary_max" }), ); @@ -491,35 +508,46 @@ impl GeomRenderer for LinearRenderer { fn modify_spec( &self, layer_spec: &mut Value, - _layer: &Layer, + layer: &Layer, context: &RenderContext, ) -> Result<()> { // Field names for coef and intercept (with aesthetic column prefix) let coef_field = naming::aesthetic_column("coef"); let intercept_field = naming::aesthetic_column("intercept"); - // Get x extent from scale (use pos1, the internal name for the first positional aesthetic) - let (x_min, x_max) = context.get_extent("pos1")?; + // Check orientation + let is_horizontal = layer + .parameters + .get("orientation") + .and_then(|v| v.as_str()) + .map(|s| s == "transposed") + .unwrap_or(false); + + // Get extent from appropriate axis: + // - Aligned (default): extent from pos1 (x-axis), compute y from x + // - Transposed: extent from pos2 (y-axis), compute x from y + let extent_aesthetic = if is_horizontal { "pos2" } else { "pos1" }; + let (primary_min, primary_max) = context.get_extent(extent_aesthetic)?; // Add transforms: - // 1. Create constant x_min/x_max fields - // 2. Compute y values at those x positions + // 1. Create constant primary_min/primary_max fields (extent of the primary axis) + // 2. Compute secondary values at those primary positions: secondary = coef * primary + intercept let transforms = json!([ { - "calculate": x_min.to_string(), - "as": "x_min" + "calculate": primary_min.to_string(), + "as": "primary_min" }, { - "calculate": x_max.to_string(), - "as": "x_max" + "calculate": primary_max.to_string(), + "as": "primary_max" }, { - "calculate": format!("datum.{} * datum.x_min + datum.{}", coef_field, intercept_field), - "as": "y_min" + "calculate": format!("datum.{} * datum.primary_min + datum.{}", coef_field, intercept_field), + "as": "secondary_min" }, { - "calculate": format!("datum.{} * datum.x_max + datum.{}", coef_field, intercept_field), - "as": "y_max" + "calculate": format!("datum.{} * datum.primary_max + datum.{}", coef_field, intercept_field), + "as": "secondary_max" } ]); @@ -1579,69 +1607,69 @@ mod tests { assert_eq!( transforms.len(), 5, - "Should have 5 transforms (x_min, x_max, y_min, y_max, filter)" + "Should have 5 transforms (primary_min, primary_max, secondary_min, secondary_max, filter)" ); - // Verify x_min/x_max transforms exist with consistent naming - let x_min_transform = transforms + // Verify primary_min/primary_max transforms exist with consistent naming + let primary_min_transform = transforms .iter() - .find(|t| t["as"] == "x_min") - .expect("x_min transform not found"); - let x_max_transform = transforms + .find(|t| t["as"] == "primary_min") + .expect("primary_min transform not found"); + let primary_max_transform = transforms .iter() - .find(|t| t["as"] == "x_max") - .expect("x_max transform not found"); + .find(|t| t["as"] == "primary_max") + .expect("primary_max transform not found"); assert!( - x_min_transform["calculate"].is_string(), - "x_min should have calculate expression" + primary_min_transform["calculate"].is_string(), + "primary_min should have calculate expression" ); assert!( - x_max_transform["calculate"].is_string(), - "x_max should have calculate expression" + primary_max_transform["calculate"].is_string(), + "primary_max should have calculate expression" ); - // Verify y_min and y_max transforms use coef and intercept with x_min/x_max - let y_min_transform = transforms + // Verify secondary_min and secondary_max transforms use coef and intercept with primary_min/primary_max + let secondary_min_transform = transforms .iter() - .find(|t| t["as"] == "y_min") - .expect("y_min transform not found"); - let y_max_transform = transforms + .find(|t| t["as"] == "secondary_min") + .expect("secondary_min transform not found"); + let secondary_max_transform = transforms .iter() - .find(|t| t["as"] == "y_max") - .expect("y_max transform not found"); + .find(|t| t["as"] == "secondary_max") + .expect("secondary_max transform not found"); - let y_min_calc = y_min_transform["calculate"] + let secondary_min_calc = secondary_min_transform["calculate"] .as_str() - .expect("y_min calculate should be string"); - let y_max_calc = y_max_transform["calculate"] + .expect("secondary_min calculate should be string"); + let secondary_max_calc = secondary_max_transform["calculate"] .as_str() - .expect("y_max calculate should be string"); + .expect("secondary_max calculate should be string"); - // Should reference coef, intercept, and x_min/x_max + // Should reference coef, intercept, and primary_min/primary_max assert!( - y_min_calc.contains("__ggsql_aes_coef__"), - "y_min should reference coef" + secondary_min_calc.contains("__ggsql_aes_coef__"), + "secondary_min should reference coef" ); assert!( - y_min_calc.contains("__ggsql_aes_intercept__"), - "y_min should reference intercept" + secondary_min_calc.contains("__ggsql_aes_intercept__"), + "secondary_min should reference intercept" ); assert!( - y_min_calc.contains("datum.x_min"), - "y_min should reference datum.x_min" + secondary_min_calc.contains("datum.primary_min"), + "secondary_min should reference datum.primary_min" ); assert!( - y_max_calc.contains("__ggsql_aes_coef__"), - "y_max should reference coef" + secondary_max_calc.contains("__ggsql_aes_coef__"), + "secondary_max should reference coef" ); assert!( - y_max_calc.contains("__ggsql_aes_intercept__"), - "y_max should reference intercept" + secondary_max_calc.contains("__ggsql_aes_intercept__"), + "secondary_max should reference intercept" ); assert!( - y_max_calc.contains("datum.x_max"), - "y_max should reference datum.x_max" + secondary_max_calc.contains("datum.primary_max"), + "secondary_max should reference datum.primary_max" ); // Verify encoding has x, x2, y, y2 with consistent field names @@ -1654,22 +1682,22 @@ mod tests { assert!(encoding.contains_key("y"), "Should have y encoding"); assert!(encoding.contains_key("y2"), "Should have y2 encoding"); - // Verify consistent naming: x_min, x_max, y_min, y_max + // Verify consistent naming: primary_min/max for x, secondary_min/max for y (default orientation) assert_eq!( - encoding["x"]["field"], "x_min", - "x should reference x_min field" + encoding["x"]["field"], "primary_min", + "x should reference primary_min field" ); assert_eq!( - encoding["x2"]["field"], "x_max", - "x2 should reference x_max field" + encoding["x2"]["field"], "primary_max", + "x2 should reference primary_max field" ); assert_eq!( - encoding["y"]["field"], "y_min", - "y should reference y_min field" + encoding["y"]["field"], "secondary_min", + "y should reference secondary_min field" ); assert_eq!( - encoding["y2"]["field"], "y_max", - "y2 should reference y_max field" + encoding["y2"]["field"], "secondary_max", + "y2 should reference secondary_max field" ); // Verify stroke encoding exists for line_id (color aesthetic becomes stroke for rule mark) @@ -1707,6 +1735,91 @@ mod tests { assert_eq!(coefs, vec![1.0, 2.0, 3.0], "Should have coefs 1, 2, and 3"); } + #[test] + fn test_linear_renderer_transposed_orientation() { + use crate::reader::{DuckDBReader, Reader}; + use crate::writer::{VegaLiteWriter, Writer}; + + // Test that linear with transposed orientation swaps x/y axes + let query = r#" + WITH points AS ( + SELECT * FROM (VALUES (0, 5), (5, 15), (10, 25)) AS t(x, y) + ), + lines AS ( + SELECT * FROM (VALUES (0.4, -1, 'A')) AS t(coef, intercept, line_id) + ) + SELECT * FROM points + VISUALISE + DRAW point MAPPING x AS x, y AS y + DRAW linear MAPPING coef AS coef, intercept AS intercept, line_id AS color FROM lines SETTING orientation => 'transposed' + "#; + + // Execute query + let reader = DuckDBReader::from_connection_string("duckdb://memory") + .expect("Failed to create reader"); + let spec = reader.execute(query).expect("Failed to execute query"); + + // Render to Vega-Lite + let writer = VegaLiteWriter::new(); + let vl_json = writer.render(&spec).expect("Failed to render spec"); + + // Parse JSON + let vl_spec: serde_json::Value = + serde_json::from_str(&vl_json).expect("Failed to parse Vega-Lite JSON"); + + // Get the linear layer (second layer) + let layers = vl_spec["layer"].as_array().expect("No layers found"); + let linear_layer = &layers[1]; + + // Verify transforms exist + let transforms = linear_layer["transform"] + .as_array() + .expect("No transforms found"); + + // Verify primary_min/max use pos2 extent (y-axis) for transposed orientation + let primary_min_transform = transforms + .iter() + .find(|t| t["as"] == "primary_min") + .expect("primary_min transform not found"); + let primary_max_transform = transforms + .iter() + .find(|t| t["as"] == "primary_max") + .expect("primary_max transform not found"); + + // The primary extent should come from the y-axis for transposed + assert!( + primary_min_transform["calculate"].is_string(), + "primary_min should have calculate expression" + ); + assert!( + primary_max_transform["calculate"].is_string(), + "primary_max should have calculate expression" + ); + + // Verify encoding has y as primary axis (mapped to primary_min/max) + let encoding = linear_layer["encoding"] + .as_object() + .expect("No encoding found"); + + // For transposed orientation: y is primary (uses primary_min/max), x is secondary + assert_eq!( + encoding["y"]["field"], "primary_min", + "y should reference primary_min field for transposed" + ); + assert_eq!( + encoding["y2"]["field"], "primary_max", + "y2 should reference primary_max field for transposed" + ); + assert_eq!( + encoding["x"]["field"], "secondary_min", + "x should reference secondary_min field for transposed" + ); + assert_eq!( + encoding["x2"]["field"], "secondary_max", + "x2 should reference secondary_max field for transposed" + ); + } + #[test] fn test_errorbar_encoding() { let renderer = ErrorBarRenderer; From 4832fd1c0e2d30d5dbdcecbc1f88563b2e47a60a Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 11 Mar 2026 11:23:24 +0100 Subject: [PATCH 07/16] update layer documentation --- doc/syntax/layer/type/area.qmd | 10 +++++++--- doc/syntax/layer/type/bar.qmd | 13 +++++++++---- doc/syntax/layer/type/boxplot.qmd | 7 +------ doc/syntax/layer/type/errorbar.qmd | 11 ++++++----- doc/syntax/layer/type/histogram.qmd | 2 +- doc/syntax/layer/type/line.qmd | 10 +++++++--- doc/syntax/layer/type/linear.qmd | 7 ++++++- doc/syntax/layer/type/path.qmd | 9 ++++++--- doc/syntax/layer/type/point.qmd | 7 +++++-- doc/syntax/layer/type/polygon.qmd | 7 +++++-- doc/syntax/layer/type/ribbon.qmd | 10 +++++----- doc/syntax/layer/type/rule.qmd | 12 +++++++----- doc/syntax/layer/type/segment.qmd | 17 +++++++++-------- 13 files changed, 74 insertions(+), 48 deletions(-) diff --git a/doc/syntax/layer/type/area.qmd b/doc/syntax/layer/type/area.qmd index 81830f39..4f034a12 100644 --- a/doc/syntax/layer/type/area.qmd +++ b/doc/syntax/layer/type/area.qmd @@ -10,8 +10,8 @@ The area layer is used to display absolute amounts over a sorted x-axis. It can The following aesthetics are recognised by the area layer. ### Required -* `x`: Position along the x-axis. -* `y`: Position along the y-axis. +* Primary axis (e.g. `x`): The value of the independent variable. +* Secondary axis (e.g. `y`): The value of the dependent variable. ### Optional * `stroke`: The colour of the contour lines. @@ -22,9 +22,13 @@ The following aesthetics are recognised by the area layer. ## Settings * `position`: Determines the position adjustment to use for the layer (default is `'stack'`) +* `orientation`: The orientation of the layer. Either `'aligned'` (default), or `'transposed'`. Determines if the layers primary axis is aligned with the coordinate systems first axis or not ## Data transformation -The area layer does not transform its data but passes it through unchanged. +The area layer sorts the data along its primary axis + +## Orientation +Area plots are sorted and connected along their primary axis. Since the primary axis cannot be deduced from the mapping it must be specified using the `orientation` setting. E.g. if you wish to create a vertical area plot you need to set `orientation => 'transposed'` to indicate that the primary layer axis follows the second axis of the coordinate system. ## Examples diff --git a/doc/syntax/layer/type/bar.qmd b/doc/syntax/layer/type/bar.qmd index 55e0484a..fd9c0bf9 100644 --- a/doc/syntax/layer/type/bar.qmd +++ b/doc/syntax/layer/type/bar.qmd @@ -100,10 +100,6 @@ SCALE BINNED x SETTING breaks => 10 ``` -And use with a polar coordinate system to create a pie chart - -**TBD** - Create a horizontal bar plot by changing the mapping ```{ggsql} @@ -111,3 +107,12 @@ VISUALISE FROM ggsql:penguins DRAW bar MAPPING species AS y ``` + +And use with a polar coordinate system to create a pie chart + +```{ggsql} +VISUALISE FROM ggsql:penguins +DRAW bar + MAPPING species AS fill +PROJECT TO polar +``` diff --git a/doc/syntax/layer/type/boxplot.qmd b/doc/syntax/layer/type/boxplot.qmd index 9527da00..45f8a65b 100644 --- a/doc/syntax/layer/type/boxplot.qmd +++ b/doc/syntax/layer/type/boxplot.qmd @@ -29,12 +29,7 @@ The following aesthetics are recognised by the boxplot layer. * `width`: Relative width of the boxes. Defaults to `0.9`. ## Data transformation -Per group, data will be divided into 4 quartiles and summary statistics will be derived from their extremes. -Because number of observations per quartile may differ by one, the result of this approach may slightly differ from a pure quantile-based approach. -The central line represents the median. -The boxes are displayed from the 25th up to the 75th percentiles. -The whiskers are calculated from the 25th/75th percentiles +/- the IQR times `coef`, but no more extreme than the data extrema. -Observations are considered outliers when they are more extreme than the whiskers. +Per group, data will be divided into 4 quartiles and summary statistics will be derived from their extremes. Because number of observations per quartile may differ by one, the result of this approach may slightly differ from a pure quantile-based approach. The central line represents the median. The boxes are displayed from the 25th up to the 75th percentiles. The whiskers are calculated from the 25th/75th percentiles +/- the IQR times `coef`, but no more extreme than the data extrema. Observations are considered outliers when they are more extreme than the whiskers. ### Calculated statistics diff --git a/doc/syntax/layer/type/errorbar.qmd b/doc/syntax/layer/type/errorbar.qmd index 44ffdc76..ae285d89 100644 --- a/doc/syntax/layer/type/errorbar.qmd +++ b/doc/syntax/layer/type/errorbar.qmd @@ -10,11 +10,9 @@ Errorbars are used to display paired metrics, typically some interval, for a var The following aesthetics are recognised by the errorbar layer. ### Required -* `x` or `y`: Position on the x- or y-axis. These are mutually exclusive. -* `xmin` or `ymin`: Position of one of the interval ends orthogonal to the main position. These are also mutually exclusive. -* `xmax` or `ymax`: Position of the other interval end orthogonal to the main position. These are also mutually exclusive. - -Note that the required aesthetics is either a set of {`x`, `ymin`, `ymax`} or {`y`, `xmin`, `xmax`} and *not* a combination of the two. +* Primary axis (e.g. `x`): Position along the primary axis +* Secondary axis min (e.g. `ymin`): Lower position along the secondary axis. +* Secondary axis max (e.g. `ymax`): Upper position along the secondary axis. ### Optional * `stroke`/`colour`: The colour of the lines in the errorbar. @@ -28,6 +26,9 @@ Note that the required aesthetics is either a set of {`x`, `ymin`, `ymax`} or {` ## Data transformation The errorbar layer does not transform its data but passes it through unchanged. +## Orientation +Ribbon layers are sorted and connected along their primary axis. The orientation can be deduced purely from the mapping since interval is mapped to the secodnary axis. To create a vertical ribbon layer you map the independent variable to `y` instead of `x` and the interval to `xmin` and `xmax` (assuming a default Cartesian coordinate system). + ## Examples ```{ggsql} diff --git a/doc/syntax/layer/type/histogram.qmd b/doc/syntax/layer/type/histogram.qmd index 954d4ce1..1a81cf24 100644 --- a/doc/syntax/layer/type/histogram.qmd +++ b/doc/syntax/layer/type/histogram.qmd @@ -27,7 +27,7 @@ The following aesthetics are recognised by the bar layer. * `closed`: Either `'left'` or `'right'` (default). Determines whether the bin intervals are closed to the left or right side ## Data transformation -The histogram layer will bin the records in each group and count them. By default it will map the count to `y`. +The histogram layer will bin the records in each group and count them. By default it will map the count to the secondary axis. ### Properties diff --git a/doc/syntax/layer/type/line.qmd b/doc/syntax/layer/type/line.qmd index a0c7136b..4fc2fba2 100644 --- a/doc/syntax/layer/type/line.qmd +++ b/doc/syntax/layer/type/line.qmd @@ -10,8 +10,8 @@ The line layer is used to create lineplots. Lineplots always connects records al The following aesthetics are recognised by the line layer. ### Required -* `x`: Position along the x-axis -* `y`: Position along the y-axis +* Primary axis (e.g. `x`): The value of the independent variable. +* Secondary axis (e.g. `y`): The value of the dependent variable. ### Optional * `colour`/`stroke`: The colour of the line @@ -21,9 +21,13 @@ The following aesthetics are recognised by the line layer. ## Settings * `position`: Determines the position adjustment to use for the layer (default is `'identity'`) +* `orientation`: The orientation of the layer. Either `'aligned'` (default), or `'transposed'`. Determines if the layers primary axis is aligned with the coordinate systems first axis or not ## Data transformation -The line layer does not transform its data but passes it through unchanged +The line layer sorts the data along its primary axis + +## Orientation +Line plots are sorted and connected along their primary axis. Since the primary axis cannot be deduced from the mapping it must be specified using the `orientation` setting. E.g. if you wish to create a vertical line plot you need to set `orientation => 'transposed'` to indicate that the primary layer axis follows the second axis of the coordinate system. ## Examples diff --git a/doc/syntax/layer/type/linear.qmd b/doc/syntax/layer/type/linear.qmd index db8565f6..3a5214d5 100644 --- a/doc/syntax/layer/type/linear.qmd +++ b/doc/syntax/layer/type/linear.qmd @@ -13,6 +13,8 @@ $$ Where $a$ is the `intercept` and $\beta$ is the `coef`. +> The linear layer doesn't have a natural extend along either axes and the scale ranges can thus not be deduced from it. If the linear layer is the only layer in the visualisation you must specify the position scale ranges manually. + ## Aesthetics The following aesthetics are recognised by the abline layer. @@ -27,11 +29,14 @@ The following aesthetics are recognised by the abline layer. * `linetype`: The type of the line, i.e. the dashing pattern ## Settings -The linear layer has no additional settings. +* `orientation`: The orientation of the layer. Either `'aligned'` (default), or `'transposed'`. Determines if the layers primary axis ($x$ in the equation) is aligned with the coordinate systems first axis or not ## Data transformation The linear layer does not transform its data but passes it through unchanged. +## Orientation +Linear layers uses the predictor ($x$) for their primary axis and the response $y$ for its secondary axis. Since the primary axis cannot be deduced from the mapping it must be specified using the `orientation` setting. E.g. if you wish to create a vertical linear plot you need to set `orientation => 'transposed'` to indicate that the primary layer axis follows the second axis of the coordinate system. + ## Examples Add a simple reference line to a scatterplot: diff --git a/doc/syntax/layer/type/path.qmd b/doc/syntax/layer/type/path.qmd index 7b755abf..94ee6126 100644 --- a/doc/syntax/layer/type/path.qmd +++ b/doc/syntax/layer/type/path.qmd @@ -10,8 +10,8 @@ The path layer is used to create lineplots, but contrary to the [line layer](lin The following aesthetics are recognised by the path layer. ### Required -* `x`: Position along the x-axis -* `y`: Position along the y-axis +* Primary axis (e.g. `x`): Position along the primary axis. +* Secondary axis (e.g. `y`): Position along the secondary axis. ### Optional * `colour`/`stroke`: The colour of the path @@ -23,7 +23,10 @@ The following aesthetics are recognised by the path layer. * `position`: Determines the position adjustment to use for the layer (default is `'identity'`) ## Data transformation -The line layer does not transform its data but passes it through unchanged +The path layer does not transform its data but passes it through unchanged + +## Orientation +The path layer is symmetric in how it regards its axes and thus doesn't exhibit any orientation. ## Examples diff --git a/doc/syntax/layer/type/point.qmd b/doc/syntax/layer/type/point.qmd index cf8875a5..6f2f0132 100644 --- a/doc/syntax/layer/type/point.qmd +++ b/doc/syntax/layer/type/point.qmd @@ -10,8 +10,8 @@ The point layer is used to create scatterplots. The scatterplot is most useful f The following aesthetics are recognised by the point layer. ### Required -* `x`: Position along the x-axis -* `y`: Position along the y-axis +* Primary axis (e.g. `x`): Position along the primary axis. +* Secondary axis (e.g. `y`): Position along the secondary axis. ### Optional * `size`: The size of each point @@ -27,6 +27,9 @@ The following aesthetics are recognised by the point layer. ## Data transformation The point layer does not transform its data but passes it through unchanged +## Orientation +The point layer is symmetric in how it regards its axes and thus doesn't exhibit any orientation. + ## Examples Create a classic scatterplot diff --git a/doc/syntax/layer/type/polygon.qmd b/doc/syntax/layer/type/polygon.qmd index 94c1188f..b8756aae 100644 --- a/doc/syntax/layer/type/polygon.qmd +++ b/doc/syntax/layer/type/polygon.qmd @@ -10,8 +10,8 @@ Polygons can be used to draw arbitrary closed shapes based on an ordered sequenc The following aesthetics are recognised by the polygon layer. ### Required -* `x` Position along the x-axis. -* `y` Position along the y-axis. +* Primary axis (e.g. `x`): Position along the primary axis. +* Secondary axis (e.g. `y`): Position along the secondary axis. ### Optional * `stroke` The colour of the contour lines. @@ -27,6 +27,9 @@ The following aesthetics are recognised by the polygon layer. ## Data transformation The polygon layer does not transform its data but passes it through unchanged +## Orientation +The polygon layer is symmetric in how it regards its axes and thus doesn't exhibit any orientation. + ## Examples ```{ggsql} diff --git a/doc/syntax/layer/type/ribbon.qmd b/doc/syntax/layer/type/ribbon.qmd index 23a3d833..c21ca3f0 100644 --- a/doc/syntax/layer/type/ribbon.qmd +++ b/doc/syntax/layer/type/ribbon.qmd @@ -10,9 +10,9 @@ The ribbon layer is used to display extrema over a sorted x-axis. It can be seen The following aesthetics are recognised by the ribbon layer. ### Required -* Primary axis (e.g. `x`): Position along the primary (domain) axis -* Secondary axis min (e.g. `ymin`): Lower position along the secondary (value) axis. -* Secondary axis max (e.g. `ymax`): Upper position along the secondary (value) axis. +* Primary axis (e.g. `x`): Position along the primary axis +* Secondary axis min (e.g. `ymin`): Lower position along the secondary axis. +* Secondary axis max (e.g. `ymax`): Upper position along the secondary axis. ### Optional * `stroke`: The colour of the contour lines. @@ -25,10 +25,10 @@ The following aesthetics are recognised by the ribbon layer. * `position`: Determines the position adjustment to use for the layer (default is `'identity'`) ## Data transformation -The ribbon layer does not transform its data but passes it through unchanged. +The ribbon layer sorts the data along its primary axis ## Orientation -The ribbon has its domain axis as the primary axis and the value range on the secondary axis. The orientation can be deduced from the mapping. To create a horizontal ribbon, map the domain to `y` and use `xmin`/`xmax` for the value range (assuming a default Cartesian coordinate system). +Ribbon layers are sorted and connected along their primary axis. The orientation can be deduced purely from the mapping since interval is mapped to the secodnary axis. To create a vertical ribbon layer you map the independent variable to `y` instead of `x` and the interval to `xmin` and `xmax` (assuming a default Cartesian coordinate system). ## Examples diff --git a/doc/syntax/layer/type/rule.qmd b/doc/syntax/layer/type/rule.qmd index 568da49d..8b57aaf1 100644 --- a/doc/syntax/layer/type/rule.qmd +++ b/doc/syntax/layer/type/rule.qmd @@ -4,16 +4,15 @@ title: "Rule" > Layers are declared with the [`DRAW` clause](../../clause/draw.qmd). Read the documentation for this clause for a thorough description of how to use it. -The rule layer is used to draw horizontal or vertical reference lines at specified values. This is useful for adding thresholds, means, medians, avent markers, cutoff dates or other guides to the plot. The lines span the full width or height of the panels. +The rule layer is used to draw reference lines perpendicular to an axis at specified values. This is useful for adding thresholds, means, medians, event markers, cutoff dates or other guides to the plot. The lines span the full width or height of the panels. + +> The rule layer doesn't have a natural extend along the secondary axis and the scale range can thus not be deduced from it. If the rule layer is the only layer in the visualisation you must specify the position scale range manually. ## Aesthetics The following aesthetics are recognised by the hline layer. ### Required -* `x`\*: The x-coordinate for the vertical line. -* `y`\*: The y-coordinate for the horizontal line - -\* Exactly one of `x` or `y` is required, not both. +* Primary axis (e.g. `x` or `y`): Position along the primary axis. ### Optional * `colour`/`stroke`: The colour of the line @@ -27,6 +26,9 @@ The rule layer has no additional settings. ## Data transformation The rule layer does not transform its data but passes it through unchanged. +## Orientation +Rules are drawn perpendicular to their primary axis. The orientation can be deduced purely from the mapping. To create a horizontal rule you map the values to `y` instead of `x` (assuming a default Cartesian coordinate system). + ## Examples Add a horizontal threshold line to a time series plot: diff --git a/doc/syntax/layer/type/segment.qmd b/doc/syntax/layer/type/segment.qmd index 37a2c65e..18b655f1 100644 --- a/doc/syntax/layer/type/segment.qmd +++ b/doc/syntax/layer/type/segment.qmd @@ -4,18 +4,18 @@ title: "Segment" > Layers are declared with the [`DRAW` clause](../../clause/draw.qmd). Read the documentation for this clause for a thorough description of how to use it. -The segment layer is used to create line segments between two endpoints. If differs from [lines](line.qmd) and [paths](path.qmd) in that it connects just two points rather than many. Data is expected to be in a different shape, with 4 coordinates for the start (x, y) and end (xend, yend) points on a single row. +The segment layer is used to create line segments between two endpoints. It differs from [lines](line.qmd) and [paths](path.qmd) in that it connects just two points rather than many. Data is expected to be in a different shape, with 4 coordinates for the start (e.g. `x`/`y`) and end (e.g. `xend`/`yend`) points on a single row. ## Aesthetics The following aesthetics are recognised by the segment layer. ### Required -* `x`: Position along the x-axis of the start point. -* `y`: Position along the y-axis of the end point. -* `xend`\*: Position along the x-axis of the end point. -* `yend`\*: Position along the y-axis of the end point. +* Primary axis (e.g. `x`): Start position along the primary axis. +* Primary axis end (e.g. `xend`): End position along the primary axis. +* Secondary axis (e.g. `y`): Start position along the secondary axis. +* Secondary axis end (e.g. `yend`): end position along the secondary axis. -\* Only one of `xend` and `yend` is required. +\* Only one of `end` value is strictly required. If one is missing, it takes on the value of the start point. ### Optional @@ -25,12 +25,13 @@ If one is missing, it takes on the value of the start point. * `linetype`: The type of the line, i.e. the dashing pattern. ## Settings -The segment layer has no additional settings. +* `position`: Determines the position adjustment to use for the layer (default is `'identity'`) ## Data transformation The segment layer does not transform its data but passes it through unchanged. -## Data transformation +## Orientation +The segment layer is symmetric in how it regards its axes and thus doesn't exhibit any orientation. ## Examples From 1902c5a3f86849cac5a4cac0e8b530bc3d5f8db0 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 11 Mar 2026 11:23:40 +0100 Subject: [PATCH 08/16] reformat --- src/execute/mod.rs | 7 ++++--- src/lib.rs | 6 +++++- src/plot/layer/orientation.rs | 30 ++++++------------------------ src/plot/scale/scale_type/mod.rs | 8 ++++++-- 4 files changed, 21 insertions(+), 30 deletions(-) diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 46bd1de3..397fc43b 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -1147,9 +1147,10 @@ pub fn prepare_data_with_reader(query: &str, reader: &R) -> Result panic!("Should reject orientation setting for bar geom"), } diff --git a/src/plot/layer/orientation.rs b/src/plot/layer/orientation.rs index 5e6060dc..98472182 100644 --- a/src/plot/layer/orientation.rs +++ b/src/plot/layer/orientation.rs @@ -343,10 +343,7 @@ mod tests { scale.scale_type = Some(ScaleType::continuous()); let scales = vec![scale]; - assert_eq!( - resolve_orientation(&layer, &scales), - TRANSPOSED - ); + assert_eq!(resolve_orientation(&layer, &scales), TRANSPOSED); } #[test] @@ -373,10 +370,7 @@ mod tests { scale2.scale_type = Some(ScaleType::discrete()); let scales = vec![scale1, scale2]; - assert_eq!( - resolve_orientation(&layer, &scales), - TRANSPOSED - ); + assert_eq!(resolve_orientation(&layer, &scales), TRANSPOSED); } #[test] @@ -500,10 +494,7 @@ mod tests { scale2.scale_type = Some(ScaleType::continuous()); let scales = vec![scale1, scale2]; - assert_eq!( - resolve_orientation(&layer, &scales), - TRANSPOSED - ); + assert_eq!(resolve_orientation(&layer, &scales), TRANSPOSED); } #[test] @@ -525,10 +516,7 @@ mod tests { scale2.scale_type = Some(ScaleType::discrete()); let scales = vec![scale1, scale2]; - assert_eq!( - resolve_orientation(&layer, &scales), - TRANSPOSED - ); + assert_eq!(resolve_orientation(&layer, &scales), TRANSPOSED); } #[test] @@ -579,10 +567,7 @@ mod tests { scale2.scale_type = Some(ScaleType::continuous()); let scales = vec![scale1, scale2]; - assert_eq!( - resolve_orientation(&layer, &scales), - TRANSPOSED - ); + assert_eq!(resolve_orientation(&layer, &scales), TRANSPOSED); } #[test] @@ -625,10 +610,7 @@ mod tests { ); let scales = vec![]; - assert_eq!( - resolve_orientation(&layer, &scales), - TRANSPOSED - ); + assert_eq!(resolve_orientation(&layer, &scales), TRANSPOSED); } #[test] diff --git a/src/plot/scale/scale_type/mod.rs b/src/plot/scale/scale_type/mod.rs index 99f704eb..5e91697e 100644 --- a/src/plot/scale/scale_type/mod.rs +++ b/src/plot/scale/scale_type/mod.rs @@ -1784,8 +1784,12 @@ pub(crate) fn resolve_common_steps( // // For polar coordinates, pos2 (theta) defaults to zero expansion since it's angular/categorical. // Users can still explicitly set expand if they want. - let (mult, add) = - get_expand_factors_for_aesthetic(&scale.properties, aesthetic, context, user_explicit_expand); + let (mult, add) = get_expand_factors_for_aesthetic( + &scale.properties, + aesthetic, + context, + user_explicit_expand, + ); // Track the original user range to know which values are explicit vs inferred let original_user_range = scale.input_range.clone(); From 09774744018d65309481979d27af55b1b8341488 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 11 Mar 2026 11:36:10 +0100 Subject: [PATCH 09/16] Align rule layer with orientation logic --- doc/syntax/layer/type/rule.qmd | 2 +- src/plot/layer/geom/rule.rs | 1 - src/plot/layer/orientation.rs | 41 ++++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/doc/syntax/layer/type/rule.qmd b/doc/syntax/layer/type/rule.qmd index 8b57aaf1..eb8122a4 100644 --- a/doc/syntax/layer/type/rule.qmd +++ b/doc/syntax/layer/type/rule.qmd @@ -9,7 +9,7 @@ The rule layer is used to draw reference lines perpendicular to an axis at speci > The rule layer doesn't have a natural extend along the secondary axis and the scale range can thus not be deduced from it. If the rule layer is the only layer in the visualisation you must specify the position scale range manually. ## Aesthetics -The following aesthetics are recognised by the hline layer. +The following aesthetics are recognised by the rule layer. ### Required * Primary axis (e.g. `x` or `y`): Position along the primary axis. diff --git a/src/plot/layer/geom/rule.rs b/src/plot/layer/geom/rule.rs index 6b34e836..0e1ce286 100644 --- a/src/plot/layer/geom/rule.rs +++ b/src/plot/layer/geom/rule.rs @@ -16,7 +16,6 @@ impl GeomTrait for Rule { DefaultAesthetics { defaults: &[ ("pos1", DefaultAestheticValue::Null), - ("pos2", DefaultAestheticValue::Null), ("stroke", DefaultAestheticValue::String("black")), ("linewidth", DefaultAestheticValue::Number(1.0)), ("opacity", DefaultAestheticValue::Number(1.0)), diff --git a/src/plot/layer/orientation.rs b/src/plot/layer/orientation.rs index 98472182..d4edd917 100644 --- a/src/plot/layer/orientation.rs +++ b/src/plot/layer/orientation.rs @@ -85,6 +85,7 @@ pub fn geom_has_implicit_orientation(geom: &GeomType) -> bool { | GeomType::Violin | GeomType::Density | GeomType::Ribbon + | GeomType::Rule ) } @@ -283,6 +284,7 @@ mod tests { assert!(geom_has_implicit_orientation(&GeomType::Violin)); assert!(geom_has_implicit_orientation(&GeomType::Density)); assert!(geom_has_implicit_orientation(&GeomType::Ribbon)); + assert!(geom_has_implicit_orientation(&GeomType::Rule)); assert!(!geom_has_implicit_orientation(&GeomType::Point)); assert!(!geom_has_implicit_orientation(&GeomType::Line)); @@ -666,4 +668,43 @@ mod tests { assert_eq!(resolve_orientation(&layer, &scales), ALIGNED); } + + #[test] + fn test_resolve_orientation_rule_vertical() { + // Rule with pos1 scale → Aligned (vertical rule) + // Real-world: `DRAW rule MAPPING 2.5 AS pos1` with `SCALE CONTINUOUS pos1` + let mut layer = Layer::new(Geom::rule()); + layer + .mappings + .insert("pos1", AestheticValue::standard_column("x_val")); + let mut scale = Scale::new("pos1"); + scale.scale_type = Some(ScaleType::continuous()); + let scales = vec![scale]; + + assert_eq!(resolve_orientation(&layer, &scales), ALIGNED); + } + + #[test] + fn test_resolve_orientation_rule_horizontal() { + // Rule with pos2 scale → Transposed (horizontal rule) + // Real-world: `DRAW rule MAPPING 15 AS pos1` with `SCALE CONTINUOUS pos2` + let mut layer = Layer::new(Geom::rule()); + layer + .mappings + .insert("pos1", AestheticValue::standard_column("y_val")); + let mut scale = Scale::new("pos2"); + scale.scale_type = Some(ScaleType::continuous()); + let scales = vec![scale]; + + assert_eq!(resolve_orientation(&layer, &scales), TRANSPOSED); + } + + #[test] + fn test_resolve_orientation_rule_default() { + // Rule with no scales → defaults to Aligned + let layer = Layer::new(Geom::rule()); + let scales = vec![]; + + assert_eq!(resolve_orientation(&layer, &scales), ALIGNED); + } } From fd02022787a09aa495447a2d1a581caf44e8cef1 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 11 Mar 2026 12:03:36 +0100 Subject: [PATCH 10/16] refactor renaming --- src/execute/layer.rs | 83 ++++++++++++++++++++++++++++++-------------- src/execute/mod.rs | 81 +----------------------------------------- 2 files changed, 57 insertions(+), 107 deletions(-) diff --git a/src/execute/layer.rs b/src/execute/layer.rs index 973d26d5..af858efc 100644 --- a/src/execute/layer.rs +++ b/src/execute/layer.rs @@ -3,6 +3,7 @@ //! This module handles building SQL queries for layers, applying pre-stat //! transformations, stat transforms, and post-query operations. +use crate::plot::aesthetic::{is_positional_aesthetic, AestheticContext}; use crate::plot::{ AestheticValue, DefaultAestheticValue, Layer, ParameterValue, Scale, Schema, SqlTypeNames, StatResult, @@ -337,7 +338,6 @@ pub fn build_layer_base_query( /// * `schema` - The layer's schema (with min/max from base_query) /// * `scales` - All resolved scales /// * `type_names` - SQL type names for the database backend -/// * `aesthetic_ctx` - Aesthetic context for name transformations /// * `execute_query` - Function to execute queries (needed for some stat transforms) /// /// # Returns @@ -349,7 +349,6 @@ pub fn apply_layer_transforms( schema: &Schema, scales: &[Scale], type_names: &SqlTypeNames, - aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, execute_query: &F, ) -> Result where @@ -590,7 +589,7 @@ where // After flip_mappings, pos1 might point to __ggsql_aes_pos2__ (and vice versa). // We update the column names so pos1 → __ggsql_aes_pos1__, etc. // The DataFrame columns will be renamed correspondingly in mod.rs. - normalize_mapping_column_names(layer, aesthetic_ctx); + normalize_mapping_column_names(layer); } // Apply explicit ORDER BY if provided @@ -611,10 +610,7 @@ where /// /// This should be called after flip_mappings during flip-back. /// The DataFrame columns should be renamed correspondingly using `flip_dataframe_columns`. -fn normalize_mapping_column_names( - layer: &mut Layer, - aesthetic_ctx: &crate::plot::aesthetic::AestheticContext, -) { +fn normalize_mapping_column_names(layer: &mut Layer) { // Collect the aesthetics to update (to avoid borrowing issues) let aesthetics_to_update: Vec = layer .mappings @@ -628,31 +624,64 @@ fn normalize_mapping_column_names( if let Some(value) = layer.mappings.aesthetics.get_mut(&aesthetic) { let expected_col = naming::aesthetic_column(&aesthetic); match value { - AestheticValue::Column { - name, - original_name, - .. - } => { - // The current column name might be wrong (e.g., __ggsql_aes_pos2__ for pos1) - // We need to flip it to match the aesthetic key - let current_col_aesthetic = - naming::extract_aesthetic_name(name).unwrap_or_default(); - let flipped_aesthetic = aesthetic_ctx.flip_positional(current_col_aesthetic); - - // If flipping results in the correct aesthetic, update the column name - if flipped_aesthetic == aesthetic { - *name = expected_col; - } - // Preserve original_name for axis labels - if original_name.is_none() { - *original_name = Some(aesthetic.clone()); - } + AestheticValue::Column { name, .. } => { + *name = expected_col; } AestheticValue::Literal(_) => { - // Literals become columns with the expected name *value = AestheticValue::standard_column(expected_col); } } } } } + +/// Flip positional column names in a DataFrame for Transposed orientation layers. +/// +/// Swaps column names like `__ggsql_aes_pos1__` ↔ `__ggsql_aes_pos2__` so that +/// the data matches the flipped mapping names. +/// +/// This is called after query execution for layers with Transposed orientation, +/// in coordination with `normalize_mapping_column_names` which updates the mappings. +pub fn flip_dataframe_positional_columns( + df: DataFrame, + aesthetic_ctx: &AestheticContext, +) -> DataFrame { + use polars::prelude::*; + + // Collect renames needed before consuming df + let renames: Vec<(String, String)> = df + .get_column_names() + .iter() + .filter_map(|col_name| { + naming::extract_aesthetic_name(col_name).and_then(|aesthetic| { + if is_positional_aesthetic(aesthetic) { + let flipped = aesthetic_ctx.flip_positional(aesthetic); + if flipped != aesthetic { + return Some((col_name.to_string(), naming::aesthetic_column(&flipped))); + } + } + None + }) + }) + .collect(); + + if renames.is_empty() { + return df; + } + + let mut lazy = df.lazy(); + + // First pass: rename to temp names + for (from, to) in &renames { + let temp = format!("{}_temp", to); + lazy = lazy.rename([from.as_str()], [temp.as_str()], true); + } + + // Second pass: remove temp suffix + for (_, to) in &renames { + let temp = format!("{}_temp", to); + lazy = lazy.rename([temp.as_str()], [to.as_str()], true); + } + + lazy.collect().expect("rename should not fail") +} diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 397fc43b..3d032f87 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -35,84 +35,6 @@ use crate::reader::Reader; #[cfg(all(feature = "duckdb", test))] use crate::reader::DuckDBReader; -// ============================================================================= -// DataFrame Column Flipping for Orientation -// ============================================================================= - -/// Flip positional column names in a DataFrame for Transposed orientation layers. -/// -/// Swaps column names like `__ggsql_aes_pos1__` ↔ `__ggsql_aes_pos2__` so that -/// the data matches the flipped mapping names. -/// -/// This is called after query execution for layers with Transposed orientation, -/// in coordination with `normalize_mapping_column_names` which updates the mappings. -fn flip_dataframe_positional_columns(df: DataFrame, aesthetic_ctx: &AestheticContext) -> DataFrame { - use polars::prelude::*; - - // Collect all positional column renames needed - let mut renames: Vec<(String, String)> = Vec::new(); - - for col_name in df.get_column_names() { - let col_str = col_name.to_string(); - - // Check if this is an aesthetic column (__ggsql_aes_XXX__) - if let Some(aesthetic) = naming::extract_aesthetic_name(&col_str) { - if is_positional_aesthetic(aesthetic) { - let flipped = aesthetic_ctx.flip_positional(aesthetic); - if flipped != aesthetic { - let new_col_name = naming::aesthetic_column(&flipped); - renames.push((col_str, new_col_name)); - } - } - } - } - - if renames.is_empty() { - return df; - } - - // To swap columns (A ↔ B), we need to: - // 1. Rename A to temp - // 2. Rename B to A - // 3. Rename temp to B - // But if both A and B exist, we need to handle them together - - let df_clone = df.clone(); // For fallback if collect fails - let mut lazy = df.lazy(); - - // Group renames into swap pairs - let mut processed: HashSet = HashSet::new(); - for (from, to) in &renames { - if processed.contains(from) { - continue; - } - - // Check if the reverse rename also exists (it's a swap) - let is_swap = renames.iter().any(|(f, t)| f == to && t == from); - - if is_swap { - // Swap: use intermediate names to avoid collision - let temp_from = format!("{}_temp_swap_", from); - let temp_to = format!("{}_temp_swap_", to); - - lazy = lazy - .rename([from.as_str()], [temp_from.as_str()], true) - .rename([to.as_str()], [temp_to.as_str()], true) - .rename([temp_from.as_str()], [to.as_str()], true) - .rename([temp_to.as_str()], [from.as_str()], true); - - processed.insert(from.clone()); - processed.insert(to.clone()); - } else { - // Simple rename (one direction only) - lazy = lazy.rename([from.as_str()], [to.as_str()], true); - processed.insert(from.clone()); - } - } - - lazy.collect().unwrap_or(df_clone) -} - // ============================================================================= // Validation // ============================================================================= @@ -1228,7 +1150,6 @@ pub fn prepare_data_with_reader(query: &str, reader: &R) -> Result(query: &str, reader: &R) -> Result Date: Wed, 11 Mar 2026 12:21:01 +0100 Subject: [PATCH 11/16] streamline orientation checks --- src/execute/layer.rs | 8 ++--- src/execute/mod.rs | 31 +++++------------ src/plot/layer/mod.rs | 2 +- src/writer/vegalite/layer.rs | 66 ++++++++++-------------------------- src/writer/vegalite/mod.rs | 2 +- 5 files changed, 30 insertions(+), 79 deletions(-) diff --git a/src/execute/layer.rs b/src/execute/layer.rs index af858efc..e41bce41 100644 --- a/src/execute/layer.rs +++ b/src/execute/layer.rs @@ -4,6 +4,7 @@ //! transformations, stat transforms, and post-query operations. use crate::plot::aesthetic::{is_positional_aesthetic, AestheticContext}; +use crate::plot::layer::is_transposed; use crate::plot::{ AestheticValue, DefaultAestheticValue, Layer, ParameterValue, Scale, Schema, SqlTypeNames, StatResult, @@ -361,12 +362,7 @@ where // Orientation detection and initial flip was already done in mod.rs before // build_layer_base_query. We just check if we need to flip back after stat. - let needs_flip = layer - .parameters - .get("orientation") - .and_then(|v| v.as_str()) - .map(|s| s == "transposed") - .unwrap_or(false); + let needs_flip = is_transposed(layer, scales); // Build the aesthetic-named schema for stat transforms // Note: Mappings were already flipped in mod.rs if needed, so schema reflects normalized orientation diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 3d032f87..80a48421 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -26,6 +26,7 @@ use crate::naming; use crate::parser; use crate::plot::aesthetic::{is_positional_aesthetic, AestheticContext}; use crate::plot::facet::{resolve_properties as resolve_facet_properties, FacetDataContext}; +use crate::plot::layer::is_transposed; use crate::plot::{AestheticValue, Layer, Scale, ScaleTypeKind, Schema}; use crate::{DataFrame, GgsqlError, Plot, Result}; use std::collections::{HashMap, HashSet}; @@ -1031,7 +1032,7 @@ pub fn prepare_data_with_reader(query: &str, reader: &R) -> Result(query: &str, reader: &R) -> Result(query: &str, reader: &R) -> Result(query: &str, reader: &R) -> Result(query: &str, reader: &R) -> Result = HashSet::new(); for layer in specs[0].layers.iter() { - let needs_flip = layer - .parameters - .get("orientation") - .and_then(|v| v.as_str()) - .map(|s| s == "transposed") - .unwrap_or(false); - if needs_flip { + if is_transposed(layer, &scales) { if let Some(ref key) = layer.data_key { if flipped_keys.insert(key.clone()) { // First time flipping this data key if let Some(df) = data_map.remove(key) { - let flipped_df = layer::flip_dataframe_positional_columns(df, &aesthetic_ctx); + let flipped_df = + layer::flip_dataframe_positional_columns(df, &aesthetic_ctx); data_map.insert(key.clone(), flipped_df); } } diff --git a/src/plot/layer/mod.rs b/src/plot/layer/mod.rs index b4ed4263..54759ace 100644 --- a/src/plot/layer/mod.rs +++ b/src/plot/layer/mod.rs @@ -16,7 +16,7 @@ pub mod orientation; pub mod position; // Re-export orientation functions and constants -pub use orientation::{is_transposed, ALIGNED, TRANSPOSED}; +pub use orientation::is_transposed; // Re-export geom types for convenience pub use geom::{ diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index 77c87f2a..a213599d 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -8,6 +8,7 @@ //! sensible defaults for standard behavior. use crate::plot::layer::geom::GeomType; +use crate::plot::layer::is_transposed; use crate::plot::ParameterValue; use crate::writer::vegalite::POINTS_TO_PIXELS; use crate::{naming, AestheticValue, DataFrame, Geom, GgsqlError, Layer, Result}; @@ -243,6 +244,7 @@ pub trait GeomRenderer: Send + Sync { _layer: &Layer, _data_key: &str, _prepared: &PreparedData, + _context: &RenderContext, ) -> Result> { Ok(vec![layer_spec]) } @@ -269,7 +271,7 @@ impl GeomRenderer for BarRenderer { &self, layer_spec: &mut Value, layer: &Layer, - _context: &RenderContext, + context: &RenderContext, ) -> Result<()> { let width = match layer.parameters.get("width") { Some(ParameterValue::Number(w)) => *w, @@ -277,12 +279,7 @@ impl GeomRenderer for BarRenderer { }; // For horizontal bars, use "height" for band size; for vertical, use "width" - let is_horizontal = layer - .parameters - .get("orientation") - .and_then(|v| v.as_str()) - .map(|s| s == "transposed") - .unwrap_or(false); + let is_horizontal = is_transposed(layer, context.scales); // For dodged bars, use expression-based size with the adjusted width // For non-dodged bars, use band-relative size @@ -450,19 +447,14 @@ impl GeomRenderer for LinearRenderer { &self, encoding: &mut Map, layer: &Layer, - _context: &RenderContext, + context: &RenderContext, ) -> Result<()> { // Remove coefficient and intercept from encoding - they're only used in transforms encoding.remove("coef"); encoding.remove("intercept"); // Check orientation - let is_horizontal = layer - .parameters - .get("orientation") - .and_then(|v| v.as_str()) - .map(|s| s == "transposed") - .unwrap_or(false); + let is_horizontal = is_transposed(layer, context.scales); // For aligned (default): x is primary axis, y is computed (secondary) // For transposed: y is primary axis, x is computed (secondary) @@ -516,12 +508,7 @@ impl GeomRenderer for LinearRenderer { let intercept_field = naming::aesthetic_column("intercept"); // Check orientation - let is_horizontal = layer - .parameters - .get("orientation") - .and_then(|v| v.as_str()) - .map(|s| s == "transposed") - .unwrap_or(false); + let is_horizontal = is_transposed(layer, context.scales); // Get extent from appropriate axis: // - Aligned (default): extent from pos1 (x-axis), compute y from x @@ -578,14 +565,9 @@ impl GeomRenderer for RibbonRenderer { &self, encoding: &mut Map, layer: &Layer, - _context: &RenderContext, + context: &RenderContext, ) -> Result<()> { - let is_horizontal = layer - .parameters - .get("orientation") - .and_then(|v| v.as_str()) - .map(|s| s == "transposed") - .unwrap_or(false); + let is_horizontal = is_transposed(layer, context.scales); // Remap min/max to primary/secondary based on orientation: // - Aligned (vertical): ymax→y, ymin→y2 @@ -662,7 +644,7 @@ impl GeomRenderer for ViolinRenderer { &self, layer_spec: &mut Value, layer: &Layer, - _context: &RenderContext, + context: &RenderContext, ) -> Result<()> { layer_spec["mark"] = json!({ "type": "line", @@ -674,12 +656,7 @@ impl GeomRenderer for ViolinRenderer { let violin_offset = format!("[datum.{offset}, -datum.{offset}]", offset = offset_col); // Read orientation from layer (already resolved during execution) - let is_horizontal = layer - .parameters - .get("orientation") - .and_then(|v| v.as_str()) - .map(|s| s == "transposed") - .unwrap_or(false); + let is_horizontal = is_transposed(layer, context.scales); // Continuous axis column for order calculation: // - Vertical: pos2 (y-axis has continuous density values) @@ -751,15 +728,10 @@ impl GeomRenderer for ViolinRenderer { &self, encoding: &mut Map, layer: &Layer, - _context: &RenderContext, + context: &RenderContext, ) -> Result<()> { // Read orientation from layer (already resolved during execution) - let is_horizontal = layer - .parameters - .get("orientation") - .and_then(|v| v.as_str()) - .map(|s| s == "transposed") - .unwrap_or(false); + let is_horizontal = is_transposed(layer, context.scales); // Categorical axis for detail encoding: // - Vertical: x channel (categorical groups on x-axis) @@ -906,6 +878,7 @@ impl GeomRenderer for ErrorBarRenderer { layer: &Layer, _data_key: &str, _prepared: &PreparedData, + _context: &RenderContext, ) -> Result> { // Get width parameter (in points) let width = if let Some(ParameterValue::Number(num)) = layer.parameters.get("width") { @@ -1035,16 +1008,12 @@ impl BoxplotRenderer { layer: &Layer, base_key: &str, has_outliers: bool, + context: &RenderContext, ) -> Result> { let mut layers: Vec = Vec::new(); // Read orientation from layer (already resolved during execution) - let is_horizontal = layer - .parameters - .get("orientation") - .and_then(|v| v.as_str()) - .map(|s| s == "transposed") - .unwrap_or(false); + let is_horizontal = is_transposed(layer, context.scales); // Value columns depend on orientation (after DataFrame column flip): // - Vertical: values in pos2/pos2end (no flip) @@ -1258,6 +1227,7 @@ impl GeomRenderer for BoxplotRenderer { layer: &Layer, data_key: &str, prepared: &PreparedData, + context: &RenderContext, ) -> Result> { let PreparedData::Composite { metadata, .. } = prepared else { return Err(GgsqlError::InternalError( @@ -1269,7 +1239,7 @@ impl GeomRenderer for BoxplotRenderer { GgsqlError::InternalError("Failed to downcast boxplot metadata".to_string()) })?; - self.render_layers(prototype, layer, data_key, info.has_outliers) + self.render_layers(prototype, layer, data_key, info.has_outliers, context) } } diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index a06e863f..e689e478 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -196,7 +196,7 @@ fn build_layers( renderer.modify_spec(&mut layer_spec, layer, &context)?; // Finalize the layer (may expand into multiple layers for composite geoms) - let final_layers = renderer.finalize(layer_spec, layer, data_key, prepared)?; + let final_layers = renderer.finalize(layer_spec, layer, data_key, prepared, &context)?; layers.extend(final_layers); } From f861c6822db2a44372ae3f81c68bf37ca294f5a3 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 11 Mar 2026 12:46:01 +0100 Subject: [PATCH 12/16] simplify orientation resolving --- src/execute/layer.rs | 46 ++++++++++++++++++++- src/execute/mod.rs | 77 +++++------------------------------ src/lib.rs | 11 ++--- src/plot/layer/geom/mod.rs | 5 +-- src/plot/layer/orientation.rs | 31 +++++++++++--- src/writer/vegalite/layer.rs | 26 ++++++------ src/writer/vegalite/mod.rs | 7 +--- 7 files changed, 103 insertions(+), 100 deletions(-) diff --git a/src/execute/layer.rs b/src/execute/layer.rs index e41bce41..c8b7fe92 100644 --- a/src/execute/layer.rs +++ b/src/execute/layer.rs @@ -5,6 +5,7 @@ use crate::plot::aesthetic::{is_positional_aesthetic, AestheticContext}; use crate::plot::layer::is_transposed; +use crate::plot::layer::orientation::{flip_mappings, resolve_orientation}; use crate::plot::{ AestheticValue, DefaultAestheticValue, Layer, ParameterValue, Scale, Schema, SqlTypeNames, StatResult, @@ -362,7 +363,7 @@ where // Orientation detection and initial flip was already done in mod.rs before // build_layer_base_query. We just check if we need to flip back after stat. - let needs_flip = is_transposed(layer, scales); + let needs_flip = is_transposed(layer); // Build the aesthetic-named schema for stat transforms // Note: Mappings were already flipped in mod.rs if needed, so schema reflects normalized orientation @@ -681,3 +682,46 @@ pub fn flip_dataframe_positional_columns( lazy.collect().expect("rename should not fail") } + +/// Resolve orientation for all layers and apply mapping flips. +/// +/// This function: +/// 1. Resolves orientation via auto-detection or explicit setting +/// 2. Stores resolved orientation in layer parameters +/// 3. Flips mappings for transposed layers +/// 4. Flips type_info column names to match flipped mappings +/// +/// Must be called BEFORE building base queries, since build_layer_base_query +/// uses layer.mappings to create SQL like `column AS __ggsql_aes_pos1__`. +/// +/// Note: Validation of orientation settings is handled by `validate_settings()`, +/// which rejects orientation for geoms that don't have it in default_params. +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); + // Store resolved orientation in parameters for downstream use (writers need it) + layer.parameters.insert( + "orientation".to_string(), + ParameterValue::String(orientation.to_string()), + ); + if is_transposed(layer) { + flip_mappings(layer); + // Also flip column names in type_info to match the flipped mappings + 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_positional(aesthetic); + if flipped != aesthetic { + *name = naming::aesthetic_column(&flipped); + } + } + } + } + } + } +} diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 80a48421..2773bcdd 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -1027,69 +1027,14 @@ pub fn prepare_data_with_reader(query: &str, reader: &R) -> Result(query: &str, reader: &R) -> Result(query: &str, reader: &R) -> Result(query: &str, reader: &R) -> Result = HashSet::new(); for layer in specs[0].layers.iter() { - if is_transposed(layer, &scales) { + if is_transposed(layer) { if let Some(ref key) = layer.data_key { if flipped_keys.insert(key.clone()) { // First time flipping this data key diff --git a/src/lib.rs b/src/lib.rs index 8bcf6cbd..7c6a649f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -867,7 +867,8 @@ mod integration_tests { // Test orientation setting behavior for different geom types let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); - // 1. Bar geom (has implicit orientation) - should reject with auto-detection message + // 1. Bar geom (has implicit orientation) - should reject + // Orientation is auto-detected based on mappings, not settable via SETTING let query = r#" SELECT 'A' as category, 10 as value VISUALISE @@ -879,8 +880,8 @@ mod integration_tests { Err(e) => { let err = e.to_string(); assert!( - err.contains("orientation is auto-detected"), - "Error should mention auto-detection: {}", + err.contains("Invalid setting 'orientation'"), + "Error should mention invalid setting: {}", err ); assert!( @@ -904,8 +905,8 @@ mod integration_tests { Err(e) => { let err2 = e.to_string(); assert!( - err2.contains("does not support orientation"), - "Error should mention lack of support: {}", + err2.contains("Invalid setting 'orientation'"), + "Error should mention invalid setting: {}", err2 ); } diff --git a/src/plot/layer/geom/mod.rs b/src/plot/layer/geom/mod.rs index 34d79a88..d3bcdf24 100644 --- a/src/plot/layer/geom/mod.rs +++ b/src/plot/layer/geom/mod.rs @@ -220,15 +220,12 @@ pub trait GeomTrait: std::fmt::Debug + std::fmt::Display + Send + Sync { /// Returns valid parameter names for SETTING clause. /// - /// Combines supported aesthetics with non-aesthetic parameters. - /// Includes "orientation" which is set internally during execution (not user-settable). + /// Combines supported aesthetics with non-aesthetic parameters from default_params. fn valid_settings(&self) -> Vec<&'static str> { let mut valid: Vec<&'static str> = self.aesthetics().supported(); for param in self.default_params() { valid.push(param.name); } - // Internal parameter set during execution (not user-settable via SETTING) - valid.push("orientation"); valid } } diff --git a/src/plot/layer/orientation.rs b/src/plot/layer/orientation.rs index d4edd917..c8fb0fa1 100644 --- a/src/plot/layer/orientation.rs +++ b/src/plot/layer/orientation.rs @@ -64,9 +64,15 @@ pub fn resolve_orientation(layer: &Layer, scales: &[Scale]) -> &'static str { /// Check if a layer is transposed (horizontal orientation). /// -/// Convenience helper for downstream code that needs to check orientation. -pub fn is_transposed(layer: &Layer, scales: &[Scale]) -> bool { - resolve_orientation(layer, scales) == TRANSPOSED +/// 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 + .get("orientation") + .and_then(|v| v.as_str()) + .map(|s| s == TRANSPOSED) + .unwrap_or(false) } /// Check if a geom type supports orientation auto-detection. @@ -302,6 +308,8 @@ mod tests { #[test] fn test_is_transposed_helper() { + use crate::plot::ParameterValue; + // Helper function should return true for transposed orientation let mut layer = Layer::new(Geom::histogram()); layer @@ -311,15 +319,26 @@ mod tests { scale.scale_type = Some(ScaleType::continuous()); let scales = vec![scale]; - assert!(is_transposed(&layer, &scales)); + // Resolve and store orientation (as done by resolve_orientations) + let orientation = resolve_orientation(&layer, &scales); + layer.parameters.insert( + "orientation".to_string(), + ParameterValue::String(orientation.to_string()), + ); + assert!(is_transposed(&layer)); // Should return false for aligned orientation - let layer2 = Layer::new(Geom::histogram()); + let mut layer2 = Layer::new(Geom::histogram()); let mut scale2 = Scale::new("pos1"); scale2.scale_type = Some(ScaleType::continuous()); let scales2 = vec![scale2]; - assert!(!is_transposed(&layer2, &scales2)); + let orientation2 = resolve_orientation(&layer2, &scales2); + layer2.parameters.insert( + "orientation".to_string(), + ParameterValue::String(orientation2.to_string()), + ); + assert!(!is_transposed(&layer2)); } #[test] diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index a213599d..b0aa85b4 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -271,7 +271,7 @@ impl GeomRenderer for BarRenderer { &self, layer_spec: &mut Value, layer: &Layer, - context: &RenderContext, + _context: &RenderContext, ) -> Result<()> { let width = match layer.parameters.get("width") { Some(ParameterValue::Number(w)) => *w, @@ -279,7 +279,7 @@ impl GeomRenderer for BarRenderer { }; // For horizontal bars, use "height" for band size; for vertical, use "width" - let is_horizontal = is_transposed(layer, context.scales); + let is_horizontal = is_transposed(layer); // For dodged bars, use expression-based size with the adjusted width // For non-dodged bars, use band-relative size @@ -447,14 +447,14 @@ impl GeomRenderer for LinearRenderer { &self, encoding: &mut Map, layer: &Layer, - context: &RenderContext, + _context: &RenderContext, ) -> Result<()> { // Remove coefficient and intercept from encoding - they're only used in transforms encoding.remove("coef"); encoding.remove("intercept"); // Check orientation - let is_horizontal = is_transposed(layer, context.scales); + let is_horizontal = is_transposed(layer); // For aligned (default): x is primary axis, y is computed (secondary) // For transposed: y is primary axis, x is computed (secondary) @@ -508,7 +508,7 @@ impl GeomRenderer for LinearRenderer { let intercept_field = naming::aesthetic_column("intercept"); // Check orientation - let is_horizontal = is_transposed(layer, context.scales); + let is_horizontal = is_transposed(layer); // Get extent from appropriate axis: // - Aligned (default): extent from pos1 (x-axis), compute y from x @@ -565,9 +565,9 @@ impl GeomRenderer for RibbonRenderer { &self, encoding: &mut Map, layer: &Layer, - context: &RenderContext, + _context: &RenderContext, ) -> Result<()> { - let is_horizontal = is_transposed(layer, context.scales); + let is_horizontal = is_transposed(layer); // Remap min/max to primary/secondary based on orientation: // - Aligned (vertical): ymax→y, ymin→y2 @@ -644,7 +644,7 @@ impl GeomRenderer for ViolinRenderer { &self, layer_spec: &mut Value, layer: &Layer, - context: &RenderContext, + _context: &RenderContext, ) -> Result<()> { layer_spec["mark"] = json!({ "type": "line", @@ -656,7 +656,7 @@ impl GeomRenderer for ViolinRenderer { let violin_offset = format!("[datum.{offset}, -datum.{offset}]", offset = offset_col); // Read orientation from layer (already resolved during execution) - let is_horizontal = is_transposed(layer, context.scales); + let is_horizontal = is_transposed(layer); // Continuous axis column for order calculation: // - Vertical: pos2 (y-axis has continuous density values) @@ -728,10 +728,10 @@ impl GeomRenderer for ViolinRenderer { &self, encoding: &mut Map, layer: &Layer, - context: &RenderContext, + _context: &RenderContext, ) -> Result<()> { // Read orientation from layer (already resolved during execution) - let is_horizontal = is_transposed(layer, context.scales); + let is_horizontal = is_transposed(layer); // Categorical axis for detail encoding: // - Vertical: x channel (categorical groups on x-axis) @@ -1008,12 +1008,12 @@ impl BoxplotRenderer { layer: &Layer, base_key: &str, has_outliers: bool, - context: &RenderContext, + _context: &RenderContext, ) -> Result> { let mut layers: Vec = Vec::new(); // Read orientation from layer (already resolved during execution) - let is_horizontal = is_transposed(layer, context.scales); + let is_horizontal = is_transposed(layer); // Value columns depend on orientation (after DataFrame column flip): // - Vertical: values in pos2/pos2end (no flip) diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index e689e478..68f936e9 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -1139,11 +1139,8 @@ impl Writer for VegaLiteWriter { layer.validate_required_aesthetics().map_err(|e| { GgsqlError::ValidationError(format!("Layer validation failed: {}", e)) })?; - - // Check SETTING parameters are valid for this geom - layer.validate_settings().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. } Ok(()) From 9d0a6054b319d12ca12422231fdd1a77f7f8cd43 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 11 Mar 2026 13:31:05 +0100 Subject: [PATCH 13/16] clean up --- src/writer/vegalite/layer.rs | 6 +----- src/writer/vegalite/mod.rs | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index b0aa85b4..3f725a24 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -244,7 +244,6 @@ pub trait GeomRenderer: Send + Sync { _layer: &Layer, _data_key: &str, _prepared: &PreparedData, - _context: &RenderContext, ) -> Result> { Ok(vec![layer_spec]) } @@ -878,7 +877,6 @@ impl GeomRenderer for ErrorBarRenderer { layer: &Layer, _data_key: &str, _prepared: &PreparedData, - _context: &RenderContext, ) -> Result> { // Get width parameter (in points) let width = if let Some(ParameterValue::Number(num)) = layer.parameters.get("width") { @@ -1008,7 +1006,6 @@ impl BoxplotRenderer { layer: &Layer, base_key: &str, has_outliers: bool, - _context: &RenderContext, ) -> Result> { let mut layers: Vec = Vec::new(); @@ -1227,7 +1224,6 @@ impl GeomRenderer for BoxplotRenderer { layer: &Layer, data_key: &str, prepared: &PreparedData, - context: &RenderContext, ) -> Result> { let PreparedData::Composite { metadata, .. } = prepared else { return Err(GgsqlError::InternalError( @@ -1239,7 +1235,7 @@ impl GeomRenderer for BoxplotRenderer { GgsqlError::InternalError("Failed to downcast boxplot metadata".to_string()) })?; - self.render_layers(prototype, layer, data_key, info.has_outliers, context) + self.render_layers(prototype, layer, data_key, info.has_outliers) } } diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index 68f936e9..649b9ebd 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -196,7 +196,7 @@ fn build_layers( renderer.modify_spec(&mut layer_spec, layer, &context)?; // Finalize the layer (may expand into multiple layers for composite geoms) - let final_layers = renderer.finalize(layer_spec, layer, data_key, prepared, &context)?; + let final_layers = renderer.finalize(layer_spec, layer, data_key, prepared)?; layers.extend(final_layers); } From b1b0b5ba230288572879c93e53b02367b0d62005 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 11 Mar 2026 13:39:27 +0100 Subject: [PATCH 14/16] more cleaning --- src/writer/vegalite/layer.rs | 6 ------ src/writer/vegalite/mod.rs | 5 +---- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index 3f725a24..316dc8bb 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -192,8 +192,6 @@ pub trait GeomRenderer: Send + Sync { df: &DataFrame, _data_key: &str, binned_columns: &HashMap>, - _layer: &Layer, - _context: &RenderContext, ) -> Result { let values = if binned_columns.is_empty() { dataframe_to_values(df)? @@ -433,8 +431,6 @@ impl GeomRenderer for LinearRenderer { df: &DataFrame, _data_key: &str, _binned_columns: &HashMap>, - _layer: &Layer, - _context: &RenderContext, ) -> Result { // Just convert DataFrame to JSON values // No need to add xmin/xmax - they'll be encoded as literal values @@ -1202,8 +1198,6 @@ impl GeomRenderer for BoxplotRenderer { df: &DataFrame, _data_key: &str, binned_columns: &HashMap>, - _layer: &Layer, - _context: &RenderContext, ) -> Result { let (components, has_outliers) = self.prepare_components(df, binned_columns)?; diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index 649b9ebd..f35ea71e 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -83,9 +83,6 @@ fn prepare_layer_data( let mut layer_renderers: Vec> = Vec::new(); let mut prepared_data: Vec = Vec::new(); - // Build context once for all layers - let context = layer::RenderContext::new(&spec.scales); - for (layer_idx, layer) in spec.layers.iter().enumerate() { let data_key = &layer_data_keys[layer_idx]; let df = data.get(data_key).ok_or_else(|| { @@ -100,7 +97,7 @@ fn prepare_layer_data( let renderer = get_renderer(&layer.geom); // Prepare data using the renderer (handles both standard and composite cases) - let prepared = renderer.prepare_data(df, data_key, binned_columns, layer, &context)?; + let prepared = renderer.prepare_data(df, data_key, binned_columns)?; // Add data to individual datasets based on prepared type match &prepared { From 35e7eb0a523ed5fe1ee9605aa41819e516c32932 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 11 Mar 2026 13:48:55 +0100 Subject: [PATCH 15/16] make sure binned are considered discrete in the context of orientation --- src/plot/layer/orientation.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/plot/layer/orientation.rs b/src/plot/layer/orientation.rs index c8fb0fa1..287d4eac 100644 --- a/src/plot/layer/orientation.rs +++ b/src/plot/layer/orientation.rs @@ -202,12 +202,15 @@ fn is_continuous_scale(scale: &Scale) -> bool { .is_some_and(|st| st.scale_type_kind() == ScaleTypeKind::Continuous) } -/// Check if a scale is discrete (categorical/ordinal). +/// Check if a scale is discrete for orientation purposes. +/// +/// Includes categorical, ordinal, and binned scales - all represent +/// discrete categories rather than continuous values. fn is_discrete_scale(scale: &Scale) -> bool { scale.scale_type.as_ref().is_some_and(|st| { matches!( st.scale_type_kind(), - ScaleTypeKind::Discrete | ScaleTypeKind::Ordinal + ScaleTypeKind::Discrete | ScaleTypeKind::Ordinal | ScaleTypeKind::Binned ) }) } From fcb1b4d0795f4b7e5df53b3b4ff093e0b3268176 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Wed, 11 Mar 2026 13:55:53 +0100 Subject: [PATCH 16/16] simplify flipping --- src/execute/layer.rs | 16 ++++----- src/execute/mod.rs | 4 ++- src/plot/layer/orientation.rs | 61 +++++++++++------------------------ 3 files changed, 29 insertions(+), 52 deletions(-) diff --git a/src/execute/layer.rs b/src/execute/layer.rs index c8b7fe92..63a46c1f 100644 --- a/src/execute/layer.rs +++ b/src/execute/layer.rs @@ -5,7 +5,7 @@ use crate::plot::aesthetic::{is_positional_aesthetic, AestheticContext}; use crate::plot::layer::is_transposed; -use crate::plot::layer::orientation::{flip_mappings, resolve_orientation}; +use crate::plot::layer::orientation::{flip_positional_aesthetics, resolve_orientation}; use crate::plot::{ AestheticValue, DefaultAestheticValue, Layer, ParameterValue, Scale, Schema, SqlTypeNames, StatResult, @@ -356,7 +356,7 @@ pub fn apply_layer_transforms( where F: Fn(&str) -> Result, { - use crate::plot::layer::orientation::{flip_mappings, flip_remappings}; + use crate::plot::layer::orientation::flip_positional_aesthetics; // Clone order_by early to avoid borrow conflicts let order_by = layer.order_by.clone(); @@ -433,7 +433,7 @@ where // We flip them to aligned orientation so they're uniform with defaults. // At the end, we flip everything back together. if needs_flip { - flip_remappings(layer); + flip_positional_aesthetics(&mut layer.remappings.aesthetics); } // Apply literal default remappings from geom defaults (e.g., y2 => 0.0 for bar baseline). @@ -580,10 +580,10 @@ where // later in mod.rs after apply_remappings_post_query creates the columns, // so that Phase 4.5 can flip those columns along with everything else. if needs_flip { - flip_mappings(layer); + flip_positional_aesthetics(&mut layer.mappings.aesthetics); // Normalize mapping column names to match their aesthetic keys. - // After flip_mappings, pos1 might point to __ggsql_aes_pos2__ (and vice versa). + // After flipping, pos1 might point to __ggsql_aes_pos2__ (and vice versa). // We update the column names so pos1 → __ggsql_aes_pos1__, etc. // The DataFrame columns will be renamed correspondingly in mod.rs. normalize_mapping_column_names(layer); @@ -601,11 +601,11 @@ where /// Normalize mapping column names to match their aesthetic keys after flip-back. /// -/// After `flip_mappings`, the mapping values (column names) may not match the keys. +/// After flipping positional aesthetics, the mapping values (column names) may not match the keys. /// For example, pos1 might point to `__ggsql_aes_pos2__`. /// This function updates the column names so pos1 → `__ggsql_aes_pos1__`, etc. /// -/// This should be called after flip_mappings during flip-back. +/// This should be called after flipping during flip-back. /// The DataFrame columns should be renamed correspondingly using `flip_dataframe_columns`. fn normalize_mapping_column_names(layer: &mut Layer) { // Collect the aesthetics to update (to avoid borrowing issues) @@ -710,7 +710,7 @@ pub fn resolve_orientations( ParameterValue::String(orientation.to_string()), ); if is_transposed(layer) { - flip_mappings(layer); + flip_positional_aesthetics(&mut layer.mappings.aesthetics); // Also flip column names in type_info to match the flipped mappings if layer_idx < layer_type_info.len() { for (name, _, _) in &mut layer_type_info[layer_idx] { diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 2773bcdd..36a30191 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -1165,7 +1165,9 @@ pub fn prepare_data_with_reader(query: &str, reader: &R) -> Result bool { }) } -/// Swap positional aesthetic pairs in layer mappings. +/// Swap positional aesthetic pairs in an aesthetics map. /// /// Swaps the following pairs: /// - pos1 ↔ pos2 @@ -224,11 +224,11 @@ fn is_discrete_scale(scale: &Scale) -> bool { /// - pos1end ↔ pos2end /// - pos1offset ↔ pos2offset /// -/// This is called before stat transforms for Secondary orientation layers, -/// so stats always see "standard" orientation. After stat transforms, -/// this is called again to flip back to the correct output positions. -pub fn flip_mappings(layer: &mut Layer) { - let pairs = [ +/// Used for both mappings and remappings when handling transposed orientation. +pub fn flip_positional_aesthetics( + aesthetics: &mut std::collections::HashMap, +) { + const PAIRS: [(&str, &str); 5] = [ ("pos1", "pos2"), ("pos1min", "pos2min"), ("pos1max", "pos2max"), @@ -236,40 +236,15 @@ pub fn flip_mappings(layer: &mut Layer) { ("pos1offset", "pos2offset"), ]; - for (a, b) in pairs { - let val_a = layer.mappings.aesthetics.remove(a); - let val_b = layer.mappings.aesthetics.remove(b); + for (a, b) in PAIRS { + let val_a = aesthetics.remove(a); + let val_b = aesthetics.remove(b); if let Some(v) = val_a { - layer.mappings.aesthetics.insert(b.to_string(), v); + aesthetics.insert(b.to_string(), v); } if let Some(v) = val_b { - layer.mappings.aesthetics.insert(a.to_string(), v); - } - } -} - -/// Swap positional aesthetic pairs in remappings. -/// -/// Same as flip_mappings but for remappings (stat output mappings). -pub fn flip_remappings(layer: &mut Layer) { - let pairs = [ - ("pos1", "pos2"), - ("pos1min", "pos2min"), - ("pos1max", "pos2max"), - ("pos1end", "pos2end"), - ("pos1offset", "pos2offset"), - ]; - - for (a, b) in pairs { - let val_a = layer.remappings.aesthetics.remove(a); - let val_b = layer.remappings.aesthetics.remove(b); - - if let Some(v) = val_a { - layer.remappings.aesthetics.insert(b.to_string(), v); - } - if let Some(v) = val_b { - layer.remappings.aesthetics.insert(a.to_string(), v); + aesthetics.insert(a.to_string(), v); } } } @@ -411,7 +386,7 @@ mod tests { } #[test] - fn test_flip_mappings() { + fn test_flip_positional_aesthetics() { let mut layer = Layer::new(Geom::bar()); layer.mappings.insert( "pos1".to_string(), @@ -426,7 +401,7 @@ mod tests { AestheticValue::standard_column("x2".to_string()), ); - flip_mappings(&mut layer); + flip_positional_aesthetics(&mut layer.mappings.aesthetics); // pos1 ↔ pos2 assert_eq!( @@ -446,15 +421,15 @@ mod tests { } #[test] - fn test_flip_mappings_empty() { + fn test_flip_positional_aesthetics_empty() { let mut layer = Layer::new(Geom::point()); // No crash with empty mappings - flip_mappings(&mut layer); + flip_positional_aesthetics(&mut layer.mappings.aesthetics); assert!(layer.mappings.aesthetics.is_empty()); } #[test] - fn test_flip_mappings_partial() { + fn test_flip_positional_aesthetics_partial() { let mut layer = Layer::new(Geom::bar()); // Only pos1 mapped layer.mappings.insert( @@ -462,7 +437,7 @@ mod tests { AestheticValue::standard_column("x".to_string()), ); - flip_mappings(&mut layer); + flip_positional_aesthetics(&mut layer.mappings.aesthetics); // pos1 moves to pos2 assert!(layer.mappings.get("pos1").is_none());