ARROW-16721: [C++] Drop support for bundled Thrift < 0.13 (apache#13292)

Our bundled Thrift is 0.16.0. Users can use other version but will not
use 0.13 or earlier.

This change also cleans FindThrift.cmake up.

This change also fixes build errors with -DARROW_HIVESERVER2=ON. We
may be able to remove cpp/src/arrow/dbi/hiveserver2/. It seems that
nobody uses it.

Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>

.env

@@ -103,4 +103,4 @@ VCPKG="89295c9"
 # Use conanio/${CONAN} for "docker-compose run --rm conan". See
 # https://github.com/conan-io/conan-docker-tools#readme for available
 # images.
-CONAN=gcc11
+CONAN=gcc10

ci/conan/all/conanfile.py

@@ -263,7 +263,7 @@ def requirements(self):
         if self._with_jemalloc():
             self.requires("jemalloc/5.2.1")
         if self._with_boost():
-            self.requires("boost/1.78.0")
+            self.requires("boost/1.79.0")
         if self._with_gflags():
             self.requires("gflags/2.2.2")
         if self._with_glog():
@@ -331,6 +331,7 @@ def _configure_cmake(self):
         if self._cmake:
             return self._cmake
         self._cmake = CMake(self)
+        self._cmake.definitions["CMAKE_FIND_PACKAGE_PREFER_CONFIG"] = True
         if tools.cross_building(self):
             cmake_system_processor = {
                 "armv8": "aarch64",

ci/scripts/conan_build.sh

@@ -28,13 +28,17 @@ export ARROW_HOME=${source_dir}
 export CONAN_HOOK_ERROR_LEVEL=40
 
 conan_args=()
+if [ -n "${ARROW_CONAN_PARQUET:-}" ]; then
+  conan_args+=(--options arrow:parquet=${ARROW_CONAN_PARQUET})
+fi
 if [ -n "${ARROW_CONAN_WITH_LZ4:-}" ]; then
   conan_args+=(--options arrow:with_lz4=${ARROW_CONAN_WITH_LZ4})
 fi
 
 version=$(grep '^set(ARROW_VERSION ' ${ARROW_HOME}/cpp/CMakeLists.txt | \
             grep -E -o '([0-9.]*)')
 
+rm -rf ~/.conan/data/arrow/
 rm -rf ${build_dir}/conan || sudo rm -rf ${build_dir}/conan
 mkdir -p ${build_dir}/conan || sudo mkdir -p ${build_dir}/conan
 if [ -w ${build_dir} ]; then

cpp/cmake_modules/DefineOptions.cmake

@@ -325,7 +325,7 @@ if("${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}")
 
   define_option(ARROW_DEPENDENCY_USE_SHARED "Link to shared libraries" ON)
 
-  define_option(ARROW_BOOST_USE_SHARED "Rely on boost shared libraries where relevant"
+  define_option(ARROW_BOOST_USE_SHARED "Rely on Boost shared libraries where relevant"
                 ${ARROW_DEPENDENCY_USE_SHARED})
 
   define_option(ARROW_BROTLI_USE_SHARED "Rely on Brotli shared libraries where relevant"

cpp/cmake_modules/FindThrift.cmake

@@ -22,22 +22,23 @@
 #                The environment variable THRIFT_HOME overrides this variable.
 #
 # This module defines
-#  THRIFT_VERSION, version string of ant if found
-#  THRIFT_INCLUDE_DIR, where to find THRIFT headers
-#  THRIFT_LIB, THRIFT library
-#  THRIFT_FOUND, If false, do not try to use ant
+#  Thrift_FOUND, whether Thrift is found or not
+#  Thrift_COMPILER_FOUND, whether Thrift compiler is found or not
+#
+#  thrift::thrift, a library target to use Thrift
+#  thrift::compiler, a executable target to use Thrift compiler
 
 function(EXTRACT_THRIFT_VERSION)
   if(THRIFT_INCLUDE_DIR)
     file(READ "${THRIFT_INCLUDE_DIR}/thrift/config.h" THRIFT_CONFIG_H_CONTENT)
     string(REGEX MATCH "#define PACKAGE_VERSION \"[0-9.]+\"" THRIFT_VERSION_DEFINITION
                  "${THRIFT_CONFIG_H_CONTENT}")
-    string(REGEX MATCH "[0-9.]+" THRIFT_VERSION "${THRIFT_VERSION_DEFINITION}")
-    set(THRIFT_VERSION
-        "${THRIFT_VERSION}"
+    string(REGEX MATCH "[0-9.]+" Thrift_VERSION "${THRIFT_VERSION_DEFINITION}")
+    set(Thrift_VERSION
+        "${Thrift_VERSION}"
         PARENT_SCOPE)
   else()
-    set(THRIFT_VERSION
+    set(Thrift_VERSION
         ""
         PARENT_SCOPE)
   endif()
@@ -102,7 +103,7 @@ else()
                  HINTS ${THRIFT_PC_PREFIX}
                  NO_DEFAULT_PATH
                  PATH_SUFFIXES "bin")
-    set(THRIFT_VERSION ${THRIFT_PC_VERSION})
+    set(Thrift_VERSION ${THRIFT_PC_VERSION})
   else()
     find_library(THRIFT_LIB
                  NAMES ${THRIFT_LIB_NAMES}
@@ -122,11 +123,10 @@ endif()
 find_package_handle_standard_args(
   Thrift
   REQUIRED_VARS THRIFT_LIB THRIFT_INCLUDE_DIR
-  VERSION_VAR THRIFT_VERSION
+  VERSION_VAR Thrift_VERSION
   HANDLE_COMPONENTS)
 
-if(Thrift_FOUND OR THRIFT_FOUND)
-  set(Thrift_FOUND TRUE)
+if(Thrift_FOUND)
   if(ARROW_THRIFT_USE_SHARED)
     add_library(thrift::thrift SHARED IMPORTED)
   else()
@@ -141,4 +141,10 @@ if(Thrift_FOUND OR THRIFT_FOUND)
     # thrift/windows/config.h for Visual C++.
     set_target_properties(thrift::thrift PROPERTIES INTERFACE_LINK_LIBRARIES "ws2_32")
   endif()
+
+  if(Thrift_COMPILER_FOUND)
+    add_executable(thrift::compiler IMPORTED)
+    set_target_properties(thrift::compiler PROPERTIES IMPORTED_LOCATION
+                                                      "${THRIFT_COMPILER}")
+  endif()
 endif()

cpp/cmake_modules/ThirdpartyToolchain.cmake

@@ -956,34 +956,6 @@ set(Boost_ADDITIONAL_VERSIONS
     "1.60.0"
     "1.60")
 
-# Thrift needs Boost if we're building the bundled version with version < 0.13,
-# so we first need to determine whether we're building it
-if(ARROW_WITH_THRIFT AND Thrift_SOURCE STREQUAL "AUTO")
-  find_package(Thrift 0.11.0 MODULE COMPONENTS ${ARROW_THRIFT_REQUIRED_COMPONENTS})
-  if(Thrift_FOUND)
-    find_package(PkgConfig QUIET)
-    pkg_check_modules(THRIFT_PC
-                      thrift
-                      NO_CMAKE_PATH
-                      NO_CMAKE_ENVIRONMENT_PATH
-                      QUIET)
-    if(THRIFT_PC_FOUND)
-      string(APPEND ARROW_PC_REQUIRES_PRIVATE " thrift")
-    endif()
-  else()
-    set(Thrift_SOURCE "BUNDLED")
-  endif()
-endif()
-
-# Thrift < 0.13 has a compile-time header dependency on boost
-if(Thrift_SOURCE STREQUAL "BUNDLED" AND ARROW_THRIFT_BUILD_VERSION VERSION_LESS "0.13")
-  set(THRIFT_REQUIRES_BOOST TRUE)
-elseif(THRIFT_VERSION VERSION_LESS "0.13")
-  set(THRIFT_REQUIRES_BOOST TRUE)
-else()
-  set(THRIFT_REQUIRES_BOOST FALSE)
-endif()
-
 # Compilers that don't support int128_t have a compile-time
 # (header-only) dependency on Boost for int128_t.
 if(ARROW_USE_UBSAN)
@@ -1011,7 +983,7 @@ if(ARROW_BUILD_INTEGRATION
   set(ARROW_USE_BOOST TRUE)
   set(ARROW_BOOST_REQUIRE_LIBRARY TRUE)
 elseif(ARROW_GANDIVA
-       OR (ARROW_WITH_THRIFT AND THRIFT_REQUIRES_BOOST)
+       OR ARROW_WITH_THRIFT
        OR (NOT ARROW_USE_NATIVE_INT128))
   set(ARROW_USE_BOOST TRUE)
   set(ARROW_BOOST_REQUIRE_LIBRARY FALSE)
@@ -1492,35 +1464,33 @@ macro(build_thrift)
   set_target_properties(thrift::thrift
                         PROPERTIES IMPORTED_LOCATION "${THRIFT_LIB}"
                                    INTERFACE_INCLUDE_DIRECTORIES "${THRIFT_INCLUDE_DIR}")
-  if(CMAKE_VERSION VERSION_LESS 3.11)
-    set_target_properties(${BOOST_LIBRARY} PROPERTIES INTERFACE_LINK_LIBRARIES
+  if(ARROW_USE_BOOST)
+    if(CMAKE_VERSION VERSION_LESS 3.11)
+      set_target_properties(thrift::thrift PROPERTIES INTERFACE_LINK_LIBRARIES
                                                       Boost::headers)
-  else()
-    target_link_libraries(thrift::thrift INTERFACE Boost::headers)
+    else()
+      target_link_libraries(thrift::thrift INTERFACE Boost::headers)
+    endif()
   endif()
   add_dependencies(toolchain thrift_ep)
   add_dependencies(thrift::thrift thrift_ep)
-  set(THRIFT_VERSION ${ARROW_THRIFT_BUILD_VERSION})
+  set(Thrift_VERSION ${ARROW_THRIFT_BUILD_VERSION})
 
   list(APPEND ARROW_BUNDLED_STATIC_LIBS thrift::thrift)
 endmacro()
 
 if(ARROW_WITH_THRIFT)
-  # We already may have looked for Thrift earlier, when considering whether
-  # to build Boost, so don't look again if already found.
-  if(NOT Thrift_FOUND)
-    # Thrift c++ code generated by 0.13 requires 0.11 or greater
-    resolve_dependency(Thrift
-                       REQUIRED_VERSION
-                       0.11.0
-                       PC_PACKAGE_NAMES
-                       thrift)
-  endif()
+  # Thrift c++ code generated by 0.13 requires 0.11 or greater
+  resolve_dependency(Thrift
+                     REQUIRED_VERSION
+                     0.11.0
+                     PC_PACKAGE_NAMES
+                     thrift)
 
-  string(REPLACE "." ";" VERSION_LIST ${THRIFT_VERSION})
-  list(GET VERSION_LIST 0 THRIFT_VERSION_MAJOR)
-  list(GET VERSION_LIST 1 THRIFT_VERSION_MINOR)
-  list(GET VERSION_LIST 2 THRIFT_VERSION_PATCH)
+  string(REPLACE "." ";" Thrift_VERSION_LIST ${Thrift_VERSION})
+  list(GET Thrift_VERSION_LIST 0 Thrift_VERSION_MAJOR)
+  list(GET Thrift_VERSION_LIST 1 Thrift_VERSION_MINOR)
+  list(GET Thrift_VERSION_LIST 2 Thrift_VERSION_PATCH)
 endif()
 
 # ----------------------------------------------------------------------

cpp/src/arrow/dbi/hiveserver2/CMakeLists.txt

@@ -31,29 +31,32 @@ set(ARROW_HIVESERVER2_SRCS
     types.cc
     util.cc)
 
+set(HIVESERVER2_THRIFT_SRC_DIR "${ARROW_BINARY_DIR}/src/arrow/dbi/hiveserver2")
+file(MAKE_DIRECTORY ${HIVESERVER2_THRIFT_SRC_DIR})
 add_subdirectory(thrift)
 
+# *_constants.* aren't generated when "const" doesn't exist in *.thrift.
 set(HIVESERVER2_THRIFT_SRC
-    ErrorCodes_constants.cpp
-    ErrorCodes_types.cpp
-    ImpalaService.cpp
-    ImpalaService_constants.cpp
-    ImpalaService_types.cpp
-    ImpalaHiveServer2Service.cpp
-    beeswax_constants.cpp
-    beeswax_types.cpp
-    BeeswaxService.cpp
-    TCLIService.cpp
-    TCLIService_constants.cpp
-    TCLIService_types.cpp
-    ExecStats_constants.cpp
-    ExecStats_types.cpp
-    hive_metastore_constants.cpp
-    hive_metastore_types.cpp
-    Status_constants.cpp
-    Status_types.cpp
-    Types_constants.cpp
-    Types_types.cpp)
+    ${HIVESERVER2_THRIFT_SRC_DIR}/ErrorCodes_constants.cpp
+    ${HIVESERVER2_THRIFT_SRC_DIR}/ErrorCodes_types.cpp
+    ${HIVESERVER2_THRIFT_SRC_DIR}/ImpalaService.cpp
+    # ${HIVESERVER2_THRIFT_SRC_DIR}/ImpalaService_constants.cpp
+    ${HIVESERVER2_THRIFT_SRC_DIR}/ImpalaService_types.cpp
+    ${HIVESERVER2_THRIFT_SRC_DIR}/ImpalaHiveServer2Service.cpp
+    # ${HIVESERVER2_THRIFT_SRC_DIR}/beeswax_constants.cpp
+    ${HIVESERVER2_THRIFT_SRC_DIR}/beeswax_types.cpp
+    ${HIVESERVER2_THRIFT_SRC_DIR}/BeeswaxService.cpp
+    ${HIVESERVER2_THRIFT_SRC_DIR}/TCLIService.cpp
+    ${HIVESERVER2_THRIFT_SRC_DIR}/TCLIService_constants.cpp
+    ${HIVESERVER2_THRIFT_SRC_DIR}/TCLIService_types.cpp
+    # ${HIVESERVER2_THRIFT_SRC_DIR}/ExecStats_constants.cpp
+    ${HIVESERVER2_THRIFT_SRC_DIR}/ExecStats_types.cpp
+    ${HIVESERVER2_THRIFT_SRC_DIR}/hive_metastore_constants.cpp
+    ${HIVESERVER2_THRIFT_SRC_DIR}/hive_metastore_types.cpp
+    # ${HIVESERVER2_THRIFT_SRC_DIR}/Status_constants.cpp
+    ${HIVESERVER2_THRIFT_SRC_DIR}/Status_types.cpp
+    # ${HIVESERVER2_THRIFT_SRC_DIR}/Types_constants.cpp
+    ${HIVESERVER2_THRIFT_SRC_DIR}/Types_types.cpp)
 
 set_source_files_properties(${HIVESERVER2_THRIFT_SRC}
                             PROPERTIES COMPILE_FLAGS

cpp/src/arrow/dbi/hiveserver2/thrift/CMakeLists.txt

@@ -39,14 +39,14 @@ function(HS2_THRIFT_GEN VAR)
     # Get basename
     get_filename_component(FIL_WE ${FIL} NAME_WE)
 
-    set(GEN_DIR "${OUTPUT_DIR}/arrow/dbi/hiveserver2")
-
     # All the output files we can determine based on filename.
     #   - Does not include .skeleton.cpp files
     #   - Does not include java output files
     set(OUTPUT_BE_FILE
-        "${GEN_DIR}/${FIL_WE}_types.cpp" "${GEN_DIR}/${FIL_WE}_types.h"
-        "${GEN_DIR}/${FIL_WE}_constants.cpp" "${GEN_DIR}/${FIL_WE}_constants.h")
+        "${HIVESERVER2_THRIFT_SRC_DIR}/${FIL_WE}_types.cpp"
+        "${HIVESERVER2_THRIFT_SRC_DIR}/${FIL_WE}_types.h"
+        "${HIVESERVER2_THRIFT_SRC_DIR}/${FIL_WE}_constants.cpp"
+        "${HIVESERVER2_THRIFT_SRC_DIR}/${FIL_WE}_constants.h")
     list(APPEND ${VAR} ${OUTPUT_BE_FILE})
 
     # BeeswaxService thrift generation
@@ -58,22 +58,22 @@ function(HS2_THRIFT_GEN VAR)
         --gen
         cpp
         -out
-        ${GEN_DIR})
+        ${HIVESERVER2_THRIFT_SRC_DIR})
     if(FIL STREQUAL "beeswax.thrift")
       set(CPP_ARGS
           -r
           -nowarn
           --gen
           cpp
           -out
-          ${GEN_DIR})
+          ${HIVESERVER2_THRIFT_SRC_DIR})
     endif(FIL STREQUAL "beeswax.thrift")
 
     # Be able to include generated ErrorCodes.thrift file
     set(CPP_ARGS ${CPP_ARGS} -I ${CMAKE_CURRENT_BINARY_DIR})
 
     add_custom_command(OUTPUT ${OUTPUT_BE_FILE}
-                       COMMAND ${THRIFT_COMPILER} ${CPP_ARGS} ${FIL}
+                       COMMAND thrift::compiler ${CPP_ARGS} ${FIL}
                        DEPENDS ${ABS_FIL}
                        COMMENT "Running thrift compiler on ${FIL}"
                        WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
@@ -85,10 +85,8 @@ function(HS2_THRIFT_GEN VAR)
       PARENT_SCOPE)
 endfunction(HS2_THRIFT_GEN)
 
-message("Using Thrift compiler: ${THRIFT_COMPILER}")
-
-set(OUTPUT_DIR ${ARROW_BINARY_DIR}/src)
-file(MAKE_DIRECTORY ${OUTPUT_DIR})
+get_target_property(THRIFT_COMPILER thrift::compiler IMPORTED_LOCATION)
+message(STATUS "Using Thrift compiler: ${THRIFT_COMPILER}")
 
 add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/ErrorCodes.thrift
                    COMMAND python generate_error_codes.py ${CMAKE_CURRENT_BINARY_DIR}

cpp/src/arrow/dbi/hiveserver2/thrift_internal.cc

@@ -191,12 +191,15 @@ Status TStatusToStatus(const hs2::TStatus& tstatus) {
     case hs2::TStatusCode::SUCCESS_STATUS:
       return Status::OK();
     case hs2::TStatusCode::SUCCESS_WITH_INFO_STATUS: {
-      std::stringstream ss;
-      for (size_t i = 0; i < tstatus.infoMessages.size(); i++) {
-        if (i != 0) ss << ",";
-        ss << tstatus.infoMessages[i];
-      }
-      return Status::OK(ss.str());
+      // We can't return OK with message since
+      // ARROW-6847/e080766e742dbdee9aefa7ca8b62f723ea60b656.
+      // std::stringstream ss;
+      // for (size_t i = 0; i < tstatus.infoMessages.size(); i++) {
+      //   if (i != 0) ss << ",";
+      //   ss << tstatus.infoMessages[i];
+      // }
+      // return Status::OK(ss.str());
+      return Status::OK();
     }
     case hs2::TStatusCode::STILL_EXECUTING_STATUS:
       return Status::ExecutionError("Still executing");
@@ -226,7 +229,7 @@ std::unique_ptr<ColumnType> TTypeDescToColumnType(const hs2::TTypeDesc& ttype_de
       return std::unique_ptr<ColumnType>(new CharacterType(
           type_id,
           qualifiers.at(hs2::g_TCLIService_constants.CHARACTER_MAXIMUM_LENGTH).i32Value));
-    } catch (std::out_of_range e) {
+    } catch (std::out_of_range& e) {
       ARROW_LOG(ERROR) << "Character type qualifiers invalid: " << e.what();
       return std::unique_ptr<ColumnType>(new PrimitiveType(ColumnType::TypeId::INVALID));
     }
@@ -240,7 +243,7 @@ std::unique_ptr<ColumnType> TTypeDescToColumnType(const hs2::TTypeDesc& ttype_de
       return std::unique_ptr<ColumnType>(new DecimalType(
           type_id, qualifiers.at(hs2::g_TCLIService_constants.PRECISION).i32Value,
           qualifiers.at(hs2::g_TCLIService_constants.SCALE).i32Value));
-    } catch (std::out_of_range e) {
+    } catch (std::out_of_range& e) {
       ARROW_LOG(ERROR) << "Decimal type qualifiers invalid: " << e.what();
       return std::unique_ptr<ColumnType>(new PrimitiveType(ColumnType::TypeId::INVALID));
     }

cpp/src/parquet/CMakeLists.txt

@@ -269,8 +269,8 @@ endif()
 
 add_dependencies(parquet ${PARQUET_LIBRARIES} thrift::thrift)
 
-add_definitions(-DPARQUET_THRIFT_VERSION_MAJOR=${THRIFT_VERSION_MAJOR})
-add_definitions(-DPARQUET_THRIFT_VERSION_MINOR=${THRIFT_VERSION_MINOR})
+add_definitions(-DPARQUET_THRIFT_VERSION_MAJOR=${Thrift_VERSION_MAJOR})
+add_definitions(-DPARQUET_THRIFT_VERSION_MINOR=${Thrift_VERSION_MINOR})
 
 # Thrift requires these definitions for some types that we use
 foreach(LIB_TARGET ${PARQUET_LIBRARIES})