feat: Add validate function for a substrait::Plan message
#281
Conversation
| # For OSX, link in CoreFoundation, needed for some system symbols like | ||
| # _CFRelease, _CFStringGetBytes, … |
There was a problem hiding this comment.
This was outside the intent of this PR, but after running into this error and attempting to reproduce locally, I found this option helped. I know very little about CMake, so any recommendations here welcome 🙇
| "failed to decode Protobuf message: " | ||
| "invalid wire type value: 7 (code 1001) (code 1001)\n")); | ||
| std::string( | ||
| "Info at plan: this version of the validator is EXPERIMENTAL. " |
There was a problem hiding this comment.
Note that before the EXPERIMENTAL info was not printed when the substrait plan failed to decode into either a Plan or PlanVersion. Given that it is otherwise always printed, it seemed more consistent to have it always be printed (even under full decode failures) and update this test, rathen than to make the code match this test.
Also refactors the parse / validation layer a bit to separate them a bit more.
wackywendell
force-pushed
the
add-validate-func
branch
from
899d45f to
4b9ec97
Compare
|
Sorry for the force-push - I usually try not to do that, but as no one had reviewed yet and the CI was failing on commit messages, I figured I'd fix those. Next time I'll plan to make a Draft PR first, get it passing CI, then mark it for review. |
|
@mbrobbel - thanks! Is this ready to merge, then? If so, can you do that - I don't have permissions? |
It doesn't really matter because we squash merge in the |
|
OK, main merged in, let's see how CI goes... ⏳ |
|
And CI passed! 🎉 |
Also refactors the parse / validation layer a bit to separate them a bit more.
Addresses #280.
I'm still fairly new to this code / repo, so I took a bit of time to see if I could address #280 in the "naive" way. Naive solutions are often as good as any... but sometimes, they are naive, so let me know if there's something I'm missing or if this is something we should avoid exposing!