Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/linux-build-base.yml
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ jobs:
- name: Build Artifact
env:
VELOX_DEPENDENCY_SOURCE: BUNDLED
Boost_SOURCE: SYSTEM

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Environment variable likely ignored – use all-caps BOOST_SOURCE instead of Boost_SOURCE.

resolve_dependency.cmake converts package names to upper-case (e.g., BoostBOOST) and then looks for BOOST_SOURCE.
With the current mixed-case key, Boost will silently fall back to the default (BUNDLED), negating the intent of using the system-installed libraries and increasing build time.

-          Boost_SOURCE: SYSTEM
+          BOOST_SOURCE: SYSTEM
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Boost_SOURCE: SYSTEM
BOOST_SOURCE: SYSTEM
🤖 Prompt for AI Agents
In .github/workflows/linux-build-base.yml at line 55, the environment variable
key is incorrectly cased as Boost_SOURCE, which is ignored by the build system.
Change the key to all uppercase BOOST_SOURCE to ensure the system-installed
Boost libraries are used instead of the default bundled ones, preventing
unnecessary build time increases.

ICU_SOURCE: SYSTEM
MAKEFLAGS: MAX_HIGH_MEM_JOBS=4 MAX_LINK_JOBS=3
run: |
Expand Down
36 changes: 36 additions & 0 deletions CMake/resolve_dependency_modules/antlr4-runtime.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# Copyright (c) Facebook, Inc. and its affiliates.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
include_guard(GLOBAL)

set(VELOX_ANTLR4_RUNTIME_VERSION 4.13.2)
set(VELOX_ANTLR4_RUNTIME_BUILD_SHA256_CHECKSUM
9f18272a9b32b622835a3365f850dd1063d60f5045fb1e12ce475ae6e18a35bb)
set(VELOX_ANTLR4_RUNTIME_SOURCE_URL
"https://github.com/antlr/antlr4/archive/refs/tags/${VELOX_ANTLR4_RUNTIME_VERSION}.tar.gz"
)

velox_resolve_dependency_url(ANTLR4_RUNTIME)

message(STATUS "Building antlr4-runtime from source")

FetchContent_Declare(
antlr4-runtime
URL ${VELOX_ANTLR4_RUNTIME_SOURCE_URL}
URL_HASH ${VELOX_ANTLR4_RUNTIME_BUILD_SHA256_CHECKSUM}
SOURCE_SUBDIR runtime/Cpp OVERRIDE_FIND_PACKAGE)
Comment on lines +28 to +31

@coderabbitai coderabbitai Bot Jun 25, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

URL_HASH missing required algorithm prefix

CMake ≥ 3.24 requires the hash value to be prefixed with the algorithm (e.g., SHA256=). Without it the download fails the integrity check.

 FetchContent_Declare(
   antlr4-runtime
   URL ${VELOX_ANTLR4_RUNTIME_SOURCE_URL}
-  URL_HASH ${VELOX_ANTLR4_RUNTIME_BUILD_SHA256_CHECKSUM}
+  URL_HASH SHA256=${VELOX_ANTLR4_RUNTIME_BUILD_SHA256_CHECKSUM}
   SOURCE_SUBDIR runtime/Cpp OVERRIDE_FIND_PACKAGE)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
antlr4-runtime
URL ${VELOX_ANTLR4_RUNTIME_SOURCE_URL}
URL_HASH ${VELOX_ANTLR4_RUNTIME_BUILD_SHA256_CHECKSUM}
SOURCE_SUBDIR runtime/Cpp OVERRIDE_FIND_PACKAGE)
FetchContent_Declare(
antlr4-runtime
URL ${VELOX_ANTLR4_RUNTIME_SOURCE_URL}
URL_HASH SHA256=${VELOX_ANTLR4_RUNTIME_BUILD_SHA256_CHECKSUM}
SOURCE_SUBDIR runtime/Cpp OVERRIDE_FIND_PACKAGE)
🤖 Prompt for AI Agents
In CMake/resolve_dependency_modules/antlr4-runtime.cmake around lines 28 to 31,
the URL_HASH value is missing the required algorithm prefix. Update the URL_HASH
to include the prefix "SHA256=" before the checksum value to comply with CMake ≥
3.24 requirements and ensure the integrity check passes.

