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

Cannot use load_plan with namespaced substrait (in Plan form) #33

Open
gforsyth opened this issue Jul 29, 2022 · 1 comment
Open

Cannot use load_plan with namespaced substrait (in Plan form) #33

gforsyth opened this issue Jul 29, 2022 · 1 comment

Comments

@gforsyth
Copy link
Member

Minor annoyance with an easy workaround.
The code in load_plan does an isinstance check for a substrait.plan_pb2.Plan here:
https://github.com/substrait-io/substrait-validator/blob/main/py/substrait_validator/__init__.py#L140-L154

But if the producer of substrait has namespaced their install, then it fails that isinstance check and raises as Unsupported.
Not sure the best way to handle the isinstance check, but as a workaround, if I instead send in the bytes from plan.SerializeToString() everything works fine.

@jvanstraten
Copy link
Collaborator

I'm not sure how to make this better. The intended path is (bytes/json ->) Python libprotobuf wrappers using known-good protos for protobuf-level error messages -> bytes -> Rust/prost wrappers. Namespaced Substrait protos provided by the user are not known-good -- if nothing else the version may differ -- so they are rejected; I'd consider that intender behavior, and what you considered to be a workaround the correct thing to do.

Semi-related; the validator should also be renamespacing the protos. Right now it only renamespaces them in the Python world (so everything is in substrait_validator), but not as far as the descriptor pool is concerned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants