chore(ci): Advance velox#27899
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the presto-native-execution Velox dependency to point to the latest upstream commit, without any code or configuration changes in this repository. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
aditi-pandit
left a comment
There was a problem hiding this comment.
@amitkdutta : The build failures are legit. They need to be fixed
From "Explain error" AI
The failure is a linker error in the native build job, not a test assertion failure.
Root cause
velox_exec now references facebook::velox::memory::CustomMemoryResourceRegistry::global(), but the affected Presto test binaries do not link the Velox library that defines that symbol.
From the job log:
- Failing target:
presto_materialized_exchange_reclaim_test - Also fails:
presto_common_test - Error:
undefined reference to facebook::velox::memory::CustomMemoryResourceRegistry::global[abi:cxx11]()
This happens in the workflow build step in .github/workflows/arrow-flight-tests.yml:
- build uses
cmake ... -DPRESTO_ENABLE_ARROW_FLIGHT_CONNECTOR=ON ... - then
ninja -C _build/release -j 4 - Source: https://github.com/prestodb/presto/blob/3abb789abd41333e3af44aa6788a2b4c98bc2980/.github/workflows/arrow-flight-tests.yml
Where to fix
The missing link dependencies are in these CMake targets:
presto-native-execution/presto_cpp/main/operators/tests/CMakeLists.txt
presto_materialized_exchange_reclaim_testcurrently links only:presto_operatorsvelox_execvelox_memoryGTest::gtest
- Source: https://github.com/prestodb/presto/blob/3abb789abd41333e3af44aa6788a2b4c98bc2980/presto-native-execution/presto_cpp/main/operators/tests/CMakeLists.txt
presto-native-execution/presto_cpp/main/common/tests/CMakeLists.txt
presto_common_testlinks several Velox libs, but not the library that now carries the custom memory resource registry symbol.- Source: https://github.com/prestodb/presto/blob/3abb789abd41333e3af44aa6788a2b4c98bc2980/presto-native-execution/presto_cpp/main/common/tests/CMakeLists.txt
Recommended fix
Add the Velox memory allocator / memory-manager implementation library that exports CustomMemoryResourceRegistry::global() to the failing test targets’ link list.
A likely patch is:
# presto-native-execution/presto_cpp/main/operators/tests/CMakeLists.txt
target_link_libraries(
presto_materialized_exchange_reclaim_test
presto_operators
velox_exec
velox_memory
velox_memory_manager
GTest::gtest
)# presto-native-execution/presto_cpp/main/common/tests/CMakeLists.txt
target_link_libraries(
presto_common_test
presto_common
presto_exception
velox_aggregates
velox_exception
velox_file
velox_functions_prestosql
velox_function_registry
velox_presto_serializer
velox_presto_types
velox_window
velox_memory_manager
${RE2}
GTest::gtest
)If velox_memory_manager is not the exact exported target name
The fix is still the same in principle: link the Velox target that provides CustomMemoryResourceRegistry::global(). In newer Velox revisions this symbol may live in a different memory-related target than velox_memory, which is why these test executables now fail to link while other binaries still succeed.
Why this is the right solution
- The failure occurs at link time, not compile time.
- The unresolved symbol originates from
velox/velox/exec/libvelox_exec.a(Task.cpp.o). - That means
velox_execwas compiled successfully, but one of its transitive symbol providers is missing from the final executable link line. - Both failing executables are small test targets with manually curated
target_link_libraries(...), so they are the right place to fix it.
Secondary issue in logs
There is also a stash restore warning:
error extracting zip archive: ... CACHEDIR.TAG: file exists
That did not stop the job; the build continued. It is not the cause of the failure.
Suggested minimal PR change
Update the two CMake test targets to include the missing Velox memory implementation target, rebuild, and rerun the arrow flight tests workflow.
|
Yes its being reverted |
Summary by Sourcery
Chores: