search

substrait-io / substrait-validator

Apache License 2.0
7 stars 6 forks source link

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

Open gforsyth opened 2 years ago

gforsyth commented 2 years ago

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 commented 2 years ago

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.