feat(clp-s)!: Change how timestamps are specified in the AST and KQL to take advantage of clp_s::timestamp_parser:#1757
Conversation
…ndence on timestamp parsing.
WalkthroughThis change replaces DateLiteral with a new TimestampLiteral type (and renames EpochDateT→TimestampT), adds a SetTimestampLiteralPrecision transformation pass, updates KQL grammar and parser to accept timestamp expressions, and adjusts build and dependency checks to include the timestamp parser. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (6)
components/core/src/clp_s/search/kql/generated/KqlBaseVisitor.his excluded by!**/generated/**components/core/src/clp_s/search/kql/generated/KqlLexer.cppis excluded by!**/generated/**components/core/src/clp_s/search/kql/generated/KqlLexer.his excluded by!**/generated/**components/core/src/clp_s/search/kql/generated/KqlParser.cppis excluded by!**/generated/**components/core/src/clp_s/search/kql/generated/KqlParser.his excluded by!**/generated/**components/core/src/clp_s/search/kql/generated/KqlVisitor.his excluded by!**/generated/**
📒 Files selected for processing (16)
components/core/cmake/Options/options.cmake(1 hunks)components/core/src/clp_s/clp-s.cpp(2 hunks)components/core/src/clp_s/kv_ir_search.cpp(3 hunks)components/core/src/clp_s/search/AddTimestampConditions.cpp(3 hunks)components/core/src/clp_s/search/AddTimestampConditions.hpp(1 hunks)components/core/src/clp_s/search/EvaluateTimestampIndex.cpp(3 hunks)components/core/src/clp_s/search/ast/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/ast/DateLiteral.cpp(1 hunks)components/core/src/clp_s/search/ast/DateLiteral.hpp(2 hunks)components/core/src/clp_s/search/ast/SetDateLiteralPrecision.cpp(1 hunks)components/core/src/clp_s/search/ast/SetDateLiteralPrecision.hpp(1 hunks)components/core/src/clp_s/search/kql/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/kql/Kql.g4(1 hunks)components/core/src/clp_s/search/kql/kql.cpp(10 hunks)components/core/src/clp_s/timestamp_parser/CMakeLists.txt(1 hunks)components/core/tests/test-kql.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/src/clp_s/search/AddTimestampConditions.hppcomponents/core/tests/test-kql.cppcomponents/core/src/clp_s/search/ast/SetDateLiteralPrecision.hppcomponents/core/src/clp_s/search/EvaluateTimestampIndex.cppcomponents/core/src/clp_s/kv_ir_search.cppcomponents/core/src/clp_s/search/ast/SetDateLiteralPrecision.cppcomponents/core/src/clp_s/clp-s.cppcomponents/core/src/clp_s/search/kql/kql.cppcomponents/core/src/clp_s/search/AddTimestampConditions.cppcomponents/core/src/clp_s/search/ast/DateLiteral.cppcomponents/core/src/clp_s/search/ast/DateLiteral.hpp
🧠 Learnings (16)
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
components/core/src/clp_s/timestamp_parser/CMakeLists.txtcomponents/core/cmake/Options/options.cmakecomponents/core/src/clp_s/search/kql/CMakeLists.txtcomponents/core/src/clp_s/search/ast/CMakeLists.txt
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
components/core/src/clp_s/timestamp_parser/CMakeLists.txtcomponents/core/cmake/Options/options.cmakecomponents/core/src/clp_s/search/kql/CMakeLists.txtcomponents/core/src/clp_s/search/ast/CMakeLists.txt
📚 Learning: 2025-06-02T18:22:24.060Z
Learnt from: gibber9809
Repo: y-scope/clp PR: 955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Applied to files:
components/core/src/clp_s/timestamp_parser/CMakeLists.txtcomponents/core/cmake/Options/options.cmakecomponents/core/src/clp_s/search/kql/CMakeLists.txtcomponents/core/src/clp_s/search/ast/CMakeLists.txt
📚 Learning: 2025-08-03T18:56:07.366Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Applied to files:
components/core/src/clp_s/timestamp_parser/CMakeLists.txtcomponents/core/cmake/Options/options.cmakecomponents/core/src/clp_s/search/kql/CMakeLists.txtcomponents/core/src/clp_s/search/ast/CMakeLists.txt
📚 Learning: 2025-06-27T01:49:15.724Z
Learnt from: sitaowang1998
Repo: y-scope/clp PR: 1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Applied to files:
components/core/src/clp_s/timestamp_parser/CMakeLists.txtcomponents/core/cmake/Options/options.cmakecomponents/core/src/clp_s/search/ast/CMakeLists.txt
📚 Learning: 2025-12-09T01:57:04.839Z
Learnt from: SharafMohamed
Repo: y-scope/clp PR: 1313
File: components/core/tests/test-SchemaSearcher.cpp:136-137
Timestamp: 2025-12-09T01:57:04.839Z
Learning: In C++ files under the components directory, keep forward declarations even when a definition follows in the same scope (such as in anonymous namespaces) to maintain consistency across multiple methods. This style reduces churn and aligns with the existing convention in this repository for both source and tests.
Applied to files:
components/core/tests/test-kql.cppcomponents/core/src/clp_s/search/EvaluateTimestampIndex.cppcomponents/core/src/clp_s/kv_ir_search.cppcomponents/core/src/clp_s/search/ast/SetDateLiteralPrecision.cppcomponents/core/src/clp_s/clp-s.cppcomponents/core/src/clp_s/search/kql/kql.cppcomponents/core/src/clp_s/search/AddTimestampConditions.cppcomponents/core/src/clp_s/search/ast/DateLiteral.cpp
📚 Learning: 2025-04-26T02:21:22.021Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 856
File: components/core/src/clp/ffi/ir_stream/search/utils.cpp:258-266
Timestamp: 2025-04-26T02:21:22.021Z
Learning: In the clp::ffi::ir_stream::search namespace, the design principle is that callers are responsible for type checking, not the called functions. If control flow reaches a function, input types should already be validated by the caller.
Applied to files:
components/core/src/clp_s/kv_ir_search.cpp
📚 Learning: 2025-07-03T20:10:43.789Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1050
File: components/clp-package-utils/clp_package_utils/scripts/search.py:100-106
Timestamp: 2025-07-03T20:10:43.789Z
Learning: In the current CLP codebase implementation, dataset validation using validate_dataset() is performed within the native scripts (like clp_package_utils/scripts/native/search.py) rather than at the wrapper script level, where the native scripts handle their own parameter validation.
Applied to files:
components/core/cmake/Options/options.cmake
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Applied to files:
components/core/cmake/Options/options.cmake
📚 Learning: 2025-08-18T05:43:07.868Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/cmake/Options/options.cmake
📚 Learning: 2025-07-24T21:56:05.806Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/cmake/Options/options.cmake
📚 Learning: 2024-11-26T19:12:43.982Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) with `std::array`.
Applied to files:
components/core/src/clp_s/search/kql/CMakeLists.txt
📚 Learning: 2025-08-13T19:58:26.058Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/src/clp_s/search/kql/CMakeLists.txt
📚 Learning: 2024-10-07T21:38:35.979Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:581-627
Timestamp: 2024-10-07T21:38:35.979Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, within the `get_archive_node_id` method, throwing a string literal as an exception is acceptable practice.
Applied to files:
components/core/src/clp_s/search/kql/kql.cpp
📚 Learning: 2024-10-07T21:35:04.362Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:735-794
Timestamp: 2024-10-07T21:35:04.362Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, within the `parse_from_ir` method, encountering errors from `kv_log_event_result.error()` aside from `std::errc::no_message_available` and `std::errc::result_out_of_range` is anticipated behavior and does not require additional error handling or logging.
Applied to files:
components/core/src/clp_s/search/kql/kql.cpp
📚 Learning: 2025-04-26T02:19:33.275Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 856
File: components/core/src/clp/ffi/ir_stream/search/utils.cpp:340-347
Timestamp: 2025-04-26T02:19:33.275Z
Learning: In the CLP codebase, the FilterOperation::EXISTS and FilterOperation::NEXISTS operations have specific semantics that differ from simply checking if a value exists. EXISTS always returns true and NEXISTS always returns false in the evaluate_filter_against_literal_type_value_pair function. The optional::has_value() is used specifically to differentiate empty objects from concrete values, not for existence checks in filter operations.
Applied to files:
components/core/src/clp_s/search/kql/kql.cpp
🧬 Code graph analysis (5)
components/core/tests/test-kql.cpp (4)
components/core/src/clp_s/search/ast/DateLiteral.cpp (1)
DateLiteral(15-18)components/core/src/clp_s/search/ast/DateLiteral.hpp (2)
DateLiteral(26-26)DateLiteral(71-71)components/core/src/clp_s/search/kql/kql.cpp (2)
parse_kql_expression(333-361)parse_kql_expression(333-333)components/core/src/clp_s/search/kql/kql.hpp (1)
parse_kql_expression(14-14)
components/core/src/clp_s/search/EvaluateTimestampIndex.cpp (2)
components/core/src/clp_s/search/ast/DateLiteral.cpp (1)
DateLiteral(15-18)components/core/src/clp_s/search/ast/DateLiteral.hpp (2)
DateLiteral(26-26)DateLiteral(71-71)
components/core/src/clp_s/search/ast/SetDateLiteralPrecision.cpp (1)
components/core/src/clp_s/search/ast/SetDateLiteralPrecision.hpp (1)
expr(21-21)
components/core/src/clp_s/search/kql/kql.cpp (4)
components/core/src/clp_s/search/ast/DateLiteral.cpp (2)
create(20-22)create(20-20)components/core/src/clp_s/search/ast/NullLiteral.cpp (2)
create(4-6)create(4-4)components/core/src/clp_s/TimestampPattern.cpp (2)
parse_timestamp(351-910)parse_timestamp(351-356)components/core/src/clp_s/search/ast/SearchUtils.hpp (1)
unescape_kql_value(60-60)
components/core/src/clp_s/search/AddTimestampConditions.cpp (2)
components/core/src/clp_s/search/ast/DateLiteral.cpp (2)
create(20-22)create(20-20)components/core/src/clp_s/search/ast/FilterExpr.cpp (4)
create(13-22)create(13-18)create(24-36)create(24-30)
🪛 Cppcheck (2.18.0)
components/core/src/clp_s/search/EvaluateTimestampIndex.cpp
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
components/core/src/clp_s/kv_ir_search.cpp
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[style] 31-31: The function 'invert' is never used.
(unusedFunction)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[style] 41-41: The function 'get_print_stream' is never used.
(unusedFunction)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
components/core/src/clp_s/search/ast/SetDateLiteralPrecision.cpp
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[style] 26-26: The function 'is_inverted' is never used.
(unusedFunction)
[style] 31-31: The function 'invert' is never used.
(unusedFunction)
components/core/src/clp_s/clp-s.cpp
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[style] 28-28: The function 'read_value' is never used.
(unusedFunction)
[style] 31-31: The function 'invert' is never used.
(unusedFunction)
[style] 31-31: The function 'set_next_stage' is never used.
(unusedFunction)
[style] 30-30: The function 'get_type' is never used.
(unusedFunction)
[style] 28-28: The function 'get_key' is never used.
(unusedFunction)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
components/core/src/clp_s/search/kql/kql.cpp
[information] 1-1: Include file
(missingIncludeSystem)
[information] 11-11: Include file
(missingIncludeSystem)
[style] 83-83: The function 'get_operation' is never used.
(unusedFunction)
[style] 88-88: The function 'get_column' is never used.
(unusedFunction)
[style] 102-102: The function 'as_array' is never used.
(unusedFunction)
[style] 175-175: The function 'descriptor_begin' is never used.
(unusedFunction)
[style] 177-177: The function 'descriptor_end' is never used.
(unusedFunction)
[style] 243-243: The function 'get_literal_type' is never used.
(unusedFunction)
[style] 248-248: The function 'get_matching_types' is never used.
(unusedFunction)
[style] 260-260: The function 'is_pure_wildcard' is never used.
(unusedFunction)
components/core/src/clp_s/search/AddTimestampConditions.cpp
[style] 46-46: The function 'as_epoch_date' is never used.
(unusedFunction)
[style] 44-44: The function 'op_end' is never used.
(unusedFunction)
components/core/src/clp_s/search/ast/DateLiteral.cpp
[information] 5-5: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[style] 20-20: The function 'create' is never used.
(unusedFunction)
[style] 28-28: The function 'as_int' is never used.
(unusedFunction)
[style] 33-33: The function 'as_float' is never used.
(unusedFunction)
[style] 53-53: The function 'set_default_precision' is never used.
(unusedFunction)
[style] 46-46: The function 'as_epoch_date' is never used.
(unusedFunction)
[style] 26-26: The function 'is_inverted' is never used.
(unusedFunction)
[style] 31-31: The function 'invert' is never used.
(unusedFunction)
[style] 42-42: The function 'op_begin' is never used.
(unusedFunction)
[style] 44-44: The function 'op_end' is never used.
(unusedFunction)
[style] 54-54: The function 'get_op_list' is never used.
(unusedFunction)
[style] 43-43: The function 'get_num_operands' is never used.
(unusedFunction)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: package-image
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-15)
🔇 Additional comments (24)
components/core/cmake/Options/options.cmake (1)
337-343: KQL now correctly declares a hard dependency onclp_s::timestamp_parser.Adding
CLP_BUILD_CLP_S_TIMESTAMP_PARSERtovalidate_clp_s_search_kql_dependenciescleanly enforces the new runtime dependency: builds will now fail early if KQL search is enabled without the timestamp parser, while letting the timestamp parser’s own validate/set functions manage its underlyingCLP_NEED_*flags. This is consistent with the existing dependency pattern elsewhere in this file.components/core/src/clp_s/search/AddTimestampConditions.hpp (1)
17-18: LGTM!The clarification that begin and end timestamps are in epoch milliseconds improves documentation and aligns with the updated DateLiteral API.
components/core/src/clp_s/search/kql/CMakeLists.txt (1)
35-35: LGTM!Adding
clp_s::timestamp_parseras a PRIVATE dependency is appropriate for supporting the newtimestamp()KQL expression parsing.components/core/src/clp_s/search/ast/CMakeLists.txt (1)
38-39: LGTM!Adding the new
SetDateLiteralPrecisionsource files to the build is correct and integrates the new transformation pass into the AST library.components/core/src/clp_s/search/EvaluateTimestampIndex.cpp (1)
113-120: LGTM!The special handling for
DateLiteralcorrectly converts to millisecond precision before evaluation, with appropriate fallback to the originalas_intpath for other literal types.components/core/src/clp_s/search/AddTimestampConditions.cpp (3)
22-24: LGTM!The
cNanosecondsInMillisecondconstant is correctly defined with explicitLLsuffixes to ensure 64-bit arithmetic.
44-47: LGTM!The conversion from epoch milliseconds to nanoseconds correctly aligns with the new
DateLiteralAPI that expects nanosecond-precision timestamps.
50-53: LGTM!The end timestamp conversion mirrors the begin timestamp logic and correctly constructs the upper bound filter.
components/core/src/clp_s/timestamp_parser/CMakeLists.txt (1)
20-21: Verify that PUBLIC linkage is necessary for the public API.Moving
ystdlib::error_handlingto PUBLIC linkage makes it transitively available to all consumers ofclp_s_timestamp_parser. Confirm that error handling types are actually exposed in the public headers of this library, as unnecessary PUBLIC linkage increases coupling and violates the principle of minimal exposure.components/core/tests/test-kql.cpp (1)
361-375: Thorough coverage of invalid timestamp expressions.The test cases cover various malformed inputs including:
- Missing arguments
- Unclosed parentheses/quotes
- Unquoted numeric arguments
- Unparseable timestamp strings
- Pattern mismatches
- Malformed patterns
components/core/src/clp_s/search/ast/SetDateLiteralPrecision.hpp (1)
15-26: LGTM! Clean transformation class design.The class correctly inherits from
Transformationand provides a clear interface. Minor observation: the default member initializer on line 25 ({DateLiteral::Precision::Nanoseconds}) is technically redundant since the explicit constructor on line 18 always initializesm_precision. However, it serves as useful documentation of the default and is harmless.components/core/src/clp_s/search/kql/kql.cpp (3)
82-146: Well-structured timestamp literal creation with proper error handling.The function correctly:
- Validates the timestamp context
- Handles both explicit patterns and default pattern fallback
- Logs meaningful error messages for all failure cases
- Returns nullptr on any parsing failure
Line 138 correctly uses
false == optional_timestamp.has_value()per coding guidelines.
289-296: Consider supporting timestamp expressions in pure value expressions.The
visitValue_expressionmethod handlesctx->literal()but doesn't appear to support timestamp expressions likevisitColumn_value_expressiondoes. If the grammar supportstimestamp("...")as a standalone value expression, this may need updating to maintain consistency.
261-287: Confirm that the grammar enforces exactly one oflitortimestampin thecolumn_range_expressionrule.The null check on line 270 safely handles cases where neither
ctx->litnorctx->timestampis set, but the grammar should use an explicitly required alternative pattern (A | Brather than(A | B)?) to ensure that one of these must always be present, eliminating the need for defensive null handling.components/core/src/clp_s/search/ast/DateLiteral.cpp (2)
9-13: LGTM! Clear constant definitions for time unit conversions.The constants are well-named and correctly cascade to avoid magic numbers. Using
LLsuffix ensures 64-bit arithmetic.
38-51: LGTM! Precision conversion is correct.The switch handles all enum cases and the default branch provides a safe fallback. Integer division correctly truncates towards zero for positive timestamps.
Note: For negative timestamps (pre-epoch), the integer division behaviour differs from floor division, which could affect timestamp comparisons near the epoch boundary. If pre-epoch timestamps are supported, consider whether truncation or floor is the desired semantic.
components/core/src/clp_s/search/kql/Kql.g4 (4)
39-41: LGTM! Newtimestamp_expressionrule is well-defined.The grammar correctly implements the
timestamp('TIMESTAMP' [, 'PATTERN'])syntax as described in the PR objectives. The optional second pattern argument is properly handled.
42-42: LGTM! Consolidatedliteralnonterminal.Clean abstraction combining
QUOTED_STRINGandUNQUOTED_LITERALinto a single nonterminal, which simplifies the grammar rules that reference it.
68-73: LGTM! ExtendedRANGE_OPERATORto include strict comparisons.Adding
<and>alongside<=and>=provides complete range operator coverage.
19-24: LGTM! Updated expression rules to support timestamp expressions.The
column_range_expressionandcolumn_value_expressionrules now correctly accept the newtimestamp_expressionalternative alongsideliteral, aligning with the PR's goal of supportingtimestamp()in KQL queries.components/core/src/clp_s/search/ast/DateLiteral.hpp (4)
29-34: LGTM! Simplified factory method.The single
create(epochtime_t v)factory method aligns with the PR objective of constructingDateLiteralfrom nanosecond-precision timestamps. The documentation clearly states the expected input format.
52-64: LGTM! Well-documented precision API.The
as_precisionandset_default_precisionmethods provide a clear interface for controlling timestamp precision. The documentation correctly notes thatas_precisionrounds towards zero.
73-76: LGTM! Private member initialization.The default member initializers (
{}) ensure safe initialization. The three members (m_timestamp,m_default_precision_timestamp,m_double_timestamp) appropriately cache different representations of the timestamp.
48-50: VerifyFilterOperationparameter usage inas_intandas_floatimplementations.Both methods accept a
FilterOperationparameter. Ensure the implementation handles different filter operations appropriately, particularly regarding rounding behavior for range comparisons and adherence to coding guidelines (preferfalse == <expression>over negation operators).
| bool DateLiteral::as_float(double& ret, FilterOperation op) { | ||
| ret = m_double_timestamp; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify: as_float always returns seconds, independent of precision setting.
The as_float method returns m_double_timestamp which is always in seconds (set in the constructor), while as_int returns the precision-adjusted value. This asymmetry may be intentional for floating-point timestamp comparisons, but it could be surprising. Consider adding a brief comment explaining this design choice.
🧰 Tools
🪛 Cppcheck (2.18.0)
[style] 33-33: The function 'as_float' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
In components/core/src/clp_s/search/ast/DateLiteral.cpp around lines 33 to 36,
the as_float method unconditionally returns m_double_timestamp (seconds) while
as_int applies precision; add a short explanatory comment above the as_float
implementation stating that floating-point timestamps are intentionally returned
in seconds (precision-neutral) to preserve fractional-second comparisons and to
contrast this behavior with as_int which applies precision scaling, so future
readers understand this asymmetry.
20001020ycx
left a comment
There was a problem hiding this comment.
I have reviewed everything except kql and tests, will look at it tmr morning. Mostly some nits and a few question for myself
| #include "Expression.hpp" | ||
|
|
||
| namespace clp_s::search::ast { | ||
| auto SetDateLiteralPrecision::run(std::shared_ptr<Expression>& expr) |
There was a problem hiding this comment.
Missing docstring, I think what you have in the pr description is clear:
Add SetDateLiteralPrecision transformation pass to set the precision of integer literals returned by DateLiteral::as_int to a set precision for all DateLiterals in the AST.
There was a problem hiding this comment.
Sure -- the transformation passes typically don't have a docstring for the run method, and instead leave the explanation for the class comment, but that's probably bad practice. I'll add a docstring.
|
|
||
| if (m_epoch_str.find(' ') != std::string::npos) { | ||
| return false; | ||
| auto DateLiteral::as_precision(Precision precision) -> epochtime_t { |
There was a problem hiding this comment.
Just to confirm my understanding, this method is used for comparison with different precision. After the optimization pass in AST (SetDateLiteralPercision), every leaf node with date literal is to be set in nanosecond, so this serves as the casting of the precision for comparison with other date literal with different precision?
Does this mean once we support nanosecond precision in both user queries and archives, this normalization step becomes unnecessary?
There was a problem hiding this comment.
Also, I didn't quite get what you said about this after reading the code:
I guess the other thing to note is that for stage 1/2 that you've outlined above all of the precisions are assumed/technically we don't actually know them.
So looks like we somehow know the precision in here?
There was a problem hiding this comment.
I think the first thing to note is that SetDateLiteralPrecision will change the precision to whatever you configure it to, which is millisecond precision for all of the use-cases in this PR.
To give full context, we do know the precision of the timestamp stored in the DateLiteral itself, since it's always epoch nanoseconds by construction. The as_precision method allows you to convert that nanosecond timestamp into a different precision epoch timestamp, which is useful for comparing against other epoch timestamps that aren't in epoch nanosecond precision.
For us, the need to do comparisons in precisions other than nanosecond comes up in the following three scenarios:
- Comparisons against timestamps in the timestamp range index -- by definition the start and end timestamps in this range are in millisecond precision (note that this isn't actually enforced in current archives though)
- Comparisons against timestamps stored in kv-ir -- these timestamps are almost always going to be in millisecond precision, though this technically isn't guaranteed by the spec
- Comparisons against timestamps stored in current archives -- when stored as
DateTimeorIntegerthey will often be millisecond precision, though are precisions are allowed
Basically in the current version of the code we're mostly certain about the precisions for the first two cases, and fairly uncertain in the 3rd case, though for all of these the best guess is millisecond precision.
Once we update the archive format the 3rd case is no longer an issue, and the undefined behaviour for the 1st case goes away (note that the range will still be stored in milliseconds, we'll just be actually ensuring that it is stored in milliseconds).
clp_s::timestamp_parser.clp_s::timestamp_parser:
Co-authored-by: ChenXing Yang <60459812+20001020ycx@users.noreply.github.com>
20001020ycx
left a comment
There was a problem hiding this comment.
LGTM! Passing to zhihao for final review
LinZhihao-723
left a comment
There was a problem hiding this comment.
Overall lgtm. Ignored some legacy format issues and only commented on the newly added code.
One question for date literal: does it make more sense if we name it TimetampLiteral?
- We call date-related specification "timestamp expression".
- We use
timestamp(xxx, xxx)to specify a date.
| public: | ||
| // Deleted copy | ||
| // Types | ||
| enum Precision : uint8_t { |
| * @return A Date Literal or nullptr if the string can not be parsed as date. | ||
| */ | ||
| static std::shared_ptr<Literal> create_from_string(std::string const& v); | ||
| static std::shared_ptr<Literal> create(epochtime_t v); |
There was a problem hiding this comment.
Since this is the factory function, shall we move it before constructors and after types?
Yeah I agree |
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
components/core/tests/test-kql.cpp (1)
356-360: Usefalse == ...instead ofREQUIRE_FALSE.Per coding guidelines, prefer explicit equality checks over
!<expression>orREQUIRE_FALSE().- REQUIRE_FALSE(filter->has_only_expression_operands()); - REQUIRE_FALSE(filter->is_inverted()); + REQUIRE(false == filter->has_only_expression_operands()); + REQUIRE(false == filter->is_inverted()); REQUIRE(filter->get_column()->is_pure_wildcard()); REQUIRE(filter->get_column()->get_namespace().empty()); - REQUIRE_FALSE(filter->get_column()->get_subtree_type().has_value()); + REQUIRE(false == filter->get_column()->get_subtree_type().has_value());components/core/src/clp_s/search/ast/TimestampLiteral.hpp (1)
18-23: Consider assigning explicit values toPrecisionenum members.The enum ordering progresses from lowest to highest precision. If this ordering is relied upon for comparisons or serialization, explicit values would make the intent clearer and prevent accidental reordering.
- enum class Precision : uint8_t { - Seconds, - Milliseconds, - Microseconds, - Nanoseconds + enum class Precision : uint8_t { + Seconds = 0, + Milliseconds = 1, + Microseconds = 2, + Nanoseconds = 3 };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
components/core/src/clp/ffi/ir_stream/search/utils.cpp(2 hunks)components/core/src/clp/ffi/ir_stream/search/utils.hpp(1 hunks)components/core/src/clp_s/SchemaTree.cpp(1 hunks)components/core/src/clp_s/clp-s.cpp(2 hunks)components/core/src/clp_s/kv_ir_search.cpp(3 hunks)components/core/src/clp_s/search/AddTimestampConditions.cpp(2 hunks)components/core/src/clp_s/search/AddTimestampConditions.hpp(1 hunks)components/core/src/clp_s/search/EvaluateTimestampIndex.cpp(4 hunks)components/core/src/clp_s/search/QueryRunner.cpp(3 hunks)components/core/src/clp_s/search/ast/CMakeLists.txt(2 hunks)components/core/src/clp_s/search/ast/Integral.hpp(1 hunks)components/core/src/clp_s/search/ast/Literal.hpp(3 hunks)components/core/src/clp_s/search/ast/NarrowTypes.cpp(1 hunks)components/core/src/clp_s/search/ast/SetTimestampLiteralPrecision.cpp(1 hunks)components/core/src/clp_s/search/ast/SetTimestampLiteralPrecision.hpp(1 hunks)components/core/src/clp_s/search/ast/TimestampLiteral.cpp(1 hunks)components/core/src/clp_s/search/ast/TimestampLiteral.hpp(1 hunks)components/core/src/clp_s/search/kql/kql.cpp(12 hunks)components/core/tests/test-kql.cpp(3 hunks)taskfiles/lint.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/src/clp_s/search/AddTimestampConditions.hppcomponents/core/src/clp_s/search/ast/SetTimestampLiteralPrecision.hppcomponents/core/src/clp_s/search/ast/SetTimestampLiteralPrecision.cppcomponents/core/src/clp_s/search/ast/Integral.hppcomponents/core/src/clp/ffi/ir_stream/search/utils.hppcomponents/core/src/clp_s/search/kql/kql.cppcomponents/core/src/clp_s/kv_ir_search.cppcomponents/core/src/clp_s/clp-s.cppcomponents/core/tests/test-kql.cppcomponents/core/src/clp/ffi/ir_stream/search/utils.cppcomponents/core/src/clp_s/SchemaTree.cppcomponents/core/src/clp_s/search/ast/TimestampLiteral.hppcomponents/core/src/clp_s/search/ast/TimestampLiteral.cppcomponents/core/src/clp_s/search/ast/Literal.hppcomponents/core/src/clp_s/search/QueryRunner.cppcomponents/core/src/clp_s/search/EvaluateTimestampIndex.cppcomponents/core/src/clp_s/search/ast/NarrowTypes.cppcomponents/core/src/clp_s/search/AddTimestampConditions.cpp
🧠 Learnings (11)
📚 Learning: 2025-12-09T01:57:04.839Z
Learnt from: SharafMohamed
Repo: y-scope/clp PR: 1313
File: components/core/tests/test-SchemaSearcher.cpp:136-137
Timestamp: 2025-12-09T01:57:04.839Z
Learning: In C++ files under the components directory, keep forward declarations even when a definition follows in the same scope (such as in anonymous namespaces) to maintain consistency across multiple methods. This style reduces churn and aligns with the existing convention in this repository for both source and tests.
Applied to files:
components/core/src/clp_s/search/ast/SetTimestampLiteralPrecision.cppcomponents/core/src/clp_s/search/kql/kql.cppcomponents/core/src/clp_s/kv_ir_search.cppcomponents/core/src/clp_s/clp-s.cppcomponents/core/tests/test-kql.cppcomponents/core/src/clp/ffi/ir_stream/search/utils.cppcomponents/core/src/clp_s/SchemaTree.cppcomponents/core/src/clp_s/search/ast/TimestampLiteral.cppcomponents/core/src/clp_s/search/QueryRunner.cppcomponents/core/src/clp_s/search/EvaluateTimestampIndex.cppcomponents/core/src/clp_s/search/ast/NarrowTypes.cppcomponents/core/src/clp_s/search/AddTimestampConditions.cpp
📚 Learning: 2025-04-26T02:21:22.021Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 856
File: components/core/src/clp/ffi/ir_stream/search/utils.cpp:258-266
Timestamp: 2025-04-26T02:21:22.021Z
Learning: In the clp::ffi::ir_stream::search namespace, the design principle is that callers are responsible for type checking, not the called functions. If control flow reaches a function, input types should already be validated by the caller.
Applied to files:
components/core/src/clp/ffi/ir_stream/search/utils.hppcomponents/core/src/clp_s/kv_ir_search.cpp
📚 Learning: 2024-10-18T02:31:18.595Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 557
File: components/core/src/clp/ffi/ir_stream/utils.hpp:0-0
Timestamp: 2024-10-18T02:31:18.595Z
Learning: In `components/core/src/clp/ffi/ir_stream/utils.hpp`, the function `size_dependent_encode_and_serialize_schema_tree_node_id` assumes that the caller checks that `node_id` fits within the range of `encoded_node_id_t` before casting.
Applied to files:
components/core/src/clp/ffi/ir_stream/search/utils.hppcomponents/core/src/clp/ffi/ir_stream/search/utils.cpp
📚 Learning: 2024-10-07T21:38:35.979Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:581-627
Timestamp: 2024-10-07T21:38:35.979Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, within the `get_archive_node_id` method, throwing a string literal as an exception is acceptable practice.
Applied to files:
components/core/src/clp_s/search/kql/kql.cpp
📚 Learning: 2025-04-26T02:19:33.275Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 856
File: components/core/src/clp/ffi/ir_stream/search/utils.cpp:340-347
Timestamp: 2025-04-26T02:19:33.275Z
Learning: In the CLP codebase, the FilterOperation::EXISTS and FilterOperation::NEXISTS operations have specific semantics that differ from simply checking if a value exists. EXISTS always returns true and NEXISTS always returns false in the evaluate_filter_against_literal_type_value_pair function. The optional::has_value() is used specifically to differentiate empty objects from concrete values, not for existence checks in filter operations.
Applied to files:
components/core/src/clp_s/search/kql/kql.cppcomponents/core/src/clp_s/search/QueryRunner.cpp
📚 Learning: 2025-05-07T16:56:35.687Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 882
File: components/core/src/clp/ffi/ir_stream/search/QueryHandlerImpl.cpp:378-402
Timestamp: 2025-05-07T16:56:35.687Z
Learning: In the CLP search component, the `evaluate_wildcard_filter` function should return `AstEvaluationResult::Pruned` when `node_id_value_pairs` is empty, not `AstEvaluationResult::False`. Empty node sets should be treated as "undetermined" rather than definitive non-matches.
Applied to files:
components/core/src/clp_s/kv_ir_search.cppcomponents/core/src/clp_s/clp-s.cppcomponents/core/src/clp_s/search/QueryRunner.cpp
📚 Learning: 2025-06-02T18:22:24.060Z
Learnt from: gibber9809
Repo: y-scope/clp PR: 955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Applied to files:
components/core/src/clp_s/search/ast/CMakeLists.txt
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
components/core/src/clp_s/search/ast/CMakeLists.txt
📚 Learning: 2025-08-03T18:56:07.366Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Applied to files:
components/core/src/clp_s/search/ast/CMakeLists.txt
📚 Learning: 2025-06-27T01:49:15.724Z
Learnt from: sitaowang1998
Repo: y-scope/clp PR: 1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Applied to files:
components/core/src/clp_s/search/ast/CMakeLists.txt
📚 Learning: 2025-07-01T14:49:07.333Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/src/glt/SQLitePreparedStatement.hpp:4-4
Timestamp: 2025-07-01T14:49:07.333Z
Learning: In the CLP codebase, standard headers like `<cstdint>` and `<string>` in alphabetical order (as seen in components/core/src/glt/SQLitePreparedStatement.hpp) are correctly ordered and do not need adjustment.
Applied to files:
taskfiles/lint.yaml
🧬 Code graph analysis (9)
components/core/src/clp_s/search/ast/SetTimestampLiteralPrecision.hpp (1)
components/core/src/clp_s/search/ast/TimestampLiteral.hpp (2)
precision(60-60)precision(67-67)
components/core/src/clp_s/search/ast/SetTimestampLiteralPrecision.cpp (4)
components/core/src/clp_s/search/EvaluateTimestampIndex.cpp (2)
run(28-142)run(28-28)components/core/src/clp_s/search/AddTimestampConditions.cpp (2)
run(26-59)run(26-26)components/core/src/clp_s/search/ast/SetTimestampLiteralPrecision.hpp (1)
expr(25-25)components/core/src/clp_s/search/kql/kql.cpp (2)
expr(69-80)expr(69-72)
components/core/src/clp_s/search/kql/kql.cpp (5)
components/core/src/clp_s/search/ast/TimestampLiteral.hpp (2)
TimestampLiteral(34-34)TimestampLiteral(74-74)components/core/src/clp_s/search/ast/TimestampLiteral.cpp (3)
TimestampLiteral(15-18)create(20-22)create(20-20)components/core/src/clp_s/search/ast/NullLiteral.cpp (2)
create(4-6)create(4-4)components/core/src/clp_s/search/ast/SearchUtils.hpp (1)
unescape_kql_value(60-60)components/core/src/clp_s/search/Projection.hpp (1)
column(51-51)
components/core/src/clp_s/kv_ir_search.cpp (3)
components/core/src/clp_s/search/ast/SetTimestampLiteralPrecision.hpp (2)
SetTimestampLiteralPrecision(17-18)SetTimestampLiteralPrecision(17-17)components/core/src/clp_s/search/ast/TimestampLiteral.hpp (2)
TimestampLiteral(34-34)TimestampLiteral(74-74)components/core/src/clp_s/search/ast/TimestampLiteral.cpp (1)
TimestampLiteral(15-18)
components/core/src/clp_s/clp-s.cpp (6)
components/core/src/clp_s/search/ast/SetTimestampLiteralPrecision.hpp (1)
expr(25-25)components/core/src/clp_s/search/AddTimestampConditions.hpp (1)
expr(42-42)components/core/src/clp_s/search/EvaluateRangeIndexFilters.hpp (1)
expr(30-30)components/core/src/clp_s/search/ast/AndExpr.cpp (1)
expr(52-52)components/core/src/clp_s/search/EvaluateTimestampIndex.hpp (1)
expr(26-26)components/core/src/clp_s/search/SchemaMatch.hpp (2)
expr(27-27)expr(143-147)
components/core/src/clp_s/search/ast/TimestampLiteral.hpp (2)
components/core/src/clp_s/search/ast/Integral.hpp (12)
v(31-31)v(38-38)v(45-45)type(57-57)type(57-57)mask(59-59)mask(59-59)mask(61-63)mask(61-61)ret(67-67)ret(69-69)ret(71-71)components/core/src/clp_s/search/ast/Literal.hpp (22)
type(49-49)type(60-81)type(60-60)mask(51-51)mask(53-53)ret(90-90)ret(90-90)ret(92-92)ret(92-92)ret(94-94)ret(94-94)ret(96-96)ret(96-96)ret(98-98)ret(98-98)ret(102-104)ret(102-102)op(100-100)op(100-100)op(108-108)op(108-108)nodiscard(43-43)
components/core/src/clp_s/search/ast/TimestampLiteral.cpp (1)
components/core/src/clp_s/search/ast/TimestampLiteral.hpp (7)
TimestampLiteral(34-34)TimestampLiteral(74-74)v(31-31)ret(51-51)ret(53-53)precision(60-60)precision(67-67)
components/core/src/clp_s/search/EvaluateTimestampIndex.cpp (2)
components/core/src/clp_s/search/ast/TimestampLiteral.hpp (2)
TimestampLiteral(34-34)TimestampLiteral(74-74)components/core/src/clp_s/search/ast/TimestampLiteral.cpp (1)
TimestampLiteral(15-18)
components/core/src/clp_s/search/AddTimestampConditions.cpp (2)
components/core/src/clp_s/search/ast/TimestampLiteral.hpp (2)
TimestampLiteral(34-34)TimestampLiteral(74-74)components/core/src/clp_s/search/ast/TimestampLiteral.cpp (3)
TimestampLiteral(15-18)create(20-22)create(20-20)
🪛 Cppcheck (2.18.0)
components/core/src/clp_s/search/ast/SetTimestampLiteralPrecision.cpp
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[style] 26-26: The function 'is_inverted' is never used.
(unusedFunction)
[style] 31-31: The function 'invert' is never used.
(unusedFunction)
components/core/src/clp_s/search/kql/kql.cpp
[information] 1-1: Include file
(missingIncludeSystem)
[information] 11-11: Include file
(missingIncludeSystem)
[style] 26-26: The function 'is_inverted' is never used.
(unusedFunction)
[style] 83-83: The function 'get_operation' is never used.
(unusedFunction)
[style] 88-88: The function 'get_column' is never used.
(unusedFunction)
[style] 102-102: The function 'as_array' is never used.
(unusedFunction)
[style] 175-175: The function 'descriptor_begin' is never used.
(unusedFunction)
[style] 177-177: The function 'descriptor_end' is never used.
(unusedFunction)
[style] 243-243: The function 'get_literal_type' is never used.
(unusedFunction)
[style] 248-248: The function 'get_matching_types' is never used.
(unusedFunction)
[style] 260-260: The function 'is_pure_wildcard' is never used.
(unusedFunction)
components/core/src/clp_s/kv_ir_search.cpp
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[style] 31-31: The function 'invert' is never used.
(unusedFunction)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[style] 41-41: The function 'get_print_stream' is never used.
(unusedFunction)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
components/core/src/clp_s/clp-s.cpp
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[style] 190-190: The function 'timestamp_is_in_search_time_range' is never used.
(unusedFunction)
components/core/src/clp/ffi/ir_stream/search/utils.cpp
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[style] 282-282: The function 'has_node' is never used.
(unusedFunction)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
components/core/src/clp_s/search/ast/TimestampLiteral.cpp
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[style] 26-26: The function 'is_inverted' is never used.
(unusedFunction)
[style] 31-31: The function 'invert' is never used.
(unusedFunction)
[style] 42-42: The function 'op_begin' is never used.
(unusedFunction)
[style] 44-44: The function 'op_end' is never used.
(unusedFunction)
[style] 54-54: The function 'get_op_list' is never used.
(unusedFunction)
[style] 43-43: The function 'get_num_operands' is never used.
(unusedFunction)
[style] 20-20: The function 'create' is never used.
(unusedFunction)
[style] 28-28: The function 'as_int' is never used.
(unusedFunction)
[style] 33-33: The function 'as_float' is never used.
(unusedFunction)
[style] 53-53: The function 'set_default_precision' is never used.
(unusedFunction)
[style] 49-49: The function 'as_timestamp' is never used.
(unusedFunction)
components/core/src/clp_s/search/QueryRunner.cpp
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
components/core/src/clp_s/search/EvaluateTimestampIndex.cpp
[information] 4-4: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 7-7: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 7-7: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 7-7: Include file
(missingIncludeSystem)
[information] 8-8: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 7-7: Include file
(missingIncludeSystem)
[information] 8-8: Include file
(missingIncludeSystem)
[information] 9-9: Include file
(missingIncludeSystem)
[information] 10-10: Include file
(missingIncludeSystem)
[information] 11-11: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 7-7: Include file
(missingIncludeSystem)
[information] 9-9: Include file
(missingIncludeSystem)
[information] 10-10: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 7-7: Include file
(missingIncludeSystem)
[information] 8-8: Include file
(missingIncludeSystem)
[information] 9-9: Include file
(missingIncludeSystem)
[information] 10-10: Include file
(missingIncludeSystem)
[information] 11-11: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 3-3: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 7-7: Include file
(missingIncludeSystem)
[information] 8-8: Include file
(missingIncludeSystem)
[information] 9-9: Include file
(missingIncludeSystem)
[information] 10-10: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
[style] 65-65: The function 'as_timestamp' is never used.
(unusedFunction)
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
components/core/src/clp_s/search/ast/NarrowTypes.cpp
[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.
(normalCheckLevelMaxBranches)
components/core/src/clp_s/search/AddTimestampConditions.cpp
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 7-7: Include file
(missingIncludeSystem)
[information] 8-8: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 7-7: Include file
(missingIncludeSystem)
[information] 8-8: Include file
(missingIncludeSystem)
[information] 9-9: Include file
(missingIncludeSystem)
[information] 10-10: Include file
(missingIncludeSystem)
[information] 11-11: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 7-7: Include file
(missingIncludeSystem)
[information] 8-8: Include file
(missingIncludeSystem)
[information] 9-9: Include file
(missingIncludeSystem)
[information] 10-10: Include file
(missingIncludeSystem)
[information] 6-6: Include file
(missingIncludeSystem)
[information] 7-7: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[information] 4-4: Include file
(missingIncludeSystem)
[information] 5-5: Include file
(missingIncludeSystem)
[style] 51-51: The function 'wildcard' is never used.
(unusedFunction)
[style] 44-44: The function 'op_end' is never used.
(unusedFunction)
[style] 49-49: The function 'as_timestamp' is never used.
(unusedFunction)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-15)
- GitHub Check: lint-check (macos-15)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: rust-checks
🔇 Additional comments (28)
components/core/src/clp/ffi/ir_stream/search/utils.hpp (1)
39-46: Documentation correctly updated for unsupported timestamp literalsThe note that
LiteralType::TimestampTis unsupported in the current IR format is accurate and consistent with the new timestamp semantics in this PR. No further changes needed here.taskfiles/lint.yaml (1)
495-496: Aligns lint excludes with new AST literal filesAdding
TimestampLiteral.cpp/.hppto the clang-tidy exclude list is consistent with the otherclp_s/search/astliterals already listed here; nothing else to adjust.components/core/src/clp_s/search/ast/CMakeLists.txt (1)
16-17: Correctly wires new timestamp AST sources into buildIncluding
TimestampLiteral.*andSetTimestampLiteralPrecision.*inCLP_S_AST_SOURCESensures the new literal type and precision pass are part of theclp_s_search_astlibrary. This matches the rest of the PR’s timestamp refactor and keeps the build configuration coherent.Also applies to: 38-39
components/core/src/clp_s/search/AddTimestampConditions.hpp (1)
16-18: Doc comment now accurately reflects millisecond semanticsThe note that begin/end timestamps are in epoch milliseconds matches the implementation in
AddTimestampConditions.cpp(conversion to nanoseconds viacNanosecondsInMillisecond). This removes the previous ambiguity around time units.components/core/src/clp_s/search/ast/NarrowTypes.cpp (1)
79-81: Timestamp narrowing updated to new literal typeUsing
literal->as_timestamp()and droppingLiteralType::TimestampTon failure is consistent with the existing narrowing strategy for other types and with the newTimestampLiteral/TimestampTsemantics. No issues here.components/core/src/clp_s/SchemaTree.cpp (1)
30-31: NodeType::DateString → LiteralType::TimestampT mapping is consistentMapping
NodeType::DateStringtoLiteralType::TimestampTaligns schema typing with the new timestamp literal representation and keeps type information consistent for downstream search and matching.components/core/src/clp_s/clp-s.cpp (1)
33-35: Millisecond precision normalisation is correctly integrated in archive searchIncluding
SetTimestampLiteralPrecision.hpp/TimestampLiteral.hppand applyingSetTimestampLiteralPrecision{TimestampLiteral::Precision::Milliseconds}insearch_archiveensures allTimestampLiteralnodes are normalised to millisecond precision before schema matching and projection, while leaving timestamp-index evaluation semantics unchanged. This is consistent with the PR’s stated precision model.Also applies to: 186-189
components/core/src/clp_s/kv_ir_search.cpp (1)
31-32: KV‑IR search now normalises timestamp literal precision to millisecondsBringing in
SetTimestampLiteralPrecision/TimestampLiteraland runningSetTimestampLiteralPrecision{TimestampLiteral::Precision::Milliseconds}onqueryaligns the kv‑IR search path with the archive search pipeline, ensuring consistent millisecond semantics forTimestampLiteral::as_intand related operations before the query is handed to the IRQueryHandler.Also applies to: 41-42, 275-276
components/core/src/clp_s/search/ast/Integral.hpp (1)
65-65: LGTM! Rename aligns with the broader timestamp refactor.The
as_epoch_date()→as_timestamp()rename is consistent with the PR's refactoring ofEpochDateTtoTimestampTacross the AST layer.components/core/src/clp/ffi/ir_stream/search/utils.cpp (2)
282-282: LGTM! Consistent with theEpochDateT→TimestampTrename.The literal type bitmask for string nodes now correctly includes
TimestampTinstead of the oldEpochDateT.
366-368: LGTM! Switch case updated to useTimestampT.The case label is correctly renamed, and the behaviour (returning
LiteralTypeUnsupported) remains unchanged.components/core/src/clp_s/search/EvaluateTimestampIndex.cpp (3)
3-4: LGTM! Required includes and using declarations forTimestampLiteral.The additions properly support the new
TimestampLiteraltype used in the timestamp index evaluation logic.Also applies to: 12-12, 22-22
25-26: LGTM! Updated to usecTimestampTypesconsistently.The constant now includes
LiteralType::TimestampTinstead of the oldEpochDateT, and the column matching check correctly uses this updated constant.Also applies to: 65-65
114-124: LGTM! Correct handling ofTimestampLiteralfor millisecond-precision index.The logic correctly:
- Attempts to cast the literal to
TimestampLiteral- If successful, extracts the value in milliseconds using
as_precision()- Falls back to
as_int()for other literal types (e.g.,Integral)This ensures
TimestampLiteralvalues (stored internally as nanoseconds) are properly converted to milliseconds for comparison against the timestamp range index.components/core/tests/test-kql.cpp (2)
15-15: LGTM! Required includes forTimestampLiteraltesting.Also applies to: 24-24
367-381: LGTM! Good coverage for invalid timestamp expression rejection.The test cases appropriately cover various malformed timestamp expressions including missing parentheses, empty arguments, unquoted arguments, invalid timestamps, and mismatched patterns.
components/core/src/clp_s/search/AddTimestampConditions.cpp (1)
3-3: LGTM! Proper setup forTimestampLiteralusage.The
cNanosecondsInMillisecondconstant is correctly defined as1,000,000(10^6), matching the conversion factor from milliseconds to nanoseconds.Also applies to: 11-11, 19-19, 22-24
components/core/src/clp_s/search/ast/Literal.hpp (1)
24-24: LGTM!The renaming from
EpochDateTtoTimestampTandas_epoch_date()toas_timestamp()is consistent and aligns with the broader refactoring in this PR.Also applies to: 76-77, 106-106
components/core/src/clp_s/search/QueryRunner.cpp (3)
230-239: LGTM!The update from
LiteralType::EpochDateTtoLiteralType::TimestampTis consistent with the enum rename.
310-315: LGTM!Consistent with the
TimestampTrenaming.
1050-1053: LGTM!The
other_typesbitmask correctly includesLiteralType::TimestampTinstead of the oldEpochDateT.components/core/src/clp_s/search/ast/TimestampLiteral.cpp (3)
33-36:as_floatalways returns seconds precision regardless ofset_default_precision.
as_intreturns the value at the configured default precision, butas_floatalways returns the timestamp in seconds (computed once at construction). This asymmetry may lead to unexpected behaviour if callers expect both methods to respect the default precision setting.Please verify this is intentional. If
as_floatshould also respect the default precision, you could compute it dynamically:bool TimestampLiteral::as_float(double& ret, FilterOperation op) { - ret = m_double_timestamp; + ret = static_cast<double>(m_default_precision_timestamp); return true; }Alternatively, document the difference in behaviour in the header.
15-22: LGTM!The constructor and factory function are implemented correctly. The nanosecond conversion constants are accurate.
38-51: LGTM!The
as_precisionmethod correctly performs integer division to convert nanoseconds to the requested precision. Thedefaultcase handling is reasonable as a fallback.components/core/src/clp_s/search/kql/kql.cpp (3)
82-146: LGTM!The
create_timestamp_literalfunction properly handles both explicit patterns and default quoted patterns. Error handling is comprehensive with clear error messages. The past review comments have been addressed (nullptr checks use prefix style, dereference uses->first).
261-287: LGTM!The range expression handling correctly supports both regular literals and timestamp expressions, with proper null checking before use.
175-181: LGTM!The
visitLiteralmethod correctly handles both quoted and unquoted literals by delegating tocreate_literal.components/core/src/clp_s/search/ast/TimestampLiteral.hpp (1)
37-53: LGTM!The method overrides are correctly implemented following the same pattern as other literal classes (e.g.,
Integral). The bitwise AND operations for type matching are consistent with the codebase conventions.
There was a problem hiding this comment.
@20001020ycx can u do a follow up PR to fix clang-tidy warnings in this file (and the implementation)?
I think it might be a good small task for you to get familiar with the C++ coding standard
LinZhihao-723
left a comment
There was a problem hiding this comment.
The multiline PR title lgtm. However, don't forget to update the DateLiteral to TimestampLiteral in the multiline description.
…to take advantage of `clp_s::timestamp_parser`: (y-scope#1757) - Refactor `clp_s::search::ast::TimestampLiteral` to rely on nanosecond precision timestamps; do not allow `TimestampLiteral` to be interpreted as a string; do not parse timestamps while constructing `TimestampLiteral`. - Remove undocumented `date()` expression from KQL and replace it with `timestamp()` expression. - Clean up KQL grammar and parse tree visitor. - Ensure that `TimestampLiteral` operands are converted to millisecond precision for comparison against the timestamp range index. - Add `SetTimestampLiteralPrecision` transformation pass to set the precision of integer literals returned by `TimestampLiteral::as_int` to a set precision for all `TimestampLiteral`s in the AST. - Use `SetTimestampLiteralPrecision` to treat timestamps as millisecond precision for archive and kv-ir search. Co-authored-by: ChenXing Yang <60459812+20001020ycx@users.noreply.github.com> Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
…to take advantage of `clp_s::timestamp_parser`: (y-scope#1757) - Refactor `clp_s::search::ast::TimestampLiteral` to rely on nanosecond precision timestamps; do not allow `TimestampLiteral` to be interpreted as a string; do not parse timestamps while constructing `TimestampLiteral`. - Remove undocumented `date()` expression from KQL and replace it with `timestamp()` expression. - Clean up KQL grammar and parse tree visitor. - Ensure that `TimestampLiteral` operands are converted to millisecond precision for comparison against the timestamp range index. - Add `SetTimestampLiteralPrecision` transformation pass to set the precision of integer literals returned by `TimestampLiteral::as_int` to a set precision for all `TimestampLiteral`s in the AST. - Use `SetTimestampLiteralPrecision` to treat timestamps as millisecond precision for archive and kv-ir search. Co-authored-by: ChenXing Yang <60459812+20001020ycx@users.noreply.github.com> Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
…to take advantage of `clp_s::timestamp_parser`: (y-scope#1757) - Refactor `clp_s::search::ast::TimestampLiteral` to rely on nanosecond precision timestamps; do not allow `TimestampLiteral` to be interpreted as a string; do not parse timestamps while constructing `TimestampLiteral`. - Remove undocumented `date()` expression from KQL and replace it with `timestamp()` expression. - Clean up KQL grammar and parse tree visitor. - Ensure that `TimestampLiteral` operands are converted to millisecond precision for comparison against the timestamp range index. - Add `SetTimestampLiteralPrecision` transformation pass to set the precision of integer literals returned by `TimestampLiteral::as_int` to a set precision for all `TimestampLiteral`s in the AST. - Use `SetTimestampLiteralPrecision` to treat timestamps as millisecond precision for archive and kv-ir search. Co-authored-by: ChenXing Yang <60459812+20001020ycx@users.noreply.github.com> Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
clp_s::search::ast::TimestampLiteralto rely on nanosecond precision timestamps; do not allowTimestampLiteralto be interpreted as a string; do not parse timestamps while constructingTimestampLiteral.date()expression from KQL and replace it withtimestamp()expression.TimestampLiteraloperands are converted to millisecond precision for comparison against the timestamp range index.SetTimestampLiteralPrecisiontransformation pass to set the precision of integer literals returned byTimestampLiteral::as_intto a set precision for allTimestampLiterals in the AST.SetTimestampLiteralPrecisionto treat timestamps as millisecond precision for archive and kv-ir search.Description
This PR makes significant changes to the KQL grammar and AST in order to integrate
clp_s::timestamp_parser, and make sure that operations involving timestamps occur at well-defined precisions. These changes have been split out of a larger set of changes aimed at introducing a new timestamp column type that relies onclp_s::timestamp_parserwhile deprecatingclp_s::TimestampPatternand theDateStringcolumn type.Note that a large part of the diff in this PR comes from generated KQL code, which can be ignored by reviewers.
In the KQL grammar we remove the undocumented
date()expression that relied onclp_s::TimestampPattern, and replace it with a newtimestamp()expression with better defined semantics.The timestamp expression has the following grammar
'timestamp(' QUOTED_STRING ( ',' QUOTED_STRING )? ')', with the semantics that the first quoted string is a timestamp, and the second optional quoted string is a timestamp pattern. When a pattern isn't provided the parser will attempt to parse the provided timestamp using the default set of timestamp patterns fromtimestamp_parser::get_all_default_quoted_timestamp_patterns. The optional pattern follows the same semantics as the patterns accepted byclp_s::timestamp_parser.Documentation for the
timestamp()expression will be added in a separate PR.The
DateLiteralin the AST has been significantly simplified, as summarized below:as_int(effectively, the actual value we compare the timestamp column against in search).The
AddTimestampConditionsandEvaluateTimestampIndexpasses have been updated to account for these new semantics.The
SetDateLiteralPrecisionpass has been added so that we can set the timestamp precision we compare against at search time for the whole AST -- it is being used to ensure that timestamp comparisons for kv-ir streams and current archives happen in millisecond precision.This overall change is breaking for two reasons:
date()expression.DateLiteralis now correctly converted to millisecond precision for comparison against the timestamp range index certain situations involving undefined behaviour caused by the stored timestamps not actually being millisecond precision will now behave differently.Checklist
breaking change.
Validation performed
--tge,--tle, and comparisons againsttimestamp()expressions return results as expected for v0.4.1 archives when using theclp-sbinary.Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.