-
Notifications
You must be signed in to change notification settings - Fork 178
Move yyjson dependency to PAX storage directory #1383
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.
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
base: main
Are you sure you want to change the base?
Conversation
Move yyjson from top-level dependency directory to `contrib/pax_storage/src/cpp/contrib/yyjson` since it's only used by the PAX storage component. Update all references in CMake build files, .gitmodules, and documentation to reflect the new location. This change improves the project structure by keeping PAX-specific dependencies together and removing the unnecessary top-level dependency directory. The move includes: - Moving yyjson directory to contrib/pax_storage/src/cpp/contrib/yyjson - Updating CMakeLists.txt and pax.cmake to reference the new location - Updating .gitmodules to reflect the new submodule path - Updating README.md documentation to show the new location
Hi @tuhaihe ! I reviewed your PR and wanted to share an alternative implementation that eliminates the need for git submodules entirely. This approach uses CMake's Alternative: CMake FetchContentI've implemented this on branch Key change: Replace the git submodule with automatic dependency fetching during CMake configuration. Implementation
if(USE_MANIFEST_API AND NOT USE_PAX_CATALOG)
include(FetchContent)
# Try system package first
find_package(yyjson QUIET)
if(NOT yyjson_FOUND)
message(STATUS "yyjson not found in system, fetching from GitHub...")
FetchContent_Declare(
yyjson
GIT_REPOSITORY https://github.com/ibireme/yyjson.git
GIT_TAG 0.12.0
GIT_SHALLOW TRUE
)
set(SAVED_BUILD_SHARED_LIBS ${BUILD_SHARED_LIBS})
set(BUILD_SHARED_LIBS ON)
FetchContent_MakeAvailable(yyjson)
set(BUILD_SHARED_LIBS ${SAVED_BUILD_SHARED_LIBS})
else()
message(STATUS "Using system yyjson package")
endif()
endif()
# Update include path to use FetchContent variable
set(pax_target_include ${pax_target_include} ${yyjson_SOURCE_DIR}/src) Remove from Benefits
Important NoteDuring my review, I noticed that yyjson is only used when both
TestingTo verify this works when yyjson IS needed: cd contrib/pax_storage/build
cmake .. -DUSE_MANIFEST_API=ON -DUSE_PAX_CATALOG=OFF
# Should see: "yyjson not found in system, fetching from GitHub..."
make RecommendationYour PR's organizational improvement is valuable regardless. This FetchContent approach is simply an alternative that:
Both approaches work - this is just offered as food for thought. Happy to discuss! Branch for reference: |
Hi @edespino, thanks for your great idea. There are other submodules needed for PAX building, such as tabulate. I prefer to keep the same behavior across yyjson and the other submodules so that users can download all required submodules with |
More: I searched and found that |
Would like to have more voices from the core PAX developers on this. cc @gfphoenix78 @jiaqizho @gongxun0928 At least, it's not good to place the |
Move yyjson from top-level dependency directory to
contrib/pax_storage/src/cpp/contrib/yyjson
since it's only used by the PAX storage component. Update all references in CMake build files, .gitmodules, and documentation to reflect the new location. This change improves the project structure by keeping PAX-specific dependencies together and removing the unnecessary top-level dependency directory.The move includes:
Fixes #ISSUE_Number
What does this PR do?
Type of Change
Breaking Changes
Test Plan
make installcheck
make -C src/test installcheck-cbdb-parallel
Impact
Performance:
User-facing changes:
Dependencies:
Checklist
Additional Context
CI Skip Instructions