Skip to content
Merged
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:**

Expand Down
102 changes: 97 additions & 5 deletions src/execute/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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]);
Expand Down Expand Up @@ -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
);
}
Expand Down Expand Up @@ -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
);
}
}
94 changes: 94 additions & 0 deletions src/plot/aesthetic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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, ...)
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions src/plot/layer/geom/area.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ impl GeomTrait for Area {
("opacity", DefaultAestheticValue::Number(0.8)),
("linewidth", DefaultAestheticValue::Number(1.0)),
("linetype", DefaultAestheticValue::String("solid")),
("pos2end", DefaultAestheticValue::Delayed),
],
}
}
Expand Down
1 change: 1 addition & 0 deletions src/plot/layer/geom/bar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,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")),
Expand Down
1 change: 1 addition & 0 deletions src/plot/layer/geom/density.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ impl GeomTrait for Density {
("linewidth", DefaultAestheticValue::Number(1.0)),
("linetype", DefaultAestheticValue::String("solid")),
("pos2", DefaultAestheticValue::Delayed), // Computed by stat
("pos2end", DefaultAestheticValue::Delayed),
],
}
}
Expand Down
1 change: 1 addition & 0 deletions src/plot/layer/geom/histogram.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,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
],
}
}
Expand Down
73 changes: 73 additions & 0 deletions src/plot/layer/geom/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -527,4 +527,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 {
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
);
}
}
}
}
Loading
Loading