Copilot commented on code in PR #60946:
URL: https://github.com/apache/doris/pull/60946#discussion_r2870968171
##########
thirdparty/patches/paimon-cpp-buildutils-static-deps.patch:
##########
@@ -97,6 +97,156 @@ diff --git a/cmake_modules/ThirdpartyToolchain.cmake
b/cmake_modules/ThirdpartyT
-Dprotobuf_DEBUG_POSTFIX=)
set(PROTOBUF_CONFIGURE SOURCE_SUBDIR "cmake" CMAKE_ARGS
${PROTOBUF_CMAKE_ARGS})
+diff --git a/cmake_modules/ThirdpartyToolchain.cmake
b/cmake_modules/ThirdpartyToolchain.cmake
+--- a/cmake_modules/ThirdpartyToolchain.cmake
++++ b/cmake_modules/ThirdpartyToolchain.cmake
+@@ -34,6 +34,16 @@ set(EP_COMMON_TOOLCHAIN
"-DCMAKE_C_COMPILER=${CMAKE_C_COMPILER}"
+ "-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}")
+
++option(PAIMON_USE_EXTERNAL_ARROW "Reuse external Arrow/Parquet instead of
building arrow_ep" OFF)
++set(PAIMON_EXTERNAL_ARROW_INCLUDE_DIR "" CACHE PATH
++ "Include directory for external Arrow/Parquet headers")
++set(PAIMON_EXTERNAL_ARROW_LIB "" CACHE FILEPATH "Path to external libarrow.a")
++set(PAIMON_EXTERNAL_ARROW_DATASET_LIB "" CACHE FILEPATH "Path to external
libarrow_dataset.a")
++set(PAIMON_EXTERNAL_ARROW_ACERO_LIB "" CACHE FILEPATH "Path to external
libarrow_acero.a")
++set(PAIMON_EXTERNAL_PARQUET_LIB "" CACHE FILEPATH "Path to external
libparquet.a")
++set(PAIMON_EXTERNAL_ARROW_BUNDLED_DEPS_LIB "" CACHE FILEPATH
++ "Path to external libarrow_bundled_dependencies.a")
++
+ macro(set_urls URLS)
+ set(${URLS} ${ARGN})
+ endmacro()
+
+diff --git a/cmake_modules/ThirdpartyToolchain.cmake
b/cmake_modules/ThirdpartyToolchain.cmake
+--- a/cmake_modules/ThirdpartyToolchain.cmake
++++ b/cmake_modules/ThirdpartyToolchain.cmake
+@@ -961,5 +961,95 @@ macro(build_orc)
+ endmacro()
+
+ macro(build_arrow)
+- message(STATUS "Building Arrow from source")
++ if(PAIMON_USE_EXTERNAL_ARROW)
++ set(ARROW_INCLUDE_DIR
"${CMAKE_CURRENT_BINARY_DIR}/doris_external_arrow_include")
++ file(MAKE_DIRECTORY "${ARROW_INCLUDE_DIR}")
++ if(NOT EXISTS "${ARROW_INCLUDE_DIR}/arrow")
++ execute_process(COMMAND "${CMAKE_COMMAND}" -E create_symlink
++ "${PAIMON_EXTERNAL_ARROW_INCLUDE_DIR}/arrow"
++ "${ARROW_INCLUDE_DIR}/arrow")
++ endif()
++ if(EXISTS "${PAIMON_EXTERNAL_ARROW_INCLUDE_DIR}/parquet"
++ AND NOT EXISTS "${ARROW_INCLUDE_DIR}/parquet")
++ execute_process(COMMAND "${CMAKE_COMMAND}" -E create_symlink
++ "${PAIMON_EXTERNAL_ARROW_INCLUDE_DIR}/parquet"
++ "${ARROW_INCLUDE_DIR}/parquet")
++ endif()
++
++ if(NOT PAIMON_EXTERNAL_ARROW_INCLUDE_DIR)
++ message(FATAL_ERROR
++ "PAIMON_EXTERNAL_ARROW_INCLUDE_DIR must be set when
PAIMON_USE_EXTERNAL_ARROW=ON"
++ )
++ endif()
++ if(NOT EXISTS "${PAIMON_EXTERNAL_ARROW_INCLUDE_DIR}")
++ message(FATAL_ERROR
++ "PAIMON_EXTERNAL_ARROW_INCLUDE_DIR not found:
${PAIMON_EXTERNAL_ARROW_INCLUDE_DIR}"
++ )
++ endif()
++
++ foreach(_paimon_external_lib
++ IN ITEMS PAIMON_EXTERNAL_ARROW_LIB
++ PAIMON_EXTERNAL_ARROW_DATASET_LIB
++ PAIMON_EXTERNAL_ARROW_ACERO_LIB
++ PAIMON_EXTERNAL_PARQUET_LIB
++ PAIMON_EXTERNAL_ARROW_BUNDLED_DEPS_LIB)
++ if(NOT ${_paimon_external_lib})
Review Comment:
`if(NOT ${_paimon_external_lib})` is unsafe here: when the cache variable is
empty it expands to `if(NOT )`, which is a CMake parse error (instead of
producing the intended FATAL_ERROR message). Use a DEFINED/empty-string check
on the named variable (e.g., `if(NOT DEFINED ... OR "${...}" STREQUAL "")`) to
fail cleanly.
```suggestion
+ if(NOT DEFINED ${_paimon_external_lib} OR
"${${_paimon_external_lib}}" STREQUAL "")
```
##########
thirdparty/patches/paimon-cpp-buildutils-static-deps.patch:
##########
@@ -97,6 +97,156 @@ diff --git a/cmake_modules/ThirdpartyToolchain.cmake
b/cmake_modules/ThirdpartyT
-Dprotobuf_DEBUG_POSTFIX=)
set(PROTOBUF_CONFIGURE SOURCE_SUBDIR "cmake" CMAKE_ARGS
${PROTOBUF_CMAKE_ARGS})
+diff --git a/cmake_modules/ThirdpartyToolchain.cmake
b/cmake_modules/ThirdpartyToolchain.cmake
+--- a/cmake_modules/ThirdpartyToolchain.cmake
++++ b/cmake_modules/ThirdpartyToolchain.cmake
+@@ -34,6 +34,16 @@ set(EP_COMMON_TOOLCHAIN
"-DCMAKE_C_COMPILER=${CMAKE_C_COMPILER}"
+ "-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}")
+
++option(PAIMON_USE_EXTERNAL_ARROW "Reuse external Arrow/Parquet instead of
building arrow_ep" OFF)
++set(PAIMON_EXTERNAL_ARROW_INCLUDE_DIR "" CACHE PATH
++ "Include directory for external Arrow/Parquet headers")
++set(PAIMON_EXTERNAL_ARROW_LIB "" CACHE FILEPATH "Path to external libarrow.a")
++set(PAIMON_EXTERNAL_ARROW_DATASET_LIB "" CACHE FILEPATH "Path to external
libarrow_dataset.a")
++set(PAIMON_EXTERNAL_ARROW_ACERO_LIB "" CACHE FILEPATH "Path to external
libarrow_acero.a")
++set(PAIMON_EXTERNAL_PARQUET_LIB "" CACHE FILEPATH "Path to external
libparquet.a")
++set(PAIMON_EXTERNAL_ARROW_BUNDLED_DEPS_LIB "" CACHE FILEPATH
++ "Path to external libarrow_bundled_dependencies.a")
++
+ macro(set_urls URLS)
+ set(${URLS} ${ARGN})
+ endmacro()
+
+diff --git a/cmake_modules/ThirdpartyToolchain.cmake
b/cmake_modules/ThirdpartyToolchain.cmake
+--- a/cmake_modules/ThirdpartyToolchain.cmake
++++ b/cmake_modules/ThirdpartyToolchain.cmake
+@@ -961,5 +961,95 @@ macro(build_orc)
+ endmacro()
+
+ macro(build_arrow)
+- message(STATUS "Building Arrow from source")
++ if(PAIMON_USE_EXTERNAL_ARROW)
++ set(ARROW_INCLUDE_DIR
"${CMAKE_CURRENT_BINARY_DIR}/doris_external_arrow_include")
++ file(MAKE_DIRECTORY "${ARROW_INCLUDE_DIR}")
++ if(NOT EXISTS "${ARROW_INCLUDE_DIR}/arrow")
++ execute_process(COMMAND "${CMAKE_COMMAND}" -E create_symlink
++ "${PAIMON_EXTERNAL_ARROW_INCLUDE_DIR}/arrow"
++ "${ARROW_INCLUDE_DIR}/arrow")
++ endif()
++ if(EXISTS "${PAIMON_EXTERNAL_ARROW_INCLUDE_DIR}/parquet"
++ AND NOT EXISTS "${ARROW_INCLUDE_DIR}/parquet")
++ execute_process(COMMAND "${CMAKE_COMMAND}" -E create_symlink
++ "${PAIMON_EXTERNAL_ARROW_INCLUDE_DIR}/parquet"
++ "${ARROW_INCLUDE_DIR}/parquet")
++ endif()
++
++ if(NOT PAIMON_EXTERNAL_ARROW_INCLUDE_DIR)
++ message(FATAL_ERROR
++ "PAIMON_EXTERNAL_ARROW_INCLUDE_DIR must be set when
PAIMON_USE_EXTERNAL_ARROW=ON"
++ )
++ endif()
++ if(NOT EXISTS "${PAIMON_EXTERNAL_ARROW_INCLUDE_DIR}")
Review Comment:
In the PAIMON_USE_EXTERNAL_ARROW branch, PAIMON_EXTERNAL_ARROW_INCLUDE_DIR
is used to create symlinks before it is validated. If the variable is empty,
this expands to "/arrow"/"/parquet" and can create incorrect symlinks or fail
in confusing ways. Validate that PAIMON_EXTERNAL_ARROW_INCLUDE_DIR is set and
exists before the execute_process() symlink creation, and only then create the
include-tree links.
--
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]