@anlowee anlowee Jul 3, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

agree. Our min CMaker version is 3.28, so this might be an issue

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@anlowee Thank you for confirming! The SHA256= prefix will ensure the integrity check works correctly across different CMake versions. This fix will prevent potential build failures when using newer CMake versions.

    /|   /|  
   ( :v:  )
    |(_)|
    |   |
   /     \
  /       \

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!


set(ANTLR4_INSTALL
ON
CACHE BOOL "" FORCE)
FetchContent_MakeAvailable(antlr4-runtime)
44 changes: 44 additions & 0 deletions CMake/resolve_dependency_modules/clp.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# Copyright (c) Facebook, Inc. and its affiliates.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
include_guard(GLOBAL)

FetchContent_Declare(
clp
GIT_REPOSITORY https://github.com/y-scope/clp.git
GIT_TAG 0798100389bd5231b520ec48ab186275795e3790)

set(CLP_BUILD_CLP_REGEX_UTILS
OFF
CACHE BOOL "Build CLP regex utils")
set(CLP_BUILD_CLP_S_JSONCONSTRUCTOR
OFF
CACHE BOOL "Build CLP-S JSON constructor")
set(CLP_BUILD_CLP_S_REDUCER_DEPENDENCIES
OFF
CACHE BOOL "Build CLP-S reducer dependencies")
set(CLP_BUILD_CLP_S_SEARCH_SQL
OFF
CACHE BOOL "Build CLP-S search SQL")
set(CLP_BUILD_EXECUTABLES
OFF
CACHE BOOL "Build CLP executables")
set(CLP_BUILD_TESTING
OFF
CACHE BOOL "Build CLP tests")

FetchContent_Populate(clp)

list(APPEND CMAKE_MODULE_PATH "${clp_SOURCE_DIR}/components/core/cmake/Modules")
add_subdirectory(${clp_SOURCE_DIR}/components/core
${clp_BINARY_DIR}/components/core)
Comment on lines +40 to +44

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

optional: switch to FetchContent_MakeAvailable for terser, future-proof code

FetchContent_MakeAvailable(clp) both populates and add_subdirectorys the project, handling repeated calls gracefully. Unless you need the fine-grained control FetchContent_Populate affords, the single macro keeps the module shorter and shields you from path woes:

-FetchContent_Populate(clp)
-
-list(APPEND CMAKE_MODULE_PATH "${clp_SOURCE_DIR}/components/core/cmake/Modules")
-add_subdirectory(${clp_SOURCE_DIR}/components/core
-                 ${clp_BINARY_DIR}/components/core)
+FetchContent_MakeAvailable(clp)

(Your cache-variable overrides are set before the call, so they still propagate.)
Feel free to skip if you prefer the explicit style.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
FetchContent_Populate(clp)
list(APPEND CMAKE_MODULE_PATH "${clp_SOURCE_DIR}/components/core/cmake/Modules")
add_subdirectory(${clp_SOURCE_DIR}/components/core
${clp_BINARY_DIR}/components/core)
FetchContent_MakeAvailable(clp)
🤖 Prompt for AI Agents
In CMake/resolve_dependency_modules/clp.cmake around lines 44 to 48, replace the
use of FetchContent_Populate followed by manual list appending and
add_subdirectory calls with a single call to FetchContent_MakeAvailable(clp).
This change simplifies the code by automatically populating and adding the
subdirectory for the clp project, while preserving any cache-variable overrides
set before the call. Remove the explicit list(APPEND CMAKE_MODULE_PATH) and
add_subdirectory commands related to clp to rely on FetchContent_MakeAvailable's
internal handling.

38 changes: 38 additions & 0 deletions CMake/resolve_dependency_modules/date.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# Copyright (c) Facebook, Inc. and its affiliates.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
include_guard(GLOBAL)

set(VELOX_DATE_BUILD_VERSION 3.0.1)
set(VELOX_DATE_BUILD_SHA256_CHECKSUM
7a390f200f0ccd207e8cff6757e04817c1a0aec3e327b006b7eb451c57ee3538)
set(VELOX_DATE_SOURCE_URL
"https://github.com/HowardHinnant/date/archive/refs/tags/v${VELOX_DATE_BUILD_VERSION}.tar.gz"
)

