fix(build): Add library required for Velox as of 44e10f4#27166
fix(build): Add library required for Velox as of 44e10f4#27166czentgr merged 1 commit intoprestodb:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideLinks the new Velox key encoder library into the main Presto native server binary to resolve a missing symbol error introduced by recent Velox changes. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since this fixes a compatibility issue with a specific Velox revision, consider guarding the
velox_key_encoderlinkage with a CMake check on the Velox version or feature so older Velox versions don’t accidentally gain an unused/invalid dependency. - If
velox_key_encoderis only required by a specific connector or component, you might want to link it just to that target rather than the top-levelpresto_cppmain target to keep link dependencies more localized.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since this fixes a compatibility issue with a specific Velox revision, consider guarding the `velox_key_encoder` linkage with a CMake check on the Velox version or feature so older Velox versions don’t accidentally gain an unused/invalid dependency.
- If `velox_key_encoder` is only required by a specific connector or component, you might want to link it just to that target rather than the top-level `presto_cpp` main target to keep link dependencies more localized.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@simoneves I assume this is only seen when building with cudf? We don't see this being an issue in the regular CI. Or is this for future (aka next velox update which is soon to happen)? |
Sorry, I should have been more clear. This fixes the linker failure that will happen (with C++ also) when Presto updates over commit 44e10f4 (@xiaoxmeng's Velox PR #16360) from Feb 13, which is literally the next commit on from where Presto is currently pinned. Like I said, please feel free to suggest an alternative solution if this is too heavy-handed. |
czentgr
left a comment
There was a problem hiding this comment.
Thanks, we could also add this with the velox update but this is fine too.
|
@czentgr I figured they would just be landed together |
|
Thanks for the fix @simoneves. |
Description
Added velox_key_encoder library to main presto_server link.
Motivation and Context
Resolves link failure with Velox as of 44e10f4 (2026/2/13)
HiveIndexReader.cpp:(.text+0x1b37): undefined reference to facebook::velox::serializer::IndexBounds::set(facebook::velox::serializer::IndexBound, facebook::velox::serializer::IndexBound)
Impact
None.
Test Plan
Local builds so far.
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Summary by Sourcery
Build: