Skip to content

Commit 385f4db

Browse files
authored
Colour semantics (#54)
* standardise spelling of the colour aesthetic * add stroke aesthetic * forgot renaming aes * Dance to clippy's puppet strings * use `.to_string()` instead * tests expect colour to be split up to stroke/fill * rework normalisation of aesthetic names * rework trickle-down defaults * move splitting to after literal columns have been materialised * remove leftover debug statement * Fix quotemarks
1 parent 440b050 commit 385f4db

29 files changed

Lines changed: 161 additions & 55 deletions

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -905,7 +905,7 @@ All clauses (MAPPING, SETTING, PARTITION BY, FILTER) are optional.
905905
Maps data values (columns or literals) to visual aesthetics. Syntax: `value AS aesthetic`
906906

907907
- **Position**: `x`, `y`, `xmin`, `xmax`, `ymin`, `ymax`
908-
- **Color**: `color`, `fill`, `opacity`
908+
- **Color**: `color`, `fill`, `stroke`, `opacity`
909909
- **Size/Shape**: `size`, `shape`, `linetype`, `linewidth`
910910
- **Text**: `label`, `family`, `fontface`
911911

ggsql-vscode/syntaxes/ggsql.tmLanguage.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@
177177
},
178178
{
179179
"name": "variable.parameter.aesthetic.ggsql",
180-
"match": "\\b(x|y|xmin|xmax|ymin|ymax|xend|yend|color|colour|fill|opacity|size|shape|linetype|linewidth|width|height|label|family|fontface|hjust|vjust)\\b"
180+
"match": "\\b(x|y|xmin|xmax|ymin|ymax|xend|yend|color|colour|fill|stroke|opacity|size|shape|linetype|linewidth|width|height|label|family|fontface|hjust|vjust)\\b"
181181
}
182182
]
183183
},