velox_resolve_dependency_url(DATE)

# Optionally set CMake variables *before* make-available
set(CMAKE_INSTALL_MESSAGE
LAZY
CACHE STRING "" FORCE)
Comment on lines +26 to +28

@coderabbitai coderabbitai Bot Jun 25, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Global override of CMAKE_INSTALL_MESSAGE may have unintended side-effects.

Forcing CMAKE_INSTALL_MESSAGE=LAZY with CACHE ... FORCE changes behaviour for all subsequent projects, not just date.
Consider setting it locally (e.g., via set_property(TARGET date PROPERTY ...)) or omitting the FORCE flag.

🤖 Prompt for AI Agents
In CMake/resolve_dependency_modules/date.cmake at lines 26 to 28, avoid globally
overriding CMAKE_INSTALL_MESSAGE with CACHE and FORCE as it affects all
projects. Instead, remove the FORCE flag to prevent forcing the value globally
or set the install message property locally on the date target using
set_property(TARGET date PROPERTY ...). This confines the setting to the date
module only and avoids side effects on other projects.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

agree

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@anlowee Thanks for confirming—glad we’re aligned. Let me know if you’d like any help scoping that CMAKE_INSTALL_MESSAGE change.


🧠 Learnings used
Learnt from: anlowee
PR: y-scope/velox#14
File: CMake/resolve_dependency_modules/date.cmake:32-36
Timestamp: 2025-07-03T17:36:47.311Z
Learning: In CMake FetchContent_Declare, the URL_HASH argument requires an algorithm prefix (e.g., SHA256=) followed by the checksum value. Without this prefix, CMake silently skips checksum validation, which defeats supply-chain protection.


message(STATUS "Building date from source")

FetchContent_Declare(
date
URL ${VELOX_DATE_SOURCE_URL}
URL_HASH ${VELOX_DATE_BUILD_SHA256_CHECKSUM}
OVERRIDE_FIND_PACKAGE)
Comment on lines +32 to +36

@coderabbitai coderabbitai Bot Jun 25, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

URL_HASH is missing the required algorithm prefix (SHA256=).

CMake expects the URL_HASH argument in the form SHA256=<hash>.
Without the prefix, checksum validation is silently skipped, defeating supply-chain protection.

 FetchContent_Declare(
   date
   URL ${VELOX_DATE_SOURCE_URL}
-  URL_HASH ${VELOX_DATE_BUILD_SHA256_CHECKSUM}
+  URL_HASH SHA256=${VELOX_DATE_BUILD_SHA256_CHECKSUM}
   OVERRIDE_FIND_PACKAGE)
🤖 Prompt for AI Agents
In CMake/resolve_dependency_modules/date.cmake around lines 32 to 36, the
URL_HASH argument is missing the required algorithm prefix. Update the URL_HASH
value to include the prefix "SHA256=" followed by the checksum, so it reads as
SHA256=${VELOX_DATE_BUILD_SHA256_CHECKSUM}. This ensures CMake performs checksum
validation correctly.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

agree

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@anlowee Thanks for confirming! You're absolutely right - the URL_HASH does need the SHA256= prefix for proper checksum validation. This will help ensure the integrity of the downloaded date library dependency.


✏️ Learnings added
Learnt from: anlowee
PR: y-scope/velox#14
File: CMake/resolve_dependency_modules/date.cmake:32-36
Timestamp: 2025-07-03T17:36:47.311Z
Learning: In CMake FetchContent_Declare, the URL_HASH argument requires an algorithm prefix (e.g., SHA256=) followed by the checksum value. Without this prefix, CMake silently skips checksum validation, which defeats supply-chain protection.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

velox_resolve_dependency adds SHA256. Manually adding it will result in errors

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!


FetchContent_MakeAvailable(date)
35 changes: 35 additions & 0 deletions CMake/resolve_dependency_modules/msgpack-cxx.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# Copyright (c) Facebook, Inc. and its affiliates.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
include_guard(GLOBAL)

