fix(build): Undefined Iceberg* symbols#27094
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR fixes a native build break by linking Iceberg’s Hive split reader library into several Presto native test binaries that use Hive/Iceberg functionality but were previously missing that dependency, causing undefined Iceberg* symbols at link time. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
31219ec to
e51e1da
Compare
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since
velox_hive_iceberg_splitreaderis now a dependency for several test targets, consider factoring the shared link library list into a common CMake variable or function to avoid duplication and keep future changes to these dependencies in sync. - If Iceberg support is optionally buildable/disabled via a CMake flag, please double-check that linking
velox_hive_iceberg_splitreaderis guarded or handled in a way that won’t break configurations where Iceberg is not compiled.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `velox_hive_iceberg_splitreader` is now a dependency for several test targets, consider factoring the shared link library list into a common CMake variable or function to avoid duplication and keep future changes to these dependencies in sync.
- If Iceberg support is optionally buildable/disabled via a CMake flag, please double-check that linking `velox_hive_iceberg_splitreader` is guarded or handled in a way that won’t break configurations where Iceberg is not compiled.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
aditi-pandit
left a comment
There was a problem hiding this comment.
Thanks Tony for this code. I'm approving for now as this is a build failure.
But please implement the sourcery-ai suggestion in a subsequent cleanup PR.
Since velox_hive_iceberg_splitreader is now a dependency for several test targets, consider factoring the shared link library list into a common CMake variable or function to avoid duplication and keep future changes to these dependencies in sync.
|
@tdcmeehan Would you please take a look at this PR? The build break is caused by velox code change. Thank you. |
e51e1da to
e7a9014
Compare
Description
Fix build break.
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Summary by Sourcery
Build: