Handled review notes.

include/substrait/common/Io.h

@@ -20,7 +20,7 @@ enum class PlanFileFormat {
 };
 
 /*
- * \\brief Loads a Substrait plan of any format from the given file.
+ * \brief Loads a Substrait plan of any format from the given file.
  *
  * loadPlan determines which file type the specified file is and then calls
  * the appropriate load/parse method to consume it preserving any error
@@ -29,16 +29,16 @@ enum class PlanFileFormat {
  * This will load the plan into memory and then convert it consuming twice the
  * amount of memory that it consumed on disk.
  *
- * \\param input_filename The filename containing the plan to convert.
- * \\return If loading was successful, returns a plan. If loading was not
+ * \param input_filename The filename containing the plan to convert.
+ * \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);
 
 /*
- * \\brief Writes the provided plan to disk.
+ * \brief Writes the provided plan to disk.
  *
  * savePlan writes the provided plan in the specified format to the specified
  * location.
@@ -47,10 +47,10 @@ absl::StatusOr<::substrait::proto::Plan> loadPlan(
  * format as the original plan as well as the annotated parse tree will need to
  * reside in memory during the process.
  *
- * \\param plan
- * \\param output_filename
- * \\param format
- * \\return
+ * \param plan
+ * \param output_filename
+ * \param format
+ * \return
  */
 absl::Status savePlan(
  const ::substrait::proto::Plan& plan,

src/substrait/common/Io.cpp

@@ -55,7 +55,6 @@ absl::StatusOr<::substrait::proto::Plan> loadPlan(
  case PlanFileFormat::kText:
  return textplan::loadFromText(*contentOrError);
  }
- return absl::UnimplementedError("Unexpected encoding requested.");
 }
 
 absl::Status savePlan(

src/substrait/common/tests/IoTest.cpp

@@ -2,6 +2,8 @@
 
 #include "substrait/common/Io.h"
 
+#include <filesystem>
+
 #include <gmock/gmock-matchers.h>
 #include <gtest/gtest.h>
 #include <protobuf-matchers/protocol-buffer-matchers.h>
@@ -42,23 +44,33 @@ TEST_F(IoTest, LoadMissingFile) {
 
 class SaveAndLoadTestFixture : public ::testing::TestWithParam<PlanFileFormat> {
  public:
- ~SaveAndLoadTestFixture() override {
- for (const auto& filename : testFiles_) {
- unlink(filename.c_str());
+ void SetUp() override {
+ testFileDirectory_ = std::filesystem::temp_directory_path() /
+ std::filesystem::path("my_temp_dir");
+
+ if (!std::filesystem::create_directory(testFileDirectory_)) {
+ std::cerr << "Failed to create temporary directory." << std::endl;
+ testFileDirectory_.clear();
+ }
+ }
+
+ void TearDown() override {
+ if (!testFileDirectory_.empty()) {
+ std::filesystem::remove_all(testFileDirectory_);
  }
  }
 
- void registerCleanup(const char* filename) {
- testFiles_.emplace_back(filename);
+ static std::string makeTempFileName() {
+ static int tempFileNum = 0;
+ return "testfile" + std::to_string(++tempFileNum);
  }
 
- private:
- std::vector<std::string> testFiles_;
+ protected:
+ std::string testFileDirectory_;
 };
 
 TEST_P(SaveAndLoadTestFixture, SaveAndLoad) {
- auto tempFilename = std::tmpnam(nullptr);
- registerCleanup(tempFilename);
+ auto tempFilename = testFileDirectory_ + "/" + makeTempFileName();
  PlanFileFormat encoding = GetParam();
 
  ::substrait::proto::Plan plan;

src/substrait/textplan/CMakeLists.txt

@@ -20,10 +20,10 @@ add_library(error_listener SubstraitErrorListener.cpp SubstraitErrorListener.h)
 
 add_library(parse_result ParseResult.cpp ParseResult.h)
 
-add_dependencies(symbol_table substrait_proto substrait_common
+add_dependencies(symbol_table substrait_proto substrait_common absl::strings
  fmt::fmt-header-only)
 
-target_link_libraries(symbol_table fmt::fmt-header-only
+target_link_libraries(symbol_table fmt::fmt-header-only absl::strings
  substrait_textplan_converter)
 
 # Provide access to the generated protobuffer headers hierarchy.

src/substrait/textplan/StringManipulation.cpp

@@ -19,16 +19,4 @@ bool endsWith(std::string_view haystack, std::string_view needle) {
  haystack.substr(haystack.size() - needle.size(), needle.size()) == needle;
 }
 
-std::string joinLines(
- std::vector<std::string> lines,
- std::string_view separator) {
- auto concatWithSeparator = [separator](std::string a, const std::string& b) {
- return std::move(a) + std::string(separator) + b;
- };
-
- auto result = std::accumulate(
- std::next(lines.begin()), lines.end(), lines[0], concatWithSeparator);
- return result;
-}
-
 } // namespace io::substrait::textplan

src/substrait/textplan/StringManipulation.h

@@ -14,9 +14,4 @@ bool startsWith(std::string_view haystack, std::string_view needle);
 // Returns true if the string 'haystack' ends with the string 'needle'.
 bool endsWith(std::string_view haystack, std::string_view needle);
 
-// Joins a vector of strings into a single string separated by separator.
-std::string joinLines(
- std::vector<std::string> lines,
- std::string_view separator = "\n");
-
 } // namespace io::substrait::textplan

src/substrait/textplan/converter/LoadBinary.cpp

@@ -2,6 +2,7 @@
 
 #include "substrait/textplan/converter/LoadBinary.h"
 
+#include <absl/strings/str_join.h>
 #include <fmt/format.h>
 #include <google/protobuf/io/tokenizer.h>
 #include <google/protobuf/text_format.h>
@@ -83,7 +84,7 @@ absl::StatusOr<::substrait::proto::Plan> loadFromProtoText(
  parser.RecordErrorsTo(&collector);
  if (!parser.ParseFromString(text, &plan)) {
  auto errors = collector.getErrors();
- return absl::InternalError(joinLines(errors));
+ return absl::InternalError(absl::StrJoin(errors, ""));
  }
  return plan;
 }

src/substrait/textplan/converter/SaveBinary.cpp

@@ -2,6 +2,7 @@
 
 #include "substrait/textplan/converter/SaveBinary.h"
 
+#include <absl/strings/str_join.h>
 #include <fmt/format.h>
 #include <google/protobuf/io/zero_copy_stream_impl.h>
 #include <google/protobuf/text_format.h>
@@ -34,8 +35,10 @@ absl::Status savePlanToBinary(
  return ::absl::UnknownError("Failed to write plan to stream.");
  }
 
+ if (!stream->Close()) {
+ return absl::AbortedError("Failed to close file descriptor.");
+ }
  delete stream;
- close(outputFileDescriptor);
  return absl::OkStatus();
 }
 
@@ -54,11 +57,10 @@ absl::Status savePlanToJson(
  return absl::UnknownError("Failed to save plan as a JSON protobuf.");
  }
  stream << output;
+ stream.close();
  if (stream.fail()) {
  return absl::UnknownError("Failed to write the plan as a JSON protobuf.");
  }
-
- stream.close();
  return absl::OkStatus();
 }
 
@@ -74,13 +76,13 @@ absl::Status savePlanToText(
  auto result = parseBinaryPlan(plan);
  auto errors = result.getAllErrors();
  if (!errors.empty()) {
- return absl::UnknownError(joinLines(errors));
+ return absl::UnknownError(absl::StrJoin(errors, ""));
  }
  stream << SymbolTablePrinter::outputToText(result.getSymbolTable());
+ stream.close();
  if (stream.fail()) {
  return absl::UnknownError("Failed to write the plan as text.");
  }
- stream.close();
  return absl::OkStatus();
 }
 
@@ -101,8 +103,10 @@ absl::Status savePlanToProtoText(
  return absl::UnknownError("Failed to save plan as a text protobuf.");
  }
 
+ if (!stream->Close()) {
+ return absl::AbortedError("Failed to close file descriptor.");
+ }
  delete stream;
- close(outputFileDescriptor);
  return absl::OkStatus();
 }
 

src/substrait/textplan/parser/LoadText.cpp

@@ -2,6 +2,8 @@
 
 #include "substrait/textplan/parser/LoadText.h"
 
+#include <absl/strings/str_join.h>
+
 #include "substrait/proto/plan.pb.h"
 #include "substrait/textplan/StringManipulation.h"
 #include "substrait/textplan/SymbolTablePrinter.h"
@@ -14,7 +16,7 @@ absl::StatusOr<::substrait::proto::Plan> loadFromText(const std::string& text) {
  auto parseResult = io::substrait::textplan::parseStream(stream);
  if (!parseResult.successful()) {
  auto errors = parseResult.getAllErrors();
- return absl::UnknownError(joinLines(errors));
+ return absl::UnknownError(absl::StrJoin(errors, ""));
  }
 
  return SymbolTablePrinter::outputToBinaryPlan(parseResult.getSymbolTable());