I/O fixes

Windows needs to a-priori know how to open a file (binary or text). If not, text-mode is assumed, which then will translate line endings `\n->\r\n`. This, in turn, will break the protobuf loader. This in turn means that windows can't rely on `loadPlan`s format detection, in case the underlying file is a binary file.

This change adds adds a `forceBinary` flag to `loadPlan`, which must be used on Windows when providing binary files.

`loadPlan` is used a few different places, and this doesn't pipe that argument in everywhere. Fixes have been added to an extent s.t., tests pass.

Author: mortbopet

include/substrait/common/Io.h

@@ -30,12 +30,15 @@ enum class PlanFileFormat {
  * amount of memory that it consumed on disk.
  *
  * \param input_filename The filename containing the plan to convert.
+ * \param force_binary If true, the plan will be opened as a binary file.
+ *  Required on Windows to avoid text mode line-ending translation.
  * \return If loading was successful, returns a plan.  If loading was not
  * successful this is a status containing a list of parse errors in the status's
  * message.
  */
 absl::StatusOr<::substrait::proto::Plan> loadPlan(
-    std::string_view input_filename);
+    std::string_view input_filename,
+    bool force_binary = false);
 
 /*
  * \brief Writes the provided plan to disk.

src/substrait/common/Io.cpp

@@ -37,8 +37,10 @@ PlanFileFormat detectFormat(std::string_view content) {
 } // namespace
 
 absl::StatusOr<::substrait::proto::Plan> loadPlan(
-    std::string_view input_filename) {
-  auto contentOrError = textplan::readFromFile(input_filename.data());
+    std::string_view input_filename,
+    bool forceBinary) {
+  auto contentOrError =
+      textplan::readFromFile(input_filename.data(), forceBinary);
   if (!contentOrError.ok()) {
     return contentOrError.status();
   }

src/substrait/common/tests/IoTest.cpp

@@ -52,7 +52,8 @@ class SaveAndLoadTestFixture : public ::testing::TestWithParam<PlanFileFormat> {
                           std::filesystem::path("my_temp_dir"))
                              .string();
 
-    if (!std::filesystem::create_directory(testFileDirectory_)) {
+    std::filesystem::create_directory(testFileDirectory_);
+    if (!std::filesystem::exists(testFileDirectory_)) {
       ASSERT_TRUE(false) << "Failed to create temporary directory.";
       testFileDirectory_.clear();
     }
@@ -87,7 +88,15 @@ TEST_P(SaveAndLoadTestFixture, SaveAndLoad) {
   auto status = ::io::substrait::savePlan(plan, tempFilename, encoding);
   ASSERT_TRUE(status.ok()) << "Save failed.\n" << status;
 
+#ifdef _WIN32
+  // Windows cannot rely on io::substrait::loadPlan to detect the file format,
+  // since it needs to a-priori specify how the file should be loaded.
+  bool forceBinary = encoding == PlanFileFormat::kBinary;
+  auto result = ::io::substrait::loadPlan(tempFilename, forceBinary);
+#else
   auto result = ::io::substrait::loadPlan(tempFilename);
+#endif
+
   ASSERT_TRUE(result.ok()) << "Load failed.\n" << result.status();
   ASSERT_THAT(
       *result,

src/substrait/textplan/converter/LoadBinary.cpp

@@ -42,17 +42,24 @@ class StringErrorCollector : public google::protobuf::io::ErrorCollector {
 
 } // namespace
 
-absl::StatusOr<std::string> readFromFile(std::string_view msgPath) {
-  std::ifstream textFile(std::string{msgPath});
-  if (textFile.fail()) {
+absl::StatusOr<std::string> readFromFile(
+    std::string_view msgPath,
+    bool forceBinary) {
+  std::ifstream file;
+  if (forceBinary)
+    file.open(std::string{msgPath}, std::ios::binary);
+  else
+    file.open(std::string{msgPath}, std::ios::in);
+
+  if (file.fail()) {
     auto currDir = std::filesystem::current_path().string();
     return absl::ErrnoToStatus(
         errno,
         fmt::format(
             "Failed to open file {} when running in {}", msgPath, currDir));
   }
   std::stringstream buffer;
-  buffer << textFile.rdbuf();
+  buffer << file.rdbuf();
   return buffer.str();
 }
 

src/substrait/textplan/converter/LoadBinary.h

@@ -12,8 +12,11 @@ class Plan;
 
 namespace io::substrait::textplan {
 
-// Read the contents of a file from disk.
-absl::StatusOr<std::string> readFromFile(std::string_view msgPath);
+// Read the contents of a file from disk. 'forceBinary' enables file reading in
+// binary mode.
+absl::StatusOr<std::string> readFromFile(
+    std::string_view msgPath,
+    bool forceBinary = false);
 
 // Reads a plan from a json-encoded text proto.
 // Returns a list of errors if the file cannot be parsed.

src/substrait/textplan/converter/SaveBinary.cpp

@@ -27,24 +27,18 @@ namespace io::substrait::textplan {
 absl::Status savePlanToBinary(
     const ::substrait::proto::Plan& plan,
     std::string_view output_filename) {
-  int outputFileDescriptor =
-      creat(std::string{output_filename}.c_str(), S_IREAD | S_IWRITE);
-  if (outputFileDescriptor == -1) {
-    return absl::ErrnoToStatus(
-        errno,
+  // Open file in binary mode and get its file descriptor
+  std::ofstream of(std::string{output_filename}, std::ios::binary);
+  if (!of) {
+    return absl::InternalError(
         fmt::format("Failed to open file {} for writing", output_filename));
   }
-  auto stream =
-      new google::protobuf::io::FileOutputStream(outputFileDescriptor);
 
-  if (!plan.SerializeToZeroCopyStream(stream)) {
+  if (!plan.SerializeToOstream(&of)) {
     return ::absl::UnknownError("Failed to write plan to stream.");
   }
 
-  if (!stream->Close()) {
-    return absl::AbortedError("Failed to close file descriptor.");
-  }
-  delete stream;
+  of.close();
   return absl::OkStatus();
 }