apply last'ish changes from old PR

thomasp85 · thomasp85 · commit b134dbae38cf · 2026-03-10T21:28:39.000+01:00
diff --git 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<String, crate::plot::ParameterValue>,
+        _execute_query: &dyn Fn(&str) -> crate::Result<polars::prelude::DataFrame>,
+    ) -> crate::Result<StatResult> {
+        // 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
@@ -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<String, crate::plot::ParameterValue>,
+        _execute_query: &dyn Fn(&str) -> crate::Result<polars::prelude::DataFrame>,
+    ) -> crate::Result<StatResult> {
+        // 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
@@ -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
@@ -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<String, crate::plot::ParameterValue>,
+        _execute_query: &dyn Fn(&str) -> crate::Result<polars::prelude::DataFrame>,
+    ) -> crate::Result<StatResult> {
+        // 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
@@ -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
@@ -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::<Vec<_>>()
+        );
+        eprintln!(
+            "Layer mappings: {:?}",
+            layer.mappings.aesthetics.keys().collect::<Vec<_>>()
+        );
+
+        // 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
@@ -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<String, Value>) -> Result<Vec<Value>
     // 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<String, Value>) -> Result<Vec<Value>
                     // 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

Original file line number	Diff line number	Diff line change
`@@ -221,11 +221,14 @@ pub trait GeomTrait: std::fmt::Debug + std::fmt::Display + Send + Sync {`
`221`	`221`	`/// Returns valid parameter names for SETTING clause.`
`222`	`222`	`///`
`223`	`223`	`/// Combines supported aesthetics with non-aesthetic parameters.`
	`224`	`+ /// Includes "orientation" for all geoms (explicit override for auto-detection).`
`224`	`225`	`fn valid_settings(&self) -> Vec<&'static str> {`
`225`	`226`	`let mut valid: Vec<&'static str> = self.aesthetics().supported();`
`226`	`227`	`for param in self.default_params() {`
`227`	`228`	`valid.push(param.name);`
`228`	`229`	`}`
	`230`	`+ // All geoms accept orientation parameter for explicit override`
	`231`	`+ valid.push("orientation");`
`229`	`232`	`valid`
`230`	`233`	`}`
`231`	`234`	`}`