Fix donuts (#309)

* initial attempt

* Use const for default polar size

* add tests

* just use the size regardless of `inner`

* remove obsolete test

Author: teunbrand

src/reader/mod.rs

@@ -1083,35 +1083,6 @@ mod tests {
         );
     }
 
-    #[test]
-    fn test_polar_project_inner_default() {
-        // Test that inner=0 (default) doesn't add scale range
-        let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap();
-        let query = r#"
-            SELECT * FROM (VALUES ('A', 10), ('B', 20)) AS t(category, value)
-            VISUALISE value AS y, category AS fill
-            DRAW bar
-            PROJECT y, x TO polar
-        "#;
-
-        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();
-
-        // Radius encoding should not have scale.range when inner=0
-        let radius = &layer["encoding"]["radius"];
-        if let Some(scale) = radius.get("scale") {
-            assert!(
-                scale.get("range").is_none(),
-                "Radius scale should not have range when inner=0, got: {:?}",
-                scale
-            );
-        }
-    }
-
     #[test]
     fn test_stacked_bar_chart() {
         // Test stacked bar chart via position => 'stack'

src/writer/vegalite/mod.rs

@@ -49,6 +49,10 @@ const POINTS_TO_PIXELS: f64 = 96.0 / 72.0;
 /// So: area_px^2 = pi * (r_pt * POINTS_TO_PIXELS)^2 = pi * r_pt^2 * (96/72)^2
 const POINTS_TO_AREA: f64 = std::f64::consts::PI * POINTS_TO_PIXELS * POINTS_TO_PIXELS;
 
+/// Default size (in pixels) for polar coordinate visualizations
+/// Used as fallback when explicit dimensions are not specified
+pub(super) const DEFAULT_POLAR_SIZE: f64 = 350.0;
+
 /// Split a label string on newlines and return appropriate JSON value
 ///
 /// Returns a JSON array if the string contains multiple lines, or a JSON string
@@ -1104,8 +1108,8 @@ impl Writer for VegaLiteWriter {
                 .as_ref()
                 .is_some_and(|p| p.coord.coord_kind() == CoordKind::Polar);
             if is_polar {
-                vl_spec["width"] = json!(350);
-                vl_spec["height"] = json!(350);
+                vl_spec["width"] = json!(DEFAULT_POLAR_SIZE);
+                vl_spec["height"] = json!(DEFAULT_POLAR_SIZE);
             }
         }
 

src/writer/vegalite/projection.rs

@@ -7,6 +7,8 @@ use crate::plot::{CoordKind, ParameterValue, Projection};
 use crate::{DataFrame, GgsqlError, Plot, Result};
 use serde_json::{json, Value};
 
+use super::DEFAULT_POLAR_SIZE;
+
 /// Apply projection transformations to the spec and data
 /// Returns (possibly transformed DataFrame, possibly modified spec)
 pub(super) fn apply_project_transforms(
@@ -146,6 +148,26 @@ fn convert_geoms_to_polar(
     end_radians: f64,
     inner: f64,
 ) -> Result<()> {
+    let is_faceted = match &spec.facet {
+        Some(facet) => !facet.get_variables().is_empty(),
+        _ => false,
+    };
+
+    let size = if is_faceted {
+        // Try to grab size from spec if available
+        let height = vl_spec.get("height").and_then(|h| h.as_f64());
+        let width = vl_spec.get("width").and_then(|w| w.as_f64());
+
+        Some(match (height, width) {
+            (Some(h), Some(w)) => h.min(w),
+            (Some(h), None) => h,
+            (None, Some(w)) => w,
+            _ => DEFAULT_POLAR_SIZE, // Fallback
+        })
+    } else {
+        None
+    };
+
     if let Some(layers_arr) = get_layers_mut(vl_spec) {
         for layer in layers_arr {
             if let Some(mark) = layer.get_mut("mark") {
@@ -156,13 +178,8 @@ fn convert_geoms_to_polar(
                 if is_arc {
                     // Arc marks natively support radius/theta channels
                     if let Some(encoding) = layer.get_mut("encoding") {
-                        // Remove dummy radius encoding — for pie charts the bar stat
-                        // creates a dummy pos1 column (all rows same value). VL's arc
-                        // mark uses full available radius by default, and a nominal
-                        // radius with a single dummy value breaks faceted arc rendering.
-                        strip_dummy_radius(encoding);
                         apply_polar_angle_range(encoding, start_radians, end_radians)?;
-                        apply_polar_radius_range(encoding, inner)?;
+                        apply_polar_radius_range(encoding, inner, size)?;
                     }
                 } else {
                     // Non-arc marks (point, line): convert polar to cartesian
@@ -385,26 +402,6 @@ fn convert_mark_to_polar(mark: &Value, _spec: &Plot) -> Result<Value> {
     Ok(json!(polar_mark))
 }
 
-/// Remove dummy radius encoding from arc marks.
-///
-/// For pie charts, the bar stat creates a dummy pos1 column where all rows have
-/// the same placeholder value. In cartesian this positions all bars at the same x.
-/// For arc marks, a nominal radius with a single dummy value is unnecessary (VL
-/// uses full available radius by default) and breaks faceted arc rendering.
-/// The dummy is identified by `"axis": null` which is set by build_column_encoding
-/// for dummy columns.
-fn strip_dummy_radius(encoding: &mut Value) {
-    let dominated = encoding
-        .get("radius")
-        .and_then(|r| r.get("axis"))
-        .is_some_and(|a| a.is_null());
-    if dominated {
-        if let Some(enc_obj) = encoding.as_object_mut() {
-            enc_obj.remove("radius");
-        }
-    }
-}
-
 /// Apply angle range to theta encoding for polar projection
 ///
 /// The encoding channels are already correctly named (theta/radius) by
@@ -454,20 +451,23 @@ fn apply_polar_angle_range(
 /// Sets the radius scale range using Vega-Lite expressions for proportional sizing.
 /// The inner parameter (0.0 to 1.0) specifies the inner radius as a proportion
 /// of the outer radius, creating a donut hole.
-fn apply_polar_radius_range(encoding: &mut Value, inner: f64) -> Result<()> {
-    // Skip if no inner radius (full pie)
-    if inner <= f64::EPSILON {
-        return Ok(());
-    }
-
+fn apply_polar_radius_range(encoding: &mut Value, inner: f64, size: Option<f64>) -> Result<()> {
     let enc_obj = encoding
         .as_object_mut()
         .ok_or_else(|| GgsqlError::WriterError("Encoding is not an object".to_string()))?;
 
     // 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 (inner_expr, outer_expr) = match size {
+        Some(dim) => (format!("{}/2*{}", dim, inner), format!("{}/2", dim)),
+        _ => {
+            // min(width,height)/2 is the default max radius in Vega-Lite
+            (
+                format!("min(width,height)/2*{}", inner),
+                "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)
@@ -498,3 +498,69 @@ fn apply_polar_radius_range(encoding: &mut Value, inner: f64) -> Result<()> {
 
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_polar_inner_radius_non_faceted() {
+        // Non-faceted donut should use dynamic min(width,height) expressions
+        let mut encoding = json!({
+            "radius": {
+                "field": "dummy",
+                "type": "nominal",
+                "scale": {"domain": ["dummy"]}
+            }
+        });
+
+        apply_polar_radius_range(&mut encoding, 0.5, None).unwrap();
+
+        let range = encoding["radius"]["scale"]["range"].as_array().unwrap();
+        assert_eq!(range.len(), 2);
+        assert_eq!(
+            range[0]["expr"].as_str().unwrap(),
+            "min(width,height)/2*0.5"
+        );
+        assert_eq!(range[1]["expr"].as_str().unwrap(), "min(width,height)/2");
+    }
+
+    #[test]
+    fn test_polar_inner_radius_faceted() {
+        // Faceted donut should use explicit size calculation
+        let mut encoding = json!({
+            "radius": {
+                "field": "dummy",
+                "type": "nominal",
+                "scale": {"domain": ["dummy"]}
+            }
+        });
+
+        apply_polar_radius_range(&mut encoding, 0.5, Some(350.0)).unwrap();
+
+        let range = encoding["radius"]["scale"]["range"].as_array().unwrap();
+        assert_eq!(range.len(), 2);
+        assert_eq!(range[0]["expr"].as_str().unwrap(), "350/2*0.5");
+        assert_eq!(range[1]["expr"].as_str().unwrap(), "350/2");
+    }
+
+    #[test]
+    fn test_polar_inner_radius_zero() {
+        // inner = 0 should still apply range (full pie, no donut hole)
+        let mut encoding = json!({
+            "radius": {
+                "field": "dummy",
+                "type": "nominal",
+                "scale": {"domain": ["dummy"]}
+            }
+        });
+
+        apply_polar_radius_range(&mut encoding, 0.0, Some(350.0)).unwrap();
+
+        // Range should be [0, 350/2] for full pie
+        let range = encoding["radius"]["scale"]["range"].as_array().unwrap();
+        assert_eq!(range.len(), 2);
+        assert_eq!(range[0]["expr"].as_str().unwrap(), "350/2*0");
+        assert_eq!(range[1]["expr"].as_str().unwrap(), "350/2");
+    }
+}