Skip to content

Conversation

@LinZhihao-723
Copy link
Member

@LinZhihao-723 LinZhihao-723 commented Apr 27, 2025

Description

References

Overview

This PR adds an enum to represent possible results for search AST evaluation.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • Ensure all workflows passed.

Summary by CodeRabbit

  • New Features

    • Introduced a new enumeration to represent the outcomes of search AST evaluations, providing clearer status reporting for search operations.
  • Chores

    • Improved build configuration by updating dependency management and cleaning up submodule usage for a more streamlined setup.

kirkrodrigues and others added 30 commits April 24, 2025 11:18
…ernal-deps task to core-all-parallel; Remove obsolete comment about submodule download parallelization.
@LinZhihao-723 LinZhihao-723 requested review from a team, gibber9809 and wraymo as code owners April 27, 2025 23:34
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Apr 27, 2025

Walkthrough

The updates introduce a new enumeration class AstEvaluationResult for representing search AST evaluation outcomes and significantly revise the CMake build configuration. The CMake changes shift dependency management from submodules to external packages, introduce conditional inclusion of dependency settings, update linking and include paths, and clean up submodule references. No changes were made to exported or public code entities except for the addition of the new enum class.

Changes

File(s) Change Summary
components/core/CMakeLists.txt Refactored to use find_package for dependencies (absl, Catch2, date, log_surgeon, nlohmann_json, simdjson, yaml-cpp); removed submodule add_subdirectory usage; added conditional inclusion of external dependency settings; updated linking and include directories; removed explicit submodule paths for SQLite3 and outcome.
components/core/src/clp/ffi/ir_stream/search/AstEvaluationResult.hpp Added new header defining AstEvaluationResult enum class with values True, False, and Pruned, representing outcomes of AST evaluation in search logic.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant BuildSystem (CMake)
    participant ExternalPackages
    participant SourceCode

    User->>BuildSystem (CMake): Configure project
    BuildSystem (CMake)->>ExternalPackages: find_package (absl, Catch2, date, etc.)
    ExternalPackages-->>BuildSystem (CMake): Provide dependency config
    BuildSystem (CMake)->>SourceCode: Set include/link directories
    BuildSystem (CMake)->>SourceCode: Build targets with new dependency management
Loading
sequenceDiagram
    participant SearchLogic
    participant AstEvaluationResult

    SearchLogic->>AstEvaluationResult: Evaluate AST
    AstEvaluationResult-->>SearchLogic: Return True/False/Pruned
Loading

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 84667a1 and a84f0f0.

