-
Notifications
You must be signed in to change notification settings - Fork 454
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] Apply patch to Velox for compilation #3520
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,6 +65,19 @@ for arg in "$@"; do | |
esac | ||
done | ||
|
||
function apply_compilation_fixes { | ||
current_dir=$1 | ||
velox_home=$2 | ||
sudo cp ${current_dir}/modify_velox.patch ${velox_home}/ | ||
sudo cp ${current_dir}/modify_arrow.patch ${velox_home}/third_party/ | ||
cd ${velox_home} | ||
git apply modify_velox.patch | ||
if [ $? -ne 0 ]; then | ||
echo "Failed to apply compilation fixes to Velox: $?." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
||
exit 1 | ||
fi | ||
} | ||
|
||
function compile { | ||
TARGET_BUILD_COMMIT=$(git rev-parse --verify HEAD) | ||
|
||
|
@@ -245,6 +258,7 @@ fi | |
echo "Target Velox commit: $TARGET_BUILD_COMMIT" | ||
|
||
check_commit | ||
apply_compilation_fixes $CURRENT_DIR $VELOX_HOME | ||
compile | ||
|
||
echo "Successfully built Velox from Source." | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake | ||
index 94f926039..f7ebf9233 100644 | ||
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake | ||
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake | ||
@@ -2501,13 +2501,9 @@ if(ARROW_WITH_ZSTD) | ||
if(ZSTD_VENDORED) | ||
set(ARROW_ZSTD_LIBZSTD zstd::libzstd_static) | ||
else() | ||
- if(ARROW_ZSTD_USE_SHARED) | ||
- set(ARROW_ZSTD_LIBZSTD zstd::libzstd_shared) | ||
- else() | ||
- set(ARROW_ZSTD_LIBZSTD zstd::libzstd_static) | ||
- endif() | ||
+ set(ARROW_ZSTD_LIBZSTD zstd::libzstd_shared) | ||
if(NOT TARGET ${ARROW_ZSTD_LIBZSTD}) | ||
- message(FATAL_ERROR "Zstandard target doesn't exist: ${ARROW_ZSTD_LIBZSTD}") | ||
+ set(ARROW_ZSTD_LIBZSTD zstd::libzstd_static) | ||
endif() | ||
message(STATUS "Found Zstandard: ${ARROW_ZSTD_LIBZSTD}") | ||
endif() | ||
diff --git a/cpp/src/arrow/c/helpers.h b/cpp/src/arrow/c/helpers.h | ||
index a24f272fe..e25f78c85 100644 | ||
--- a/cpp/src/arrow/c/helpers.h | ||
+++ b/cpp/src/arrow/c/helpers.h | ||
@@ -17,6 +17,7 @@ | ||
|
||
#pragma once | ||
|
||
+#include <cassert> | ||
#include <stdio.h> | ||
#include <stdlib.h> | ||
#include <string.h> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,152 @@ | ||
diff --git a/CMake/Findlz4.cmake b/CMake/Findlz4.cmake | ||
index d49115f12..1aaa8e532 100644 | ||
--- a/CMake/Findlz4.cmake | ||
+++ b/CMake/Findlz4.cmake | ||
@@ -21,18 +21,19 @@ find_package_handle_standard_args(lz4 DEFAULT_MSG LZ4_LIBRARY LZ4_INCLUDE_DIR) | ||
|
||
mark_as_advanced(LZ4_LIBRARY LZ4_INCLUDE_DIR) | ||
|
||
-get_filename_component(liblz4_ext ${LZ4_LIBRARY} EXT) | ||
-if(liblz4_ext STREQUAL ".a") | ||
- set(liblz4_type STATIC) | ||
-else() | ||
- set(liblz4_type SHARED) | ||
-endif() | ||
- | ||
if(NOT TARGET lz4::lz4) | ||
- add_library(lz4::lz4 ${liblz4_type} IMPORTED) | ||
- set_target_properties(lz4::lz4 PROPERTIES INTERFACE_INCLUDE_DIRECTORIES | ||
- "${LZ4_INCLUDE_DIR}") | ||
- set_target_properties( | ||
- lz4::lz4 PROPERTIES IMPORTED_LINK_INTERFACE_LANGUAGES "C" | ||
- IMPORTED_LOCATION "${LZ4_LIBRARIES}") | ||
+ add_library(lz4::lz4 UNKNOWN IMPORTED) | ||
+ set_target_properties(lz4::lz4 PROPERTIES | ||
+ INTERFACE_INCLUDE_DIRECTORIES "${LZ4_INCLUDE_DIR}" | ||
+ IMPORTED_LINK_INTERFACE_LANGUAGES "C" | ||
+ IMPORTED_LOCATION_RELEASE "${LZ4_LIBRARY_RELEASE}") | ||
+ set_property(TARGET lz4::lz4 APPEND PROPERTY | ||
+ IMPORTED_CONFIGURATIONS RELEASE) | ||
+ | ||
+ if(LZ4_LIBRARY_DEBUG) | ||
+ set_property(TARGET lz4::lz4 APPEND PROPERTY | ||
+ IMPORTED_CONFIGURATIONS DEBUG) | ||
+ set_property(TARGET lz4::lz4 PROPERTY | ||
+ IMPORTED_LOCATION_DEBUG "${LZ4_LIBRARY_DEBUG}") | ||
+ endif() | ||
endif() | ||
diff --git a/CMakeLists.txt b/CMakeLists.txt | ||
index b34966b7a..1999a50f0 100644 | ||
--- a/CMakeLists.txt | ||
+++ b/CMakeLists.txt | ||
@@ -211,10 +211,15 @@ if(VELOX_ENABLE_ABFS) | ||
endif() | ||
|
||
if(VELOX_ENABLE_HDFS) | ||
- find_library( | ||
- LIBHDFS3 | ||
- NAMES libhdfs3.so libhdfs3.dylib | ||
- HINTS "${CMAKE_SOURCE_DIR}/hawq/depends/libhdfs3/_build/src/" REQUIRED) | ||
+ find_package(libhdfs3) | ||
+ if(libhdfs3_FOUND AND TARGET HDFS::hdfs3) | ||
+ set(LIBHDFS3 HDFS::hdfs3) | ||
+ else() | ||
+ find_library( | ||
+ LIBHDFS3 | ||
+ NAMES libhdfs3.so libhdfs3.dylib | ||
+ HINTS "${CMAKE_SOURCE_DIR}/hawq/depends/libhdfs3/_build/src/" REQUIRED) | ||
+ endif() | ||
add_definitions(-DVELOX_ENABLE_HDFS3) | ||
endif() | ||
|
||
@@ -380,7 +385,7 @@ resolve_dependency(Boost 1.66.0 COMPONENTS ${BOOST_INCLUDE_LIBRARIES}) | ||
# for reference. find_package(range-v3) | ||
|
||
set_source(gflags) | ||
-resolve_dependency(gflags COMPONENTS shared) | ||
+resolve_dependency(gflags) | ||
if(NOT TARGET gflags::gflags) | ||
# This is a bit convoluted, but we want to be able to use gflags::gflags as a | ||
# target even when velox is built as a subproject which uses | ||
diff --git a/scripts/setup-ubuntu.sh b/scripts/setup-ubuntu.sh | ||
index a1dda4b6c..dc423c826 100755 | ||
--- a/scripts/setup-ubuntu.sh | ||
+++ b/scripts/setup-ubuntu.sh | ||
@@ -31,7 +31,7 @@ export CMAKE_BUILD_TYPE=Release | ||
# Install all velox and folly dependencies. | ||
# The is an issue on 22.04 where a version conflict prevents glog install, | ||
# installing libunwind first fixes this. | ||
-sudo --preserve-env apt update && sudo --preserve-env apt install -y libunwind-dev && \ | ||
+sudo --preserve-env apt update && \ | ||
sudo --preserve-env apt install -y \ | ||
g++ \ | ||
cmake \ | ||
diff --git a/third_party/CMakeLists.txt b/third_party/CMakeLists.txt | ||
index 570af4593..6b20e7908 100644 | ||
--- a/third_party/CMakeLists.txt | ||
+++ b/third_party/CMakeLists.txt | ||
@@ -27,6 +27,7 @@ if(VELOX_ENABLE_ARROW) | ||
set(ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep") | ||
set(ARROW_CMAKE_ARGS | ||
-DARROW_PARQUET=ON | ||
+ -DARROW_FILESYSTEM=ON | ||
-DARROW_WITH_LZ4=ON | ||
-DARROW_WITH_SNAPPY=ON | ||
-DARROW_WITH_ZLIB=ON | ||
@@ -68,6 +69,7 @@ if(VELOX_ENABLE_ARROW) | ||
arrow_ep | ||
PREFIX ${ARROW_PREFIX} | ||
URL ${VELOX_ARROW_SOURCE_URL} | ||
+ PATCH_COMMAND patch -p1 < ${CMAKE_CURRENT_SOURCE_DIR}/modify_arrow.patch | ||
URL_HASH ${VELOX_ARROW_BUILD_SHA256_CHECKSUM} | ||
SOURCE_SUBDIR cpp | ||
CMAKE_ARGS ${ARROW_CMAKE_ARGS} | ||
diff --git a/velox/common/process/tests/CMakeLists.txt b/velox/common/process/tests/CMakeLists.txt | ||
index d64466568..52927c4a5 100644 | ||
--- a/velox/common/process/tests/CMakeLists.txt | ||
+++ b/velox/common/process/tests/CMakeLists.txt | ||
@@ -17,4 +17,4 @@ add_executable(velox_process_test TraceContextTest.cpp) | ||
add_test(velox_process_test velox_process_test) | ||
|
||
target_link_libraries(velox_process_test PRIVATE velox_process fmt::fmt gtest | ||
- gtest_main) | ||
+ gtest_main glog::glog gflags::gflags) | ||
diff --git a/velox/connectors/hive/storage_adapters/hdfs/tests/HdfsMiniCluster.cpp b/velox/connectors/hive/storage_adapters/hdfs/tests/HdfsMiniCluster.cpp | ||
index 10ee508ba..027a58ecc 100644 | ||
--- a/velox/connectors/hive/storage_adapters/hdfs/tests/HdfsMiniCluster.cpp | ||
+++ b/velox/connectors/hive/storage_adapters/hdfs/tests/HdfsMiniCluster.cpp | ||
@@ -72,7 +72,7 @@ HdfsMiniCluster::HdfsMiniCluster() { | ||
"Failed to find minicluster executable {}'", miniClusterExecutableName); | ||
} | ||
boost::filesystem::path hadoopHomeDirectory = exePath_; | ||
- hadoopHomeDirectory.remove_leaf().remove_leaf(); | ||
+ hadoopHomeDirectory.remove_filename().remove_filename(); | ||
setupEnvironment(hadoopHomeDirectory.string()); | ||
} | ||
|
||
diff --git a/velox/dwio/common/CMakeLists.txt b/velox/dwio/common/CMakeLists.txt | ||
index c4b767317..8bcf766e7 100644 | ||
--- a/velox/dwio/common/CMakeLists.txt | ||
+++ b/velox/dwio/common/CMakeLists.txt | ||
@@ -73,4 +73,5 @@ target_link_libraries( | ||
velox_exec | ||
Boost::regex | ||
Folly::folly | ||
- glog::glog) | ||
+ glog::glog | ||
+ protobuf::libprotobuf) | ||
diff --git a/velox/dwio/parquet/writer/arrow/tests/CMakeLists.txt b/velox/dwio/parquet/writer/arrow/tests/CMakeLists.txt | ||
index ce1bc6782..3139314b4 100644 | ||
--- a/velox/dwio/parquet/writer/arrow/tests/CMakeLists.txt | ||
+++ b/velox/dwio/parquet/writer/arrow/tests/CMakeLists.txt | ||
@@ -40,7 +40,9 @@ target_link_libraries( | ||
gtest_main | ||
parquet | ||
arrow | ||
- arrow_testing) | ||
+ arrow_testing | ||
+ glog::glog | ||
+ gflags::gflags) | ||
|
||
add_library( | ||
velox_dwio_arrow_parquet_writer_test_lib |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rui-mo Do we need to git apply modify_arrow.patch like modify_velox.patch in line 74?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is applied to Arrow during compilation: https://github.com/oap-project/gluten/pull/3520/files#diff-b41402fae332ead78c4646843c9bcf66a2813203cdc82521c9c9f130b48146dbR100.