feat: add api for loading plans of all types #80
Conversation
features:
* supports reading/writing all of the major formats
caveats:
* only read/writes to filenames so in-memory use should use other interfaces
* does not support compression
* does not have a zero copy interface
|
@westonpace Is there anything else you'd like to see done here? |
There was a problem hiding this comment.
I'm happy with the API so I'm marking this approved. I did go through and have a closer look at the rest. I have a bunch of minor suggestions but I don't think any of them really impact correctness. Please don't feel obliged to fix them all. Feel free to resolve, ignore, or defer to a follow-up at your leisure.
| } | ||
|
|
||
| std::string output; | ||
| auto status = ::google::protobuf::util::MessageToJsonString(plan, &output); |
There was a problem hiding this comment.
output here is created from an ofstream. In the binary method it is created from google::protobuf::io::FileOutputStream. Does FileOutputStream not work with MessageToJsonString?
There was a problem hiding this comment.
I believe I attempted to use it originally and had to fall back to a separate implementation.
| } | ||
| stream << output; | ||
| if (stream.fail()) { | ||
| return absl::UnknownError("Failed to write the plan as a JSON protobuf."); |
There was a problem hiding this comment.
Very minor but this message could be annoyingly vague (e.g. maybe this happened because we ran out of disk space?) Perhaps https://en.cppreference.com/w/cpp/io/basic_ios/exceptions could be used to generate exceptions immediately (which would hopefully be more informative).
That being said, these failures will be rare, and we can always fix this later.
There was a problem hiding this comment.
I decided to push this to later. I would like to get better messages but don't want to create an exception to get them. Maybe more use of std::error_code is the answer.
|
|
||
| } // namespace | ||
|
|
||
| absl::StatusOr<::substrait::proto::Plan> loadPlan( |
There was a problem hiding this comment.
Since the load and save methods return StatusOr then I, as a user, would not expect to get an exception. I think we still rely on exceptions in some places (maybe this isn't true?) If so, we should probably wrap these methods with a try/catch and wrap the exception in an invalid status.
There was a problem hiding this comment.
I've removed most of the cases where we use SUBSTRAIT_FAIL. There are about 5 remaining and most of those are for nearly impossible cases. I'd prefer to finish removing them over wrapping this code in an exception. If it turns out we're getting exceptions from other libraries then we will be forced to add it.
There was a problem hiding this comment.
(And the fuzz testing work will help illuminate if we need to wrap the code.)
features:
* supports reading/writing all of the major formats
caveats:
* only read/writes to filenames so in-memory use should use other interfaces
* does not support compression
* does not have a zero copy interface