📒 Files selected for processing (1)
  • components/core/src/clp/ffi/ir_stream/search/AstEvaluationResult.hpp (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • components/core/src/clp/ffi/ir_stream/search/AstEvaluationResult.hpp
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: ubuntu-jammy-static-linked-bins
  • GitHub Check: ubuntu-jammy-lint
  • GitHub Check: ubuntu-jammy-dynamic-linked-bins
  • GitHub Check: centos-stream-9-static-linked-bins
  • GitHub Check: centos-stream-9-dynamic-linked-bins
  • GitHub Check: lint-check (ubuntu-latest)
  • GitHub Check: build (macos-latest)
✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@LinZhihao-723 LinZhihao-723 changed the title feat(kv-ir): Add AstEvaluationResult to represent AST search results. [WIP] feat(kv-ir): Add AstEvaluationResult to represent AST search results. Apr 27, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (10)
components/core/src/clp/ffi/ir_stream/utils.hpp (1)

12-13: Replace submodule includes for JSON and Outcome
Switched from single-header submodule paths to the standard <nlohmann/json.hpp> and <outcome/outcome.hpp> includes. Consider using only <nlohmann/json_fwd.hpp> here to minimize compile overhead in headers, moving the full <nlohmann/json.hpp> into the corresponding .cpp.

components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (1)

15-15: Add JSON forward declaration header Including <nlohmann/json_fwd.hpp> supports forward declarations and may reduce compile-time dependencies. Consider placing it before <nlohmann/json.hpp> for clarity.

taskfiles/deps/utils.yaml (2)

20-30: Make checksum combination deterministic.
Concatenating files via a glob may produce non-deterministic ordering. Consider sorting the list before combining:

- for file in "{{.G_DEPS_CORE_CHECKSUMS_DIR}}"/*.md5; do
+ for file in $(ls "{{.G_DEPS_CORE_CHECKSUMS_DIR}}"/*.md5 | sort); do

This ensures reproducible all.md5 contents.


32-39: Combine directory creation commands.
Multiple mkdir -p calls can be merged for readability and slight performance gain:

- cmds:
-   - "mkdir -p '{{.G_DEPS_DIR}}'"
-   - "mkdir -p '{{.G_DEPS_CORE_DIR}}'"
-   - "mkdir -p '{{.G_DEPS_CORE_CHECKSUMS_DIR}}'"
-   - "mkdir -p '{{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}'"
+ cmds:
+   - "mkdir -p '{{.G_DEPS_DIR}}' '{{.G_DEPS_CORE_DIR}}' '{{.G_DEPS_CORE_CHECKSUMS_DIR}}' '{{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}'"
taskfiles/deps/main.yaml (6)

7-12: Enhance comments for directory variables.
The G_DEPS_* variables are self-explanatory, but consider adding brief descriptions or grouping them under a clear comment block for maintainability.


13-16: Document checksum file naming convention.
You have all.md5 and log-viewer.md5. It may help future maintainers to outline the naming pattern in a comment.


17-19: Avoid duplicating CMake settings path.
The sync note warns to update components/core/CMakeLists.txt. Consider centralizing this path in a shared config or environment variable to reduce duplication.


31-33: Merge removal and creation of settings directory.
You clear and recreate G_DEPS_CORE_CMAKE_SETTINGS_DIR. For conciseness and atomicity:

- rm -rf '{{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}'
- mkdir -p '{{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}'
+ rm -rf '{{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}' && mkdir -p '{{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}'

68-81: DRY up repetitive install tasks.
The absl task mirrors other library subtasks. You could use YAML anchors or templates to define the common utils:install-remote-cmake-lib invocation and override only the library-specific vars.


104-108: Use heredoc for multi-line CMake settings.
The current echo block may mishandle newlines. A cat <<EOF heredoc is clearer:

- echo "set(
- ANTLR_EXECUTABLE \"{{.OUTPUT_FILE}}\"
- )" > "{{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.DEP_NAME}}.cmake"
+ cat <<EOF > "{{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.DEP_NAME}}.cmake"
+ set(ANTLR_EXECUTABLE "{{.OUTPUT_FILE}}")
+ EOF
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ecc6d0d and 6068f24.

📒 Files selected for processing (107)
  • .github/workflows/clp-core-build-macos.yaml (2 hunks)
  • .github/workflows/clp-core-build.yaml (3 hunks)
  • .gitmodules (0 hunks)
  • components/core/.clang-format (1 hunks)
  • components/core/.gitignore (0 hunks)
  • components/core/CMakeLists.txt (7 hunks)
  • components/core/cmake/Modules/FindANTLR.cmake (0 hunks)
  • components/core/src/clp/GlobalMetadataDBConfig.cpp (1 hunks)
  • components/core/src/clp/SQLiteDB.hpp (1 hunks)
  • components/core/src/clp/TimestampPattern.cpp (1 hunks)
  • components/core/src/clp/clg/CMakeLists.txt (1 hunks)
  • components/core/src/clp/clo/CMakeLists.txt (2 hunks)
  • components/core/src/clp/clo/clo.cpp (1 hunks)
  • components/core/src/clp/clp/CMakeLists.txt (2 hunks)
  • components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (1 hunks)
  • components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (1 hunks)
  • components/core/src/clp/ffi/ir_stream/Deserializer.hpp (1 hunks)
  • components/core/src/clp/ffi/ir_stream/Serializer.cpp (1 hunks)
  • components/core/src/clp/ffi/ir_stream/Serializer.hpp (1 hunks)
  • components/core/src/clp/ffi/ir_stream/encoding_methods.cpp (1 hunks)
  • components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cpp (1 hunks)
  • components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hpp (1 hunks)
  • components/core/src/clp/ffi/ir_stream/search/AstEvaluationResult.hpp (1 hunks)
  • components/core/src/clp/ffi/ir_stream/search/NewProjectedSchemaTreeNodeCallbackReq.hpp (1 hunks)
  • components/core/src/clp/ffi/ir_stream/utils.cpp (1 hunks)
  • components/core/src/clp/ffi/ir_stream/utils.hpp (1 hunks)
  • components/core/src/clp/ir/LogEventDeserializer.cpp (1 hunks)
  • components/core/src/clp/ir/LogEventDeserializer.hpp (1 hunks)
  • components/core/src/clp/make_dictionaries_readable/CMakeLists.txt (0 hunks)
  • components/core/src/clp/regex_utils/CMakeLists.txt (1 hunks)
  • components/core/src/clp/regex_utils/regex_translation_utils.cpp (1 hunks)
  • components/core/src/clp/regex_utils/regex_translation_utils.hpp (1 hunks)
  • components/core/src/clp/streaming_archive/writer/Archive.cpp (1 hunks)
  • components/core/src/clp_s/ArchiveWriter.cpp (1 hunks)
  • components/core/src/clp_s/CMakeLists.txt (2 hunks)
  • components/core/src/clp_s/JsonConstructor.cpp (1 hunks)
  • components/core/src/clp_s/TimestampPattern.cpp (1 hunks)
  • components/core/src/clp_s/clp-s.cpp (1 hunks)
  • components/core/src/clp_s/indexer/CMakeLists.txt (1 hunks)
  • components/core/src/clp_s/search/ast/CMakeLists.txt (1 hunks)
  • components/core/src/glt/GlobalMetadataDBConfig.cpp (1 hunks)
  • components/core/src/glt/SQLiteDB.hpp (1 hunks)
  • components/core/src/glt/TimestampPattern.cpp (1 hunks)
  • components/core/src/glt/ffi/ir_stream/encoding_methods.cpp (1 hunks)
  • components/core/src/glt/glt/CMakeLists.txt (2 hunks)
  • components/core/src/glt/ir/LogEventDeserializer.cpp (1 hunks)
  • components/core/src/glt/ir/LogEventDeserializer.hpp (1 hunks)
  • components/core/src/glt/streaming_archive/writer/Archive.cpp (1 hunks)
  • components/core/src/reducer/CMakeLists.txt (1 hunks)
  • components/core/src/reducer/DeserializedRecordGroup.cpp (1 hunks)
  • components/core/src/reducer/DeserializedRecordGroup.hpp (1 hunks)
  • components/core/src/reducer/JsonArrayRecordIterator.hpp (1 hunks)
  • components/core/src/reducer/JsonRecord.hpp (1 hunks)
  • components/core/src/reducer/ServerContext.cpp (1 hunks)
  • components/core/src/reducer/ServerContext.hpp (1 hunks)
  • components/core/src/reducer/reducer_server.cpp (1 hunks)
  • components/core/submodules/Catch2 (0 hunks)
  • components/core/submodules/abseil-cpp (0 hunks)
  • components/core/submodules/date (0 hunks)
  • components/core/submodules/json (0 hunks)
  • components/core/submodules/log-surgeon (0 hunks)
  • components/core/submodules/outcome (0 hunks)
  • components/core/submodules/simdjson (0 hunks)
  • components/core/submodules/utfcpp (0 hunks)
  • components/core/submodules/yaml-cpp (0 hunks)
  • components/core/submodules/ystdlib-cpp (0 hunks)
  • components/core/tests/test-BoundedReader.cpp (1 hunks)
  • components/core/tests/test-BufferedFileReader.cpp (1 hunks)
  • components/core/tests/test-EncodedVariableInterpreter.cpp (1 hunks)
  • components/core/tests/test-FileDescriptorReader.cpp (1 hunks)
  • components/core/tests/test-Grep.cpp (1 hunks)
  • components/core/tests/test-MemoryMappedFile.cpp (1 hunks)
  • components/core/tests/test-NetworkReader.cpp (1 hunks)
  • components/core/tests/test-ParserWithUserSchema.cpp (1 hunks)
  • components/core/tests/test-SQLiteDB.cpp (1 hunks)
  • components/core/tests/test-Segment.cpp (1 hunks)
  • components/core/tests/test-Stopwatch.cpp (1 hunks)
  • components/core/tests/test-StreamingCompression.cpp (1 hunks)
  • components/core/tests/test-TimestampPattern.cpp (1 hunks)
  • components/core/tests/test-Utils.cpp (1 hunks)
  • components/core/tests/test-clp_s-end_to_end.cpp (1 hunks)
  • components/core/tests/test-clp_s-search.cpp (1 hunks)
  • components/core/tests/test-encoding_methods.cpp (1 hunks)
  • components/core/tests/test-ffi_IrUnitHandlerInterface.cpp (1 hunks)
  • components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (1 hunks)
  • components/core/tests/test-ffi_SchemaTree.cpp (1 hunks)
  • components/core/tests/test-hash_utils.cpp (1 hunks)
  • components/core/tests/test-ir_encoding_methods.cpp (1 hunks)
  • components/core/tests/test-ir_parsing.cpp (1 hunks)
  • components/core/tests/test-ir_serializer.cpp (1 hunks)
  • components/core/tests/test-kql.cpp (1 hunks)
  • components/core/tests/test-main.cpp (1 hunks)
  • components/core/tests/test-math_utils.cpp (1 hunks)
  • components/core/tests/test-query_methods.cpp (1 hunks)
  • components/core/tests/test-regex_utils.cpp (1 hunks)
  • components/core/tests/test-sql.cpp (1 hunks)
  • components/core/tests/test-string_utils.cpp (1 hunks)
  • components/core/tests/test-utf8_utils.cpp (1 hunks)
  • components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh (1 hunks)
  • components/core/tools/scripts/lib_install/ubuntu-focal/install-prebuilt-packages.sh (1 hunks)
  • components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (1 hunks)
  • taskfile.yaml (2 hunks)
  • taskfiles/deps.yaml (0 hunks)
  • taskfiles/deps/main.yaml (1 hunks)
  • taskfiles/deps/utils.yaml (1 hunks)
  • taskfiles/lint.yaml (1 hunks)
  • tools/scripts/deps-download/init.sh (1 hunks)
⛔ Files not processed due to max files limit (1)
  • tools/yscope-dev-utils
💤 Files with no reviewable changes (15)
  • components/core/submodules/abseil-cpp
  • components/core/submodules/date
  • components/core/submodules/yaml-cpp
  • components/core/.gitignore
  • components/core/submodules/log-surgeon
  • components/core/src/clp/make_dictionaries_readable/CMakeLists.txt
  • components/core/submodules/Catch2
  • .gitmodules
  • components/core/submodules/simdjson
  • components/core/cmake/Modules/FindANTLR.cmake
  • components/core/submodules/json
  • components/core/submodules/utfcpp
  • components/core/submodules/outcome
  • components/core/submodules/ystdlib-cpp
  • taskfiles/deps.yaml
🧰 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}: - Prefer false == <expression> rather than !<expression>.

  • components/core/src/reducer/ServerContext.hpp
  • components/core/src/clp/GlobalMetadataDBConfig.cpp
  • components/core/src/clp/ffi/ir_stream/encoding_methods.cpp
  • components/core/tests/test-clp_s-end_to_end.cpp
  • components/core/tests/test-Segment.cpp
  • components/core/tests/test-Utils.cpp
  • components/core/tests/test-hash_utils.cpp
  • components/core/src/clp/SQLiteDB.hpp
  • components/core/tests/test-StreamingCompression.cpp
  • components/core/src/clp/regex_utils/regex_translation_utils.hpp
  • components/core/src/clp/regex_utils/regex_translation_utils.cpp
  • components/core/tests/test-sql.cpp
  • components/core/tests/test-ir_parsing.cpp
  • components/core/src/reducer/DeserializedRecordGroup.hpp
  • components/core/src/clp_s/TimestampPattern.cpp
  • components/core/tests/test-ffi_IrUnitHandlerInterface.cpp
  • components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hpp
  • components/core/tests/test-kql.cpp
  • components/core/tests/test-main.cpp
  • components/core/tests/test-ir_serializer.cpp
  • components/core/src/clp/ffi/ir_stream/search/NewProjectedSchemaTreeNodeCallbackReq.hpp
  • components/core/src/clp_s/JsonConstructor.cpp
  • components/core/src/glt/GlobalMetadataDBConfig.cpp
  • components/core/tests/test-ParserWithUserSchema.cpp
  • components/core/tests/test-Stopwatch.cpp
  • components/core/src/reducer/DeserializedRecordGroup.cpp
  • components/core/tests/test-BoundedReader.cpp
  • components/core/src/clp_s/ArchiveWriter.cpp
  • components/core/src/reducer/ServerContext.cpp
  • components/core/src/glt/ir/LogEventDeserializer.hpp
  • components/core/src/glt/SQLiteDB.hpp
  • components/core/tests/test-EncodedVariableInterpreter.cpp
  • components/core/tests/test-encoding_methods.cpp
  • components/core/tests/test-MemoryMappedFile.cpp
  • components/core/tests/test-ffi_SchemaTree.cpp
  • components/core/src/clp/TimestampPattern.cpp
  • components/core/src/reducer/JsonRecord.hpp
  • components/core/src/glt/TimestampPattern.cpp
  • components/core/src/reducer/JsonArrayRecordIterator.hpp
  • components/core/src/clp/ffi/ir_stream/utils.hpp
  • components/core/src/clp/streaming_archive/writer/Archive.cpp
  • components/core/tests/test-Grep.cpp
  • components/core/src/reducer/reducer_server.cpp
  • components/core/src/glt/ffi/ir_stream/encoding_methods.cpp
  • components/core/tests/test-string_utils.cpp
  • components/core/tests/test-regex_utils.cpp
  • components/core/src/clp/ffi/ir_stream/Deserializer.hpp
  • components/core/src/glt/ir/LogEventDeserializer.cpp
  • components/core/src/clp/ffi/KeyValuePairLogEvent.cpp
  • components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cpp
  • components/core/src/clp/ffi/ir_stream/Serializer.cpp
  • components/core/src/clp/ffi/ir_stream/Serializer.hpp
  • components/core/tests/test-TimestampPattern.cpp
  • components/core/tests/test-BufferedFileReader.cpp
  • components/core/src/clp/ffi/KeyValuePairLogEvent.hpp
  • components/core/tests/test-NetworkReader.cpp
  • components/core/tests/test-clp_s-search.cpp
  • components/core/src/clp/clo/clo.cpp
  • components/core/tests/test-ir_encoding_methods.cpp
  • components/core/src/clp_s/clp-s.cpp
  • components/core/src/clp/ir/LogEventDeserializer.cpp
  • components/core/src/clp/ffi/ir_stream/utils.cpp
  • components/core/src/glt/streaming_archive/writer/Archive.cpp
  • components/core/tests/test-ffi_KeyValuePairLogEvent.cpp
  • components/core/tests/test-SQLiteDB.cpp
  • components/core/tests/test-utf8_utils.cpp
  • components/core/tests/test-math_utils.cpp
  • components/core/src/clp/ffi/ir_stream/search/AstEvaluationResult.hpp
  • components/core/src/clp/ir/LogEventDeserializer.hpp
  • components/core/tests/test-query_methods.cpp
  • components/core/tests/test-FileDescriptorReader.cpp
🧠 Learnings (6)
components/core/src/clp/ffi/ir_stream/search/NewProjectedSchemaTreeNodeCallbackReq.hpp (1)
Learnt from: gibber9809
PR: y-scope/clp#584
File: components/core/src/clp_s/SchemaTree.hpp:171-171
Timestamp: 2024-11-12T18:46:20.933Z
Learning: In `components/core/src/clp_s/SchemaTree.hpp`, it's acceptable to use `std::string_view` as keys in `m_node_map` because `SchemaNode`'s `m_key_name` remains valid even after move operations or reallocations, preventing dangling references.
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
components/core/tests/test-ir_encoding_methods.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
components/core/src/clp/ir/LogEventDeserializer.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: centos-stream-9-deps-image
  • GitHub Check: ubuntu-jammy-deps-image
  • GitHub Check: build-macos (macos-15, true)
  • GitHub Check: build-macos (macos-14, true)
  • GitHub Check: build-macos (macos-13, true)
  • GitHub Check: build-macos (macos-14, false)
  • GitHub Check: build-macos (macos-13, false)
🔇 Additional comments (140)
components/core/src/clp/SQLiteDB.hpp (1)

6-6: Appropriate include reordering for SQLite3 header
Moving <sqlite3/sqlite3.h> immediately after the standard <string> include improves clarity and groups external dependencies together. No functionality is affected, and this aligns with the project’s include conventions.

components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh (1)

23-24: Add unzip and xz to package list
Including unzip and xz explicitly ensures extraction utilities are available for dependency workflows on CentOS Stream 9. This change is consistent with other platform scripts and does not introduce any regressions.

components/core/src/glt/ffi/ir_stream/encoding_methods.cpp (1)

3-3: Standardize JSON include path
Updating the include to <nlohmann/json.hpp> aligns with the external dependency management and removes references to the removed submodule. The change is correct and requires no further updates in this file.

components/core/.clang-format (1)

7-9: Update IncludeCategories regex for nlohmann
The regex has been amended to recognise nlohmann in library headers, matching the new JSON include paths. This supports consistent formatting across the codebase.

components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (1)

28-29: Install software-properties-common and unzip
Adding software-properties-common and unzip ensures the necessary tools for PPA management and archive extraction are available on Ubuntu Jammy. This aligns with other distribution scripts.

components/core/tests/test-Utils.cpp (1)

11-11: Align Catch2 include path
The include directive has been simplified to <catch2/catch.hpp>, matching the externally managed Catch2 installation.

components/core/src/clp/TimestampPattern.cpp (1)

7-7: Align date include path
The date library include was updated to <date/date.h> to use the externally managed date package.

components/core/src/clp/ffi/ir_stream/search/NewProjectedSchemaTreeNodeCallbackReq.hpp (1)

7-7: Align Outcome include path
The single-header Outcome include was updated to <outcome/outcome.hpp>, matching the external library layout.

components/core/src/clp/streaming_archive/writer/Archive.cpp (1)

15-15: Align JSON include path
Updated to use <nlohmann/json.hpp> to match the externally provided nlohmann_json installation.

components/core/tests/test-ParserWithUserSchema.cpp (1)

10-10: Align Catch2 include path
The Catch2 include path was updated to <catch2/catch.hpp> to reflect the external dependency.

components/core/tests/test-ir_serializer.cpp (1)

8-8: Catch2 include path updated
The test now includes <catch2/catch.hpp>, aligning with the external CMake find_package for Catch2. This simplification correctly removes the old submodule path.

components/core/tests/test-math_utils.cpp (1)

1-1: Catch2 include path updated
Switched to <catch2/catch.hpp> to match the project’s standardized testing includes and external Catch2 installation.

components/core/tests/test-ir_parsing.cpp (1)

1-1: Catch2 include path updated
Changed to <catch2/catch.hpp>, consistent with the removal of the submodule and use of the externally managed Catch2 package.

components/core/tests/test-main.cpp (1)

2-2: Catch2 include path updated
Updated the main test runner to use <catch2/catch.hpp>, conforming with the external dependency setup.

components/core/tests/test-hash_utils.cpp (1)

4-4: Catch2 include path updated
Simplified to <catch2/catch.hpp>, ensuring consistency across all test files after submodule removal.

components/core/src/glt/TimestampPattern.cpp (1)

7-7: Update include path for external date library

The include directive has been updated from the submodule path to the standard <date/date.h> to align with the new external dependency. Please ensure that find_package(date) or equivalent CMake configuration is correctly set up to locate this header.

components/core/tests/test-ffi_SchemaTree.cpp (1)

3-3: Standardize Catch2 include path

The Catch2 include path has been updated to <catch2/catch.hpp> reflecting the external dependency layout. Ensure the CMake target Catch2::Catch2 is linked appropriately in the test target.

components/core/tests/test-kql.cpp (1)

5-5: Standardize Catch2 include path

Catch2 include path updated to <catch2/catch.hpp> to use the installed Catch2 package. Verify that the test target links against Catch2::Catch2.

components/core/src/reducer/ServerContext.hpp (1)

10-10: Update include path for nlohmann JSON

The JSON include has been simplified from the submodule nested path to <nlohmann/json.hpp>. Confirm that find_package(nlohmann_json) and target nlohmann_json::nlohmann_json are correctly configured in CMake.

components/core/src/clp_s/TimestampPattern.cpp (1)

11-11: Update include path for external date library

The include directive now uses <date/date.h> instead of the submodule path. Ensure that the build system is updated to find the date package and link date::date.

components/core/tests/test-BoundedReader.cpp (1)

6-6: Approve updated Catch2 include path
The include directive has been correctly updated from the old submodule path to <catch2/catch.hpp>, aligning with the project’s move to an external Catch2 package.

components/core/tests/test-TimestampPattern.cpp (1)

1-1: Approve updated Catch2 include path
Standardizing the Catch2 include to <catch2/catch.hpp> is correct and consistent with other test files following the removal of the submodule.

components/core/src/glt/ir/LogEventDeserializer.hpp (1)

6-6: Approve updated Outcome include path
Replacing the single-header include with #include <outcome/outcome.hpp> matches the external Outcome package layout and aligns with the broader dependency refactoring.

components/core/src/glt/GlobalMetadataDBConfig.cpp (1)

4-4: Approve updated yaml-cpp include path
Switching to the standard <yaml-cpp/yaml.h> include correctly reflects the external yaml-cpp package usage after submodule removal.

components/core/src/reducer/JsonArrayRecordIterator.hpp (1)

4-4: Approve updated nlohmann-json include path
The new include directive <nlohmann/json.hpp> aligns with the external nlohmann/json package and is consistent with the updated build configuration.

components/core/src/reducer/JsonRecord.hpp (1)

4-4: Include path updated: The include directive has been standardized to use the external package path <nlohmann/json.hpp>. This aligns correctly with the CMake find_package(nlohmann_json) configuration.

components/core/src/clp/ir/LogEventDeserializer.hpp (1)

6-6: Include path updated: The Outcome library include has been switched to <outcome/outcome.hpp>, matching the new external package management and CMake find_package(Outcome) setup.

components/core/src/clp_s/clp-s.cpp (1)

11-11: Include path updated: The JSON include has been updated to <nlohmann/json.hpp>, consistent with the project-wide migration to external dependency management.

components/core/src/clp/clo/clo.cpp (1)

7-7: Include path updated: Standardized the JSON include to <nlohmann/json.hpp> to align with the updated build configuration and external package usage.

components/core/src/reducer/DeserializedRecordGroup.hpp (1)

8-8: Include path updated: The include directive now references <nlohmann/json.hpp>, which matches the CMake nlohmann_json target and the project's dependency migration.

components/core/tests/test-Stopwatch.cpp (1)

3-3: Include path standardization is correct

Updating the Catch2 include to <catch2/catch.hpp> matches the new external dependency approach and aligns with project-wide conventions.

components/core/tests/test-string_utils.cpp (1)

5-5: Simplified Catch2 include is consistent

The change to #include <catch2/catch.hpp> standardizes the Catch2 include path in line with the updated dependency management.

components/core/src/clp_s/JsonConstructor.cpp (1)

12-12: Update to nlohmann/json.hpp is appropriate

Switching the JSON include to <nlohmann/json.hpp> correctly reflects the removal of the submodule and use of the installed nlohmann_json library.

components/core/src/clp/regex_utils/regex_translation_utils.hpp (1)

7-7: Outcome include path updated correctly

Changing the Outcome header to <outcome/outcome.hpp> aligns with the new FindPackage-based configuration and removes the old submodule reference.

components/core/tests/test-StreamingCompression.cpp (1)

11-11: Catch2 include path simplified

The replacement of the Catch2 path with <catch2/catch.hpp> follows the project’s dependency overhaul and matches other test files.

components/core/tests/test-sql.cpp (1)

4-4: Standardize Catch2 include directive
The include path has been updated to the standardized <catch2/catch.hpp>, aligning with the external package layout and improving consistency across tests.

components/core/tests/test-BufferedFileReader.cpp (1)

4-4: Update Catch2 header path
Switching from the nested submodule path to <catch2/catch.hpp> ensures tests reference the installed Catch2 package directly.

components/core/tests/test-SQLiteDB.cpp (1)

13-13: Use standardized Catch2 include
Replacing the old submodule include with <catch2/catch.hpp> aligns this test with the new dependency management strategy.

components/core/tests/test-MemoryMappedFile.cpp (1)

7-7: Simplify Catch2 include path
Updated to <catch2/catch.hpp> to remove reliance on the Catch2 submodule and use the externally managed header.

components/core/src/glt/SQLiteDB.hpp (1)

6-6: Adjust include order for SQLite3 header
Moved <sqlite3/sqlite3.h> immediately after <string> to group external C library headers together, improving readability and consistency.

components/core/src/clp/ffi/ir_stream/encoding_methods.cpp (1)

3-3: Include path updated to standard nlohmann JSON header
Using <nlohmann/json.hpp> correctly reflects the externally managed JSON dependency and matches the updated CMake find_package approach.

components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hpp (1)

9-9: Outcome include path simplified to multi-header variant
Switching to <outcome/outcome.hpp> aligns with the external Outcome package and the updated build configuration.

components/core/tests/test-ffi_IrUnitHandlerInterface.cpp (1)

6-6: Catch2 include path updated to external package
Replacing the submodule path with <catch2/catch.hpp> is correct and consistent with the new dependency layout in CMake.

components/core/tests/test-EncodedVariableInterpreter.cpp (1)

3-3: Catch2 include standardized
The update to <catch2/catch.hpp> properly reflects usage of the externally managed Catch2 framework.

components/core/tests/test-Grep.cpp (1)

3-3: Catch2 include adjusted for external dependency
Switching from the single-header submodule include to <catch2/catch.hpp> matches the global shift to find_package for Catch2.

components/core/src/clp/GlobalMetadataDBConfig.cpp (1)

4-4: YAML-CPP include path updated
The include directive has been simplified to <yaml-cpp/yaml.h> to align with the externally managed yaml-cpp package. No functional changes detected.

components/core/tests/test-encoding_methods.cpp (1)

1-1: Catch2 include path updated
The test now includes Catch2 via <catch2/catch.hpp>, matching the external dependency rather than the old submodule path. All test logic remains unchanged.

components/core/tests/test-FileDescriptorReader.cpp (1)

7-7: Catch2 include path updated
Standardized the Catch2 include to <catch2/catch.hpp> to reflect the move to an external Catch2 package. No other modifications.

components/core/tests/test-Segment.cpp (1)

4-4: Catch2 include path updated
Updated the Catch2 header include to <catch2/catch.hpp> in line with external dependency usage. Test implementation remains identical.

components/core/tests/test-query_methods.cpp (1)

3-3: Catch2 include path updated
Changed the Catch2 include directive to <catch2/catch.hpp>, consistent with external package management. No logic changes.

components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cpp (1)

13-13: Include path update looks good

The include directive has been updated from the single-header variant to the standard multi-header variant, which aligns with the project's move from submodules to external dependency management.

components/core/src/clp_s/ArchiveWriter.cpp (1)

7-7: JSON library include path properly updated

The include directive has been simplified from the nested submodule path to the standard package include path, which is consistent with the project's transition to external dependency management.

components/core/src/reducer/DeserializedRecordGroup.cpp (1)

5-5: JSON library include path properly updated

The include directive has been simplified from the nested submodule path to the standard package include path, consistent with the project's move from submodules to external package management.

components/core/tests/test-clp_s-end_to_end.cpp (1)

8-8: Catch2 include path properly updated

The include directive has been simplified from the nested submodule path to the standard package include path, consistent with the project's move to external package management for testing dependencies.

components/core/src/reducer/ServerContext.cpp (1)

12-12: JSON library include path properly updated

The include directive has been simplified from the nested submodule path to the standard package include path, consistent with the project's transition away from submodules.

components/core/src/glt/streaming_archive/writer/Archive.cpp (1)

13-13: Updated JSON include path to use installed dependency
This corrects the nested submodule include and aligns with external nlohmann_json management. Ensure the top-level CMakeLists invokes find_package(nlohmann_json REQUIRED) and links the nlohmann_json::nlohmann_json target so this header is available.

components/core/tools/scripts/lib_install/ubuntu-focal/install-prebuilt-packages.sh (1)

31-31: Add unzip to required packages
Installing unzip ensures archive extraction tasks can proceed, matching changes in other platform scripts. Confirm this utility is used downstream in Taskfiles or scripts and does not introduce unwanted dependencies.

components/core/src/clp_s/search/ast/CMakeLists.txt (1)

65-65: Link against simdjson::simdjson imported target
Switching to the CMake imported target removes submodule coupling. Verify that find_package(simdjson REQUIRED) is present in the parent CMake configuration to supply this target.

components/core/src/reducer/CMakeLists.txt (1)

51-51: Link nlohmann_json::nlohmann_json in reducer-server
Adding the external JSON library target aligns with the removal of the json submodule. Ensure that find_package(nlohmann_json REQUIRED) is declared before this target to make the imported target available.

components/core/src/clp/regex_utils/CMakeLists.txt (1)

18-18: Use CLP_OUTCOME_INCLUDE_DIRECTORY for Outcome includes
Replacing the hardcoded submodule path with this CMake variable externalizes the dependency correctly. Confirm that CLP_OUTCOME_INCLUDE_DIRECTORY is defined (via find_package(Outcome) or similar) and exported to this directory.

components/core/src/clp/ffi/ir_stream/Serializer.cpp (1)

15-16: Standardize JSON and Outcome includes
Updated the include directives from submodule single-header paths to <nlohmann/json.hpp> and <outcome/outcome.hpp> to rely on the externally managed packages. This change aligns with the new Taskfile-based dependency management and removal of the JSON and Outcome submodules.

components/core/src/clp/ffi/ir_stream/utils.cpp (1)

8-9: Align JSON includes with external package
Replaced the old submodule include with <nlohmann/json.hpp> and added <nlohmann/json_fwd.hpp> for forward declarations. This ensures full JSON functionality is available while supporting faster compile times where only declarations are needed.

components/core/src/reducer/reducer_server.cpp (1)

9-9: Use external JSON package include
Changed the JSON include from the removed submodule path to <nlohmann/json.hpp>, matching the project-wide dependency refactor. No functional logic was affected.

components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (1)

14-16: Update JSON and Outcome includes to external packages
Replaced the submodule single-header includes with <nlohmann/json.hpp>, <nlohmann/json_fwd.hpp>, and <outcome/outcome.hpp>. This aligns with the removal of bundled dependencies and the shift to external package management.

components/core/src/clp/regex_utils/regex_translation_utils.cpp (1)

8-8: Updated Outcome include to installed library path. This change replaces the bundled single-header submodule include with the externally managed library path <outcome/outcome.hpp>, aligning with the project’s external dependency management.

.github/workflows/clp-core-build.yaml (3)

12-12: Expand workflow triggers to include all taskfiles. Changed the pull_request path filter to "taskfiles/**" so that edits to any dependency Taskfile under taskfiles/ will trigger the build.


23-23: Mirror change for push triggers. Updated the push path filter to include "taskfiles/**" as well, ensuring consistency across event types.


91-91: Update filter for CLP changes. Expanded the clp filter in the paths-filter job to cover "taskfiles/**", aligning with the new Taskfile structure for dependency management.

.github/workflows/clp-core-build-macos.yaml (2)

16-16: Expand macOS workflow trigger to cover all taskfiles. Updated the pull_request paths to include "taskfiles/**", capturing changes to any Taskfile for macOS CI builds.


30-30: Include Taskfiles in push paths. Added "taskfiles/**" to the push paths, ensuring the macOS workflow runs when any Taskfile is modified.

components/core/src/clp/ffi/ir_stream/Deserializer.hpp (1)

12-13: Use installed libraries include paths. Updated includes to <nlohmann/json.hpp> and <outcome/outcome.hpp>, replacing the bundled single-header paths and ensuring the code uses externally managed dependencies.

components/core/src/glt/ir/LogEventDeserializer.cpp (1)

5-6: Solidify include path updates for JSON and Outcome.
The updated include directives align with the external dependency management and remove submodule-based paths as intended.

components/core/src/clp/ffi/ir_stream/Serializer.hpp (1)

11-12: Standardise include paths for JSON and Outcome libraries.
These changes correctly switch from single-header submodule includes to installed package headers.

components/core/tests/test-regex_utils.cpp (1)

3-3: Update Catch2 include to external package path.
The new include matches the modern Catch2 installation layout and maintains test functionality.

components/core/src/clp/ir/LogEventDeserializer.cpp (1)

5-6: Switch to external JSON and Outcome headers.
The include paths now reference installed libraries instead of submodule single-header files, consistent with the rest of the refactoring.

tools/scripts/deps-download/init.sh (1)

14-15: Bump yscope-dev-utils to commit e300d1b.
Ensure the URL and extraction directory align with the updated dependency version specified in the Taskfile.

components/core/tests/test-clp_s-search.cpp (2)

12-12: Update Catch2 include path The include directive has been simplified from the submodule path to the installed package path, aligning with the project's external dependency management.


14-14: Update JSON include path Simplified the nlohmann JSON include to refer to the installed package instead of the submodule path.

components/core/tests/test-utf8_utils.cpp (2)

10-10: Update Catch2 include path Changed to use the installed Catch2 include path, removing reliance on submodule.


11-11: Update JSON include path Updated the JSON include to use the installed nlohmann_json header location.

taskfiles/lint.yaml (1)

121-121: Extend YAML lint scope The lint task now covers the entire taskfiles directory instead of specific YAML files, ensuring that all newly added and existing taskfiles are validated.

components/core/tests/test-NetworkReader.cpp (2)

12-12: Update Catch2 include path Simplified the Catch2 include to use the installed package path, consistent with other tests.


15-15: Update JSON include path Adjusted the nlohmann JSON include to reference the externally managed library.

components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (2)

13-13: Update Catch2 include path The Catch2 include path is simplified to the installed package location, aligning with updated dependency management.


14-14: Update JSON include path Changed to use <nlohmann/json.hpp> instead of the single-include submodule path.

components/core/tests/test-ir_encoding_methods.cpp (2)

15-15: Adopt external Catch2 include
The include directive has been updated to use the installed catch2/catch.hpp path, aligning with the new external dependency management.


17-17: Adopt external nlohmann/json include
The include directive now references nlohmann/json.hpp instead of the removed submodule path, consistent with project-wide changes.

components/core/src/clp/ffi/ir_stream/search/AstEvaluationResult.hpp (5)

1-3: Verify header guard naming
The header guard CLP_FFI_IR_STREAM_SEARCH_ASTEVALUATIONRESULT_HPP matches the file path and prevents multiple inclusion.


4-4: Include <cstdint> for fixed-width types
Standard <cstdint> is correctly pulled in to back the uint8_t underlying type.


6-15: Document AST evaluation outcomes
The Doxygen-style comment clearly explains the three states—True, False, and Pruned—for AST evaluation.


16-20: Define compact AstEvaluationResult enum
Using enum class with uint8_t underlying type is efficient; the values are clearly scoped.


23-23: Close header guard
The #endif comment aligns with the guard name for clarity.

components/core/src/clp_s/indexer/CMakeLists.txt (1)

91-91: Ensure imported targets for new libraries
You’ve linked date::date, simdjson::simdjson, and yaml-cpp. Please verify that corresponding find_package calls or imported targets are declared earlier in the CMake hierarchy.

Also applies to: 94-94, 96-96

taskfile.yaml (2)

4-4: Update dependency include path
The deps include has been changed to taskfiles/deps/main.yaml. Confirm that the new path exists and is correct in the repository.


30-30: Update utils taskfile reference
The G_UTILS_TASKFILE variable now points to .../utils/utils.yaml. Ensure tasks that reference this variable still resolve the correct utility definitions.

components/core/src/clp/clg/CMakeLists.txt (3)

120-120: Include SQLite amalgamation source
Adding "${CLP_SQLITE3_SOURCE_DIRECTORY}/sqlite3.c" to CLG_SOURCES assumes the variable is set and the amalgamation file is present. Please verify that CLP_SQLITE3_SOURCE_DIRECTORY is defined and valid.


128-132: Adjust include directories for new dependencies
The target now uses ${CLP_OUTCOME_INCLUDE_DIRECTORY} and ${CLP_SQLITE3_INCLUDE_DIRECTORY} instead of a blanket submodule path. Confirm these variables are defined and point to the correct include locations.


136-136: Link against external libraries
You’ve added date::date, nlohmann_json::nlohmann_json, and yaml-cpp to target_link_libraries. Ensure that appropriate find_package invocations for these libraries exist upstream.

Also applies to: 140-140, 145-145

components/core/src/glt/glt/CMakeLists.txt (5)

154-154: Ensure CLP_SQLITE3_SOURCE_DIRECTORY is defined upstream.
You added "${CLP_SQLITE3_SOURCE_DIRECTORY}/sqlite3.c" to GLT_SOURCES; confirm that CLP_SQLITE3_SOURCE_DIRECTORY is correctly set (e.g., in the parent components/core/CMakeLists.txt) to avoid an undefined variable at configure time.


176-180: Add outcome and sqlite include directories for GLT target.
Including ${CLP_OUTCOME_INCLUDE_DIRECTORY} and ${CLP_SQLITE3_INCLUDE_DIRECTORY} under PRIVATE ensures the GLT target compiles against the external libraries instead of submodules. This aligns with the project’s external dependency strategy.


184-184: Link date::date to GLT executable.
Adding date::date in target_link_libraries properly replaces the old submodule linkage. Ensure that find_package(date) is invoked before this in the parent CMake configuration.


190-190: Link nlohmann_json::nlohmann_json to GLT executable.
The new JSON dependency is listed correctly. Confirm that find_package(nlohmann_json) appears in the top-level CMakeLists so this target is available.


193-193: Simplify YAML-C++ linkage.
Switching from yaml-cpp::yaml-cpp to yaml-cpp matches the installed package target name. This is consistent with other targets and avoids the submodule-qualified name.

components/core/src/clp_s/CMakeLists.txt (3)

212-213: Link date::date in clp_s_TimestampPattern.
This addition replaces the submodule include and ensures the TimestampPattern library builds with the external date package. Verify find_package(date) is declared at the top level.


237-240: Add JSON and simdjson dependencies to clp-s.
Linking nlohmann_json::nlohmann_json and simdjson::simdjson aligns with the external package approach. Ensure both find_package(nlohmann_json) and find_package(simdjson) are available in the parent CMake context.


245-248: Update include directories for clp-s.
Replacing the hardcoded submodules path with ${CLP_OUTCOME_INCLUDE_DIRECTORY} under PRIVATE correctly centralizes third-party includes. This follows the new dependency management pattern.

components/core/src/clp/clp/CMakeLists.txt (5)

150-150: Incorporate sqlite3.c via variable.
Adding "${CLP_SQLITE3_SOURCE_DIRECTORY}/sqlite3.c" ensures SQLite3 is sourced from the configured external directory. Double-check the variable definition upstream to prevent missing file errors.


170-174: Enhance clp include directories.
Using ${CLP_OUTCOME_INCLUDE_DIRECTORY} and ${CLP_SQLITE3_INCLUDE_DIRECTORY} under target_include_directories removes the submodule dependency and is consistent with other components.


178-178: Link date::date for clp executable.
Incorporating the external date library replaces the submodule version. Ensure the version requirements are satisfied at configure time.


185-185: Link nlohmann_json::nlohmann_json for clp.
External JSON dependency is now correctly referenced. Confirm find_package(nlohmann_json REQUIRED) exists in the top-level CMake.


188-188: Simplify YAML-C++ target.
Linking the yaml-cpp target without namespace qualifiers matches the installed package and removes the old submodule path.

components/core/src/clp/clo/CMakeLists.txt (4)

122-122: Source SQLite3 via external directory.
The single entry "${CLP_SQLITE3_SOURCE_DIRECTORY}/sqlite3.c" is appropriate; just verify that CLP_SQLITE3_SOURCE_DIRECTORY points to the correct location.


156-160: Refactor include paths for clo.
Moving from a hardcoded submodule include to ${CLP_OUTCOME_INCLUDE_DIRECTORY} and ${CLP_SQLITE3_INCLUDE_DIRECTORY} aligns with external dependency management.


164-164: Link date::date for clo executable.
This replaces the inline date header and ensures the external package is used. Confirm that date is located before this target.


169-169: Link JSON for clo.
Adding nlohmann_json::nlohmann_json corrects the dependency. Make sure find_package(nlohmann_json) is declared globally.

components/core/CMakeLists.txt (15)

99-115: Conditional inclusion of external deps settings.
The PROJECT_IS_TOP_LEVEL guard and optional include(... deps/core/cmake-settings/all.cmake) is clear. Consider documenting the expected location of all.cmake for new contributors.


141-145: Introduce Catch2 via find_package.
Adding find_package(Catch2 REQUIRED) and emitting a status message ensures unit tests link against the installed Catch2.


146-149: Add date as a required package.
The find_package(date REQUIRED) block correctly locates the external date library.


162-166: Locate log_surgeon externally.
Switching to find_package(log_surgeon REQUIRED) aligns with the new strategy.


167-170: Find nlohmann_json.
This block ensures the JSON library is available globally.


172-175: Find simdjson.
Locating simdjson guarantees it’s linked across all subprojects.


261-264: Find YAML-C++.
The yaml-cpp block properly imports the YAML library.


266-269: Add external ystdlib subdirectory.
Adding ${CLP_YSTDLIB_SOURCE_DIRECTORY} as an external subdirectory is consistent – ensure that variable is defined and the path is valid.


414-476: Include new AstEvaluationResult.hpp in unit tests.
Adding the AST evaluation result header to the unitTest sources makes tests aware of the new enum. Confirm that corresponding source files include this header and that no implementation is missing.


681-683: Update unitTest include directories.
Including ${CLP_OUTCOME_INCLUDE_DIRECTORY} and ${CLP_SQLITE3_INCLUDE_DIRECTORY} for the unitTest target enforces the external dependency includes consistently.


688-688: Link Catch2 for unitTest target.
Using Catch2::Catch2 ensures the test framework is linked; verify that the test main is provided (tests/test-main.cpp).


692-692: Link date::date for unitTest.
This adds the date library to the unit tests.


699-699: Link JSON for unitTest.
Including nlohmann_json::nlohmann_json ensures JSON support in tests.


700-700: Link simdjson for unitTest.
Adding simdjson::simdjson is consistent with the new dependency model.


708-708: Link YAML-C++ for unitTest.
The yaml-cpp target added here aligns with the externalized dependencies.

taskfiles/deps/utils.yaml (4)

1-5: Confirm PR target changes.
This PR was described as adding AstEvaluationResult for AST search results, but this file defines only dependency utility tasks. Please verify that the feature files for AST evaluation are included in this PR.


3-5: Include directive looks correct.
The external utils taskfile path appears accurate in relation to this file’s location.


57-63: Ensure checksum:validate include patterns cover nested files.
Currently, INCLUDE_PATTERNS is set to - "{{.INSTALL_PREFIX}}", which may not match all nested files. Verify whether the validation task needs a glob, e.g. "{{.INSTALL_PREFIX}}/**/*".


74-79: Verify checksum:compute patterns.
Similar to validation, INCLUDE_PATTERNS for compute only lists the prefix directory. Confirm that the compute task captures all relevant files under INSTALL_PREFIX.

taskfiles/deps/main.yaml (6)

1-5: Confirm PR target changes.
This PR was described as adding AstEvaluationResult for AST search results, but this file defines dependency management tasks. Please verify that the intended feature files are included.


3-5: Verify includes for taskfiles.
The local utils.yaml include and the external yscope-dev-utils path look correct relative to this file.


20-25: Review default task composition.
The default task runs both core and log-viewer. Evaluate if these should always be bundled or if separate entrypoints would give better control and parallelism.


33-37: Validate install-deps-and-generate-settings parameters.
Ensure DEP_TASK: "core-all-parallel" correctly triggers all subtasks and that downstream CMake scripts consume the generated settings files.


51-67: Parallel core-all-parallel task is well-structured.
Grouping all core dependency subtasks here improves build performance and clarity.


116-129: Library-specific subtasks correctly follow established patterns for remote download, checksum validation, and CMake settings generation. No further comments.

Also applies to: 130-142, 143-155, 156-172, 174-200, 208-220, 221-250, 251-270, 271-284, 285-304

@LinZhihao-723 LinZhihao-723 changed the title [WIP] feat(kv-ir): Add AstEvaluationResult to represent AST search results. feat(kv-ir): Add AstEvaluationResult to represent AST search results. Apr 28, 2025
gibber9809
gibber9809 previously approved these changes Apr 28, 2025
Copy link
Contributor

@gibber9809 gibber9809 left a comment

Choose a reason for hiding this comment

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

LGTM. Title is fine by me as well.

Co-authored-by: kirkrodrigues <[email protected]>
@LinZhihao-723 LinZhihao-723 merged commit d40ecf0 into y-scope:main Apr 28, 2025
20 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants