refactor(clp-s): Split clp-s into several smaller libraries to ease re-use in other projects. #924
Conversation
… on CommandLineArguments
…sient dependencies to many archive read-side components
…ect that links against it can build properly
WalkthroughThis change modularizes the codebase by moving concrete Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainApp as clp-s
participant OutputHandlerImpl
participant OutputHandler (Abstract)
participant InputConfig
participant ReaderInterface
User->>MainApp: Run search/compress command
MainApp->>InputConfig: try_create_reader(path, network_auth)
InputConfig->>ReaderInterface: (Creates appropriate reader)
InputConfig-->>MainApp: shared_ptr<ReaderInterface>
MainApp->>OutputHandlerImpl: Instantiate concrete OutputHandler (e.g., StandardOutputHandler)
OutputHandlerImpl->>OutputHandler: (Uses abstract interface)
MainApp->>OutputHandlerImpl: write()/flush()/finish()
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (2)
components/core/src/clp_s/OutputHandlerImpl.cpp (2)
39-54:⚠️ Potential issueHandle partial sends in network write operation.
The
send()call may not send all the data in one operation. You should loop until all bytes are sent to avoid data loss.Apply this diff to handle partial sends properly:
void NetworkOutputHandler::write( string_view message, epochtime_t timestamp, string_view archive_id, int64_t log_event_idx ) { static constexpr string_view cOrigFilePathPlaceholder{""}; msgpack::type::tuple<epochtime_t, string, string, string, int64_t> const src(timestamp, message, cOrigFilePathPlaceholder, archive_id, log_event_idx); msgpack::sbuffer m; msgpack::pack(m, src); - if (-1 == send(m_socket_fd, m.data(), m.size(), 0)) { - throw OperationFailed(ErrorCode::ErrorCodeFailureNetwork, __FILE__, __LINE__); + size_t total_sent = 0; + while (total_sent < m.size()) { + ssize_t sent = send(m_socket_fd, m.data() + total_sent, m.size() - total_sent, 0); + if (-1 == sent) { + throw OperationFailed(ErrorCode::ErrorCodeFailureNetwork, __FILE__, __LINE__); + } + total_sent += sent; } }
71-73: 🛠️ Refactor suggestionPreserve MongoDB exception details for better debugging.
The MongoDB exception messages contain valuable debugging information that is currently discarded. Consider logging the exception details before returning the error code.
Apply this pattern to all MongoDB exception handlers:
} catch (mongocxx::exception const& e) { + SPDLOG_ERROR("MongoDB operation failed: {}", e.what()); throw OperationFailed(ErrorCode::ErrorCodeBadParamDbUri, __FILENAME__, __LINE__); }And for the flush method:
} catch (mongocxx::exception const& e) { + SPDLOG_ERROR("MongoDB bulk write failed: {}", e.what()); return ErrorCode::ErrorCodeFailureDbBulkWrite; }Also applies to: 116-118, 126-128
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (38)
components/core/.clang-format(1 hunks)components/core/CMakeLists.txt(2 hunks)components/core/src/clp_s/ArchiveReaderAdaptor.cpp(1 hunks)components/core/src/clp_s/ArchiveWriter.cpp(1 hunks)components/core/src/clp_s/CMakeLists.txt(4 hunks)components/core/src/clp_s/ColumnWriter.hpp(0 hunks)components/core/src/clp_s/CommandLineArguments.hpp(0 hunks)components/core/src/clp_s/DictionaryWriter.cpp(1 hunks)components/core/src/clp_s/InputConfig.cpp(2 hunks)components/core/src/clp_s/InputConfig.hpp(2 hunks)components/core/src/clp_s/JsonConstructor.cpp(1 hunks)components/core/src/clp_s/JsonConstructor.hpp(0 hunks)components/core/src/clp_s/JsonParser.cpp(3 hunks)components/core/src/clp_s/JsonParser.hpp(1 hunks)components/core/src/clp_s/OutputHandlerImpl.cpp(4 hunks)components/core/src/clp_s/OutputHandlerImpl.hpp(1 hunks)components/core/src/clp_s/ReaderUtils.cpp(0 hunks)components/core/src/clp_s/ReaderUtils.hpp(1 hunks)components/core/src/clp_s/VariableDecoder.cpp(1 hunks)components/core/src/clp_s/VariableEncoder.hpp(0 hunks)components/core/src/clp_s/ZstdCompressor.cpp(1 hunks)components/core/src/clp_s/ZstdCompressor.hpp(0 hunks)components/core/src/clp_s/clp-s.cpp(4 hunks)components/core/src/clp_s/indexer/indexer.cpp(0 hunks)components/core/src/clp_s/kv_ir_search.cpp(1 hunks)components/core/src/clp_s/search/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/Output.cpp(1 hunks)components/core/src/clp_s/search/Output.hpp(0 hunks)components/core/src/clp_s/search/OutputHandler.hpp(2 hunks)components/core/src/clp_s/search/ast/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/kql/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/sql/CMakeLists.txt(1 hunks)components/core/tests/clp_s_test_utils.cpp(2 hunks)components/core/tests/clp_s_test_utils.hpp(2 hunks)components/core/tests/test-clp_s-end_to_end.cpp(1 hunks)components/core/tests/test-clp_s-range_index.cpp(1 hunks)components/core/tests/test-clp_s-search.cpp(6 hunks)taskfiles/lint.yaml(1 hunks)
💤 Files with no reviewable changes (8)
- components/core/src/clp_s/indexer/indexer.cpp
- components/core/src/clp_s/search/Output.hpp
- components/core/src/clp_s/CommandLineArguments.hpp
- components/core/src/clp_s/ColumnWriter.hpp
- components/core/src/clp_s/VariableEncoder.hpp
- components/core/src/clp_s/JsonConstructor.hpp
- components/core/src/clp_s/ZstdCompressor.hpp
- components/core/src/clp_s/ReaderUtils.cpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp_s/JsonConstructor.cppcomponents/core/src/clp_s/ZstdCompressor.cppcomponents/core/src/clp_s/VariableDecoder.cppcomponents/core/src/clp_s/ArchiveWriter.cppcomponents/core/src/clp_s/DictionaryWriter.cppcomponents/core/src/clp_s/kv_ir_search.cppcomponents/core/tests/clp_s_test_utils.hppcomponents/core/src/clp_s/search/Output.cppcomponents/core/tests/test-clp_s-end_to_end.cppcomponents/core/tests/clp_s_test_utils.cppcomponents/core/src/clp_s/JsonParser.cppcomponents/core/tests/test-clp_s-range_index.cppcomponents/core/src/clp_s/ReaderUtils.hppcomponents/core/src/clp_s/clp-s.cppcomponents/core/src/clp_s/ArchiveReaderAdaptor.cppcomponents/core/src/clp_s/JsonParser.hppcomponents/core/tests/test-clp_s-search.cppcomponents/core/src/clp_s/OutputHandlerImpl.cppcomponents/core/src/clp_s/InputConfig.cppcomponents/core/src/clp_s/InputConfig.hppcomponents/core/src/clp_s/search/OutputHandler.hppcomponents/core/src/clp_s/OutputHandlerImpl.hpp
🧬 Code Graph Analysis (4)
components/core/src/clp_s/kv_ir_search.cpp (2)
components/core/src/clp_s/InputConfig.hpp (1)
try_create_reader(122-123)components/core/src/clp_s/InputConfig.cpp (2)
try_create_reader(168-177)try_create_reader(168-169)
components/core/src/clp_s/clp-s.cpp (1)
components/core/src/clp_s/kv_ir_search.cpp (1)
command_line_arguments(62-63)
components/core/src/clp_s/ArchiveReaderAdaptor.cpp (1)
components/core/src/clp_s/InputConfig.cpp (2)
try_create_reader(168-177)try_create_reader(168-169)
components/core/src/clp_s/OutputHandlerImpl.cpp (1)
components/core/src/clp_s/OutputHandlerImpl.hpp (2)
NetworkOutputHandler(60-64)NetworkOutputHandler(67-71)
🔇 Additional comments (52)
components/core/src/clp_s/JsonConstructor.cpp (1)
13-13: LGTM: Spdlog include addition is appropriate.The addition of
spdlog/spdlog.hinclude is necessary to support the existingSPDLOG_WARNusage on line 42. This change aligns with the broader modularization effort described in the PR.components/core/src/clp_s/ArchiveWriter.cpp (1)
8-8: LGTM: Spdlog include addition is appropriate.The addition of
spdlog/spdlog.hinclude is necessary to support the existingSPDLOG_ERRORusage on lines 33 and 39. This change aligns with the broader modularization effort described in the PR.components/core/src/clp_s/search/Output.cpp (1)
6-6: LGTM: Spdlog include addition is appropriate.The addition of
spdlog/spdlog.hinclude is necessary to support the existingSPDLOG_ERRORusage on lines 117 and 126 for error handling in output handler operations. This change aligns with the broader modularization effort described in the PR.components/core/src/clp_s/ZstdCompressor.cpp (1)
4-4: LGTM: Spdlog include addition is appropriate.The addition of
spdlog/spdlog.hinclude is necessary to support the existingSPDLOG_ERRORusage on lines 13, 36, 79, and 108 for comprehensive error reporting in compression operations. This change aligns with the broader modularization effort described in the PR.components/core/src/clp_s/search/ast/CMakeLists.txt (1)
66-66: LGTM! Proper CMake include directory configuration.The addition of the public include directory correctly exposes the core headers needed for the AST library's public interface, supporting the modularization effort.
components/core/src/clp_s/DictionaryWriter.cpp (1)
5-5: LGTM! Necessary include for logging functionality.The spdlog include is required for the
SPDLOG_ERRORusage on line 18 and aligns with the consistent logging pattern being established across the codebase.components/core/src/clp_s/VariableDecoder.cpp (1)
5-5: LGTM! Required include for error logging.The spdlog include is necessary for the
SPDLOG_ERRORusage in the variable count validation (lines 19-25) and maintains consistency with the logging approach across the codebase.components/core/tests/clp_s_test_utils.cpp (2)
16-16: LGTM! Correct namespace refactoring.The parameter type change from
CommandLineArguments::FileTypetoclp_s::FileTypeproperly reflects the enum's relocation to the clp_s namespace.
42-44: LGTM! Consistent enum usage after refactoring.The enum value references have been correctly updated to use the new
clp_s::FileTypenamespace, maintaining the same logic while simplifying the type qualification.components/core/src/clp_s/kv_ir_search.cpp (1)
252-252: LGTM! Correct function call after refactoring.The change from qualified
ReaderUtils::try_create_readerto unqualifiedtry_create_readerproperly reflects the function's relocation toInputConfig.hpp(included on line 29). The function signature and behaviour remain unchanged.components/core/tests/test-clp_s-range_index.cpp (1)
175-175: LGTM! Enum qualifier simplification is correct.The changes correctly update the enum usage from
clp_s::CommandLineArguments::FileTypetoclp_s::FileType, which aligns with the refactoring that moved theFileTypeenum fromCommandLineArgumentsto theclp_snamespace inInputConfig.hpp. The required include is present on line 21.Also applies to: 179-179
components/core/tests/test-clp_s-end_to_end.cpp (1)
110-110: LGTM! Consistent enum qualifier simplification.The change correctly simplifies the enum qualifier from
clp_s::CommandLineArguments::FileType::Jsontoclp_s::FileType::Json, maintaining consistency with the refactoring that moved theFileTypeenum to theclp_snamespace. TheInputConfig.hppinclude on line 12 ensures the enum is accessible.components/core/src/clp_s/JsonParser.cpp (2)
31-31: LGTM! Proper include added for refactored functionality.The addition of
#include "InputConfig.hpp"is necessary to access thetry_create_readerfunction that was moved fromReaderUtilstoInputConfigas part of the modularization effort.
488-488: LGTM! Reader creation calls updated correctly.The changes properly replace
ReaderUtils::try_create_readerwith the newtry_create_readerfunction fromInputConfig. The function signature and parameters remain the same, ensuring a seamless transition. The necessary include was added on line 31.Also applies to: 951-951
components/core/.clang-format (1)
7-9: LGTM! Clang-format configuration updated for modularization.The addition of "clp_s" to the library headers regex pattern is appropriate for the modularization effort. This ensures that headers from the newly introduced clp_s libraries (clp_s::IO, clp_s::ArchiveReader, clp_s::ArchiveWriter, etc.) are properly grouped during include sorting and formatting.
components/core/src/clp_s/JsonParser.hpp (1)
39-39: LGTM! Clean enum usage after refactoring.The change properly uses the
FileTypeenum from theInputConfig.hppheader, which aligns with the PR's objective to modularize the codebase by moving the enum fromCommandLineArgumentstoInputConfig.taskfiles/lint.yaml (1)
432-433: LGTM! Lint configuration updated to match file refactoring.The exclusion list correctly reflects the file renaming from
OutputHandlertoOutputHandlerImplfiles, ensuring the new implementation files are properly included in the linting process.components/core/src/clp_s/clp-s.cpp (3)
25-25: LGTM! Necessary include for output handler implementations.The include for
OutputHandlerImpl.hppis required to access the concrete output handler classes that were moved from the originalOutputHandler.hppas part of the modularization effort.
111-111: LGTM! Correct enum usage after refactoring.The change to use
clp_s::FileType::KeyValueIris consistent with moving theFileTypeenum fromCommandLineArgumentsto theclp_snamespace inInputConfig.hpp.
231-258: LGTM! Proper namespace prefixes for output handlers.The explicit
clp_s::namespace prefixes are correct as the concrete output handler implementations were moved from theclp_s::searchnamespace to theclp_snamespace as part of the modularization effort. This ensures proper access to the output handler classes defined inOutputHandlerImpl.hpp.components/core/tests/clp_s_test_utils.hpp (2)
5-5: LGTM! Updated include for FileType enum.The include change from
CommandLineArguments.hpptoInputConfig.hppis correct as theFileTypeenum was moved toInputConfigas part of the modularization effort.
23-23: LGTM! Simplified FileType parameter type.The parameter type change from
clp_s::CommandLineArguments::FileTypetoclp_s::FileTypecorrectly uses the simplified enum type after it was moved to theclp_snamespace inInputConfig.hpp.components/core/src/clp_s/ArchiveReaderAdaptor.cpp (1)
263-263: LGTM! Correctly updated function call after refactoring.The change from
ReaderUtils::try_create_readerto the free functiontry_create_readeris consistent with the broader refactoring that moved this functionality fromReaderUtilstoInputConfig. The function signature and usage remain unchanged.components/core/src/clp_s/ReaderUtils.hpp (1)
4-5: LGTM! Header includes correctly updated after method removal.The replacement of specific headers with standard library headers is appropriate since the
try_create_readermethod was moved out of this class. The remaining functionality still requires<map>and<memory>for theSchemaMaptype alias and shared pointer usage.components/core/src/clp_s/search/kql/CMakeLists.txt (1)
9-25: LGTM! Excellent target renaming and aliasing for modularization.The change from
kqltoclp_s_search_kqlwith the aliasclp_s::search::kqlfollows CMake best practices for:
- Avoiding target name conflicts with more descriptive naming
- Providing clean namespaced interfaces through aliases
- Maintaining consistency across all target references
All build configuration and dependencies are correctly preserved.
components/core/src/clp_s/search/sql/CMakeLists.txt (1)
9-24: LGTM! Consistent target renaming following the same pattern as KQL.The change from
sqltoclp_s_search_sqlwith the aliasclp_s::search::sqlmaintains consistency with the KQL module changes and follows the same CMake best practices for modularization. All build configuration is correctly preserved and target references are consistently updated.components/core/src/clp_s/InputConfig.hpp (3)
5-5: LGTM! Well-structured include additions.The new includes are appropriate for the functionality being added -
<memory>forstd::shared_ptrandReaderInterface.hppfor the return type oftry_create_reader.Also applies to: 11-11
19-25: LGTM! Well-designed enum class.The
FileTypeenum class follows best practices with:
- Explicit underlying type (
uint8_t)- Explicit value for the first member to ensure stability
- Clear, descriptive member names
116-123: LGTM! Well-designed function signature.The
try_create_readerfunction declaration follows good practices with:
- Appropriate const-correctness for parameters
- Clear return type using
std::shared_ptr[[nodiscard]]attribute to prevent ignoring the return value- Descriptive parameter names
components/core/tests/test-clp_s-search.cpp (3)
19-19: LGTM! Include path correctly updated.The include path change from
"../src/clp_s/search/OutputHandler.hpp"to"../src/clp_s/OutputHandlerImpl.hpp"correctly reflects the file reorganization where concrete output handler implementations were moved to a separate header.
45-45: LGTM! Consistent namespace updates.All references to
VectorOutputHandlerhave been consistently updated fromclp_s::search::VectorOutputHandlertoclp_s::VectorOutputHandler, properly reflecting the namespace restructuring where concrete output handlers moved from the search namespace to the main clp_s namespace.Also applies to: 60-60, 99-99, 121-121
163-163: LGTM! FileType enum reference correctly updated.The change from
clp_s::CommandLineArguments::FileType::Jsontoclp_s::FileType::Jsoncorrectly reflects the movement of theFileTypeenum fromCommandLineArgumentstoInputConfig.components/core/src/clp_s/search/CMakeLists.txt (3)
1-3: LGTM! Proper subdirectory structure.The subdirectory additions for
ast,kql, andsqlare correctly organized to support the modular search library architecture.
5-29: LGTM! Well-organized source file list.The source file organization is logical, grouping related functionality together and using appropriate relative paths. The inclusion of both implementation and header files ensures proper compilation dependencies.
31-47: LGTM! Modern CMake library configuration.The library definition follows modern CMake best practices with:
- Proper alias target (
clp_s::search)- Appropriate PUBLIC/PRIVATE linkage distinctions
- Correct dependency specification
- C++20 standard requirement
The PUBLIC linkage for core dependencies and PRIVATE linkage for logging is well-designed.
components/core/CMakeLists.txt (2)
318-319: LGTM! Source files correctly updated.The replacement of the old
OutputHandler.cppreference with the newOutputHandlerImpl.cppandOutputHandlerImpl.hppfiles correctly reflects the refactoring where concrete output handler implementations were moved to separate files.
689-690: LGTM! Library dependencies properly namespaced.The update from
kqlandsqltoclp_s::search::kqlandclp_s::search::sqlcorrectly uses the fully qualified library names from the new modular structure, ensuring proper dependency resolution.components/core/src/clp_s/search/OutputHandler.hpp (1)
14-64: Well-designed abstract interface!The refactoring to a pure abstract interface is clean and follows SOLID principles. The [[nodiscard]] attributes on error-returning methods and the trailing return type syntax are excellent modern C++ practices.
components/core/src/clp_s/OutputHandlerImpl.hpp (7)
1-22: LGTM! Well-organized header structure.The includes are properly grouped by category (system, standard library, third-party, internal) and the header guard follows the standard naming convention.
24-44: LGTM! Simple and effective stdout handler.The implementation correctly overrides both write methods and provides useful context in the output format.
46-87: LGTM! Proper RAII implementation for network handling.The destructor correctly checks and closes the socket, and the exception class provides proper error handling infrastructure. The delegation pattern in the simpler write method avoids code duplication.
89-170: LGTM! Well-designed MongoDB cache handler.The implementation properly handles batching, result limits, and priority-based result management. The QueryResult struct captures all necessary metadata, and the comparator correctly implements timestamp-based ordering for the priority queue.
172-200: LGTM! Purpose-built count aggregation handler.The empty implementation of the metadata write method is appropriate for a count-only handler. The error code documentation for the finish method is helpful.
202-238: LGTM! Correct time-bucketing implementation.The bucket calculation properly floors timestamps to bucket boundaries, and using std::map ensures chronological ordering of buckets.
240-286: LGTM! Efficient vector-based result collection.The implementation efficiently uses emplace_back and properly handles both write method variants. Storing a reference to the output vector is appropriate for this use case.
components/core/src/clp_s/CMakeLists.txt (7)
1-2: LGTM! Proper subdirectory organization.Adding the search subdirectory aligns with the modularization goals of the PR.
83-109: LGTM! Well-structured temporary library.The library is properly documented as temporary and has correct dependency specifications with appropriate PUBLIC/PRIVATE visibility.
134-148: LGTM! Consistent temporary library pattern.The reducer dependencies library follows the same pattern as clp_dependencies and has appropriate minimal dependencies.
170-185: LGTM! Well-defined IO library.The library groups IO-related functionality with appropriate dependencies for file and network operations.
228-311: LGTM! Clear separation of archive reading and writing.The ArchiveWriter and ArchiveReader libraries are well-structured with appropriate dependencies. The separation enables selective linking based on application needs.
321-336: LGTM! Smart separation of MongoDB-dependent functionality.Isolating JsonConstructor with its MongoDB dependency from ArchiveReader allows other projects to use the reader functionality without unnecessary transitive dependencies.
362-406: LGTM! Executable properly uses the new modular structure.The clp-s executable correctly links against all the new modular libraries, demonstrating the successful refactoring from monolithic to modular build structure.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/tests/test-clp_s-search.cpp(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/tests/test-clp_s-search.cpp
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (3)
components/core/tests/test-clp_s-search.cpp (3)
56-58: Alignvalidate_resultssignature with refactored namespace
The function signature now correctly referencesclp_s::VectorOutputHandler::QueryResultinstead of the oldclp_s::searchnamespace. This aligns with the new location ofVectorOutputHandler.
152-152: Declare results vector using refactored type
The test now instantiatesstd::vector<clp_s::VectorOutputHandler::QueryResult>(instead of the search namespace variant), which matches the updated handler. Looks good.
216-216: Use relocatedFileTypeenum
Swappingclp_s::CommandLineArguments::FileType::Jsonforclp_s::FileType::Jsonreflects the move of the enum into the core namespace. Ensure that the signature ofcompress_archiveinclp_s_test_utils.hppwas updated accordingly.
wraymo
left a comment
wraymo
left a comment
There was a problem hiding this comment.
Great PR! I like the idea of splitting OutputHandler and moving try_create_reader to InputConfig
| - Regex: "<(absl|antlr4|archive|boost|bsoncxx|catch2|curl|date|fmt|log_surgeon|lzma|mongocxx\ | ||
| |msgpack|mysql|nlohmann|openssl|outcome|regex_utils|simdjson|spdlog|sqlite3|string_utils|yaml-cpp\ | ||
| |ystdlib|zstd)" | ||
| - Regex: "<(absl|antlr4|archive|boost|bsoncxx|catch2|clp_s|curl|date|fmt|log_surgeon|lzma\ |
There was a problem hiding this comment.
Not sure if clp_s should be priority 3 or 4
There was a problem hiding this comment.
I put priority 3 just because our string_utils library is also in this list.
| spdlog::spdlog | ||
| ZStd::ZStd | ||
| ) | ||
| target_include_directories(clp_s_IO PUBLIC ../) |
There was a problem hiding this comment.
Why do we need this? It seems that we still use ../clp instead of clp to include headers. And if we want to apply it to all targets. Can we simply use include_directories?
There was a problem hiding this comment.
These are generally to make it so that users that include these libraries have the option to include them like #include <clp_s/...>. I didn't change how any of the existing includes work within clp though to minimize refactoring.
Probably makes sense to refactor the ir stream search code to use that include style for the AST components it pulls in in a follow-up PR though.
There was a problem hiding this comment.
Yeah, I think in the future we can use the relative path from components/core/src for the headers (to get rid of headers like ../, ../../)
| ${CLP_S_REDUCER_SOURCES} | ||
| ) | ||
| add_library(clp_s::reducer_dependencies ALIAS clp_s_reducer_dependencies) | ||
| target_compile_features(clp_s_reducer_dependencies PRIVATE cxx_std_20) |
There was a problem hiding this comment.
Can we use set(CMAKE_CXX_STANDARD 20) at the project level?
There was a problem hiding this comment.
That might make sense, but I'm sort of just following the existing convention throughout the project. Maybe @kirkrodrigues has some thoughts?
There was a problem hiding this comment.
Afaik, target_compile_features is a requirement...
Specifies compiler features required when compiling a given target.
... whereas CMAKE_CXX_STANDARD (CXX_STANDARD's default value) is a request...
This property specifies the C++ standard whose features are requested to build this target. ... If the value requested does not result in a compile flag being added for the compiler in use, a previous standard flag will be added instead.
So in that sense, I think we should continue with target_compile_features since we do require C++20.
There was a problem hiding this comment.
What about combining with CXX_STANDARD_REQUIRED
There was a problem hiding this comment.
I guess that could work then.
There was a problem hiding this comment.
What was the conclusion of this discussion?
There was a problem hiding this comment.
I think leave it for now.
Also I think a parent project might be able to override the CXX_STANDARD flag by setting it to a certain value as a cache variable, so might be better to set it at the target level in general.
| PUBLIC | ||
| absl::flat_hash_map | ||
| clp_s::IO | ||
| nlohmann_json::nlohmann_json | ||
| PRIVATE |
There was a problem hiding this comment.
Did you follow any rules that what should be public and what should be private?
There was a problem hiding this comment.
My general approach was:
- start with everything as PRIVATE and move dependencies into PUBLIC until other components that depended on the library could build
- put any libraries that are definitely needed to properly use some library into PUBLIC
A lot of dependencies had to be made PUBLIC for each library just because there were a small number of header files from that library that directly included headers from one of the dependencies which made it so that users of the library also needed to link against that dependency in order to be able to find the headers.
There was a problem hiding this comment.
Do you think there are any risks in making most of the libraries PUBLIC? I’ve noticed that in some cases, we include CLP-S files in another project, and those files in turn include headers from these libraries. If the libraries are not marked as PUBLIC, we might need to explicitly link them in the other project.
There was a problem hiding this comment.
No real risks -- we'd be linking against them anyway. This just lets other projects that depend on us build correctly without having to explicitly link against every library that is transitively included by some header of ours.
taskfiles/lint.yaml
Outdated
| - "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/JsonParser.cpp" | ||
| - "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/JsonParser.hpp" | ||
| - "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/JsonSerializer.hpp" | ||
| - "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/OutputHandlerImpl.cpp" |
There was a problem hiding this comment.
it seems OutputHandlerImpl.cpp is in {{.G_CORE_COMPONENT_DIR}}/src/clp_s/
| - "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/Output.cpp" | ||
| - "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/Output.hpp" | ||
| - "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/OutputHandler.cpp" | ||
| - "{{.G_CORE_COMPONENT_DIR}}/src/clp_s/search/OutputHandler.hpp" |
There was a problem hiding this comment.
Yeah, I ended up just trying to fix the clang-tidy warnings for this file since it is pretty small anyway after these changes.
| * @param network_auth | ||
| * @return the opened clp::ReaderInterface or nullptr on error | ||
| */ | ||
| [[nodiscard]] auto try_create_reader(Path const& path, NetworkAuthOption const& network_auth) |
There was a problem hiding this comment.
We may find a better place for this method, but it's fine to put it here for now.
| PRIVATE | ||
| spdlog::spdlog | ||
| ) | ||
| target_include_directories(clp_s_search PUBLIC ../../) |
There was a problem hiding this comment.
Nit: Can we reorder this before target_link_libraries for the sake of alphabetization? Ditto in other places. Unless we're purposely doing this for relative includes for some reason?
| ${CLP_S_REDUCER_SOURCES} | ||
| ) | ||
| add_library(clp_s::reducer_dependencies ALIAS clp_s_reducer_dependencies) | ||
| target_compile_features(clp_s_reducer_dependencies PRIVATE cxx_std_20) |
There was a problem hiding this comment.
What was the conclusion of this discussion?
| ) | ||
|
|
||
| add_library( | ||
| clp_s_IO |
There was a problem hiding this comment.
How about clp_s_io? In the broader context, I think we should establish a naming convention for CMake libraries. We've been using snake_case in ystdlib as well as the sql & kql libraries, so how about carrying that over here? So ArchiveWriter would become archive_writer, etc.
There was a problem hiding this comment.
I think we roughly have a convention that subdirectory structure should be nested namespace structure should be library name that we don't completely follow right now.
I intentionally didn't mess with the directory structure or namespaces in this PR to avoid extra refactoring, but I can change the names to lowercase to fit the convention a bit better.
| target_include_directories(clp_s_TimestampPattern PUBLIC ../) | ||
|
|
||
| set( | ||
| CLP_S_SOURCES |
There was a problem hiding this comment.
How about CLP_S_EXE_SOURCES?
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
components/core/src/clp_s/CMakeLists.txt (1)
347-354: 🧹 Nitpick (assertive)Inconsistent alias naming for TimestampPattern
The aliasclp_s::TimestampPatternuses PascalCase, whereas other libraries use lowercase names (e.g.,clp_s::archive_reader). Consider renaming toclp_s::timestamp_patternfor consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/core/src/clp_s/CMakeLists.txt(5 hunks)components/core/src/clp_s/search/CMakeLists.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (27)
components/core/src/clp_s/search/CMakeLists.txt (5)
1-3: Subdirectory inclusion is correct
Theadd_subdirectory(ast),add_subdirectory(kql), andadd_subdirectory(sql)calls are in the proper order and necessary for building the respective search submodules.
5-29: Explicit source listing is good
Listing each file inCLP_S_SEARCH_SOURCESavoids unpredictable globbing and keeps the build deterministic.
31-34: Library definition follows conventions
Definingclp_s_searchand aliasing it toclp_s::searchaligns with the project’s modular structure.
35-37: Compile features and include paths
Requiring C++20 and exposing../../for headers is appropriate for this target’s public interface.
38-47: Link dependencies scoped correctly
The PUBLIC/PRIVATE separation ensures that only necessary transitive dependencies are exposed.components/core/src/clp_s/CMakeLists.txt (22)
2-2:searchsubdirectory ordering is fine
Includingsearchafterindexermaintains logical grouping and alphabetical order.
83-91: Core-dependencies library setup is consistent
Theclp_s_clp_dependenciestarget and its alias are declared correctly, and C++20 is enforced.
95-109:clp_s_clp_dependencieslink libraries look appropriate
PUBLIC vs PRIVATE scopes appear correctly assigned for transitive header usage.
111-120: Reducer-dependencies source list is clear
Explicit listing maintains build determinism.
136-144: Reducer library declaration is correct
clp_s_reducer_dependenciesand its alias follow naming conventions.
142-148: Reducer link configuration is valid
Dependencies are scoped properly, only exposing JSON as PUBLIC.
150-158: I/O sources defined explicitly
Good to avoid globbing for I/O module.
170-176: I/O library target is well-formed
Aliasclp_s::ioand C++20 feature requirement are consistent with other modules.
175-185: I/O link dependencies look correct
BOOST and ZStd are private, core dependencies are linked properly.
187-195: Archive-writer source set is explicit
Listing all sources avoids non-determinism in the archive writer module.
228-236: Archive-writer library declaration is good
Aliasclp_s::archive_writerand C++20 enforcement match project conventions.
233-250: Archive-writer link libraries scoped appropriately
Public and private dependencies are correctly separated to avoid polluting consumer link lines.
252-260: Archive-reader sources listed clearly
Deterministic source listing is maintained.
289-297: Archive-reader library setup is correct
Alias and C++20 feature usage align with other modules.
313-319: JSON-constructor source set is clear
Explicit listing avoids globbing.
321-328: JSON-constructor library declaration is correct
Aliasclp_s::json_constructorand C++20 enforcement are consistent.
326-336: JSON-constructor link libraries defined properly
Dependencies scoped PUBLIC/PRIVATE appear accurate.
338-345: Timestamp-pattern sources listed explicitly
Good for deterministic builds.
354-360: Timestamp-pattern link configuration is fine
Dependencies are scoped privately as expected.
362-367: Executable source list is explicit
Good practice for reproducible builds.
374-381: Executable target and compile features
clp-sis defined correctly with C++20 enabled.
402-406: Runtime output directory is set appropriately
Redirecting the executable toPROJECT_BINARY_DIRensures binaries land in the expected location.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
components/core/src/clp_s/CMakeLists.txt (1)
346-352: 🧹 Nitpick (assertive)TimestampPattern alias naming inconsistency
Theclp_s_TimestampPatterntarget and aliasclp_s::TimestampPatternuse PascalCase, whereas others use snake_case (e.g.,clp_s::io). Consider standardizing on one style for all aliases to improve consistency.
♻️ Duplicate comments (2)
components/core/src/clp_s/CMakeLists.txt (2)
5-5: 🧹 Nitpick (assertive)Clarify source-set variable name
Consider renamingCLP_S_CLP_SOURCEStoCLP_S_CLP_DEPENDENCIES_SOURCESto more clearly reflect that these sources back theclp_s_clp_dependencieslibrary.
385-395: 🧹 Nitpick (assertive)Remove redundant search subcomponent linking
You link bothclp_s::searchand its internal parts (clp_s::search::ast,clp_s::search::kql). Sinceclp_s::searchshould already PUBLICly depend on its sub-libraries, you can remove the explicitclp_s::search::astandclp_s::search::kqlentries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/src/clp_s/CMakeLists.txt(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (20)
components/core/src/clp_s/CMakeLists.txt (20)
2-2: Modular build: Include search subdirectory
The newadd_subdirectory(search)call correctly integrates the standalone search library. Ensure thesearch/CMakeLists.txtdefines its targets and export aliases to maintain encapsulation.
111-113: Library sources grouping is clear
Theset(CLP_S_REDUCER_SOURCES …)grouping cleanly organizes reducer sources forclp_s_reducer_dependencies.
136-140: Reducer dependencies library creation
The newclp_s_reducer_dependenciestarget and aliasclp_s::reducer_dependenciescorrectly encapsulate reducer logic.
141-148: Reducer dependencies linkage
Linkingnlohmann_jsonPUBLIC andclp_s::clp_dependenciesPRIVATE aligns with the intended dependency flow.
151-158: I/O library sources grouping
OrganizingCLP_S_IO_SOURCESunder a dedicatedclp_s_iotarget improves separation of concerns for IO utilities.
170-174: I/O library alias and features
Theclp_s_iolibrary with aliasclp_s::ioand C++20 feature requirement is well-structured.
187-192: Archive writer sources grouping
DefiningCLP_S_ARCHIVE_WRITER_SOURCESclearly lists all writer-specific files.
228-233: Archive writer library alias
Theclp_s_archive_writertarget aliasclp_s::archive_writerfollows the pattern well.
234-238: Archive writer compile and include
C++20 requirement and include directory setup are correct for the writer.
239-243: Archive writer linkage
PUBLICly linkingabsl::flat_hash_mapandclp_s::iowhile keeping other dependencies PRIVATE reflects proper encapsulation.
253-260: Archive reader sources grouping
TheCLP_S_ARCHIVE_READER_SOURCESgrouping sets up reader-specific files cleanly.
289-294: Archive reader library alias and features
clp_s_archive_readerwith aliasclp_s::archive_readerand C++20 requirement is consistent with other libraries.
295-302: Archive reader linkage
PUBLICly linkingabsl::flat_hash_map,clp_s::io, andnlohmann_jsonthen privately linking other dependencies aligns with the intended API boundary.
313-319: JSON constructor sources grouping
ListingCLP_S_JSON_CONSTRUCTOR_SOURCESfor the JSON constructor module is well organized.
320-326: JSON constructor library alias and features
Theclp_s_json_constructortarget with aliasclp_s::json_constructorand C++20 requirement is well-defined.
327-333: JSON constructor linkage
PUBLICly depending onclp_s::archive_readerand privately onfmt,MongoCXX, andspdlogensures minimal transitive dependencies.
338-344: TimestampPattern sources grouping
TheCLP_S_TIMESTAMP_PATTERN_SOURCESset correctly lists all timestamp-pattern files.
361-370: Executable sources grouping
DefiningCLP_S_EXE_SOURCESfor downstream components promotes clarity in theclp-starget.
373-376: Executable target setup
Theclp-sexecutable now cleanly includes theCLP_S_EXE_SOURCESset.
379-382: Executable include directories
Using theCLP_OUTCOME_INCLUDE_DIRECTORYis appropriate for outcome integration.
| PRIVATE | ||
| simdjson::simdjson | ||
| ) | ||
| target_include_directories(clp_s_search_ast PUBLIC ../../../) |
There was a problem hiding this comment.
Nit: Move above target_link_libraries.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/core/CMakeLists.txt(2 hunks)components/core/src/clp_s/CMakeLists.txt(4 hunks)components/core/src/clp_s/search/ast/CMakeLists.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (13)
components/core/src/clp_s/search/ast/CMakeLists.txt (1)
62-64: Correct linkage oftimestamp_patterndependency
Theclp_s_search_asttarget now properly declaresclp_s::timestamp_patternas a PUBLIC dependency, matching the AST’s need for timestamp pattern parsing.components/core/CMakeLists.txt (2)
318-319: Add new OutputHandlerImpl files to unit test sources
The newOutputHandlerImpl.cppandOutputHandlerImpl.hppare correctly included in theclp_sunit tests, ensuring the refactored output handler implementations are covered.
688-691: Link unit tests against new modular search and timestamp libraries
TheunitTesttarget now links againstclp_s::search::ast,clp_s::search::kql,clp_s::search::sql, andclp_s::timestamp_pattern, reflecting the intended modular build structure.components/core/src/clp_s/CMakeLists.txt (10)
2-2: Register newsearchsubdirectory
Addingadd_subdirectory(search)integrates the consolidated search module (AST, KQL, SQL) into theclp_sbuild and aligns with the new modular namespace.
83-90: Verify inclusion of outcome headers inclp_s::clp_dependencies
target_include_directories(clp_s_clp_dependencies PUBLIC "${CLP_OUTCOME_INCLUDE_DIRECTORY}")exposes outcome headers externally. Ensure${CLP_OUTCOME_INCLUDE_DIRECTORY}is defined and points to the correct location so downstream consumers can resolve these includes.
111-118: Modularize reducer dependencies
Theclp_s::reducer_dependencieslibrary cleanly encapsulates reducer sources, publicly linking only againstnlohmann_jsonand privately onclp_s::clp_dependencies. This aligns with the goal of splitting the monolith into focused libraries.Also applies to: 136-142
228-235: Review public vs private dependencies ofarchive_writer
clp_s::archive_writerpublicly linksabsl::flat_hash_mapandclp_s::io, whileBoost::urlandmsgpack-cxxremain private. Verify that none of the public headers expose types from the private dependencies—if they do, those libraries should be promoted toPUBLIC.
289-296: Review public vs private dependencies ofarchive_reader
Similarly, confirm thatArchiveReader.hppand related public headers don’t expose types fromBoost::urlormsgpack-cxx. If they do, adjust those linkages toPUBLICfor correct transitive propagation.
320-327: Validatejson_constructordependencies
Theclp_s::json_constructorlibrary publicly usesclp_s::archive_readerand privatelyfmt,mongocxx, andspdlog. EnsureJsonConstructor.hppdoes not expose anymongocxxor formatting types; if it does, those should bePUBLIC.
347-353: Reviewtimestamp_patternAPI dependencies
IfTimestampPattern.hpppublicly uses types fromdate::date(e.g.,year_month_day), linkingdate::dateasPRIVATEmay be insufficient. Promote it toPUBLICif its types appear in the public API.
362-369: Include new output handler implementations in executable
TheOutputHandlerImpl.cppand.hppfiles are correctly added to theclp-sexecutable’s sources, replacing the oldsearchvariants and supporting the refactored namespace layout.
375-382: EnsureCLP_OUTCOME_INCLUDE_DIRECTORYis set
clp-sprivately includes${CLP_OUTCOME_INCLUDE_DIRECTORY}. Verify that this variable is always defined when buildingclp-s, otherwise the include path may be missing.
386-395:❌ Incorrect review comment
Streamline
clp-slinkage toclp_s::search
Sinceclp_s::searchalreadyPUBLICly depends on its sublibraries (ast,kql, andsql), you can remove the explicitclp_s::search::astandclp_s::search::kqlentries. Also verify whether you need to explicitly linkclp_s::search::sqlor rely on its transitive linkage.
🏁 Script executed:
#!/usr/bin/env bash # Locate definitions of clp_s::search and its sub-libraries in CMakeLists rg -n 'add_library.*search::ast' -C2 rg -n 'add_library.*search::kql' -C2 rg -n 'add_library.*search::sql' -C2 rg -n 'add_library.*clp_s::search' -C2 rg -n 'target_link_libraries.*clp_s::search' -C2Length of output: 3516
🏁 Script executed:
#!/usr/bin/env bash # Show all target_link_libraries calls in the clp_s::search CMakeLists rg -n 'target_link_libraries' components/core/src/clp_s/search/CMakeLists.txt -C3Length of output: 347
Incorrect assumption about transitive linkage in
clp_s::search
Thecomponents/core/src/clp_s/search/CMakeLists.txtshows thatclp_s::searchonly links againstabsl::flat_hash_mapand does not publicly link itsast,kqlorsqlsub-libraries. Removing the explicitclp_s::search::ast clp_s::search::kqlentries from the parent
target_link_librarieswould break downstream consumers. You have two options:
- Keep the explicit
clp_s::search::ast/kqllinks where they’re currently used.- Or, modify
clp_s::search’s own CMakeLists (around line 38) to:This would make those sub-libraries transitively available throughtarget_link_libraries( clp_s_search PUBLIC absl::flat_hash_map + clp_s::search::ast + clp_s::search::kql + clp_s::search::sql )clp_s::search.Likely an incorrect or invalid review comment.
3 participants
Description
This PR splits the clp-s build into several smaller library builds in order to make it easier to selectively use certain components of clp-s in other projects. Care has been taken to ensure that each library doesn't force users to include unnecessary transitive dependencies.
The following table outlines each library in the clp-s build its purpose:
clp_s::clp_dependenciesclp_s::reducer_dependenciesclp_s::IOclp_s::ArchiveWriterArchiveWriterand all other classes needed to write an archiveclp_s::ArchiveReaderArchiveReaderand all other classes needed to read an archiveclp_s::JsonConstructorJsonConstructor-- implements various decompression flows for clp-s binaryclp_s::TimestampPatternTimestampPatternand other classes needed to parse timestampsclp_s::searchclp_s::search::astclp_s::search::kqlclp_s::search::sqlNote that not all of these libraries are new and that the kql and sql libraries have been renamed so that they are properly namespaced.
Also note that
JsonParseris included in theclp_s::ArchiveWriterlibrary (unlikeJsonConstructorfor theArchiveReaderlibrary) because including it does not introduce any extra dependencies (unlikeJsonConstructorwhich links against the mongodb c++ library).Since many of these components depend on the temporary
clp_s::clp_dependenciesandclp_s::reducer_dependencieslibraries we have chosen not to replace uses of clp-s components within the codebase with linkage against these libraries.Decoupling these libraries did require some code changes including:
OutputHandlerintoOutputHandler.hppandOutputHandlerImpl.{cpp,hpp}in order to eliminate transitive depencies from various output handlers inclp_s::searchFileTypeenum fromCommandLineArgumentstoInputConfigto remove transitive dependency onCommandLineArgumentsin several components.try_create_readerfromReaderUtilstoInputConfigso that it can be part ofclp_s::IOwithout introducingArchiveReaderspecific dependenciesChecklist
breaking change.
Validation performed
clp_s::searchcan build properly when linking against onlyclp_s::search.Summary by CodeRabbit
New Features
Refactor
Style
Chores
Tests