More windows-related fixes (#126)

- 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.
- substrait-cpp tags an ancient version of protobuf (23.4) which itself
uses an ancient version of abseil, that fails to build on MSVC. Bump
protobuf to the newest version (28.2).
- various nitty changes related to the protobuf bump

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)

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

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

src/substrait/common/tests/IoTest.cpp

@@ -7,7 +7,10 @@
 #include <gmock/gmock-matchers.h>
 #include <gtest/gtest.h>
 #include <protobuf-matchers/protocol-buffer-matchers.h>
+
+#ifndef _WIN32
 #include <unistd.h>
+#endif
 
 using ::protobuf_matchers::EqualsProto;
 using ::protobuf_matchers::Partially;
@@ -45,8 +48,9 @@ TEST_F(IoTest, LoadMissingFile) {
 class SaveAndLoadTestFixture : public ::testing::TestWithParam<PlanFileFormat> {
  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.";

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<std::string> getErrors() const {

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.

src/substrait/textplan/parser/CMakeLists.txt

@@ -36,12 +36,13 @@ 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)

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
@@ -48,9 +51,15 @@ 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"
-  COMMAND ${CMAKE_COMMAND} -E copy "${TEXTPLAN_SOURCE_DIR}/data/*.json"
-          "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/")
+    "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/tests/data/q6_first_stage.json")
+
+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"

src/substrait/textplan/tests/RoundtripTest.cpp

@@ -59,7 +59,7 @@ std::vector<std::string> 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);
@@ -69,6 +69,8 @@ std::vector<std::string> getTestCases() {
   return filenames;
 }
 
+GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(RoundTripBinaryToTextFixture);
+
 TEST_P(RoundTripBinaryToTextFixture, RoundTrip) {
   auto filename = GetParam();
   auto jsonOrError = readFromFile(filename);

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 "")