From aa44efcd278bcd285df693768c3b53675c13ddc2 Mon Sep 17 00:00:00 2001 From: Morten Borup Petersen Date: Thu, 10 Oct 2024 10:28:09 +0200 Subject: [PATCH 1/9] More windows-related nits - use `!` instead of a `not` macro - Don't build the `planparser` tool on windows; this uses unix commands to parse arguments. - std::filesystem::path doesn't implicitly convert to std::string on msvc - A shared library must specify export symbols (or set `CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS`) for a .lib file to be generated, which is required for things to link against said library. - Can't use `*` (wildcard) on windows in a CMake command; use built-in CMake globbing instead. --- export/planloader/CMakeLists.txt | 1 + src/substrait/common/tests/IoTest.cpp | 8 ++++++-- .../textplan/converter/ReferenceNormalizer.cpp | 2 +- src/substrait/textplan/parser/CMakeLists.txt | 10 ++++++---- src/substrait/textplan/tests/CMakeLists.txt | 16 ++++++++++++++-- src/substrait/textplan/tests/RoundtripTest.cpp | 2 +- 6 files changed, 29 insertions(+), 10 deletions(-) diff --git a/export/planloader/CMakeLists.txt b/export/planloader/CMakeLists.txt index e08e839c..cc256930 100644 --- a/export/planloader/CMakeLists.txt +++ b/export/planloader/CMakeLists.txt @@ -1,5 +1,6 @@ # SPDX-License-Identifier: Apache-2.0 +set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON) add_library(planloader SHARED planloader.cpp) add_dependencies(planloader substrait_io) diff --git a/src/substrait/common/tests/IoTest.cpp b/src/substrait/common/tests/IoTest.cpp index 183594bd..e97ea063 100644 --- a/src/substrait/common/tests/IoTest.cpp +++ b/src/substrait/common/tests/IoTest.cpp @@ -7,7 +7,10 @@ #include #include #include + +#ifndef _WIN32 #include +#endif using ::protobuf_matchers::EqualsProto; using ::protobuf_matchers::Partially; @@ -45,8 +48,9 @@ TEST_F(IoTest, LoadMissingFile) { class SaveAndLoadTestFixture : public ::testing::TestWithParam { public: void SetUp() override { - testFileDirectory_ = std::filesystem::temp_directory_path() / - std::filesystem::path("my_temp_dir"); + testFileDirectory_ = (std::filesystem::temp_directory_path() / + std::filesystem::path("my_temp_dir")) + .string(); if (!std::filesystem::create_directory(testFileDirectory_)) { ASSERT_TRUE(false) << "Failed to create temporary directory."; diff --git a/src/substrait/textplan/converter/ReferenceNormalizer.cpp b/src/substrait/textplan/converter/ReferenceNormalizer.cpp index 61754a61..d668385f 100644 --- a/src/substrait/textplan/converter/ReferenceNormalizer.cpp +++ b/src/substrait/textplan/converter/ReferenceNormalizer.cpp @@ -18,7 +18,7 @@ bool compareExtensionFunctions( return make_tuple( // First sort so that extension functions precede any other kind of // extension. - not decl.has_extension_function(), + !decl.has_extension_function(), // Next sort by space. decl.extension_function().extension_uri_reference(), // Finally sort by name within a space. diff --git a/src/substrait/textplan/parser/CMakeLists.txt b/src/substrait/textplan/parser/CMakeLists.txt index e593908e..e654c4aa 100644 --- a/src/substrait/textplan/parser/CMakeLists.txt +++ b/src/substrait/textplan/parser/CMakeLists.txt @@ -36,12 +36,14 @@ target_link_libraries( absl::status absl::statusor) -add_executable(planparser Tool.cpp) - -target_link_libraries(planparser substrait_textplan_loader error_listener) +if(UNIX) + add_executable(planparser Tool.cpp) + target_link_libraries(planparser substrait_textplan_loader error_listener) + install(TARGETS planparser EXPORT SubstraitTargets) +endif() +install(TARGETS substrait_textplan_loader EXPORT SubstraitTargets) if(${SUBSTRAIT_CPP_BUILD_TESTING}) add_subdirectory(tests) endif() -install(TARGETS planparser substrait_textplan_loader EXPORT SubstraitTargets) diff --git a/src/substrait/textplan/tests/CMakeLists.txt b/src/substrait/textplan/tests/CMakeLists.txt index a2815a27..699d17dd 100644 --- a/src/substrait/textplan/tests/CMakeLists.txt +++ b/src/substrait/textplan/tests/CMakeLists.txt @@ -39,6 +39,9 @@ add_test_case( cmake_path(GET CMAKE_CURRENT_SOURCE_DIR PARENT_PATH TEXTPLAN_SOURCE_DIR) +# Get all JSON files in the data directory +file(GLOB JSON_FILES "${TEXTPLAN_SOURCE_DIR}/data/*.json") + add_custom_command( TARGET substrait_textplan_round_trip_test POST_BUILD @@ -49,8 +52,17 @@ add_custom_command( ${CMAKE_COMMAND} -E copy "${TEXTPLAN_SOURCE_DIR}/converter/data/q6_first_stage.json" "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/q6_first_stage.json" - COMMAND ${CMAKE_COMMAND} -E copy "${TEXTPLAN_SOURCE_DIR}/data/*.json" - "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/") +) + +foreach(json_file ${TEXTPLAN_JSON_FILES}) + add_custom_command( + TARGET substrait_textplan_round_trip_test + POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy + "${json_file}" + "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/" + ) +endforeach() message( STATUS "test data will be here: ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data" diff --git a/src/substrait/textplan/tests/RoundtripTest.cpp b/src/substrait/textplan/tests/RoundtripTest.cpp index 2ccc7176..19dd2377 100644 --- a/src/substrait/textplan/tests/RoundtripTest.cpp +++ b/src/substrait/textplan/tests/RoundtripTest.cpp @@ -59,7 +59,7 @@ std::vector getTestCases() { testDataPath.append("data"); for (auto const& dirEntry : std::filesystem::recursive_directory_iterator{testDataPath}) { - std::string pathName = dirEntry.path(); + std::string pathName = dirEntry.path().string(); if (endsWith(pathName, ".json") && !endsWith(pathName, "q6_first_stage.json")) { filenames.push_back(pathName); From 0d6fa108b41f06d94dd484d3f4031051021de5b7 Mon Sep 17 00:00:00 2001 From: Morten Borup Petersen Date: Thu, 10 Oct 2024 11:08:30 +0200 Subject: [PATCH 2/9] bump protobuf --- third_party/protobuf.cmake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/protobuf.cmake b/third_party/protobuf.cmake index e7942cf3..ef05a6ae 100644 --- a/third_party/protobuf.cmake +++ b/third_party/protobuf.cmake @@ -12,7 +12,7 @@ FetchContent_Declare(GTest ) FetchContent_Declare(Protobuf GIT_REPOSITORY https://github.com/protocolbuffers/protobuf.git - GIT_TAG v23.4 + GIT_TAG v28.2 OVERRIDE_FIND_PACKAGE ) set(protobuf_BUILD_TESTS OFF CACHE INTERNAL "") From 6b2aea10d6e8f06dad7edeba8b4f857b51dc26ee Mon Sep 17 00:00:00 2001 From: Morten Borup Petersen Date: Thu, 10 Oct 2024 11:09:55 +0200 Subject: [PATCH 3/9] format --- src/substrait/textplan/tests/CMakeLists.txt | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/substrait/textplan/tests/CMakeLists.txt b/src/substrait/textplan/tests/CMakeLists.txt index 699d17dd..cf66972d 100644 --- a/src/substrait/textplan/tests/CMakeLists.txt +++ b/src/substrait/textplan/tests/CMakeLists.txt @@ -51,8 +51,7 @@ add_custom_command( COMMAND ${CMAKE_COMMAND} -E copy "${TEXTPLAN_SOURCE_DIR}/converter/data/q6_first_stage.json" - "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/q6_first_stage.json" -) + "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/q6_first_stage.json") foreach(json_file ${TEXTPLAN_JSON_FILES}) add_custom_command( @@ -60,8 +59,7 @@ foreach(json_file ${TEXTPLAN_JSON_FILES}) POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy "${json_file}" - "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/" - ) + "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/") endforeach() message( From fb8b7cdac6eaa8a68c30e0dd8e80d92412858fa8 Mon Sep 17 00:00:00 2001 From: Morten Borup Petersen Date: Thu, 10 Oct 2024 11:50:46 +0200 Subject: [PATCH 4/9] protobuf related change --- src/substrait/textplan/converter/LoadBinary.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/substrait/textplan/converter/LoadBinary.cpp b/src/substrait/textplan/converter/LoadBinary.cpp index c5d9f4ce..6691b507 100644 --- a/src/substrait/textplan/converter/LoadBinary.cpp +++ b/src/substrait/textplan/converter/LoadBinary.cpp @@ -23,10 +23,13 @@ namespace { class StringErrorCollector : public google::protobuf::io::ErrorCollector { public: - void AddError(int line, int column, const std::string& message) override { + void RecordError( + int line, + google::protobuf::io::ColumnNumber column, + absl::string_view message) override { errors_.push_back( std::to_string(line + 1) + ":" + std::to_string(column + 1) + " → " + - message); + std::string(message)); } [[nodiscard]] std::vector getErrors() const { From f14fa7852cf921d689273aed7accaa5a7883d87e Mon Sep 17 00:00:00 2001 From: Morten Borup Petersen Date: Thu, 10 Oct 2024 13:00:02 +0200 Subject: [PATCH 5/9] format --- src/substrait/textplan/parser/CMakeLists.txt | 1 - src/substrait/textplan/tests/CMakeLists.txt | 5 ++--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/substrait/textplan/parser/CMakeLists.txt b/src/substrait/textplan/parser/CMakeLists.txt index e654c4aa..5c95f5b2 100644 --- a/src/substrait/textplan/parser/CMakeLists.txt +++ b/src/substrait/textplan/parser/CMakeLists.txt @@ -46,4 +46,3 @@ install(TARGETS substrait_textplan_loader EXPORT SubstraitTargets) if(${SUBSTRAIT_CPP_BUILD_TESTING}) add_subdirectory(tests) endif() - diff --git a/src/substrait/textplan/tests/CMakeLists.txt b/src/substrait/textplan/tests/CMakeLists.txt index cf66972d..1f9c08fd 100644 --- a/src/substrait/textplan/tests/CMakeLists.txt +++ b/src/substrait/textplan/tests/CMakeLists.txt @@ -57,9 +57,8 @@ foreach(json_file ${TEXTPLAN_JSON_FILES}) add_custom_command( TARGET substrait_textplan_round_trip_test POST_BUILD - COMMAND ${CMAKE_COMMAND} -E copy - "${json_file}" - "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/") + COMMAND ${CMAKE_COMMAND} -E copy "${json_file}" + "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/") endforeach() message( From 7a8e874c18b30427f6f409e361d701bfb72ed055 Mon Sep 17 00:00:00 2001 From: Morten Borup Petersen Date: Fri, 11 Oct 2024 09:01:05 +0200 Subject: [PATCH 6/9] bump protobuf-matchers tag --- third_party/protobuf-matchers | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/protobuf-matchers b/third_party/protobuf-matchers index a76296f5..bd0b02a6 160000 --- a/third_party/protobuf-matchers +++ b/third_party/protobuf-matchers @@ -1 +1 @@ -Subproject commit a76296f5d5369d57f44126acf59c290a397490c6 +Subproject commit bd0b02a6b648676011732a4dae13d8782fc8d82c From 318ff2277c50cc465963a67c7aa6cda4371ce8d3 Mon Sep 17 00:00:00 2001 From: Morten Borup Petersen Date: Fri, 11 Oct 2024 09:37:25 +0200 Subject: [PATCH 7/9] Bump protobuf version in CI --- scripts/setup-ubuntu.sh | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/scripts/setup-ubuntu.sh b/scripts/setup-ubuntu.sh index e503d536..9be1fbf8 100755 --- a/scripts/setup-ubuntu.sh +++ b/scripts/setup-ubuntu.sh @@ -18,10 +18,16 @@ sudo --preserve-env apt install -y \ clang-tidy \ git \ wget \ - protobuf-compiler \ clang-format \ uuid-dev \ default-jre \ libcurl4-openssl-dev +# Install the currently supported version of protobuf: +PB_REL="https://github.com/protocolbuffers/protobuf/releases" +PB_VER="28.2" +curl -LO $PB_REL/download/v$PB_VER/protoc-$PB_VER-linux-x86_64.zip +unzip protoc-$PB_VER-linux-x86_64.zip -d $HOME/.local +export PATH="$PATH:$HOME/.local/bin" + pip install cmake-format From ab6bf268d263ba12f32bbe1d5041b6542adc4ccf Mon Sep 17 00:00:00 2001 From: Morten Borup Petersen Date: Fri, 11 Oct 2024 10:00:01 +0200 Subject: [PATCH 8/9] try fix gtest --- src/substrait/textplan/tests/RoundtripTest.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/substrait/textplan/tests/RoundtripTest.cpp b/src/substrait/textplan/tests/RoundtripTest.cpp index 19dd2377..08ea9d97 100644 --- a/src/substrait/textplan/tests/RoundtripTest.cpp +++ b/src/substrait/textplan/tests/RoundtripTest.cpp @@ -69,6 +69,8 @@ std::vector getTestCases() { return filenames; } +GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(RoundTripBinaryToTextFixture); + TEST_P(RoundTripBinaryToTextFixture, RoundTrip) { auto filename = GetParam(); auto jsonOrError = readFromFile(filename); From 646ab610b9e3d636217da2d3d287bc8079234373 Mon Sep 17 00:00:00 2001 From: Morten Borup Petersen Date: Fri, 11 Oct 2024 14:26:50 +0200 Subject: [PATCH 9/9] reduce parallelism --- scripts/run-clang-tidy.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/run-clang-tidy.sh b/scripts/run-clang-tidy.sh index 293ef869..89b27492 100755 --- a/scripts/run-clang-tidy.sh +++ b/scripts/run-clang-tidy.sh @@ -6,7 +6,7 @@ WORKDIR="$( cd $SCRIPTDIR/.. && pwd )" # Make compile_command.json rm -rf tmp && mkdir tmp && cmake -Btmp -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DSUBSTRAIT_CPP_ROUNDTRIP_TESTING=ON # Build substrait protobuf -pushd tmp/src/substrait/proto && make -j && popd || exit +pushd tmp/src/substrait/proto && make -j 2 && popd || exit # Build textplan grammar pushd tmp/src/substrait/textplan/parser/grammar && make -j antlr4_runtime textplan_grammar_headers && popd || exit # Run clang-tidy