From 228e23ddfe9db2edfd881529eb61e5b27fd8122c Mon Sep 17 00:00:00 2001 From: David Sisson Date: Wed, 26 Jul 2023 18:52:03 -0700 Subject: [PATCH] Added a convenience function for joining lines of text. --- src/substrait/common/Io.cpp | 1 + src/substrait/textplan/StringManipulation.cpp | 19 +++++++++++++++++-- src/substrait/textplan/StringManipulation.h | 5 +++++ .../textplan/converter/LoadBinary.cpp | 4 ++-- .../textplan/converter/SaveBinary.cpp | 4 ++-- .../tests/BinaryToTextPlanConversionTest.cpp | 15 +++++++++------ src/substrait/textplan/parser/LoadText.cpp | 4 ++-- 7 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/substrait/common/Io.cpp b/src/substrait/common/Io.cpp index ba9ed972..aa315b24 100644 --- a/src/substrait/common/Io.cpp +++ b/src/substrait/common/Io.cpp @@ -5,6 +5,7 @@ #include #include +#include "substrait/proto/plan.pb.h" #include "substrait/textplan/converter/LoadBinary.h" #include "substrait/textplan/converter/SaveBinary.h" #include "substrait/textplan/parser/LoadText.h" diff --git a/src/substrait/textplan/StringManipulation.cpp b/src/substrait/textplan/StringManipulation.cpp index eac3c56a..a0f5b896 100644 --- a/src/substrait/textplan/StringManipulation.cpp +++ b/src/substrait/textplan/StringManipulation.cpp @@ -2,18 +2,33 @@ #include "StringManipulation.h" +#include +#include +#include +#include + namespace io::substrait::textplan { -// Yields true if the string 'haystack' starts with the string 'needle'. bool startsWith(std::string_view haystack, std::string_view needle) { return haystack.size() > needle.size() && haystack.substr(0, needle.size()) == needle; } -// Returns true if the string 'haystack' ends with the string 'needle'. bool endsWith(std::string_view haystack, std::string_view needle) { return haystack.size() > needle.size() && 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 9c24418f..1cc85e39 100644 --- a/src/substrait/textplan/StringManipulation.h +++ b/src/substrait/textplan/StringManipulation.h @@ -12,4 +12,9 @@ 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 e6ac8fe7..3c6b315b 100644 --- a/src/substrait/textplan/converter/LoadBinary.cpp +++ b/src/substrait/textplan/converter/LoadBinary.cpp @@ -14,6 +14,7 @@ #include #include "substrait/proto/plan.pb.h" +#include "substrait/textplan/StringManipulation.h" namespace io::substrait::textplan { @@ -82,8 +83,7 @@ absl::StatusOr<::substrait::proto::Plan> loadFromProtoText( parser.RecordErrorsTo(&collector); if (!parser.ParseFromString(text, &plan)) { auto errors = collector.getErrors(); - return absl::InternalError( - std::accumulate(errors.begin(), errors.end(), std::string("\n"))); + return absl::InternalError(joinLines(errors)); } return plan; } diff --git a/src/substrait/textplan/converter/SaveBinary.cpp b/src/substrait/textplan/converter/SaveBinary.cpp index dc7a9ff9..77f50109 100644 --- a/src/substrait/textplan/converter/SaveBinary.cpp +++ b/src/substrait/textplan/converter/SaveBinary.cpp @@ -9,6 +9,7 @@ #include #include +#include "substrait/textplan/StringManipulation.h" #include "substrait/textplan/SymbolTablePrinter.h" #include "substrait/textplan/converter/ParseBinary.h" @@ -72,8 +73,7 @@ absl::Status savePlanToText( auto result = parseBinaryPlan(plan); auto errors = result.getAllErrors(); if (!errors.empty()) { - return absl::UnknownError( - std::accumulate(errors.begin(), errors.end(), std::string("\n"))); + return absl::UnknownError(joinLines(errors)); } stream << SymbolTablePrinter::outputToText(result.getSymbolTable()); stream.close(); diff --git a/src/substrait/textplan/converter/tests/BinaryToTextPlanConversionTest.cpp b/src/substrait/textplan/converter/tests/BinaryToTextPlanConversionTest.cpp index cde3c92a..3b0ae97f 100644 --- a/src/substrait/textplan/converter/tests/BinaryToTextPlanConversionTest.cpp +++ b/src/substrait/textplan/converter/tests/BinaryToTextPlanConversionTest.cpp @@ -597,9 +597,10 @@ std::vector getTestCases() { TEST_P(BinaryToTextPlanConverterTestFixture, Parse) { auto [name, input, matcher] = GetParam(); - auto planOrError = loadFromText(input); + auto planOrError = loadFromProtoText(input); if (!planOrError.ok()) { - ParseResult result(SymbolTable(), planOrError.errors(), {}); + ParseResult result( + SymbolTable(), {std::string(planOrError.status().message())}, {}); ASSERT_THAT(result, matcher); return; } @@ -627,13 +628,15 @@ INSTANTIATE_TEST_SUITE_P( class BinaryToTextPlanConversionTest : public ::testing::Test {}; TEST_F(BinaryToTextPlanConversionTest, FullSample) { - std::string json = readFromFile("data/q6_first_stage.json"); - auto planOrError = loadFromJson(json); + auto jsonOrError = readFromFile("data/q6_first_stage.json"); + ASSERT_TRUE(jsonOrError.ok()); + auto planOrError = loadFromJson(*jsonOrError); ASSERT_TRUE(planOrError.ok()); auto plan = *planOrError; EXPECT_THAT(plan.extensions_size(), ::testing::Eq(7)); - std::string expectedOutput = readFromFile("data/q6_first_stage.golden.splan"); + auto expectedOutputOrError = readFromFile("data/q6_first_stage.golden.splan"); + ASSERT_TRUE(expectedOutputOrError.ok()); auto result = parseBinaryPlan(plan); auto symbols = result.getSymbolTable().getSymbols(); @@ -668,7 +671,7 @@ TEST_F(BinaryToTextPlanConversionTest, FullSample) { SymbolType::kSource, SymbolType::kSchema, }), - WhenSerialized(EqSquashingWhitespace(expectedOutput)))) + WhenSerialized(EqSquashingWhitespace(*expectedOutputOrError)))) << result.getSymbolTable().toDebugString(); } diff --git a/src/substrait/textplan/parser/LoadText.cpp b/src/substrait/textplan/parser/LoadText.cpp index 59af341f..ee2c7078 100644 --- a/src/substrait/textplan/parser/LoadText.cpp +++ b/src/substrait/textplan/parser/LoadText.cpp @@ -3,6 +3,7 @@ #include "substrait/textplan/parser/LoadText.h" #include "substrait/proto/plan.pb.h" +#include "substrait/textplan/StringManipulation.h" #include "substrait/textplan/SymbolTablePrinter.h" #include "substrait/textplan/parser/ParseText.h" @@ -13,8 +14,7 @@ absl::StatusOr<::substrait::proto::Plan> loadFromText(std::string_view text) { auto parseResult = io::substrait::textplan::parseStream(stream); if (!parseResult.successful()) { auto errors = parseResult.getAllErrors(); - return absl::UnknownError( - std::accumulate(errors.begin(), errors.end(), std::string("\n"))); + return absl::UnknownError(joinLines(errors)); } return SymbolTablePrinter::outputToBinaryPlan(parseResult.getSymbolTable());