philo-he commented on code in PR #12625:
URL: https://github.com/apache/gluten/pull/12625#discussion_r3655278329
##########
cpp/velox/benchmarks/CMakeLists.txt:
##########
@@ -20,6 +20,9 @@ target_include_directories(
${CMAKE_SOURCE_DIR}/core)
target_link_libraries(velox_benchmark_common PUBLIC velox benchmark::benchmark
google::glog)
+if(BUILD_TESTS)
+ target_link_libraries(velox_benchmark_common PRIVATE GTest::gtest)
+endif()
Review Comment:
Just curious why we need the above code. Could you clarify?
##########
dev/vcpkg/ports/arrow/vcpkg.json:
##########
@@ -0,0 +1,149 @@
+{
+ "name": "arrow",
+ "version": "18.0.0",
Review Comment:
It seems that the other build path still uses arrow 15.0.0. Should we use a
consistent version? Perhaps, we need a follow-up PR to upgrade the arrow
version in the following link.
https://github.com/apache/gluten/blob/474ded1f0e81cbc65993028b0a0fdc19483cdc80/dev/build-arrow.sh#L23
##########
cpp/velox/CMakeLists.txt:
##########
@@ -342,12 +347,28 @@ if(BUILD_TESTS)
import_library(
facebook::velox::file_test_utils
${VELOX_BUILD_PATH}/velox/common/file/tests/libvelox_file_test_utils.a)
+ import_library(
+ facebook::velox::vector_fuzzer_util
+ ${VELOX_BUILD_PATH}/velox/vector/fuzzer/libvelox_vector_fuzzer_util.a)
+ import_library(
+ facebook::velox::constrained_vector_generator
+
${VELOX_BUILD_PATH}/velox/vector/fuzzer/libvelox_constrained_vector_generator.a
+ )
+ target_link_libraries(facebook::velox::constrained_vector_generator
+ INTERFACE facebook::velox::vector_fuzzer_util)
+ import_library(
+ facebook::velox::vector_fuzzer
+ ${VELOX_BUILD_PATH}/velox/vector/fuzzer/libvelox_vector_fuzzer.a)
+ target_link_libraries(
+ facebook::velox::vector_fuzzer
+ INTERFACE facebook::velox::vector_fuzzer_util
+ facebook::velox::constrained_vector_generator)
Review Comment:
Are the above changes also related to this PR?
##########
ep/build-velox/src/get-velox.sh:
##########
@@ -153,11 +153,7 @@ function apply_provided_velox_patch {
}
function apply_compilation_fixes {
- local SUDO_CMD=""
- if [ "$OS" == "Linux" ]; then
- SUDO_CMD="sudo"
- fi
- $SUDO_CMD cp ${CURRENT_DIR}/modify_arrow.patch
${VELOX_HOME}/CMake/resolve_dependency_modules/arrow/
+ cp ${CURRENT_DIR}/modify_arrow.patch
${VELOX_HOME}/CMake/resolve_dependency_modules/arrow/
Review Comment:
Could you check the historical commit that introduced `SUDO_CMD="sudo"`?
Not sure whether we still need it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
