From b211de7312d24778f8bd0f58023af8e3745bcab0 Mon Sep 17 00:00:00 2001 From: Carson Date: Wed, 4 Mar 2026 12:01:51 -0600 Subject: [PATCH 1/2] fix: strip disallowed properties from secondary encoding channels (x2, y2) Vega-Lite secondary channels (x2, y2, theta2, radius2) only allow a restricted set of properties (field, aggregate, bandPosition, bin, timeUnit, title, value). The VegaLiteWriter was emitting type, scale, and axis on these channels, causing Altair schema validation errors. Added is_secondary_channel() to detect internal aesthetics (pos1end, pos2end) that map to secondary channels, and an early return in build_column_encoding() that emits only the allowed properties. Co-Authored-By: Claude Opus 4.6 --- ggsql-python/tests/test_ggsql.py | 27 +++++++++++++++++++++++++++ src/plot/aesthetic.rs | 16 ++++++++++++++++ src/writer/vegalite/encoding.rs | 23 ++++++++++++++++++++++- 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/ggsql-python/tests/test_ggsql.py b/ggsql-python/tests/test_ggsql.py index 8b12f103..a6e9a3e7 100644 --- a/ggsql-python/tests/test_ggsql.py +++ b/ggsql-python/tests/test_ggsql.py @@ -194,6 +194,33 @@ def test_render_multi_layer(self): assert "layer" in spec_dict +class TestSecondaryChannelEncoding: + """Tests for Vega-Lite secondary channel encoding (x2, y2).""" + + def test_bar_y2_has_no_disallowed_properties(self): + """Bar chart y2 encoding should not have type, scale, or axis properties.""" + reader = ggsql.DuckDBReader("duckdb://memory") + df = pl.DataFrame({"survived": [0, 1, 1, 0, 0, 1, 0, 1, 0, 0]}) + reader.register("titanic", df) + spec = reader.execute( + "SELECT survived FROM titanic " + "VISUALISE survived AS x " + "DRAW bar " + "LABEL title => 'Survival Count'" + ) + writer = ggsql.VegaLiteWriter() + vl = json.loads(writer.render(spec)) + + # Find y2 in any layer + for layer in vl.get("layer", [vl]): + y2 = layer.get("encoding", {}).get("y2") + if y2 is not None: + assert "axis" not in y2, f"y2 should not have 'axis': {y2}" + assert "scale" not in y2, f"y2 should not have 'scale': {y2}" + assert "type" not in y2, f"y2 should not have 'type': {y2}" + assert "field" in y2, f"y2 should have 'field': {y2}" + + class TestRenderAltairDataFrameConversion: """Tests for DataFrame handling in render_altair().""" diff --git a/src/plot/aesthetic.rs b/src/plot/aesthetic.rs index b6253ed8..44c0a096 100644 --- a/src/plot/aesthetic.rs +++ b/src/plot/aesthetic.rs @@ -331,6 +331,22 @@ pub fn is_facet_aesthetic(aesthetic: &str) -> bool { false } +/// Check if an internal aesthetic maps to a Vega-Lite **secondary** encoding channel +/// (x2, y2, theta2, radius2). +/// +/// Vega-Lite secondary channels only support a restricted set of properties +/// (`field`, `aggregate`, `bandPosition`, `bin`, `timeUnit`, `title`, `value`). +/// Properties like `type`, `scale`, and `axis` must not be emitted for these. +/// +/// Matches internal names like `pos1end`, `pos2end`, etc. +#[inline] +pub fn is_secondary_channel(name: &str) -> bool { + name.starts_with("pos") && name.ends_with("end") && { + let mid = &name[3..name.len() - 3]; + !mid.is_empty() && mid.chars().all(|c| c.is_ascii_digit()) + } +} + /// Check if aesthetic is an internal positional (pos1, pos1min, pos2max, etc.) /// /// This function works with **internal** aesthetic names after transformation. diff --git a/src/writer/vegalite/encoding.rs b/src/writer/vegalite/encoding.rs index f91ace3a..cabcb57d 100644 --- a/src/writer/vegalite/encoding.rs +++ b/src/writer/vegalite/encoding.rs @@ -3,7 +3,7 @@ //! This module handles building Vega-Lite encoding channels from ggsql aesthetic mappings, //! including type inference, scale properties, and title handling. -use crate::plot::aesthetic::{is_positional_aesthetic, AestheticContext}; +use crate::plot::aesthetic::{is_positional_aesthetic, is_secondary_channel, AestheticContext}; use crate::plot::scale::{linetype_to_stroke_dash, shape_to_svg_path, ScaleTypeKind}; use crate::plot::{CoordKind, ParameterValue}; use crate::{AestheticValue, DataFrame, Plot, Result}; @@ -819,6 +819,27 @@ fn build_column_encoding( let primary = aesthetic_ctx .primary_internal_positional(aesthetic) .unwrap_or(aesthetic); + + // Vega-Lite secondary channels (x2, y2, theta2, radius2) only allow a restricted + // set of properties: field, aggregate, bandPosition, bin, timeUnit, title, value. + // Properties like type, scale, and axis must not be emitted. + if is_secondary_channel(aesthetic) { + let mut encoding = json!({ "field": col }); + + // Apply title handling (title is allowed on secondary channels) + apply_title_to_encoding( + &mut encoding, + aesthetic, + original_name, + ctx.spec, + ctx.titled_families, + ctx.primary_aesthetics, + &aesthetic_ctx, + ); + + return Ok(encoding); + } + let mut identity_scale = false; // Determine field type from scale or infer from data From 06797629893575079257ba6aec43066f913241e4 Mon Sep 17 00:00:00 2001 From: Carson Date: Fri, 6 Mar 2026 09:56:27 -0600 Subject: [PATCH 2/2] refactor: move secondary channel handling to vegalite writer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR review feedback: - Remove is_secondary_channel() from src/plot/aesthetic.rs (VL-specific logic doesn't belong in the general aesthetic module) - Remove early-exit from encoding.rs build_column_encoding() - Add early-exit in mod.rs build_layer_encoding() that intercepts by VL channel name (x2, y2, theta2, radius2) before build_encoding_channel is called — handles both column and literal values in one place - Move Python test to Rust vegalite writer test using segment geom Co-Authored-By: Claude Opus 4.6 --- ggsql-python/tests/test_ggsql.py | 27 ------------ src/plot/aesthetic.rs | 16 ------- src/writer/vegalite/encoding.rs | 23 +--------- src/writer/vegalite/mod.rs | 76 ++++++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 65 deletions(-) diff --git a/ggsql-python/tests/test_ggsql.py b/ggsql-python/tests/test_ggsql.py index a6e9a3e7..8b12f103 100644 --- a/ggsql-python/tests/test_ggsql.py +++ b/ggsql-python/tests/test_ggsql.py @@ -194,33 +194,6 @@ def test_render_multi_layer(self): assert "layer" in spec_dict -class TestSecondaryChannelEncoding: - """Tests for Vega-Lite secondary channel encoding (x2, y2).""" - - def test_bar_y2_has_no_disallowed_properties(self): - """Bar chart y2 encoding should not have type, scale, or axis properties.""" - reader = ggsql.DuckDBReader("duckdb://memory") - df = pl.DataFrame({"survived": [0, 1, 1, 0, 0, 1, 0, 1, 0, 0]}) - reader.register("titanic", df) - spec = reader.execute( - "SELECT survived FROM titanic " - "VISUALISE survived AS x " - "DRAW bar " - "LABEL title => 'Survival Count'" - ) - writer = ggsql.VegaLiteWriter() - vl = json.loads(writer.render(spec)) - - # Find y2 in any layer - for layer in vl.get("layer", [vl]): - y2 = layer.get("encoding", {}).get("y2") - if y2 is not None: - assert "axis" not in y2, f"y2 should not have 'axis': {y2}" - assert "scale" not in y2, f"y2 should not have 'scale': {y2}" - assert "type" not in y2, f"y2 should not have 'type': {y2}" - assert "field" in y2, f"y2 should have 'field': {y2}" - - class TestRenderAltairDataFrameConversion: """Tests for DataFrame handling in render_altair().""" diff --git a/src/plot/aesthetic.rs b/src/plot/aesthetic.rs index 44c0a096..b6253ed8 100644 --- a/src/plot/aesthetic.rs +++ b/src/plot/aesthetic.rs @@ -331,22 +331,6 @@ pub fn is_facet_aesthetic(aesthetic: &str) -> bool { false } -/// Check if an internal aesthetic maps to a Vega-Lite **secondary** encoding channel -/// (x2, y2, theta2, radius2). -/// -/// Vega-Lite secondary channels only support a restricted set of properties -/// (`field`, `aggregate`, `bandPosition`, `bin`, `timeUnit`, `title`, `value`). -/// Properties like `type`, `scale`, and `axis` must not be emitted for these. -/// -/// Matches internal names like `pos1end`, `pos2end`, etc. -#[inline] -pub fn is_secondary_channel(name: &str) -> bool { - name.starts_with("pos") && name.ends_with("end") && { - let mid = &name[3..name.len() - 3]; - !mid.is_empty() && mid.chars().all(|c| c.is_ascii_digit()) - } -} - /// Check if aesthetic is an internal positional (pos1, pos1min, pos2max, etc.) /// /// This function works with **internal** aesthetic names after transformation. diff --git a/src/writer/vegalite/encoding.rs b/src/writer/vegalite/encoding.rs index cabcb57d..f91ace3a 100644 --- a/src/writer/vegalite/encoding.rs +++ b/src/writer/vegalite/encoding.rs @@ -3,7 +3,7 @@ //! This module handles building Vega-Lite encoding channels from ggsql aesthetic mappings, //! including type inference, scale properties, and title handling. -use crate::plot::aesthetic::{is_positional_aesthetic, is_secondary_channel, AestheticContext}; +use crate::plot::aesthetic::{is_positional_aesthetic, AestheticContext}; use crate::plot::scale::{linetype_to_stroke_dash, shape_to_svg_path, ScaleTypeKind}; use crate::plot::{CoordKind, ParameterValue}; use crate::{AestheticValue, DataFrame, Plot, Result}; @@ -819,27 +819,6 @@ fn build_column_encoding( let primary = aesthetic_ctx .primary_internal_positional(aesthetic) .unwrap_or(aesthetic); - - // Vega-Lite secondary channels (x2, y2, theta2, radius2) only allow a restricted - // set of properties: field, aggregate, bandPosition, bin, timeUnit, title, value. - // Properties like type, scale, and axis must not be emitted. - if is_secondary_channel(aesthetic) { - let mut encoding = json!({ "field": col }); - - // Apply title handling (title is allowed on secondary channels) - apply_title_to_encoding( - &mut encoding, - aesthetic, - original_name, - ctx.spec, - ctx.titled_families, - ctx.primary_aesthetics, - &aesthetic_ctx, - ); - - return Ok(encoding); - } - let mut identity_scale = false; // Determine field type from scale or infer from data diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index 7e5c7ef6..850bcd53 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -271,6 +271,17 @@ fn build_layer_encoding( channel_name = "fillOpacity".to_string(); } + // Secondary positional channels (x2, y2, theta2, radius2) only support + // field/datum/value in Vega-Lite — not type, scale, axis, or title + if matches!(channel_name.as_str(), "x2" | "y2" | "theta2" | "radius2") { + let secondary_encoding = match value { + AestheticValue::Column { name: col, .. } => json!({"field": col}), + AestheticValue::Literal(lit) => json!({"value": lit.to_json()}), + }; + encoding.insert(channel_name, secondary_encoding); + continue; + } + let channel_encoding = build_encoding_channel(aesthetic, value, &mut enc_ctx)?; encoding.insert(channel_name, channel_encoding); @@ -2345,4 +2356,69 @@ mod tests { "x encoding SHOULD have domain when using fixed scales" ); } + + #[test] + fn test_secondary_channels_have_no_disallowed_properties() { + // Vega-Lite secondary channels (x2, y2, theta2, radius2) only support: + // field, aggregate, bandPosition, bin, timeUnit, title, value. + // Properties like type, scale, and axis must NOT be emitted. + let writer = VegaLiteWriter::new(); + + // Segment geom requires pos1end and pos2end as column mappings, + // which map to x2 and y2 in Vega-Lite. + let mut spec = Plot::new(); + let layer = Layer::new(Geom::segment()) + .with_aesthetic( + "x".to_string(), + AestheticValue::standard_column("x1".to_string()), + ) + .with_aesthetic( + "y".to_string(), + AestheticValue::standard_column("y1".to_string()), + ) + .with_aesthetic( + "xend".to_string(), + AestheticValue::standard_column("x2".to_string()), + ) + .with_aesthetic( + "yend".to_string(), + AestheticValue::standard_column("y2".to_string()), + ); + spec.layers.push(layer); + + let df = df! { + "x1" => &[0, 1], + "y1" => &[0, 1], + "x2" => &[1, 2], + "y2" => &[1, 2], + } + .unwrap(); + + transform_spec(&mut spec); + let json_str = writer.write(&spec, &wrap_data(df)).unwrap(); + let vl_spec: Value = serde_json::from_str(&json_str).unwrap(); + + for channel in ["x2", "y2"] { + for layer in vl_spec["layer"].as_array().unwrap() { + if let Some(enc) = layer.get("encoding").and_then(|e| e.get(channel)) { + assert!( + enc.get("field").is_some(), + "{channel} should have 'field': {enc}" + ); + assert!( + enc.get("type").is_none(), + "{channel} should not have 'type': {enc}" + ); + assert!( + enc.get("scale").is_none(), + "{channel} should not have 'scale': {enc}" + ); + assert!( + enc.get("axis").is_none(), + "{channel} should not have 'axis': {enc}" + ); + } + } + } + } }