Merge commit 'd6f6765a705689fe4106f7fac9e35e5884a9d142'

Author: thomasp85

CLAUDE.md

@@ -449,7 +449,7 @@ pub struct Theme {
 - `Layer::with_parameter(parameter, value)` - Add a geom parameter (builder pattern)
 - `Layer::get_column(aesthetic)` - Get column name for an aesthetic (if mapped to column)
 - `Layer::get_literal(aesthetic)` - Get literal value for an aesthetic (if literal)
-- `Layer::validate_required_aesthetics()` - Validate that required aesthetics are present for the geom type
+- `Layer::validate_mapping()` - Validate that required aesthetics are present for the geom type
 
 **Type conversions:**
 

src/execute/mod.rs

@@ -49,14 +49,18 @@ use crate::reader::DuckDBReader;
 /// - Partition_by columns exist in schema
 /// - Remapping target aesthetics are supported by geom
 /// - Remapping source columns are valid stat columns for geom
-fn validate(layers: &[Layer], layer_schemas: &[Schema]) -> Result<()> {
+fn validate(
+    layers: &[Layer],
+    layer_schemas: &[Schema],
+    aesthetic_context: &Option<AestheticContext>,
+) -> Result<()> {
     for (idx, (layer, schema)) in layers.iter().zip(layer_schemas.iter()).enumerate() {
         let schema_columns: HashSet<&str> = schema.iter().map(|c| c.name.as_str()).collect();
         let supported = layer.geom.aesthetics().supported();
 
         // Validate required aesthetics for this geom
         layer
-            .validate_required_aesthetics()
+            .validate_mapping(aesthetic_context, false)
             .map_err(|e| GgsqlError::ValidationError(format!("Layer {}: {}", idx + 1, e)))?;
 
         // Validate SETTING parameters are valid for this geom
@@ -1029,7 +1033,11 @@ pub fn prepare_data_with_reader<R: Reader>(query: &str, reader: &R) -> Result<Pr
 
     // Validate all layers against their schemas
     // This must happen BEFORE build_layer_query because stat transforms remove consumed aesthetics
-    validate(&specs[0].layers, &layer_schemas)?;
+    validate(
+        &specs[0].layers,
+        &layer_schemas,
+        &specs[0].aesthetic_context,
+    )?;
 
     // Create scales for all mapped aesthetics that don't have explicit SCALE clauses
     scale::create_missing_scales(&mut specs[0]);
@@ -2503,8 +2511,8 @@ mod tests {
         match result {
             Err(GgsqlError::ValidationError(msg)) => {
                 assert!(
-                    msg.contains("pos2"),
-                    "Error should mention missing pos2 aesthetic: {}",
+                    msg.contains("y"),
+                    "Error should mention missing y aesthetic: {}",
                     msg
                 );
             }
@@ -2645,4 +2653,88 @@ mod tests {
             "line layer should not have fill due to null AS fill"
         );
     }
+
+    // ========================================================================
+    // Validation Error Message Tests (User-facing aesthetic names)
+    // ========================================================================
+
+    #[cfg(feature = "duckdb")]
+    #[test]
+    fn test_validation_error_shows_user_facing_names_for_missing_aesthetics() {
+        // Test that validation errors show user-facing names (x, y) instead of internal (pos1, pos2)
+        let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap();
+
+        reader
+            .connection()
+            .execute(
+                "CREATE TABLE test_data AS SELECT * FROM (VALUES (1, 2)) AS t(a, b)",
+                duckdb::params![],
+            )
+            .unwrap();
+
+        // Query missing required aesthetic 'y' - should show 'y' not 'pos2'
+        let query = r#"
+            SELECT * FROM test_data
+            VISUALISE
+            DRAW point MAPPING a AS x
+        "#;
+
+        let result = prepare_data_with_reader(query, &reader);
+        assert!(result.is_err(), "Expected validation error");
+
+        let err_msg = match result {
+            Err(e) => e.to_string(),
+            Ok(_) => panic!("Expected error"),
+        };
+        assert!(
+            err_msg.contains("`y`"),
+            "Error should mention user-facing name 'y', got: {}",
+            err_msg
+        );
+        assert!(
+            !err_msg.contains("pos2"),
+            "Error should not mention internal name 'pos2', got: {}",
+            err_msg
+        );
+    }
+
+    #[cfg(feature = "duckdb")]
+    #[test]
+    fn test_validation_error_shows_user_facing_names_for_unsupported_aesthetics() {
+        // Test that validation errors show user-facing names for unsupported aesthetics
+        let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap();
+
+        reader
+            .connection()
+            .execute(
+                "CREATE TABLE test_data AS SELECT * FROM (VALUES (1, 2, 3)) AS t(a, b, c)",
+                duckdb::params![],
+            )
+            .unwrap();
+
+        // Query with unsupported aesthetic 'xmin' for point - should show 'xmin' not 'pos1min'
+        let query = r#"
+            SELECT * FROM test_data
+            VISUALISE
+            DRAW point MAPPING a AS x, b AS y, c AS xmin
+        "#;
+
+        let result = prepare_data_with_reader(query, &reader);
+        assert!(result.is_err(), "Expected validation error");
+
+        let err_msg = match result {
+            Err(e) => e.to_string(),
+            Ok(_) => panic!("Expected error"),
+        };
+        assert!(
+            err_msg.contains("`xmin`"),
+            "Error should mention user-facing name 'xmin', got: {}",
+            err_msg
+        );
+        assert!(
+            !err_msg.contains("pos1min"),
+            "Error should not mention internal name 'pos1min', got: {}",
+            err_msg
+        );
+    }
 }

src/plot/aesthetic.rs

@@ -240,6 +240,36 @@ impl AestheticContext {
         }
     }
 
+    /// Map internal aesthetic to user-facing name (reverse of map_user_to_internal).
+    ///
+    /// Positional: "pos1" → "x", "pos2min" → "ymin", "pos1" → "theta" (for polar)
+    /// Facet: "facet1" → "panel" (wrap), "facet1" → "row" (grid), "facet2" → "column" (grid)
+    /// Non-positional: "color" → "color" (unchanged)
+    ///
+    /// Returns None if the internal aesthetic is not recognized.
+    pub fn map_internal_to_user(&self, internal_aesthetic: &str) -> String {
+        // Check internal facet (facet1, facet2)
+        if let Some(idx) = self
+            .internal_facet
+            .iter()
+            .position(|i| i == internal_aesthetic)
+        {
+            return self.user_facet[idx].to_string();
+        }
+
+        // Check internal positional (pos1, pos1min, pos2, etc.)
+        // Iterate through user_to_internal to find reverse mapping
+        for (user, internal) in &self.user_to_internal {
+            if internal == internal_aesthetic {
+                return user.to_string();
+            }
+        }
+
+        // Non-positional aesthetics (color, size, etc.)
+        // Internal is the same as external
+        internal_aesthetic.to_string()
+    }
+
     // === Checking (O(1) HashMap lookups) ===
 
     /// Check if internal aesthetic is primary positional (pos1, pos2, ...)
@@ -633,6 +663,70 @@ mod tests {
         assert_eq!(ctx.primary_internal_positional("color"), Some("color"));
     }
 
+    #[test]
+    fn test_aesthetic_context_internal_to_user_cartesian() {
+        let ctx = AestheticContext::from_static(&["x", "y"], &[]);
+
+        // Primary aesthetics
+        assert_eq!(ctx.map_internal_to_user("pos1"), "x");
+        assert_eq!(ctx.map_internal_to_user("pos2"), "y");
+
+        // Variants
+        assert_eq!(ctx.map_internal_to_user("pos1min"), "xmin");
+        assert_eq!(ctx.map_internal_to_user("pos1max"), "xmax");
+        assert_eq!(ctx.map_internal_to_user("pos1end"), "xend");
+        assert_eq!(ctx.map_internal_to_user("pos2min"), "ymin");
+        assert_eq!(ctx.map_internal_to_user("pos2max"), "ymax");
+        assert_eq!(ctx.map_internal_to_user("pos2end"), "yend");
+
+        // Non-positional aesthetics remain unchanged
+        assert_eq!(ctx.map_internal_to_user("color"), "color");
+        assert_eq!(ctx.map_internal_to_user("size"), "size");
+        assert_eq!(ctx.map_internal_to_user("fill"), "fill");
+    }
+
+    #[test]
+    fn test_aesthetic_context_internal_to_user_polar() {
+        let ctx = AestheticContext::from_static(&["theta", "radius"], &[]);
+
+        // Primary aesthetics map to polar names
+        assert_eq!(ctx.map_internal_to_user("pos1"), "theta");
+        assert_eq!(ctx.map_internal_to_user("pos2"), "radius");
+
+        // Variants
+        assert_eq!(ctx.map_internal_to_user("pos1end"), "thetaend");
+        assert_eq!(ctx.map_internal_to_user("pos2min"), "radiusmin");
+        assert_eq!(ctx.map_internal_to_user("pos2max"), "radiusmax");
+    }
+
+    #[test]
+    fn test_aesthetic_context_internal_to_user_facets() {
+        // Wrap facet (panel)
+        let ctx_wrap = AestheticContext::from_static(&["x", "y"], &["panel"]);
+        assert_eq!(ctx_wrap.map_internal_to_user("facet1"), "panel");
+
+        // Grid facet (row, column)
+        let ctx_grid = AestheticContext::from_static(&["x", "y"], &["row", "column"]);
+        assert_eq!(ctx_grid.map_internal_to_user("facet1"), "row");
+        assert_eq!(ctx_grid.map_internal_to_user("facet2"), "column");
+    }
+
+    #[test]
+    fn test_aesthetic_context_roundtrip() {
+        // Test that user -> internal -> user roundtrips correctly
+        let ctx = AestheticContext::from_static(&["x", "y"], &["panel"]);
+
+        // Positional
+        let internal = ctx.map_user_to_internal("x").unwrap();
+        assert_eq!(ctx.map_internal_to_user(internal), "x");
+
+        let internal = ctx.map_user_to_internal("ymin").unwrap();
+        assert_eq!(ctx.map_internal_to_user(internal), "ymin");
+
+        // Facet
+        let internal = ctx.map_user_to_internal("panel").unwrap();
+        assert_eq!(ctx.map_internal_to_user(internal), "panel");
+    }
     #[test]
     fn test_parse_positional() {
         // Primary positional

src/plot/layer/geom/area.rs

@@ -27,6 +27,7 @@ impl GeomTrait for Area {
                 ("opacity", DefaultAestheticValue::Number(0.8)),
                 ("linewidth", DefaultAestheticValue::Number(1.0)),
                 ("linetype", DefaultAestheticValue::String("solid")),
+                ("pos2end", DefaultAestheticValue::Delayed),
             ],
         }
     }

src/plot/layer/geom/bar.rs

@@ -36,6 +36,7 @@ impl GeomTrait for Bar {
             defaults: &[
                 ("pos1", DefaultAestheticValue::Null), // Optional - stat may provide
                 ("pos2", DefaultAestheticValue::Null), // Optional - stat may compute
+                ("pos2end", DefaultAestheticValue::Delayed),
                 ("weight", DefaultAestheticValue::Null),
                 ("fill", DefaultAestheticValue::String("black")),
                 ("stroke", DefaultAestheticValue::String("black")),

src/plot/layer/geom/density.rs

@@ -49,6 +49,7 @@ impl GeomTrait for Density {
                 ("linewidth", DefaultAestheticValue::Number(1.0)),
                 ("linetype", DefaultAestheticValue::String("solid")),
                 ("pos2", DefaultAestheticValue::Delayed), // Computed by stat
+                ("pos2end", DefaultAestheticValue::Delayed),
             ],
         }
     }

src/plot/layer/geom/histogram.rs

@@ -34,6 +34,7 @@ impl GeomTrait for Histogram {
                 // pos2 and pos1end are produced by stat_histogram but not valid for manual MAPPING
                 ("pos2", DefaultAestheticValue::Delayed),
                 ("pos1end", DefaultAestheticValue::Delayed),
+                ("pos2end", DefaultAestheticValue::Delayed), // baseline value
             ],
         }
     }

src/plot/layer/geom/mod.rs

@@ -529,4 +529,77 @@ mod tests {
         let deserialized: Geom = serde_json::from_str(&json).unwrap();
         assert_eq!(deserialized.geom_type(), GeomType::Point);
     }
+
+    #[test]
+    fn test_default_remappings_are_in_aesthetics() {
+        // Test that every aesthetic in default_remappings() exists in aesthetics().defaults
+        // This ensures that remapped aesthetics are properly declared (usually as Delayed)
+
+        let all_geom_types = [
+            GeomType::Point,
+            GeomType::Line,
+            GeomType::Path,
+            GeomType::Bar,
+            GeomType::Area,
+            GeomType::Rect,
+            GeomType::Polygon,
+            GeomType::Ribbon,
+            GeomType::Histogram,
+            GeomType::Density,
+            GeomType::Smooth,
+            GeomType::Boxplot,
+            GeomType::Violin,
+            GeomType::Text,
+            GeomType::Segment,
+            GeomType::Arrow,
+            GeomType::Rule,
+            GeomType::Linear,
+            GeomType::ErrorBar,
+        ];
+
+        // This test is rigged to trigger a compiler error when new variants are added.
+        // Add the new layer to both the array above and as match arm below.
+        let _exhaustive_check = |t: GeomType| match t {
+            GeomType::Point
+            | GeomType::Line
+            | GeomType::Path
+            | GeomType::Bar
+            | GeomType::Area
+            | GeomType::Rect
+            | GeomType::Polygon
+            | GeomType::Ribbon
+            | GeomType::Histogram
+            | GeomType::Density
+            | GeomType::Smooth
+            | GeomType::Boxplot
+            | GeomType::Violin
+            | GeomType::Text
+            | GeomType::Segment
+            | GeomType::Arrow
+            | GeomType::Rule
+            | GeomType::Linear
+            | GeomType::ErrorBar => {}
+        };
+
+        for geom_type in all_geom_types {
+            let geom = Geom::from_type(geom_type);
+            let remappings = geom.default_remappings();
+            let aesthetics = geom.aesthetics();
+
+            // Collect all aesthetic names from aesthetics().defaults
+            let aesthetic_names: std::collections::HashSet<&str> =
+                aesthetics.defaults.iter().map(|(name, _)| *name).collect();
+
+            // Check each remapping name exists in aesthetics
+            for (name, _) in remappings.defaults {
+                assert!(
+                    aesthetic_names.contains(name),
+                    "Geom '{}' has '{}' in default_remappings() but not in aesthetics().defaults. \
+                     Add it as DefaultAestheticValue::Delayed if it's a stat-produced aesthetic.",
+                    geom_type,
+                    name
+                );
+            }
+        }
+    }
 }