Skip to content

build: Upgrade clp submodule to y-scope/clp@19cd534. #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kirkrodrigues merged 22 commits into from
Sep 2, 2025
Merged

build: Upgrade clp submodule to y-scope/clp@19cd534. #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
43f2f28
allow duplicate columns in Projection
wraymo Aug 19, 2025
8d87f5d
advance clp
wraymo Aug 21, 2025
d43ba81
init
anlowee Aug 21, 2025
5040225
Fix
anlowee Aug 24, 2025
842e25e
Revert Ray changes
anlowee Aug 24, 2025
37fb615
Fix lint
anlowee Aug 24, 2025
1cc4e7e
Make shared
anlowee Aug 26, 2025
e931722
Try fix
anlowee Aug 26, 2025
d01a65a
Try fix
anlowee Aug 26, 2025
4a17d79
Remove zstd.cmake
anlowee Aug 26, 2025
7a28a3c
Use bundled Zstd to rely primarily on Zstd's CMake scripts.
kirkrodrigues Aug 28, 2025
f11380a
Create ALIAS targets after calling find_package.
kirkrodrigues Aug 28, 2025
ceb5c95
Replace NEEDS logic with macro.
kirkrodrigues Aug 28, 2025
4b38444
Revert "Replace NEEDS logic with macro."
kirkrodrigues Aug 28, 2025
b8e2d51
Revert "Create ALIAS targets after calling find_package."
kirkrodrigues Aug 28, 2025
34adc3a
Use correct source directory for Zstd's CMake build.
kirkrodrigues Aug 28, 2025
fd2f391
Run precommit
anlowee Aug 28, 2025
889309c
Revert "Use correct source directory for Zstd's CMake build."
kirkrodrigues Aug 30, 2025
43688dc
Revert "Use bundled Zstd to rely primarily on Zstd's CMake scripts."
kirkrodrigues Aug 30, 2025
eb19bd2
Define libzstd_static and libzstd_shared ALIAS targets in Findzstd.cm…
kirkrodrigues Aug 30, 2025
6acd5ff
Address comments
anlowee Sep 1, 2025
fb2f155
Apply suggestions from code review
anlowee Sep 2, 2025
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
8 changes: 8 additions & 0 deletions CMake/Findzstd.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,11 @@ if(NOT TARGET zstd::zstd)
zstd::zstd PROPERTIES IMPORTED_LINK_INTERFACE_LANGUAGES "C"
IMPORTED_LOCATION "${ZSTD_LIBRARIES}")
endif()

if(NOT TARGET zstd::libzstd_shared)
add_library(zstd::libzstd_shared ALIAS zstd::zstd)
endif()

if(NOT TARGET zstd::libzstd_static)
add_library(zstd::libzstd_static ALIAS zstd::zstd)
endif()
3 changes: 1 addition & 2 deletions CMake/resolve_dependency_modules/clp.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ include_guard(GLOBAL)
FetchContent_Declare(
clp
GIT_REPOSITORY https://github.com/y-scope/clp.git
GIT_TAG 581bd46198a97a89b174849851cc3f1f2100e466)
GIT_TAG 19cd534e629d746395efc64343a60f768b0c9a2d)

Comment on lines 16 to 20

@coderabbitai coderabbitai Bot Sep 1, 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.

💡 Verification agent

🧩 Analysis chain

Make CLP repo/tag overridable and enable shallow clone for faster CI

Avoid hardcoding and allow consumers/CI to override; also add GIT_SHALLOW and progress for speed/observability.

-FetchContent_Declare(
-  clp
-  GIT_REPOSITORY https://github.com/y-scope/clp.git
-  GIT_TAG 19cd534e629d746395efc64343a60f768b0c9a2d)
+set(CLP_GIT_REPOSITORY "https://github.com/y-scope/clp.git" CACHE STRING "CLP Git repository URL")
+set(CLP_GIT_TAG "19cd534e629d746395efc64343a60f768b0c9a2d" CACHE STRING "CLP Git commit/tag to build")
+FetchContent_Declare(
+  clp
+  GIT_REPOSITORY ${CLP_GIT_REPOSITORY}
+  GIT_TAG        ${CLP_GIT_TAG}
+  GIT_SHALLOW    TRUE
+  GIT_PROGRESS   TRUE)

Run to confirm the pinned commit exists upstream:

🏁 Script executed:

#!/bin/bash
git ls-remote --exit-code https://github.com/y-scope/clp.git 19cd534e629d746395efc64343a60f768b0c9a2d >/dev/null && echo "Commit found" || echo "NOT FOUND"

Length of output: 132

Update CLP_GIT_TAG to a valid existing commit or tag
The SHA “19cd534e629d746395efc64343a60f768b0c9a2d” wasn’t found in the upstream repo—please verify and correct CLP_GIT_TAG to a valid commit or tag before making it overridable and enabling GIT_SHALLOW/GIT_PROGRESS.

🤖 Prompt for AI Agents 
In CMake/resolve_dependency_modules/clp.cmake around lines 16 to 20, the GIT_TAG
value currently set to "19cd534e629d746395efc64343a60f768b0c9a2d" does not exist
upstream; replace CLP_GIT_TAG with a valid commit SHA or annotated tag from
https://github.com/y-scope/clp (verify by checking the repo history), make the
variable configurable (set a default via set(... CACHE STRING ...) so it can be
overridden), and then enable GIT_SHALLOW and GIT_PROGRESS in the
FetchContent_Declare call (add GIT_SHALLOW TRUE and GIT_PROGRESS TRUE) so
fetching is efficient and shows progress.

set(CLP_BUILD_CLP_REGEX_UTILS
OFF
Expand All @@ -39,6 +39,5 @@ set(CLP_BUILD_TESTING

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)
25 changes: 25 additions & 0 deletions CMake/resolve_dependency_modules/log_surgeon.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# 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(
log_surgeon
GIT_REPOSITORY https://github.com/y-scope/log-surgeon.git
GIT_TAG 85d4f2c09c0e55f1fb87cdc8b0f4d13fb1a733e1
OVERRIDE_FIND_PACKAGE)

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +16 to +21

@coderabbitai coderabbitai Bot Sep 2, 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)

Add progress output; avoid shallow; include FetchContent locally.

Safer logging without the shallow-fetch breakage you hit. Also, make this file self-sufficient.

Apply:

 FetchContent_Declare(
   log_surgeon
   GIT_REPOSITORY https://github.com/y-scope/log-surgeon.git
   GIT_TAG 85d4f2c09c0e55f1fb87cdc8b0f4d13fb1a733e1
-  OVERRIDE_FIND_PACKAGE)
+  OVERRIDE_FIND_PACKAGE
+  GIT_PROGRESS TRUE)

Add near the top (outside this hunk):

include(FetchContent)
🤖 Prompt for AI Agents 
In CMake/resolve_dependency_modules/log_surgeon.cmake around lines 16-21, make
the file self-sufficient by adding an include(FetchContent) near the top (before
any FetchContent_Declare), and update the FetchContent_Declare call to disable
shallow clones and enable progress (set GIT_SHALLOW to FALSE and PROGRESS to
TRUE) and then call FetchContent_MakeAvailable (or FetchContent_GetProperties +
FetchContent_Populate) so the content is fetched locally; this ensures safer
logging/output and avoids shallow-fetch breakage.

FetchContent_MakeAvailable(log_surgeon)

Comment on lines +16 to +23

@coderabbitai coderabbitai Bot Aug 26, 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)

Confirm log_surgeon’s public link interface only exposes what consumers need.

If log_surgeon PUBLICly links GSL, exporting GSL is reasonable; otherwise make the GSL link PRIVATE in log_surgeon and drop the export plumbing here.

I can open a follow-up to upstream a change to log_surgeon so its install/export set is self-consistent (avoiding top-level install hacks). Want me to draft it?

🤖 Prompt for AI Agents 
In CMake/resolve_dependency_modules/log_surgeon.cmake around lines 16-23,
confirm whether log_surgeon PUBLICly links GSL; if its public interface does not
require consumers to see GSL, change the log_surgeon target to link GSL PRIVATE
in the log_surgeon project (or adjust the target_link_libraries call there to
PRIVATE) and remove any export/install plumbing here that re-exports GSL; if you
cannot change upstream immediately, override by calling
target_link_libraries(log_surgeon PRIVATE GSL::gsl) or setting
INTERFACE_LINK_LIBRARIES appropriately after FetchContent_MakeAvailable to
ensure GSL is not propagated to consumers, and optionally prepare an upstream
patch to make the install/export set self-consistent so top-level projects don’t
have to hack exports.

# To work around y-scope/log-surgeon#155
install(TARGETS GSL EXPORT log_surgeon-targets)
34 changes: 34 additions & 0 deletions CMake/resolve_dependency_modules/microsoft_gsl.cmake
View file
Open in desktop
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)

# Version you want to build
set(VELOX_GSL_BUILD_VERSION 4.0.0)
set(VELOX_GSL_BUILD_SHA256_CHECKSUM
f0e32cb10654fea91ad56bde89170d78cfbf4363ee0b01d8f097de2ba49f6ce9)
set(VELOX_GSL_SOURCE_URL
"https://github.com/microsoft/GSL/archive/refs/tags/v${VELOX_GSL_BUILD_VERSION}.tar.gz"
)
Comment on lines +16 to +22

@coderabbitai coderabbitai Bot Aug 26, 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)

Expose GSL version/checksum as cache variables for override.

This makes it easy to test upgrades and ensures reproducibility.

-# Version you want to build
-set(VELOX_GSL_BUILD_VERSION 4.0.0)
-set(VELOX_GSL_BUILD_SHA256_CHECKSUM
-    f0e32cb10654fea91ad56bde89170d78cfbf4363ee0b01d8f097de2ba49f6ce9)
+# Version you want to build (override via -D on the CMake command line)
+set(VELOX_GSL_BUILD_VERSION 4.0.0 CACHE STRING "Microsoft.GSL version to build")
+set(VELOX_GSL_BUILD_SHA256_CHECKSUM
+    f0e32cb10654fea91ad56bde89170d78cfbf4363ee0b01d8f097de2ba49f6ce9
+    CACHE STRING "SHA256 checksum of the Microsoft.GSL source tarball")
📝 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
# Version you want to build
set(VELOX_GSL_BUILD_VERSION 4.0.0)
set(VELOX_GSL_BUILD_SHA256_CHECKSUM
f0e32cb10654fea91ad56bde89170d78cfbf4363ee0b01d8f097de2ba49f6ce9)
set(VELOX_GSL_SOURCE_URL
"https://github.com/microsoft/GSL/archive/refs/tags/v${VELOX_GSL_BUILD_VERSION}.tar.gz"
)
# Version you want to build (override via -D on the CMake command line)
set(VELOX_GSL_BUILD_VERSION 4.0.0 CACHE STRING "Microsoft.GSL version to build")
set(VELOX_GSL_BUILD_SHA256_CHECKSUM
f0e32cb10654fea91ad56bde89170d78cfbf4363ee0b01d8f097de2ba49f6ce9
CACHE STRING "SHA256 checksum of the Microsoft.GSL source tarball")
set(VELOX_GSL_SOURCE_URL
"https://github.com/microsoft/GSL/archive/refs/tags/v${VELOX_GSL_BUILD_VERSION}.tar.gz"
)
🤖 Prompt for AI Agents 
In CMake/resolve_dependency_modules/microsoft_gsl.cmake around lines 16-22, the
GSL version, checksum and source URL are hardcoded; change their definitions to
CMake cache variables so they can be overridden (e.g., use set(... CACHE STRING
"description") for VELOX_GSL_BUILD_VERSION, VELOX_GSL_BUILD_SHA256_CHECKSUM and
VELOX_GSL_SOURCE_URL), include a clear description for each cache entry, and
keep the default values the same so tests can override versions/checksums for
upgrades and reproducible builds.


velox_resolve_dependency_url(GSL)

message(STATUS "Building Microsoft.GSL from source")

FetchContent_Declare(
Microsoft.GSL
URL ${VELOX_GSL_SOURCE_URL}
URL_HASH ${VELOX_GSL_BUILD_SHA256_CHECKSUM}
OVERRIDE_FIND_PACKAGE EXCLUDE_FROM_ALL SYSTEM)

FetchContent_MakeAvailable(Microsoft.GSL)
16 changes: 8 additions & 8 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,20 +509,13 @@ if(${VELOX_BUILD_MINIMAL_WITH_DWIO} OR ${VELOX_ENABLE_HIVE_CONNECTOR})
find_package(lzo2 REQUIRED)
find_package(zstd REQUIRED)
find_package(Snappy REQUIRED)
if(NOT TARGET zstd::zstd)
if(TARGET zstd::libzstd_static)
set(ZSTD_TYPE static)
else()
set(ZSTD_TYPE shared)
endif()
add_library(zstd::zstd ALIAS zstd::libzstd_${ZSTD_TYPE})
endif()
endif()

velox_set_source(re2)
velox_resolve_dependency(re2)

if(${VELOX_ENABLE_CLP_CONNECTOR})
# Independent dependencies
velox_set_source(absl)
velox_resolve_dependency(absl)

Expand All @@ -532,6 +525,9 @@ if(${VELOX_ENABLE_CLP_CONNECTOR})
velox_set_source(date)
velox_resolve_dependency(date)

velox_set_source(microsoft_gsl)
velox_resolve_dependency(microsoft_gsl)

Comment on lines +528 to +530

@coderabbitai coderabbitai Bot Sep 2, 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.

💡 Verification agent

🧩 Analysis chain

Order is correct (GSL before log_surgeon); verify target naming consistency.

Ensure microsoft_gsl defines the target name(s) that log_surgeon expects (GSL vs Microsoft.GSL::GSL) to avoid alias churn and export collisions.

Run:

If the resolver exposes only Microsoft.GSL::GSL, add a local ALIAS named GSL where needed to match log_surgeon until upstream fixes land.

Also applies to: 543-546

🏁 Script executed:

#!/bin/bash
# Inspect GSL target names and log_surgeon linkage/exports
rg -n -C2 -g 'CMake/**' -P $'(^|\\b)(add_library|add_library\\s*\\().*GSL|Microsoft\\.GSL::GSL|^\\s*install\\(\\s*TARGETS\\s+GSL\\b|target_link_libraries\\(\\s*log_surgeon\\b.*\\)'

Length of output: 420

🏁 Script executed:

#!/bin/bash
# List available dependency resolver modules to find microsoft_gsl
ls CMake/resolve_dependency_modules | grep -i gsl

Length of output: 71

🏁 Script executed:

#!/bin/bash
# Inspect microsoft_gsl resolver for target definitions and aliases
rg -n -C3 'add_library\(' CMake/resolve_dependency_modules/microsoft_gsl.cmake
rg -n -C3 'add_library.*ALIAS' CMake/resolve_dependency_modules/microsoft_gsl.cmake

Length of output: 166

🏁 Script executed:

#!/bin/bash
# Show contents of the microsoft_gsl resolver to verify what targets it defines or aliases
sed -n '1,200p' CMake/resolve_dependency_modules/microsoft_gsl.cmake

# Search for find_package and alias patterns in the resolver
rg -n -C3 'find_package' CMake/resolve_dependency_modules/microsoft_gsl.cmake
rg -n -C3 'add_library.*ALIAS' CMake/resolve_dependency_modules/microsoft_gsl.cmake
rg -n -C3 'add_library' CMake/resolve_dependency_modules/microsoft_gsl.cmake

Length of output: 1513

Add missing GSL alias in microsoft_gsl resolver
In CMake/resolve_dependency_modules/microsoft_gsl.cmake, immediately after

FetchContent_MakeAvailable(Microsoft.GSL)

insert

add_library(GSL ALIAS Microsoft.GSL::GSL)

so that the install(TARGETS GSL …) in log_surgeon.cmake succeeds. Applies to both the calls at lines 528–530 and 543–546.

🤖 Prompt for AI Agents 
In CMakeLists.txt around lines 528–530 (and similarly for the other call at
543–546), the microsoft_gsl resolver does a
FetchContent_MakeAvailable(Microsoft.GSL) but does not create the expected GSL
target alias; add a step immediately after the FetchContent_MakeAvailable call
to create an alias for Microsoft.GSL::GSL using add_library(GSL ALIAS
Microsoft.GSL::GSL) so that downstream install(TARGETS GSL ...) in
log_surgeon.cmake can find the GSL target; apply this insertion in both
locations mentioned.

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

Expand All @@ -544,6 +540,10 @@ if(${VELOX_ENABLE_CLP_CONNECTOR})
velox_set_source(spdlog)
velox_resolve_dependency(spdlog)

# Dependencies that depend on other dependencies
velox_set_source(log_surgeon)
velox_resolve_dependency(log_surgeon)

set(ystdlib_cpp_SOURCE BUNDLED)
velox_resolve_dependency(ystdlib_cpp)

Expand Down
1 change: 0 additions & 1 deletion velox/connectors/clp/search_lib/ClpQueryRunner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

using namespace clp_s;
using namespace clp_s::search;
using namespace clp_s::search::clp_search;

@wraymo wraymo Aug 26, 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.

can we remove unrelated changes?

@anlowee anlowee Aug 27, 2025

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.

This is an unsolve symvol and will cause build fail.


namespace facebook::velox::connector::clp::search_lib {

Expand Down