-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat: create an exportable version of plan loading #91
Conversation
Since this depends on the #89 I'm switching this PR temporarily into draft mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know when the dependent PR merges and then I will formally review this.
This should be ready to go now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall approve but I suggest we bit a bit more formal on external API documentation. Easier to start formal and consistent than to try and catch up later.
if(NOT BUILD_SUBDIR_NAME EQUAL "release") | ||
message( | ||
SEND_ERROR, | ||
"The planloader library does not work in Debug mode due to its dependencies." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not? Does this mean you can't build the planloader in debug mode (annoying but ok)? Or does this mean that someone linking to the planloader cannot build in debug (probably an issue)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is all of the sanitization stuff that we're including in debug mode. An unfettered debug mode would be fine.
extern "C" { | ||
|
||
using SerializedPlan = struct { | ||
// If set, contains a serialized ::substrait::proto::Plan object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've become used to //
indicating "internal comment" and ///
indicating "formal documentation". Do we want to stick to that convention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I want to formalize this interface. If someone wants to use the C++ code they should be using the recently exposed substrait_io library. This version is for wrapping purposes only so I'd like it to be more obscure.
This PR adds an installable target of the plan loading routines (notably the text format) that is more readily consumed by other languages.