Skip to content

chore(ci): Advance velox#27899

Merged
amitkdutta merged 1 commit into
prestodb:masterfrom
amitkdutta:pop50
Jun 1, 2026
Merged

chore(ci): Advance velox#27899
amitkdutta merged 1 commit into
prestodb:masterfrom
amitkdutta:pop50

Conversation

@amitkdutta
Copy link
Copy Markdown
Contributor

@amitkdutta amitkdutta commented May 29, 2026

== NO RELEASE NOTE ==

Summary by Sourcery

Chores:

  • Update the Velox submodule pointer under presto-native-execution to track the latest desired upstream commit.

@amitkdutta amitkdutta requested review from a team as code owners May 29, 2026 20:07
@prestodb-ci prestodb-ci added the from:Meta PR from Meta label May 29, 2026
@sourcery-ai
Copy link
Copy Markdown
Contributor

sourcery-ai Bot commented May 29, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Updates 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

Change Details Files
Advance the Velox submodule reference to the latest upstream commit.
  • Update the Velox git reference in presto-native-execution/velox to a newer upstream commit hash
  • No source files, build files, or configuration in this repo are modified beyond the submodule pointer
presto-native-execution/velox

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

gggrace14
gggrace14 previously approved these changes May 29, 2026
Copy link
Copy Markdown
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

Where to fix
The missing link dependencies are in these CMake targets:

  1. presto-native-execution/presto_cpp/main/operators/tests/CMakeLists.txt
  1. 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_exec was 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.

@amitkdutta
Copy link
Copy Markdown
Contributor Author

Yes its being reverted
facebookincubator/velox#17672

Copy link
Copy Markdown
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @amitkdutta

@gggrace14 gggrace14 self-requested a review June 1, 2026 16:54
@amitkdutta amitkdutta marked this pull request as ready for review June 1, 2026 16:57
@amitkdutta amitkdutta merged commit bccdb0d into prestodb:master Jun 1, 2026
85 of 87 checks passed
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:Meta PR from Meta

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants