From b0a956f443597111f3e5ee5311343aba2db13973 Mon Sep 17 00:00:00 2001 From: David Sisson Date: Fri, 25 Aug 2023 17:19:33 -0700 Subject: [PATCH] Handled review notes. --- include/substrait/common/Io.h | 16 +++++----- src/substrait/common/Io.cpp | 1 - src/substrait/common/tests/IoTest.cpp | 30 +++++++++++++------ src/substrait/textplan/CMakeLists.txt | 4 +-- src/substrait/textplan/StringManipulation.cpp | 12 -------- src/substrait/textplan/StringManipulation.h | 5 ---- .../textplan/converter/LoadBinary.cpp | 3 +- .../textplan/converter/SaveBinary.cpp | 16 ++++++---- src/substrait/textplan/parser/LoadText.cpp | 4 ++- 9 files changed, 46 insertions(+), 45 deletions(-) diff --git a/include/substrait/common/Io.h b/include/substrait/common/Io.h index a2c18726..a53697d8 100644 --- a/include/substrait/common/Io.h +++ b/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,8 +29,8 @@ 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. */ @@ -38,7 +38,7 @@ 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, diff --git a/src/substrait/common/Io.cpp b/src/substrait/common/Io.cpp index 4c5b1c24..af06066c 100644 --- a/src/substrait/common/Io.cpp +++ b/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( diff --git a/src/substrait/common/tests/IoTest.cpp b/src/substrait/common/tests/IoTest.cpp index bf8dd00d..09a7283e 100644 --- a/src/substrait/common/tests/IoTest.cpp +++ b/src/substrait/common/tests/IoTest.cpp @@ -2,6 +2,8 @@ #include "substrait/common/Io.h" +#include + #include #include #include @@ -42,23 +44,33 @@ TEST_F(IoTest, LoadMissingFile) { class SaveAndLoadTestFixture : public ::testing::TestWithParam { 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 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; diff --git a/src/substrait/textplan/CMakeLists.txt b/src/substrait/textplan/CMakeLists.txt index 1e8b7dbb..eda37d58 100644 --- a/src/substrait/textplan/CMakeLists.txt +++ b/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. diff --git a/src/substrait/textplan/StringManipulation.cpp b/src/substrait/textplan/StringManipulation.cpp index ce37c9bb..cb11e53a 100644 --- a/src/substrait/textplan/StringManipulation.cpp +++ b/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 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 diff --git a/src/substrait/textplan/StringManipulation.h b/src/substrait/textplan/StringManipulation.h index d0d602e6..8edf7ea5 100644 --- a/src/substrait/textplan/StringManipulation.h +++ b/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 lines, - std::string_view separator = "\n"); - } // namespace io::substrait::textplan diff --git a/src/substrait/textplan/converter/LoadBinary.cpp b/src/substrait/textplan/converter/LoadBinary.cpp index 65e5694c..c5d9f4ce 100644 --- a/src/substrait/textplan/converter/LoadBinary.cpp +++ b/src/substrait/textplan/converter/LoadBinary.cpp @@ -2,6 +2,7 @@ #include "substrait/textplan/converter/LoadBinary.h" +#include #include #include #include @@ -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; } diff --git a/src/substrait/textplan/converter/SaveBinary.cpp b/src/substrait/textplan/converter/SaveBinary.cpp index e7347269..c8dd6c07 100644 --- a/src/substrait/textplan/converter/SaveBinary.cpp +++ b/src/substrait/textplan/converter/SaveBinary.cpp @@ -2,6 +2,7 @@ #include "substrait/textplan/converter/SaveBinary.h" +#include #include #include #include @@ -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(); } diff --git a/src/substrait/textplan/parser/LoadText.cpp b/src/substrait/textplan/parser/LoadText.cpp index 0850befe..c76a7b31 100644 --- a/src/substrait/textplan/parser/LoadText.cpp +++ b/src/substrait/textplan/parser/LoadText.cpp @@ -2,6 +2,8 @@ #include "substrait/textplan/parser/LoadText.h" +#include + #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());