src/execute.rs

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -728,6 +728,7 @@ where
728728
/// 3. Expands wildcards by adding mappings only for supported aesthetics that:
729729
/// - Are not already mapped (either from global or layer)
730730
/// - Have a matching column in the layer's schema
731+
/// 4. Moreover it propagates 'color' to 'fill' and 'stroke'
731732
fn merge_global_mappings_into_layers(specs: &mut [Plot], layer_schemas: &[Schema]) {
732733
for spec in specs {
733734
for (layer, schema) in spec.layers.iter_mut().zip(layer_schemas.iter()) {
@@ -754,7 +755,7 @@ fn merge_global_mappings_into_layers(specs: &mut [Plot], layer_schemas: &[Schema
754755
layer
755756
.mappings
756757
.aesthetics
757-
.entry(aes.to_string())
758+
.entry(crate::parser::builder::normalise_aes_name(aes))
758759
.or_insert(AestheticValue::standard_column(aes));
759760
}
760761
}
@@ -834,6 +835,27 @@ fn with_has_trailing_select(with_node: &Node) -> bool {
834835
false
835836
}
836837

838+
// Let 'color' aesthetics fill defaults for the 'stroke' and 'fill' aesthetics
839+
fn split_color_aesthetic(layers: &mut Vec<Layer>) {
840+
for layer in layers {
841+
if !layer.mappings.aesthetics.contains_key("color") {
842+
continue;
843+
}
844+
let supported = layer.geom.aesthetics().supported;
845+
for &aes in &["stroke", "fill"] {
846+
if !supported.contains(&aes) {
847+
continue;
848+
}
849+
let color = layer.mappings.aesthetics.get("color").unwrap().clone();
850+
layer
851+
.mappings
852+
.aesthetics
853+
.entry(aes.to_string())
854+
.or_insert(color);
855+
}
856+
}
857+
}
858+
837859
/// Result of preparing data for visualization
838860
pub struct PreparedData {
839861
/// Data map with global and layer-specific DataFrames
@@ -1089,6 +1111,9 @@ where
10891111
replace_literals_with_columns(spec);
10901112
// Compute aesthetic labels (uses first non-constant column, respects user-specified labels)
10911113
spec.compute_aesthetic_labels();
1114+
// Divide 'color' over 'stroke' and 'fill'. This needs to happens after
1115+
// literals have associated columns.
1116+
split_color_aesthetic(&mut spec.layers);
10921117
}
10931118

10941119
Ok(PreparedData {
@@ -2682,4 +2707,59 @@ mod tests {
26822707

26832708
assert!(y_values.contains(&30.0), "Should have sum values");
26842709
}
2710+
2711+
#[cfg(feature = "duckdb")]
2712+
#[test]
2713+
fn test_expansion_of_color_aesthetic() {
2714+
let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap();
2715+
2716+
// Colors as standard columns
2717+
let query = r#"
2718+
VISUALISE bill_len AS x, bill_dep AS y FROM ggsql:penguins
2719+
DRAW point MAPPING species AS color, island AS fill
2720+
"#;
2721+
2722+
let result = prepare_data(query, &reader).unwrap();
2723+
2724+
let aes = &result.specs[0].layers[0].mappings.aesthetics;
2725+
2726+
assert!(aes.contains_key("stroke"));
2727+
assert!(aes.contains_key("fill"));
2728+
2729+
let stroke = aes.get("stroke").unwrap().column_name().unwrap();
2730+
assert_eq!(stroke, "species");
2731+
2732+
let fill = aes.get("fill").unwrap().column_name().unwrap();
2733+
assert_eq!(fill, "island");
2734+
2735+
// Colors as global constant
2736+
let query = r#"
2737+
VISUALISE bill_len AS x, bill_dep AS y, 'blue' AS color FROM ggsql:penguins
2738+
DRAW point MAPPING island AS stroke
2739+
"#;
2740+
2741+
let result = prepare_data(query, &reader).unwrap();
2742+
let aes = &result.specs[0].layers[0].mappings.aesthetics;
2743+
2744+
let stroke = aes.get("stroke").unwrap();
2745+
assert_eq!(stroke.column_name().unwrap(), "island");
2746+
2747+
let fill = aes.get("fill").unwrap();
2748+
assert_eq!(fill.column_name().unwrap(), "__ggsql_const_color_0__");
2749+
2750+
// Colors as layer constant
2751+
let query = r#"
2752+
VISUALISE bill_len AS x, bill_dep AS y, island AS fill FROM ggsql:penguins
2753+
DRAW point MAPPING 'blue' AS color
2754+
"#;
2755+
2756+
let result = prepare_data(query, &reader).unwrap();
2757+
let aes = &result.specs[0].layers[0].mappings.aesthetics;
2758+
2759+
let stroke = aes.get("stroke").unwrap();
2760+
assert_eq!(stroke.column_name().unwrap(), "__ggsql_const_color_0__");
2761+
2762+
let fill = aes.get("fill").unwrap();
2763+
assert_eq!(fill.column_name().unwrap(), "island");
2764+
}
26852765
}

src/lib.rs

Lines changed: 34 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -524,8 +524,8 @@ mod integration_tests {
524524
let query = r#"
525525
SELECT 1 as x, 10 as y
526526
VISUALISE x, y
527-
DRAW line MAPPING 'value' AS color
528-
DRAW point MAPPING 'value2' AS color
527+
DRAW line MAPPING 'value' AS linetype
528+
DRAW point MAPPING 'value2' AS shape
529529
"#;
530530

531531
// Prepare data - this parses, injects constants into global data, and replaces literals with columns
@@ -553,12 +553,12 @@ mod integration_tests {
553553
let global_df = prepared.data.get(naming::GLOBAL_DATA_KEY).unwrap();
554554
let col_names = global_df.get_column_names();
555555
assert!(
556-
col_names.iter().any(|c| *c == "__ggsql_const_color_0__"),
556+
col_names.iter().any(|c| *c == "__ggsql_const_linetype_0__"),
557557
"Global data should have layer 0 color constant: {:?}",
558558
col_names
559559
);
560560
assert!(
561-
col_names.iter().any(|c| *c == "__ggsql_const_color_1__"),
561+
col_names.iter().any(|c| *c == "__ggsql_const_shape_1__"),
562562
"Global data should have layer 1 color constant: {:?}",
563563
col_names
564564
);
@@ -572,18 +572,18 @@ mod integration_tests {
572572
assert_eq!(vl_spec["layer"].as_array().unwrap().len(), 2);
573573

574574
// Verify the color aesthetic is mapped to layer-indexed synthetic columns
575-
let layer0_color = &vl_spec["layer"][0]["encoding"]["color"];
576-
let layer1_color = &vl_spec["layer"][1]["encoding"]["color"];
575+
let layer0_color = &vl_spec["layer"][0]["encoding"]["linetype"];
576+
let layer1_color = &vl_spec["layer"][1]["encoding"]["shape"];
577577

578-
// Color should be field-mapped to layer-indexed columns
578+
// Constants should be field-mapped to layer-indexed columns
579579
assert_eq!(
580580
layer0_color["field"].as_str().unwrap(),
581-
"__ggsql_const_color_0__",
581+
"__ggsql_const_linetype_0__",
582582
"Layer 0 color should map to layer-indexed column"
583583
);
584584
assert_eq!(
585585
layer1_color["field"].as_str().unwrap(),
586-
"__ggsql_const_color_1__",
586+
"__ggsql_const_shape_1__",
587587
"Layer 1 color should map to layer-indexed column"
588588
);
589589

@@ -594,18 +594,18 @@ mod integration_tests {
594594
// Verify constant values appear in the global data with layer-indexed names
595595
let data_row = &global_data.as_array().unwrap()[0];
596596
assert_eq!(
597-
data_row["__ggsql_const_color_0__"], "value",
597+
data_row["__ggsql_const_linetype_0__"], "value",
598598
"Layer 0 constant should be 'value'"
599599
);
600600
assert_eq!(
601-
data_row["__ggsql_const_color_1__"], "value2",
601+
data_row["__ggsql_const_shape_1__"], "value2",
602602
"Layer 1 constant should be 'value2'"
603603
);
604604
}
605605

606606
#[test]
607-
fn test_end_to_end_facet_with_constant_colors() {
608-
// Test faceting with multiple layers that have constant color mappings
607+
fn test_end_to_end_facet_with_constant_strokes() {
608+
// Test faceting with multiple layers that have constant stroke mappings
609609
// This verifies the fix for faceting compatibility with constants
610610

611611
let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap();
@@ -629,10 +629,10 @@ mod integration_tests {
629629
SELECT month, region, category, revenue, quantity * 10 as qty_scaled
630630
FROM facet_test
631631
VISUALISE month AS x
632-
DRAW line MAPPING revenue AS y, 'value' AS color
633-
DRAW point MAPPING revenue AS y, 'value2' AS color SETTING size => 30
634-
DRAW line MAPPING qty_scaled AS y, 'value3' AS color
635-
DRAW point MAPPING qty_scaled AS y, 'value4' AS color SETTING size => 30
632+
DRAW line MAPPING revenue AS y, 'value' AS stroke
633+
DRAW point MAPPING revenue AS y, 'value2' AS stroke SETTING size => 30
634+
DRAW line MAPPING qty_scaled AS y, 'value3' AS stroke
635+
DRAW point MAPPING qty_scaled AS y, 'value4' AS stroke SETTING size => 30
636636
SCALE x SETTING type => 'date'
637637
FACET region BY category
638638
"#;
@@ -667,19 +667,19 @@ mod integration_tests {
667667
let global_df = prepared.data.get(naming::GLOBAL_DATA_KEY).unwrap();
668668
let col_names = global_df.get_column_names();
669669
assert!(
670-
col_names.iter().any(|c| *c == "__ggsql_const_color_0__"),
670+
col_names.iter().any(|c| *c == "__ggsql_const_stroke_0__"),
671671
"Should have layer 0 color constant"
672672
);
673673
assert!(
674-
col_names.iter().any(|c| *c == "__ggsql_const_color_1__"),
674+
col_names.iter().any(|c| *c == "__ggsql_const_stroke_1__"),
675675
"Should have layer 1 color constant"
676676
);
677677
assert!(
678-
col_names.iter().any(|c| *c == "__ggsql_const_color_2__"),
678+
col_names.iter().any(|c| *c == "__ggsql_const_stroke_2__"),
679679
"Should have layer 2 color constant"
680680
);
681681
assert!(
682-
col_names.iter().any(|c| *c == "__ggsql_const_color_3__"),
682+
col_names.iter().any(|c| *c == "__ggsql_const_stroke_3__"),
683683
"Should have layer 3 color constant"
684684
);
685685

@@ -699,7 +699,7 @@ mod integration_tests {
699699
#[test]
700700
fn test_end_to_end_global_constant_in_visualise() {
701701
// Test that global constants in VISUALISE clause work correctly
702-
// e.g., VISUALISE date AS x, value AS y, 'value' AS color
702+
// e.g., VISUALISE date AS x, value AS y, 'value' AS stroke
703703

704704
let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap();
705705

@@ -716,10 +716,10 @@ mod integration_tests {
716716
)
717717
.unwrap();
718718

719-
// Query with global constant color in VISUALISE clause
719+
// Query with global constant stroke in VISUALISE clause
720720
let query = r#"
721721
SELECT date, value FROM timeseries
722-
VISUALISE date AS x, value AS y, 'value' AS color
722+
VISUALISE date AS x, value AS y, 'value' AS stroke
723723
DRAW line
724724
DRAW point SETTING size => 50
725725
SCALE x SETTING type => 'date'
@@ -738,13 +738,13 @@ mod integration_tests {
738738
let global_df = prepared.data.get(naming::GLOBAL_DATA_KEY).unwrap();
739739
let col_names = global_df.get_column_names();
740740
assert!(
741-
col_names.iter().any(|c| *c == "__ggsql_const_color_0__"),
742-
"Should have layer 0 color constant: {:?}",
741+
col_names.iter().any(|c| *c == "__ggsql_const_stroke_0__"),
742+
"Should have layer 0 stroke constant: {:?}",
743743
col_names
744744
);
745745
assert!(
746-
col_names.iter().any(|c| *c == "__ggsql_const_color_1__"),
747-
"Should have layer 1 color constant: {:?}",
746+
col_names.iter().any(|c| *c == "__ggsql_const_stroke_1__"),
747+
"Should have layer 1 stroke constant: {:?}",
748748
col_names
749749
);
750750

@@ -756,23 +756,23 @@ mod integration_tests {
756756
// Both layers should have color field-mapped to their indexed constant columns
757757
assert_eq!(vl_spec["layer"].as_array().unwrap().len(), 2);
758758
assert_eq!(
759-
vl_spec["layer"][0]["encoding"]["color"]["field"]
759+
vl_spec["layer"][0]["encoding"]["stroke"]["field"]
760760
.as_str()
761761
.unwrap(),
762-
"__ggsql_const_color_0__"
762+
"__ggsql_const_stroke_0__"
763763
);
764764
assert_eq!(
765-
vl_spec["layer"][1]["encoding"]["color"]["field"]
765+
vl_spec["layer"][1]["encoding"]["stroke"]["field"]
766766
.as_str()
767767
.unwrap(),
768-
"__ggsql_const_color_1__"
768+
"__ggsql_const_stroke_1__"
769769
);
770770

771771
// Both constants should have the same value "value"
772772
let data = &vl_spec["datasets"][naming::GLOBAL_DATA_KEY]
773773
.as_array()
774774
.unwrap()[0];
775-
assert_eq!(data["__ggsql_const_color_0__"], "value");
776-
assert_eq!(data["__ggsql_const_color_1__"], "value");
775+
assert_eq!(data["__ggsql_const_stroke_0__"], "value");
776+
assert_eq!(data["__ggsql_const_stroke_1__"], "value");
777777
}
778778
}

src/parser/builder.rs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -387,11 +387,14 @@ fn parse_mapping_element(node: &Node, source: &str, mappings: &mut Mappings) ->
387387
}
388388
"explicit_mapping" => {
389389
let (aesthetic, value) = parse_explicit_mapping(&child, source)?;
390-
mappings.insert(aesthetic, value);
390+
mappings.insert(normalise_aes_name(&aesthetic), value);
391391
}
392392
"implicit_mapping" | "identifier" => {
393393
let name = get_node_text(&child, source);
394-
mappings.insert(&name, AestheticValue::standard_column(&name));
394+
mappings.insert(
395+
normalise_aes_name(&name),
396+
AestheticValue::standard_column(&name),
397+
);
395398
}
396399
_ => continue,
397400
}
@@ -1006,6 +1009,7 @@ fn is_aesthetic_name(name: &str) -> bool {
10061009
| "color"
10071010
| "colour"
10081011
| "fill"
1012+
| "stroke"
10091013
| "opacity"
10101014
| "size"
10111015
| "shape"
@@ -1356,6 +1360,13 @@ fn with_statement_has_trailing_select(with_node: &Node) -> bool {
13561360
false
13571361
}
13581362

1363+
pub fn normalise_aes_name(name: &str) -> String {
1364+
match name {
1365+
"col" | "colour" => "color".to_string(),
1366+
_ => name.to_string(),
1367+
}
1368+
}
1369+
13591370
fn color_to_hex(value: &str) -> String {
13601371
match csscolorparser::parse(value) {
13611372
Ok(value) => value.to_css_hex(),
@@ -2884,6 +2895,7 @@ mod tests {
28842895
assert_eq!(specs[0].layers[0].mappings.len(), 0);
28852896

28862897
// Point layer should have color from layer MAPPING
2898+
// color should expand into stroke and fill
28872899
assert_eq!(specs[0].layers[1].mappings.len(), 1);
28882900
assert!(specs[0].layers[1].mappings.contains_key("color"));
28892901
}

src/plot/layer/geom/abline.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ impl GeomTrait for AbLine {
1818
"intercept",
1919
"color",
2020
"colour",
21+
"stroke",
2122
"linetype",
2223
"linewidth",
2324
"opacity",

src/plot/layer/geom/area.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ impl GeomTrait for Area {
1313

1414
fn aesthetics(&self) -> GeomAesthetics {
1515
GeomAesthetics {
16-
supported: &["x", "y", "color", "colour", "fill", "opacity"],
16+
supported: &["x", "y", "color", "colour", "fill", "stroke", "opacity"],
1717
required: &["x", "y"],
1818
hidden: &[],
1919
}

src/plot/layer/geom/arrow.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ impl GeomTrait for Arrow {
2020
"yend",
2121
"color",
2222
"colour",
23+
"stroke",
2324
"linetype",
2425
"linewidth",
2526
"opacity",

src/plot/layer/geom/bar.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ impl GeomTrait for Bar {
2727
// If y is missing: stat computes COUNT or SUM(weight)
2828
// weight: optional, if mapped uses SUM(weight) instead of COUNT(*)
2929
supported: &[
30-
"x", "y", "weight", "color", "colour", "fill", "width", "opacity",
30+
"x", "y", "weight", "color", "colour", "fill", "stroke", "width", "opacity",
3131
],
3232
required: &[],
3333
hidden: &[],

0 commit comments

Comments
 (0)