Skip to content

Commit b211de7

Browse files
cpsievertclaude
andcommitted
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 <noreply@anthropic.com>
1 parent 254a2b0 commit b211de7

3 files changed

Lines changed: 65 additions & 1 deletion

File tree

ggsql-python/tests/test_ggsql.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,33 @@ def test_render_multi_layer(self):
194194
assert "layer" in spec_dict
195195

196196

197+
class TestSecondaryChannelEncoding:
198+
"""Tests for Vega-Lite secondary channel encoding (x2, y2)."""
199+
200+
def test_bar_y2_has_no_disallowed_properties(self):
201+
"""Bar chart y2 encoding should not have type, scale, or axis properties."""
202+
reader = ggsql.DuckDBReader("duckdb://memory")
203+
df = pl.DataFrame({"survived": [0, 1, 1, 0, 0, 1, 0, 1, 0, 0]})
204+
reader.register("titanic", df)
205+
spec = reader.execute(
206+
"SELECT survived FROM titanic "
207+
"VISUALISE survived AS x "
208+
"DRAW bar "
209+
"LABEL title => 'Survival Count'"
210+
)
211+
writer = ggsql.VegaLiteWriter()
212+
vl = json.loads(writer.render(spec))
213+
214+
# Find y2 in any layer
215+
for layer in vl.get("layer", [vl]):
216+
y2 = layer.get("encoding", {}).get("y2")
217+
if y2 is not None:
218+
assert "axis" not in y2, f"y2 should not have 'axis': {y2}"
219+
assert "scale" not in y2, f"y2 should not have 'scale': {y2}"
220+
assert "type" not in y2, f"y2 should not have 'type': {y2}"
221+
assert "field" in y2, f"y2 should have 'field': {y2}"
222+
223+
197224
class TestRenderAltairDataFrameConversion:
198225
"""Tests for DataFrame handling in render_altair()."""
199226

src/plot/aesthetic.rs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,22 @@ pub fn is_facet_aesthetic(aesthetic: &str) -> bool {
331331
false
332332
}
333333

334+
/// Check if an internal aesthetic maps to a Vega-Lite **secondary** encoding channel
335+
/// (x2, y2, theta2, radius2).
336+
///
337+
/// Vega-Lite secondary channels only support a restricted set of properties
338+
/// (`field`, `aggregate`, `bandPosition`, `bin`, `timeUnit`, `title`, `value`).
339+
/// Properties like `type`, `scale`, and `axis` must not be emitted for these.
340+
///
341+
/// Matches internal names like `pos1end`, `pos2end`, etc.
342+
#[inline]
343+
pub fn is_secondary_channel(name: &str) -> bool {
344+
name.starts_with("pos") && name.ends_with("end") && {
345+
let mid = &name[3..name.len() - 3];
346+
!mid.is_empty() && mid.chars().all(|c| c.is_ascii_digit())
347+
}
348+
}
349+
334350
/// Check if aesthetic is an internal positional (pos1, pos1min, pos2max, etc.)
335351
///
336352
/// This function works with **internal** aesthetic names after transformation.

src/writer/vegalite/encoding.rs

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
//! This module handles building Vega-Lite encoding channels from ggsql aesthetic mappings,
44
//! including type inference, scale properties, and title handling.
55
6-
use crate::plot::aesthetic::{is_positional_aesthetic, AestheticContext};
6+
use crate::plot::aesthetic::{is_positional_aesthetic, is_secondary_channel, AestheticContext};
77
use crate::plot::scale::{linetype_to_stroke_dash, shape_to_svg_path, ScaleTypeKind};
88
use crate::plot::{CoordKind, ParameterValue};
99
use crate::{AestheticValue, DataFrame, Plot, Result};
@@ -819,6 +819,27 @@ fn build_column_encoding(
819819
let primary = aesthetic_ctx
820820
.primary_internal_positional(aesthetic)
821821
.unwrap_or(aesthetic);
822+
823+
// Vega-Lite secondary channels (x2, y2, theta2, radius2) only allow a restricted
824+
// set of properties: field, aggregate, bandPosition, bin, timeUnit, title, value.
825+
// Properties like type, scale, and axis must not be emitted.
826+
if is_secondary_channel(aesthetic) {
827+
let mut encoding = json!({ "field": col });
828+
829+
// Apply title handling (title is allowed on secondary channels)
830+
apply_title_to_encoding(
831+
&mut encoding,
832+
aesthetic,
833+
original_name,
834+
ctx.spec,
835+
ctx.titled_families,
836+
ctx.primary_aesthetics,
837+
&aesthetic_ctx,
838+
);
839+
840+
return Ok(encoding);
841+
}
842+
822843
let mut identity_scale = false;
823844

824845
// Determine field type from scale or infer from data

0 commit comments

Comments
 (0)