fix: strip disallowed properties from secondary encoding channels#169
fix: strip disallowed properties from secondary encoding channels#169cpsievert wants to merge 2 commits intoposit-dev:mainfrom
Conversation
…, 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>
cpsievert
force-pushed
the
fix/vegalite-secondary-channel-encoding
branch
from
9dd54f8 to
b211de7
Compare
teunbrand
left a comment
teunbrand
left a comment
There was a problem hiding this comment.
Thanks Carson, I think this mostly looks good with a few nits to pick
src/plot/aesthetic.rs
Outdated
| /// Matches internal names like `pos1end`, `pos2end`, etc. | ||
| #[inline] | ||
| pub fn is_secondary_channel(name: &str) -> bool { | ||
| name.starts_with("pos") && name.ends_with("end") && { |
There was a problem hiding this comment.
I think this may miss pos1max.
This should be double checked, but I think you could do something like the following:
name != self.primary_internal_positional(name)
This avoids having to introduce new naming parsing/logic
There was a problem hiding this comment.
I'm not sure min/max are an issue here... they aren't mapped to the "2" versions in vegalite...
Since this is all in the service of vegalite, can we move this function there?
ggsql-python/tests/test_ggsql.py
Outdated
| assert "layer" in spec_dict | ||
|
|
||
|
|
||
| class TestSecondaryChannelEncoding: |
There was a problem hiding this comment.
I think the intent of this test is good, but it should live in the vegalite writer part of the repo
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 <noreply@anthropic.com>
|
I think 0679762 should address the feedback -- let me know if I've missed anything. |
|
Also, this makes me wonder if the rust package should do more to verify that the Vega spec that we're producing adheres to the actual schema, not just our expectations of what the schema are.
When in doubt, just let Claude rip #175 |
3 participants
Summary
x2,y2,theta2,radius2) only allow a restricted set of properties (field,aggregate,bandPosition,bin,timeUnit,title,value)VegaLiteWriterwas emittingtype,scale, andaxison these channels (e.g.,y2for bar chart baselines), causing Altair schema validation errors like:Additional properties are not allowed ('axis', 'scale' were unexpected)is_secondary_channel()to detect internal*endaesthetics (pos1end,pos2end) that map to secondary channelsbuild_column_encoding()that emits onlyfieldandtitlefor these channelsTest plan
TestSecondaryChannelEncoding::test_bar_y2_has_no_disallowed_properties— verifies bar charty2encoding has noaxis,scale, ortypeproperties🤖 Generated with Claude Code