Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panic when field name includes a : #485

Open
boydgreenfield opened this issue Apr 25, 2024 · 6 comments
Open

Panic when field name includes a : #485

boydgreenfield opened this issue Apr 25, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@boydgreenfield
Copy link

boydgreenfield commented Apr 25, 2024

When a field name includes a :, vegafusion cannot handle it and raises a PanicException. Here's a minimal example for which the spec works fine in the Vega Editor:

import vegafusion as vf
import vl_convert as vlc

spec = {
  "$schema": "https://vega.github.io/schema/vega-lite/v5.json",
  "data": {"values": [        
        {"Horsepower:": 225, "Miles_per_Gallon": 14, "Origin": "USA"},
        {"Horsepower:": 95, "Miles_per_Gallon": 24, "Origin": "Japan"},
        {"Horsepower:": 95, "Miles_per_Gallon": 22, "Origin": "USA"},
        {"Horsepower:": 97, "Miles_per_Gallon": 18, "Origin": "USA"},
      ]},
  "mark": "point",
  "encoding": {
    "x": {"field": "Horsepower:", "type": "quantitative"},
    "y": {"field": "Miles_per_Gallon", "type": "quantitative"},
    "color": {"field": "Origin", "type": "nominal"},
  }
}


vlc.vegalite_to_vega(spec)  # works
vf.runtime.build_pre_transform_spec_plan(vlc.vegalite_to_vega(spec))  # raises PanicException
@jonmmease jonmmease added the bug Something isn't working label Apr 25, 2024
@jonmmease
Copy link
Collaborator

Can confirm that this panics in VegaFusion and works in Vega-Lite. Thanks for the report!

@jonmmease
Copy link
Collaborator

Is this blocking for you right now @boydgreenfield?

@boydgreenfield
Copy link
Author

boydgreenfield commented May 16, 2024

Is this blocking for you right now @boydgreenfield?

We have an open issue tracking it @jonmmease, but I've only seen it "in the wild" once so far. Note @Keats and I are working on this together and he may have some findings/detail to report in the AM.

@jonmmease
Copy link
Collaborator

Ok, thanks for the context. I'll get to it over the next couple of weeks. I think the check itself can just be removed, but I need to dig into why I added it in the first place.

@Keats
Copy link
Contributor

Keats commented May 16, 2024

Yeah that's what I did as well on my branch https://github.com/Keats/vegafusion but I was digging in the various libraries to see why it was there

@jonmmease
Copy link
Collaborator

Could you open a PR with that change @Keats? I'll dig into whether there are any downstream implications of it when I review it. Thanks!

BTW, I'm a big fan of Zola and use it for my personal website!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants