ARROW-16902: [C++][FlightRPC] Fix DLL linkage in Flight SQL (#13434)

Also, enable Flight SQL in Windows CI builds. Authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
apache · Jul 7, 2022 · 9c93f82 · 9c93f82
1 parent 3dcf2d8
commit 9c93f82
Show file tree

Hide file tree

Showing 21 changed files with 205 additions and 58 deletions.
diff --git a/.github/workflows/cpp.yml b/.github/workflows/cpp.yml
@@ -281,6 +281,7 @@ jobs:
       ARROW_BUILD_TYPE: release
       ARROW_DATASET: ON
       ARROW_FLIGHT: ON
+      ARROW_FLIGHT_SQL: ON
       ARROW_GANDIVA: ON
       ARROW_GCS: ON
       ARROW_HDFS: OFF

diff --git a/appveyor.yml b/appveyor.yml
@@ -46,6 +46,7 @@ environment:
     CLCACHE_COMPRESS: 1
     CLCACHE_COMPRESSLEVEL: 6
     ARROW_BUILD_FLIGHT: "OFF"
+    ARROW_BUILD_FLIGHT_SQL: "OFF"
     ARROW_BUILD_GANDIVA: "OFF"
     ARROW_LLVM_VERSION: "7.0.*"
     ARROW_S3: "OFF"
@@ -60,6 +61,7 @@ environment:
       ARROW_GCS: "ON"
       ARROW_S3: "ON"
       ARROW_BUILD_FLIGHT: "ON"
+      ARROW_BUILD_FLIGHT_SQL: "ON"
       ARROW_BUILD_GANDIVA: "ON"
     - JOB: "Build_Debug"
       GENERATOR: Ninja

diff --git a/ci/appveyor-cpp-build.bat b/ci/appveyor-cpp-build.bat
@@ -103,6 +103,7 @@ cmake -G "%GENERATOR%" %CMAKE_ARGS% ^
       -DARROW_DATASET=ON ^
       -DARROW_ENABLE_TIMING_TESTS=OFF ^
       -DARROW_FLIGHT=%ARROW_BUILD_FLIGHT% ^
+      -DARROW_FLIGHT_SQL=%ARROW_BUILD_FLIGHT_SQL% ^
       -DARROW_GANDIVA=%ARROW_BUILD_GANDIVA% ^
       -DARROW_MIMALLOC=ON ^
       -DARROW_PARQUET=ON ^

diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
@@ -386,6 +386,7 @@ function(ADD_ARROW_LIB LIB_NAME)
     if(WIN32)
       target_compile_definitions(${LIB_NAME}_static PUBLIC ARROW_STATIC)
       target_compile_definitions(${LIB_NAME}_static PUBLIC ARROW_FLIGHT_STATIC)
+      target_compile_definitions(${LIB_NAME}_static PUBLIC ARROW_FLIGHT_SQL_STATIC)
     endif()
 
     set_target_properties(${LIB_NAME}_static

diff --git a/cpp/src/arrow/flight/sql/CMakeLists.txt b/cpp/src/arrow/flight/sql/CMakeLists.txt
@@ -29,19 +29,19 @@ set(PROTO_DEPENDS ${FLIGHT_SQL_PROTO} ${ARROW_PROTOBUF_LIBPROTOBUF})
 
 add_custom_command(OUTPUT ${FLIGHT_SQL_GENERATED_PROTO_FILES}
                    COMMAND ${ARROW_PROTOBUF_PROTOC} "-I${FLIGHT_SQL_PROTO_PATH}"
-                           "--cpp_out=${CMAKE_CURRENT_BINARY_DIR}" "${FLIGHT_SQL_PROTO}"
+                           "--cpp_out=dllexport_decl=ARROW_FLIGHT_SQL_EXPORT:${CMAKE_CURRENT_BINARY_DIR}"
+                           "${FLIGHT_SQL_PROTO}"
                    DEPENDS ${PROTO_DEPENDS})
 
 set_source_files_properties(${FLIGHT_SQL_GENERATED_PROTO_FILES} PROPERTIES GENERATED TRUE)
-
 add_custom_target(flight_sql_protobuf_gen ALL DEPENDS ${FLIGHT_SQL_GENERATED_PROTO_FILES})
 
 set(ARROW_FLIGHT_SQL_SRCS
     server.cc
     sql_info_internal.cc
     column_metadata.cc
     client.cc
-    "${CMAKE_CURRENT_BINARY_DIR}/FlightSql.pb.cc")
+    protocol_internal.cc)
 
 add_arrow_lib(arrow_flight_sql
               CMAKE_PACKAGE_NAME
@@ -63,6 +63,14 @@ add_arrow_lib(arrow_flight_sql
               PRIVATE_INCLUDES
               "${Protobuf_INCLUDE_DIRS}")
 
+if(MSVC)
+  # Suppress warnings caused by Protobuf (casts)
+  set_source_files_properties(protocol_internal.cc PROPERTIES COMPILE_FLAGS "/wd4267")
+endif()
+foreach(LIB_TARGET ${ARROW_FLIGHT_SQL_LIBRARIES})
+  target_compile_definitions(${LIB_TARGET} PRIVATE ARROW_FLIGHT_SQL_EXPORTING)
+endforeach()
+
 if(ARROW_FLIGHT_TEST_LINKAGE STREQUAL "static" AND ARROW_BUILD_STATIC)
   set(ARROW_FLIGHT_SQL_TEST_LINK_LIBS arrow_flight_sql_static)
 else()
@@ -81,11 +89,18 @@ if(ARROW_BUILD_TESTS OR ARROW_BUILD_EXAMPLES)
       example/sqlite_statement_batch_reader.cc
       example/sqlite_server.cc
       example/sqlite_tables_schema_batch_reader.cc)
+  set(ARROW_FLIGHT_SQL_TEST_SRCS server_test.cc)
+  if(NOT MSVC AND NOT MINGW)
+    # ARROW-16902: getting Protobuf generated code to have all the
+    # proper dllexport/dllimport declarations is difficult, since
+    # protoc does not insert them everywhere needed to satisfy both
+    # MinGW and MSVC, and the Protobuf team recommends against it
+    list(APPEND ARROW_FLIGHT_SQL_TEST_SRCS client_test.cc)
+  endif()
 
   add_arrow_test(flight_sql_test
                  SOURCES
-                 client_test.cc
-                 server_test.cc
+                 ${ARROW_FLIGHT_SQL_TEST_SRCS}
                  ${ARROW_FLIGHT_SQL_TEST_SERVER_SRCS}
                  STATIC_LINK_LIBS
                  ${ARROW_FLIGHT_SQL_TEST_LINK_LIBS}
@@ -102,4 +117,10 @@ if(ARROW_BUILD_TESTS OR ARROW_BUILD_EXAMPLES)
   add_executable(flight-sql-test-app test_app_cli.cc)
   target_link_libraries(flight-sql-test-app PRIVATE ${ARROW_FLIGHT_SQL_TEST_LINK_LIBS}
                                                     ${GFLAGS_LIBRARIES})
+
+  if(ARROW_FLIGHT_TEST_LINKAGE STREQUAL "static" AND ARROW_BUILD_STATIC)
+    target_compile_definitions(flight_sql_test
+                               flight-sql-test-server flight-sql-test-app
+                               PUBLIC ARROW_FLIGHT_STATIC ARROW_FLIGHT_SQL_STATIC)
+  endif()
 endif()
diff --git a/cpp/src/arrow/flight/sql/client.cc b/cpp/src/arrow/flight/sql/client.cc
@@ -15,12 +15,15 @@
 // specific language governing permissions and limitations
 // under the License.
 
+// Platform-specific defines
+#include "arrow/flight/platform.h"
+
 #include "arrow/flight/sql/client.h"
 
 #include <google/protobuf/any.pb.h>
 
 #include "arrow/buffer.h"
-#include "arrow/flight/sql/FlightSql.pb.h"
+#include "arrow/flight/sql/protocol_internal.h"
 #include "arrow/flight/types.h"
 #include "arrow/io/memory.h"
 #include "arrow/ipc/reader.h"

diff --git a/cpp/src/arrow/flight/sql/client.h b/cpp/src/arrow/flight/sql/client.h
@@ -22,6 +22,7 @@
 
 #include "arrow/flight/client.h"
 #include "arrow/flight/sql/types.h"
+#include "arrow/flight/sql/visibility.h"
 #include "arrow/flight/types.h"
 #include "arrow/result.h"
 #include "arrow/status.h"
@@ -35,7 +36,7 @@ class PreparedStatement;
 /// \brief Flight client with Flight SQL semantics.
 ///
 /// Wraps a Flight client to provide the Flight SQL RPC calls.
-class ARROW_EXPORT FlightSqlClient {
+class ARROW_FLIGHT_SQL_EXPORT FlightSqlClient {
   friend class PreparedStatement;
 
  private:
@@ -202,7 +203,7 @@ class ARROW_EXPORT FlightSqlClient {
 };
 
 /// \brief A prepared statement that can be executed.
-class ARROW_EXPORT PreparedStatement {
+class ARROW_FLIGHT_SQL_EXPORT PreparedStatement {
  public:
   /// \brief Create a new prepared statement. However, applications
   /// should generally use FlightSqlClient::Prepare.

diff --git a/cpp/src/arrow/flight/sql/client_test.cc b/cpp/src/arrow/flight/sql/client_test.cc
@@ -15,6 +15,9 @@
 // specific language governing permissions and limitations
 // under the License.
 
+// Platform-specific defines
+#include "arrow/flight/platform.h"
+
 #include "arrow/flight/client.h"
 
 #include <gmock/gmock.h>
@@ -24,8 +27,8 @@
 #include <utility>
 
 #include "arrow/buffer.h"
-#include "arrow/flight/sql/FlightSql.pb.h"
 #include "arrow/flight/sql/api.h"
+#include "arrow/flight/sql/protocol_internal.h"
 #include "arrow/testing/gtest_util.h"
 
 namespace pb = arrow::flight::protocol;

diff --git a/cpp/src/arrow/flight/sql/column_metadata.h b/cpp/src/arrow/flight/sql/column_metadata.h
@@ -19,14 +19,15 @@
 
 #include <string>
 
+#include "arrow/flight/sql/visibility.h"
 #include "arrow/util/key_value_metadata.h"
 
 namespace arrow {
 namespace flight {
 namespace sql {
 
 /// \brief Helper class to set column metadata.
-class ColumnMetadata {
+class ARROW_FLIGHT_SQL_EXPORT ColumnMetadata {
  private:
   std::shared_ptr<const arrow::KeyValueMetadata> metadata_map_;
 
@@ -114,7 +115,7 @@ class ColumnMetadata {
   const std::shared_ptr<const arrow::KeyValueMetadata>& metadata_map() const;
 
   /// \brief A builder class to construct the ColumnMetadata object.
-  class ColumnMetadataBuilder {
+  class ARROW_FLIGHT_SQL_EXPORT ColumnMetadataBuilder {
    public:
     friend class ColumnMetadata;
 

diff --git a/cpp/src/arrow/flight/sql/example/sqlite_server.cc b/cpp/src/arrow/flight/sql/example/sqlite_server.cc
@@ -249,9 +249,11 @@ class SQLiteFlightSqlServer::Impl {
   std::string GenerateRandomString() {
     uint32_t length = 16;
 
-    std::uniform_int_distribution<char> dist('0', 'z');
+    // MSVC doesn't support char types here
+    std::uniform_int_distribution<uint16_t> dist(static_cast<uint16_t>('0'),
+                                                 static_cast<uint16_t>('z'));
     std::string ret(length, 0);
-    auto get_random_char = [&]() { return dist(gen_); };
+    auto get_random_char = [&]() { return static_cast<char>(dist(gen_)); };
     std::generate_n(ret.begin(), length, get_random_char);
     return ret;
   }
@@ -384,14 +386,14 @@ class SQLiteFlightSqlServer::Impl {
       const ActionCreatePreparedStatementRequest& request) {
     std::shared_ptr<SqliteStatement> statement;
     ARROW_ASSIGN_OR_RAISE(statement, SqliteStatement::Create(db_, request.query));
-    const std::string handle = GenerateRandomString();
+    std::string handle = GenerateRandomString();
     prepared_statements_[handle] = statement;
 
     ARROW_ASSIGN_OR_RAISE(auto dataset_schema, statement->GetSchema());
 
     sqlite3_stmt* stmt = statement->GetSqlite3Stmt();
     const int parameter_count = sqlite3_bind_parameter_count(stmt);
-    std::vector<std::shared_ptr<arrow::Field>> parameter_fields;
+    FieldVector parameter_fields;
     parameter_fields.reserve(parameter_count);
 
     // As SQLite doesn't know the parameter types before executing the query, the
@@ -410,13 +412,9 @@ class SQLiteFlightSqlServer::Impl {
       parameter_fields.push_back(field(parameter_name, dense_union_type));
     }
 
-    const std::shared_ptr<Schema>& parameter_schema = arrow::schema(parameter_fields);
-
-    ActionCreatePreparedStatementResult result{.dataset_schema = dataset_schema,
-                                               .parameter_schema = parameter_schema,
-                                               .prepared_statement_handle = handle};
-
-    return result;
+    std::shared_ptr<Schema> parameter_schema = arrow::schema(parameter_fields);
+    return ActionCreatePreparedStatementResult{
+        std::move(dataset_schema), std::move(parameter_schema), std::move(handle)};
   }
 
   Status ClosePreparedStatement(const ServerCallContext& context,

diff --git a/cpp/src/arrow/flight/sql/example/sqlite_statement_batch_reader.cc b/cpp/src/arrow/flight/sql/example/sqlite_statement_batch_reader.cc
@@ -63,16 +63,17 @@
     break;                                                                \
   }
 
-#define FLOAT_BUILDER_CASE(TYPE_CLASS, STMT, COLUMN)                      \
-  case TYPE_CLASS##Type::type_id: {                                       \
-    auto builder = reinterpret_cast<TYPE_CLASS##Builder*>(array_builder); \
-    if (sqlite3_column_type(stmt_, i) == SQLITE_NULL) {                   \
-      ARROW_RETURN_NOT_OK(builder->AppendNull());                         \
-      break;                                                              \
-    }                                                                     \
-    const double value = sqlite3_column_double(STMT, COLUMN);             \
-    ARROW_RETURN_NOT_OK(builder->Append(value));                          \
-    break;                                                                \
+#define FLOAT_BUILDER_CASE(TYPE_CLASS, STMT, COLUMN)                          \
+  case TYPE_CLASS##Type::type_id: {                                           \
+    auto builder = reinterpret_cast<TYPE_CLASS##Builder*>(array_builder);     \
+    if (sqlite3_column_type(stmt_, i) == SQLITE_NULL) {                       \
+      ARROW_RETURN_NOT_OK(builder->AppendNull());                             \
+      break;                                                                  \
+    }                                                                         \
+    const double value = sqlite3_column_double(STMT, COLUMN);                 \
+    ARROW_RETURN_NOT_OK(                                                      \
+        builder->Append(static_cast<const TYPE_CLASS##Type::c_type>(value))); \
+    break;                                                                    \
   }
 
 namespace arrow {

diff --git a/cpp/src/arrow/flight/sql/protocol_internal.cc b/cpp/src/arrow/flight/sql/protocol_internal.cc
@@ -0,0 +1,23 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+
+#include "arrow/flight/sql/protocol_internal.h"
+
+// NOTE(lidavidm): Normally this is forbidden, but on Windows to get
+// the dllexport/dllimport macro in the right places, we need to
+// ensure our header gets included (and Protobuf will not insert the
+// include for you)
+#include "arrow/flight/sql/FlightSql.pb.cc"  // NOLINT
diff --git a/cpp/src/arrow/flight/sql/protocol_internal.h b/cpp/src/arrow/flight/sql/protocol_internal.h
@@ -0,0 +1,26 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+
+#pragma once
+
+// This addresses platform-specific defines, e.g. on Windows
+#include "arrow/flight/platform.h"  // IWYU pragma: keep
+
+// This header holds the Flight SQL definitions.
+
+#include "arrow/flight/sql/visibility.h"
+
+#include "arrow/flight/sql/FlightSql.pb.h"  // IWYU pragma: export
diff --git a/cpp/src/arrow/flight/sql/server.cc b/cpp/src/arrow/flight/sql/server.cc
@@ -18,13 +18,16 @@
 // Interfaces to use for defining Flight RPC servers. API should be considered
 // experimental for now
 
+// Platform-specific defines
+#include "arrow/flight/platform.h"
+
 #include "arrow/flight/sql/server.h"
 
 #include <google/protobuf/any.pb.h>
 
 #include "arrow/buffer.h"
 #include "arrow/builder.h"
-#include "arrow/flight/sql/FlightSql.pb.h"
+#include "arrow/flight/sql/protocol_internal.h"
 #include "arrow/flight/sql/sql_info_internal.h"
 #include "arrow/type.h"
 #include "arrow/util/checked_cast.h"