feat(velox-connector): Add Velox's header dependencies to allow building without having them pre-installed. - #12
Conversation
📝 WalkthroughWalkthroughThe PR expands Velox connector dependency setup, threads ChangesVelox Connector Dependency Expansion
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@velox-connector/CMakeLists.txt`:
- Line 36: The include() command in the CMakeLists.txt file at the line that
includes
"${LIBCLP_PLUGIN_VELOX_CONNECTOR_DEPS_DIR}/cmake-settings/all-deps.cmake" is
using an invalid REQUIRED parameter. Remove the REQUIRED keyword from this
include() call since it is not a valid parameter for the include() command.
Valid parameters for include() are OPTIONAL, RESULT_VARIABLE, and
NO_POLICY_SCOPE. Simply delete the REQUIRED text from the end of the include()
statement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d32ca16a-4ef6-403f-b885-30c455e11942
📒 Files selected for processing (4)
taskfiles/velox-connector/deps.yamltaskfiles/velox-connector/main.yamlvelox-connector/CMakeLists.txtvelox-connector/README.md
Builds on the dependency Taskfile by adding the C++ dependencies needed to compile the plugin against the Presto/Velox headers: folly (built from source for its generated config header), glog, gflags, double-conversion, fast_float, xsimd, and re2, plus the compile-time include paths and PIC/AVX2 settings in CMakeLists.txt. Most of these are header-only for the plugin; their implementations are resolved at runtime by the Presto worker.
59b06eb to
9256c10
Compare
20001020ycx
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@velox-connector/README.md`:
- Around line 6-8: The requirements section mentions GCC 11 is required but does
not provide guidance on how to explicitly specify it when performing manual
CMake builds on systems where the default compiler differs. Add a clarification
note or example after the GCC 11 requirement that explains how to force the use
of gcc-11 and g++-11 during CMake configuration, such as by setting the
CMAKE_C_COMPILER and CMAKE_CXX_COMPILER variables, to help users avoid build
failures caused by using an incorrect default compiler version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ab88605-d68b-4fba-909b-6bffe35f8b77
📒 Files selected for processing (4)
taskfiles/velox-connector/deps.yamltaskfiles/velox-connector/main.yamlvelox-connector/CMakeLists.txtvelox-connector/README.md
…01020ycx/clp-plugin-presto-connector into feat/2026-06-22-velox-connector-deps
There was a problem hiding this comment.
♻️ Duplicate comments (1)
velox-connector/README.md (1)
13-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix contradictory compiler export example.
Line 13 says to use GCC 11, but Line 17 and Line 18 export generic
gcc/g++, which can still resolve to GCC 12.Suggested docs fix
- export CC="gcc" - export CXX="g++" + export CC="gcc-11" + export CXX="g++-11"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/README.md` around lines 13 - 18, The documentation states that GCC 11 should be used to work around a GCC 12 bug, but the export statements use generic cc and cxx commands which may still resolve to GCC 12. Update the export statements for CC and CXX to explicitly reference the GCC 11 binaries (such as gcc-11 and g++-11) instead of the generic gcc and g++ commands to ensure the correct compiler version is used during the build process.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@velox-connector/README.md`:
- Around line 13-18: The documentation states that GCC 11 should be used to work
around a GCC 12 bug, but the export statements use generic cc and cxx commands
which may still resolve to GCC 12. Update the export statements for CC and CXX
to explicitly reference the GCC 11 binaries (such as gcc-11 and g++-11) instead
of the generic gcc and g++ commands to ensure the correct compiler version is
used during the build process.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d760cdc0-7bdc-4c0e-a079-20485f6d8554
📒 Files selected for processing (3)
taskfiles/velox-connector/deps.yamltaskfiles/velox-connector/main.yamlvelox-connector/README.md
💤 Files with no reviewable changes (1)
- taskfiles/velox-connector/main.yaml
xsimd is header-only and consumed only through the plugin's include path, so download-and-extract it (like re2) rather than configuring/building it. Point the include path at xsimd-source/include.
include() doesn't accept REQUIRED (valid keywords are OPTIONAL, RESULT_VARIABLE, NO_POLICY_SCOPE); it's a no-op since include() already errors on a missing file.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
velox-connector/CMakeLists.txt (2)
34-39: 🗄️ Data Integrity & Integration | 🟠 MajorRemove the invalid
REQUIREDparameter from theinclude()call.The
include()CMake command does not acceptREQUIREDas a parameter. Valid parameters areOPTIONAL,RESULT_VARIABLE, andNO_POLICY_SCOPE. On line 39, removeREQUIREDfrom:include("${LIBCLP_PLUGIN_VELOX_CONNECTOR_DEPS_DIR}/cmake-settings/all-deps.cmake" REQUIRED)Change to:
include("${LIBCLP_PLUGIN_VELOX_CONNECTOR_DEPS_DIR}/cmake-settings/all-deps.cmake")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/CMakeLists.txt` around lines 34 - 39, The include() CMake command on line 39 uses an invalid REQUIRED parameter which is not supported by CMake. Remove the REQUIRED keyword from the include() call that loads the cmake-settings/all-deps.cmake file referenced by the LIBCLP_PLUGIN_VELOX_CONNECTOR_DEPS_DIR variable. The include() command only accepts OPTIONAL, RESULT_VARIABLE, and NO_POLICY_SCOPE as valid parameters, so simply invoke include() with just the file path argument.
85-98: 🗄️ Data Integrity & Integration | 🔴 CriticalFix incorrect re2 include path: use
re2-source/re2instead ofre2-source.The re2 library headers are located in a
re2/subdirectory within the source directory (e.g.,re2/re2.h,re2/stringpiece.h). The current path${LIBCLP_PLUGIN_VELOX_CONNECTOR_DEPS_DIR}/re2-sourcewill fail to find these headers.Correct the include path to:
${LIBCLP_PLUGIN_VELOX_CONNECTOR_DEPS_DIR}/re2-source/re2The xsimd path is correct as-is (
xsimd-source/include), since xsimd is a header-only library with its headers organized in aninclude/subdirectory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/CMakeLists.txt` around lines 85 - 98, In the include_directories function call, the re2 include path is incorrect and missing a subdirectory. Change the line containing ${LIBCLP_PLUGIN_VELOX_CONNECTOR_DEPS_DIR}/re2-source to instead reference ${LIBCLP_PLUGIN_VELOX_CONNECTOR_DEPS_DIR}/re2-source/re2 so that the compiler can properly locate the re2 headers which are organized under the re2 subdirectory within the re2-source directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@velox-connector/CMakeLists.txt`:
- Around line 34-39: The include() CMake command on line 39 uses an invalid
REQUIRED parameter which is not supported by CMake. Remove the REQUIRED keyword
from the include() call that loads the cmake-settings/all-deps.cmake file
referenced by the LIBCLP_PLUGIN_VELOX_CONNECTOR_DEPS_DIR variable. The include()
command only accepts OPTIONAL, RESULT_VARIABLE, and NO_POLICY_SCOPE as valid
parameters, so simply invoke include() with just the file path argument.
- Around line 85-98: In the include_directories function call, the re2 include
path is incorrect and missing a subdirectory. Change the line containing
${LIBCLP_PLUGIN_VELOX_CONNECTOR_DEPS_DIR}/re2-source to instead reference
${LIBCLP_PLUGIN_VELOX_CONNECTOR_DEPS_DIR}/re2-source/re2 so that the compiler
can properly locate the re2 headers which are organized under the re2
subdirectory within the re2-source directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: af4c9fdd-71c4-4b32-80cc-30a2d8194e2f
📒 Files selected for processing (2)
taskfiles/velox-connector/deps.yamlvelox-connector/CMakeLists.txt
| * System libraries: `libssl-dev`, `libevent-dev`, `libcurl4-openssl-dev` | ||
| * In a container with Velox's [dependencies][velox-deps-setup] installed, such as Presto's dev | ||
| container ([prestodb/presto-native-dependency][presto-native-dependency]): | ||
| * NOTE: Due to a bug in the container's GCC 12, log-surgeon won't compile; set `CC`/`CXX` to |
There was a problem hiding this comment.
Let's create a issue to upgrade clp (log surgeon) get resolve this issue
jackluo923
left a comment
jackluo923
left a comment
There was a problem hiding this comment.
These 5 dep tasks were added without CMAKE_JOBS, unlike every other dep task (fmt, absl, spdlog, …) which caps cmake --build --parallel. With it missing, yscope-dev-utils:cmake:install-remote-tar forwards an empty JOBS → cmake --build --parallel "" → unlimited parallelism, which compiles every translation unit at once and OOMs at the Folly link step on RAM-constrained runners (e.g. the 4-core/16 GB GHA runner, or build.sh's default). Adding the line makes them honour CLP_CPP_MAX_PARALLELISM_PER_BUILD_TASK like the rest of the deps.
I am a bit confused, I dont see this in every other dep task:
There's only Also, you mentioned this is for memory constraint runner, so its a bit orthogoal to what we implemented in this PR, at least for the validation part (memory was not under the testing scope of my validation). Shall we defer these changes to another PR? |
sure, let's move it to another PR |
…01020ycx/clp-plugin-presto-connector into feat/2026-06-22-velox-connector-deps
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
High-level review. Still need to go more in depth later.
Also, task lint:check fails.
| # Plugin target | ||
| # ============================================================================== | ||
|
|
||
| # Compile-time include paths for the third-party headers transitively pulled in by the Velox |
There was a problem hiding this comment.
Couldn't we add these to the generated all-deps.cmake file?
There was a problem hiding this comment.
The reason CLP doesn't need explicit include paths is that CLP calls find_package() on its dependencies,so the headers are found automatically.
For the Velox deps we don't want to link their implementation; we rely on the Presto worker to provide it at runtime for consistency. So the plugin doesn't call find_package() (link) them. Therefore, there's no imported target to carry the include paths, making us to set them explicitly via include_directories().
I've pushed a change so the installed deps' paths come from the _ROOT variables which all-deps.cmake already sets (e.g. ${LIBCLP_PLUGIN_VELOX_CONNECTOR_DEPS_DIR}/absl-install/include → ${absl_ROOT}/include). (re2/xsimd are extracted rather than installed, so they have no _ROOT and keep their source paths.)
Drop a trailing space on the version comment and wrap the double-conversion URL (>100 chars) using the backslash-continuation style used by the other deps.
…15.4 to 2.22.0 in /presto-connector (y-scope#16) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…-deps.cmake. The installed Velox header deps' include paths now come from the <pkg>_ROOT cache variables all-deps.cmake provides (the same source CLP's find_package() uses), instead of hardcoding the <pkg>-install/include paths. re2 and xsimd are extracted rather than installed, so they have no _ROOT and keep their source directories.
20001020ycx
changed the title
…sto commit tag. - Expose re2/xsimd via <pkg>_ROOT in all-deps.cmake so CMakeLists.txt references them consistently with the other dependencies. - Define the Presto commit once as G_PRESTO_GIT_TAG and pass it to CMake, removing the hash that was duplicated in CMakeLists.txt.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
velox-connector/README.md (1)
16-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse explicit GCC 11 binaries in the workaround example.
gcc/g++do not guarantee GCC 11, so this snippet can still pick up the broken container toolchain and fail to build. Please switch the example togcc-11/g++-11to match the note.Suggested docs tweak
- export CC="gcc" - export CXX="g++" + export CC="gcc-11" + export CXX="g++-11"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@velox-connector/README.md` around lines 16 - 22, The workaround example in the README still uses generic gcc/g++ binaries, which may resolve to the broken GCC 12 toolchain instead of GCC 11. Update the build workaround in the documentation to explicitly reference the GCC 11 binaries in the example so it matches the note; adjust the shell snippet in the README section describing the CC/CXX override.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@velox-connector/README.md`:
- Around line 16-22: The workaround example in the README still uses generic
gcc/g++ binaries, which may resolve to the broken GCC 12 toolchain instead of
GCC 11. Update the build workaround in the documentation to explicitly reference
the GCC 11 binaries in the example so it matches the note; adjust the shell
snippet in the README section describing the CC/CXX override.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6fac3145-9772-42d3-bd93-02e1ecd0b269
📒 Files selected for processing (4)
taskfiles/velox-connector/deps.yamltaskfiles/velox-connector/main.yamlvelox-connector/CMakeLists.txtvelox-connector/README.md
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat(velox-connector): Add Velox's header dependencies to allow building without having them pre-installed.
3 participants
Description
This PR extends deps.yaml with the build-time dependencies required by the Velox headers the plugin includes, so the build no longer relies on the official Presto dev container.
The plugin pulls in Presto/Velox headers but doesn't link these libraries, their implementations are resolved at runtime by the Presto worker that dlopens the plugin. So these new dependencies exist only to make the headers compile: none of them are linked into the plugin, and they're built without -fPIC, since their code never ends up in the plugin.
This PR also explains the system libraries required: CLP requires OpenSSL and CURL, and Folly requires LibEvent.
Reviewer's Note
A few notes on why most of these deps are built still though
"yscope-dev-utils:cmake:install-remote-tar", even though we do not link them:Validation Performed
Full clean build in an Ubuntu 24.04 container (
task velox-connector:cleanthentask velox-connector:build), provided that task and system libs are installed:libclp-plugin-velox-connector.so.Summary by CodeRabbit
PRESTO_GIT_TAGto be provided; missing it will stop the build with guidance.