set(VELOX_MSGPACK_BUILD_VERSION cpp-7.0.0)
set(VELOX_MSGPACK_BUILD_SHA256_CHECKSUM
070881ebea9208cf7e731fd5a46a11404025b2f260ab9527e32dfcb7c689fbfc)
set(VELOX_MSGPACK_SOURCE_URL
"https://github.com/msgpack/msgpack-c/archive/refs/tags/${VELOX_MSGPACK_BUILD_VERSION}.tar.gz"
)

velox_resolve_dependency_url(MSGPACK)

message(STATUS "Building msgpack-cxx from source")
Comment on lines +23 to +25

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Status message may be misleading after URL override.

velox_resolve_dependency_url(MSGPACK) can rewrite the URL to a mirror or
pre-downloaded cache, yet the unconditional
message(STATUS "Building msgpack-cxx from source")
follows immediately. If a binary package is later introduced (or a system
package is found), the log would still announce a source build, confusing
developers.

Consider printing the message only when the resolver decided to build from
source, or phrase it more generically (“Using msgpack-cxx from
${VELOX_MSGPACK_SOURCE_URL}”).

🤖 Prompt for AI Agents
In CMake/resolve_dependency_modules/msgpack-cxx.cmake around lines 23 to 25, the
status message always states "Building msgpack-cxx from source" immediately
after calling velox_resolve_dependency_url(MSGPACK), which may be misleading if
the URL was overridden or a binary package is used later. Modify the code to
print the message conditionally only when the build is actually from source, or
change the message to a more generic form like "Using msgpack-cxx from
${VELOX_MSGPACK_SOURCE_URL}" to accurately reflect the source or binary usage.


FetchContent_Declare(
msgpack-cxx
URL ${VELOX_MSGPACK_SOURCE_URL}
URL_HASH ${VELOX_MSGPACK_BUILD_SHA256_CHECKSUM}
OVERRIDE_FIND_PACKAGE EXCLUDE_FROM_ALL SYSTEM)

set(MSGPACK_USE_BOOST OFF)

FetchContent_MakeAvailable(msgpack-cxx)
Comment on lines +27 to +35

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Expose an imported target for downstream linking convenience.

Many third-party projects expect to link against an msgpackc or
msgpack-cxx target created by the upstream CMake. After
FetchContent_MakeAvailable, that target lives inside the
msgpack-cxx sub-build and is not guaranteed to be globally visible.

Add:

# Ensure an ALIAS exists at the top level.
if (NOT TARGET msgpackc::msgpackc AND TARGET msgpackc)
  add_library(msgpackc::msgpackc ALIAS msgpackc)
endif()

This small alias spares every dependent Velox component from writing custom
find_package_handle code.

🤖 Prompt for AI Agents
In CMake/resolve_dependency_modules/msgpack-cxx.cmake around lines 27 to 35, the
imported target for msgpack-cxx is not globally visible after
FetchContent_MakeAvailable, which complicates downstream linking. To fix this,
add an alias target at the top level by checking if msgpackc::msgpackc does not
exist but msgpackc does, then create an ALIAS target named msgpackc::msgpackc
pointing to msgpackc. This will make the target globally accessible for
dependent projects.

37 changes: 37 additions & 0 deletions CMake/resolve_dependency_modules/nlohmann_json.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Copyright (c) Facebook, Inc. and its affiliates.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
include_guard(GLOBAL)

set(VELOX_NLOHMANN_JSON_BUILD_VERSION 3.11.3)
set(VELOX_NLOHMANN_JSON_BUILD_SHA256_CHECKSUM
0d8ef5af7f9794e3263480193c491549b2ba6cc74bb018906202ada498a79406)
set(VELOX_NLOHMANN_JSON_SOURCE_URL
"https://github.com/nlohmann/json/archive/refs/tags/v${VELOX_NLOHMANN_JSON_BUILD_VERSION}.tar.gz"
)

velox_resolve_dependency_url(NLOHMANN_JSON)

message(STATUS "Building nlohmann_json from source")

FetchContent_Declare(
nlohmann_json
URL ${VELOX_NLOHMANN_JSON_SOURCE_URL}
URL_HASH ${VELOX_NLOHMANN_JSON_BUILD_SHA256_CHECKSUM}
OVERRIDE_FIND_PACKAGE)

set(JSON_BuildTests
OFF
CACHE INTERNAL "")

