search

voltrondata / substrait-r

An R Interface to the 'Substrait' Cross-Language Serialization for Relational Algebra
Other
27 stars 7 forks source link

Stop modifying .proto files from Substrait before including them in the package! #232

Closed paleolimbot closed 1 year ago

paleolimbot commented 1 year ago

This is just a test for #231 to see if undoing one of the renames fixes an issue.

(The fact that the .proto files were modified in the first place is obviously not ideal but was needed to work around an initial issue. If this test works, I'll follow up with a PR that undoes all the modifications!)

drin commented 1 year ago

since this fixes #231, is the workaround no longer needed (the initial issue is no longer an issue)?

paleolimbot commented 1 year ago

Totally still an issue...this was just a quick way to confirm that modifying the .proto files actually caused your problem. I will use this PR to make an actual fix and merge it so that nobody else has this problem again!

paleolimbot commented 1 year ago

Ok! No longer a test...the way that I had initially written the "update substrait" script was totally insane. Modifying the .proto files is not good and results in errors like the one that prompted this issue. Now it doesn't modify any names, uses a much more sane Python script to make a list of the protocol buffer fully qualified types, and surrounds problematic names with backticks.

The long-term solution for the "multiple .proto files in one process" problem is something like upb, but in the meantime this should keep people from having unresolvable problems!