Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VL] Branch 1.2: Make sure the same thrift lib bundled in arrow build is used for building Velox #6451

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions cpp/velox/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -614,21 +614,6 @@ else()
endif()
set(CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES_BCK})

if(ENABLE_GLUTEN_VCPKG)
find_package(Thrift CONFIG)
else()
# Prefer the shared library on system.
set(ARROW_THRIFT_USE_SHARED ON)
find_package(Thrift)
endif()

if(Thrift_FOUND)
target_link_libraries(velox PUBLIC thrift::thrift)
else()
add_velox_dependency(
thrift "${ARROW_HOME}/src/arrow_ep-build/thrift_ep-install/lib/libthrift.a")
endif()

if(BUILD_TESTS)
add_subdirectory(tests)
endif()
Expand Down
47 changes: 35 additions & 12 deletions ep/build-velox/src/modify_velox.patch
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,20 @@ index d49115f12..1aaa8e532 100644
+ endif()
endif()
diff --git a/CMake/resolve_dependency_modules/arrow/CMakeLists.txt b/CMake/resolve_dependency_modules/arrow/CMakeLists.txt
index 3f01df2fd..a8da374a2 100644
index 3f01df2fd..bd3ea501d 100644
--- a/CMake/resolve_dependency_modules/arrow/CMakeLists.txt
+++ b/CMake/resolve_dependency_modules/arrow/CMakeLists.txt
@@ -23,7 +23,11 @@ if(VELOX_ENABLE_ARROW)

@@ -14,16 +14,13 @@
project(Arrow)

if(VELOX_ENABLE_ARROW)
- find_package(Thrift)
- if(Thrift_FOUND)
- set(THRIFT_SOURCE "SYSTEM")
- else()
- set(THRIFT_SOURCE "BUNDLED")
- endif()
-
set(ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep")
set(ARROW_CMAKE_ARGS
- -DARROW_PARQUET=OFF
Expand All @@ -52,16 +61,30 @@ index 3f01df2fd..a8da374a2 100644
-DARROW_WITH_THRIFT=ON
-DARROW_WITH_LZ4=ON
-DARROW_WITH_SNAPPY=ON
@@ -37,7 +41,7 @@ if(VELOX_ENABLE_ARROW)
@@ -37,17 +34,15 @@ if(VELOX_ENABLE_ARROW)
-DCMAKE_INSTALL_PREFIX=${ARROW_PREFIX}/install
-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
-DARROW_BUILD_STATIC=ON
- -DThrift_SOURCE=${THRIFT_SOURCE})
+ -DARROW_BUILD_SHARED=OFF)
set(ARROW_LIBDIR ${ARROW_PREFIX}/install/${CMAKE_INSTALL_LIBDIR})

add_library(thrift STATIC IMPORTED GLOBAL)
@@ -66,6 +70,9 @@ if(VELOX_ENABLE_ARROW)
- if(NOT Thrift_FOUND)
- set(THRIFT_ROOT ${ARROW_PREFIX}/src/arrow_ep-build/thrift_ep-install)
- set(THRIFT_LIB ${THRIFT_ROOT}/lib/libthrift.a)
+ set(THRIFT_ROOT ${ARROW_PREFIX}/src/arrow_ep-build/thrift_ep-install)
+ set(THRIFT_LIB ${THRIFT_ROOT}/lib/libthrift.a)

- file(MAKE_DIRECTORY ${THRIFT_ROOT}/include)
- set(THRIFT_INCLUDE_DIR ${THRIFT_ROOT}/include)
- endif()
+ file(MAKE_DIRECTORY ${THRIFT_ROOT}/include)
+ set(THRIFT_INCLUDE_DIR ${THRIFT_ROOT}/include)

set_property(TARGET thrift PROPERTY INTERFACE_INCLUDE_DIRECTORIES
${THRIFT_INCLUDE_DIR})
@@ -66,6 +61,9 @@ if(VELOX_ENABLE_ARROW)
arrow_ep
PREFIX ${ARROW_PREFIX}
URL ${VELOX_ARROW_SOURCE_URL}
Expand All @@ -77,7 +100,7 @@ index bb7c49907..3372d48b4 100644
+++ b/CMakeLists.txt
@@ -234,10 +234,15 @@ if(VELOX_ENABLE_ABFS)
endif()

if(VELOX_ENABLE_HDFS)
- find_library(
- LIBHDFS3
Expand All @@ -94,10 +117,10 @@ index bb7c49907..3372d48b4 100644
+ endif()
add_definitions(-DVELOX_ENABLE_HDFS3)
endif()

@@ -378,7 +383,7 @@ resolve_dependency(Boost 1.77.0 COMPONENTS ${BOOST_INCLUDE_LIBRARIES})
# for reference. find_package(range-v3)

set_source(gflags)
-resolve_dependency(gflags COMPONENTS ${VELOX_GFLAGS_TYPE})
+resolve_dependency(gflags)
Expand All @@ -110,7 +133,7 @@ index 6797697a1..3e241f8f7 100644
+++ b/velox/common/process/tests/CMakeLists.txt
@@ -18,4 +18,4 @@ add_executable(velox_process_test ProfilerTest.cpp ThreadLocalRegistryTest.cpp
add_test(velox_process_test velox_process_test)

target_link_libraries(velox_process_test PRIVATE velox_process fmt::fmt gtest
- velox_time gtest_main)
+ velox_time gtest_main glog::glog gflags::gflags)
Expand All @@ -119,7 +142,7 @@ index e2a638df6..e383cf205 100644
--- a/velox/connectors/hive/storage_adapters/abfs/RegisterAbfsFileSystem.cpp
+++ b/velox/connectors/hive/storage_adapters/abfs/RegisterAbfsFileSystem.cpp
@@ -38,7 +38,6 @@ std::shared_ptr<FileSystem> abfsFileSystemGenerator(

void registerAbfsFileSystem() {
#ifdef VELOX_ENABLE_ABFS
- LOG(INFO) << "Register ABFS";
Expand All @@ -138,7 +161,7 @@ index 10ee508ba..027a58ecc 100644
+ hadoopHomeDirectory.remove_filename().remove_filename();
setupEnvironment(hadoopHomeDirectory.string());
}

diff --git a/velox/dwio/parquet/writer/arrow/tests/CMakeLists.txt b/velox/dwio/parquet/writer/arrow/tests/CMakeLists.txt
index 2cabfc29a..54329ce23 100644
--- a/velox/dwio/parquet/writer/arrow/tests/CMakeLists.txt
Expand Down
Loading