FetchContent_MakeAvailable(nlohmann_json)
3 changes: 2 additions & 1 deletion CMake/resolve_dependency_modules/simdjson.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ message(STATUS "Building simdjson from source")
FetchContent_Declare(
simdjson
URL ${VELOX_SIMDJSON_SOURCE_URL}
URL_HASH ${VELOX_SIMDJSON_BUILD_SHA256_CHECKSUM})
URL_HASH ${VELOX_SIMDJSON_BUILD_SHA256_CHECKSUM}
OVERRIDE_FIND_PACKAGE)

if(${VELOX_SIMDJSON_SKIPUTF8VALIDATION})
set(SIMDJSON_SKIPUTF8VALIDATION ON)
Expand Down
34 changes: 34 additions & 0 deletions CMake/resolve_dependency_modules/spdlog.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# Copyright (c) Facebook, Inc. and its affiliates.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
include_guard(GLOBAL)

set(VELOX_SPDLOG_BUILD_VERSION 1.12.0)
set(VELOX_SPDLOG_BUILD_SHA256_CHECKSUM
4dccf2d10f410c1e2feaff89966bfc49a1abb29ef6f08246335b110e001e09a9)
set(VELOX_SPDLOG_SOURCE_URL
"https://github.com/gabime/spdlog/archive/refs/tags/v${VELOX_SPDLOG_BUILD_VERSION}.tar.gz"
)

velox_resolve_dependency_url(SPDLOG)

message(STATUS "Building spdlog from source")

FetchContent_Declare(
spdlog
URL ${VELOX_SPDLOG_SOURCE_URL}
URL_HASH ${VELOX_SPDLOG_BUILD_SHA256_CHECKSUM}
OVERRIDE_FIND_PACKAGE EXCLUDE_FROM_ALL SYSTEM)

set(SPDLOG_FMT_EXTERNAL ON)
FetchContent_MakeAvailable(spdlog)
23 changes: 23 additions & 0 deletions CMake/resolve_dependency_modules/ystdlib_cpp.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Copyright (c) Facebook, Inc. and its affiliates.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
include_guard(GLOBAL)

FetchContent_Declare(
ystdlib_cpp
GIT_REPOSITORY https://github.com/y-scope/ystdlib-cpp.git
GIT_TAG 0ae886c6a7ee706a3c6e1950262b63d72f71fe63)

FetchContent_Populate(ystdlib_cpp)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Once we upgrade clp to install ystdlib as a package, we should be able to use MakeAvailable instead.


set(CLP_YSTDLIB_SOURCE_DIRECTORY "${ystdlib_cpp_SOURCE_DIR}")
33 changes: 33 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ set(VELOX_GFLAGS_TYPE
)
option(VELOX_ENABLE_EXEC "Build exec." ON)
option(VELOX_ENABLE_AGGREGATES "Build aggregates." ON)
option(VELOX_ENABLE_CLP_CONNECTOR "Build CLP connector." ON)
option(VELOX_ENABLE_HIVE_CONNECTOR "Build Hive connector." ON)
Comment on lines 130 to 131

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Missing compile-time define for the new flag

Other feature toggles (S3, ABFS, PARQUET, …) inject a -DVELOX_ENABLE_* definition immediately after the option is parsed.
VELOX_ENABLE_CLP_CONNECTOR never sets such a macro, so C++ sources cannot conditionally compile on it.

 option(VELOX_ENABLE_CLP_CONNECTOR "Build CLP connector." ON)
+
+if(VELOX_ENABLE_CLP_CONNECTOR)
+  add_definitions(-DVELOX_ENABLE_CLP_CONNECTOR)
+endif()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
option(VELOX_ENABLE_CLP_CONNECTOR "Build CLP connector." ON)
option(VELOX_ENABLE_HIVE_CONNECTOR "Build Hive connector." ON)
option(VELOX_ENABLE_CLP_CONNECTOR "Build CLP connector." ON)
if(VELOX_ENABLE_CLP_CONNECTOR)
add_definitions(-DVELOX_ENABLE_CLP_CONNECTOR)
endif()
option(VELOX_ENABLE_HIVE_CONNECTOR "Build Hive connector." ON)
🤖 Prompt for AI Agents
In CMakeLists.txt around lines 130 to 131, the option VELOX_ENABLE_CLP_CONNECTOR
is defined but does not set a corresponding compile-time macro. To fix this, add
a check after the option definition that appends -DVELOX_ENABLE_CLP_CONNECTOR to
the compiler flags if the option is ON, similar to how other feature toggles
like S3 or ABFS do it. This will enable conditional compilation in the C++
sources based on this flag.

option(VELOX_ENABLE_TPCH_CONNECTOR "Build TPC-H connector." ON)
option(VELOX_ENABLE_PRESTO_FUNCTIONS "Build Presto SQL functions." ON)
Expand Down Expand Up @@ -174,6 +175,7 @@ if(${VELOX_BUILD_MINIMAL} OR ${VELOX_BUILD_MINIMAL_WITH_DWIO})
set(VELOX_ENABLE_EXPRESSION ON)
set(VELOX_ENABLE_EXEC OFF)
set(VELOX_ENABLE_AGGREGATES OFF)
set(VELOX_ENABLE_CLP_CONNECTOR OFF)
set(VELOX_ENABLE_HIVE_CONNECTOR OFF)
set(VELOX_ENABLE_TPCH_CONNECTOR OFF)
set(VELOX_ENABLE_SPARK_FUNCTIONS OFF)
Expand Down Expand Up @@ -459,9 +461,11 @@ set(BOOST_INCLUDE_LIBRARIES
context
date_time
filesystem
iostreams
program_options
regex
system
url
thread)

velox_set_source(Boost)
Expand Down Expand Up @@ -518,6 +522,35 @@ endif()
velox_set_source(re2)
velox_resolve_dependency(re2)

if(${VELOX_ENABLE_CLP_CONNECTOR})
velox_set_source(absl)
velox_resolve_dependency(absl)

velox_set_source(antlr4-runtime)
velox_resolve_dependency(antlr4-runtime)

velox_set_source(date)
velox_resolve_dependency(date)

velox_set_source(msgpack-cxx)
velox_resolve_dependency(msgpack-cxx)

velox_set_source(nlohmann_json)
velox_resolve_dependency(nlohmann_json)

velox_set_source(simdjson)
velox_resolve_dependency(simdjson)

velox_set_source(spdlog)
velox_resolve_dependency(spdlog)

set(ystdlib_cpp_SOURCE BUNDLED)
velox_resolve_dependency(ystdlib_cpp)

set(clp_SOURCE BUNDLED)
velox_resolve_dependency(clp)
endif()

if(${VELOX_BUILD_PYTHON_PACKAGE})
find_package(
Python 3.9
Expand Down
4 changes: 4 additions & 0 deletions velox/connectors/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ velox_link_libraries(velox_connector velox_common_config velox_vector)

add_subdirectory(fuzzer)

if(${VELOX_ENABLE_CLP_CONNECTOR})
add_subdirectory(clp)
endif()

if(${VELOX_ENABLE_HIVE_CONNECTOR})
add_subdirectory(hive)
endif()
Expand Down
31 changes: 31 additions & 0 deletions velox/connectors/clp/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# Copyright (c) Facebook, Inc. and its affiliates.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#set(CLP_SRC_DIR ${clp_SOURCE_DIR}/components/core/src)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Remove commented code.

This commented line appears to be leftover code and should be removed to keep the build file clean.

-#set(CLP_SRC_DIR ${clp_SOURCE_DIR}/components/core/src)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#set(CLP_SRC_DIR ${clp_SOURCE_DIR}/components/core/src)
🤖 Prompt for AI Agents
In velox/connectors/clp/CMakeLists.txt at line 14, remove the commented out line
setting CLP_SRC_DIR as it is leftover code and not needed, to keep the build
file clean and maintainable.

add_subdirectory(search_lib)

velox_add_library(
velox_clp_connector
OBJECT
ClpConfig.cpp
ClpConnector.cpp
ClpDataSource.cpp
ClpTableHandle.cpp)

velox_link_libraries(velox_clp_connector
PRIVATE clp-s-search simdjson::simdjson velox_connector)
target_compile_features(velox_clp_connector PRIVATE cxx_std_20)

if(${VELOX_BUILD_TESTING})
add_subdirectory(tests)
endif()
Loading
Loading