Skip to content

feat(clp-s): Implement part of the parsing side of the new timestamp_parser utility. #1385

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

feat(clp-s): Implement part of the parsing side of the new timestamp_parser utility. #1385

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
2ea3998
Add error code and error category for timestamp parsing.
gibber9809 Oct 2, 2025
4db78ef
Add clp_s::timestamp_parser build target
gibber9809 Oct 2, 2025
e79d682
Add support for subset of timestamp format specifiers.
gibber9809 Oct 2, 2025
fb0a3f0
Add validation for weekday.
gibber9809 Oct 3, 2025
10a5199
Move timestamp parser into dedicated subdirectory and namespace; fix …
gibber9809 Oct 3, 2025
8c26f52
Add basic test for parse_timestamp
gibber9809 Oct 3, 2025
377b176
Fix escaping bug in timestamp parser.
gibber9809 Oct 6, 2025
ba6d645
Fix bug in number parsing; add nodiscard tag to several functions.
gibber9809 Oct 6, 2025
3e22cbc
Add test cases for every implemented timestamp format specifier.
gibber9809 Oct 6, 2025
a843fec
Fix typo
gibber9809 Oct 6, 2025
8293c92
Add docstring for parse_timestamp
gibber9809 Oct 6, 2025
c673b7d
Merge remote-tracking branch 'upstream/main' into timestamp-parser
gibber9809 Oct 6, 2025
bdcd7b1
Address rabbit comment.
gibber9809 Oct 6, 2025
921f622
Address rabbit comments.
gibber9809 Oct 14, 2025
6e5915b
Address some review comments.
gibber9809 Oct 14, 2025
c247921
Update comments about result returns.
gibber9809 Oct 14, 2025
b21dbc4
Merge remote-tracking branch 'upstream/main' into timestamp-parser
gibber9809 Oct 14, 2025
a8222d0
Address most clang-tidy warnings.
gibber9809 Oct 14, 2025
7d0e335
Fix more clang-tidy warnings.
gibber9809 Oct 14, 2025
36ad265
Address rabbit comment.
gibber9809 Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions components/core/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,7 @@ set(SOURCE_FILES_clp_s_unitTest
src/clp_s/TimestampDictionaryWriter.hpp
src/clp_s/TimestampEntry.cpp
src/clp_s/TimestampEntry.hpp
src/clp_s/timestamp_parser/test/test_TimestampParser.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Protect test sources behind the build option.

If CLP_BUILD_CLP_S_TIMESTAMP_PARSER is OFF, this source file should not be compiled. Consider wrapping the addition with if(CLP_BUILD_CLP_S_TIMESTAMP_PARSER) to avoid missing-file errors when the feature is disabled.

 set(SOURCE_FILES_clp_s_unitTest
     ...
-    src/clp_s/timestamp_parser/test/test_TimestampParser.cpp
+    $<$<BOOL:${CLP_BUILD_CLP_S_TIMESTAMP_PARSER}>:src/clp_s/timestamp_parser/test/test_TimestampParser.cpp>
     ...
 )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
src/clp_s/timestamp_parser/test/test_TimestampParser.cpp
set(SOURCE_FILES_clp_s_unitTest
...
$<$<BOOL:${CLP_BUILD_CLP_S_TIMESTAMP_PARSER}>:src/clp_s/timestamp_parser/test/test_TimestampParser.cpp>
...
)
🤖 Prompt for AI Agents 
In components/core/CMakeLists.txt around line 409, the test source
src/clp_s/timestamp_parser/test/test_TimestampParser.cpp is unconditionally
listed which causes missing-file errors when CLP_BUILD_CLP_S_TIMESTAMP_PARSER is
OFF; wrap the addition of this source (or the whole target that references it)
in an if(CLP_BUILD_CLP_S_TIMESTAMP_PARSER) ... endif() block so the file is only
appended/added when the feature is enabled, or alternatively conditionally call
target_sources/add_executable only when that CMake option is TRUE.

src/clp_s/Utils.cpp
src/clp_s/Utils.hpp
src/clp_s/ZstdCompressor.cpp
Expand Down Expand Up @@ -750,6 +751,7 @@ if(CLP_BUILD_TESTING)
clp_s::search::ast
clp_s::search::kql
clp_s::search::sql
clp_s::timestamp_parser
clp_s::timestamp_pattern
date::date
fmt::fmt
Expand Down
24 changes: 24 additions & 0 deletions components/core/cmake/Options/options.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,12 @@ option(
ON
)

option(
CLP_BUILD_CLP_S_TIMESTAMP_PARSER
"Build clp_s::timestamp_parser"
ON
)
Comment on lines +86 to +90
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Add trailing period for option description.

All other option descriptions end in a period. Matching that pattern keeps help text tidy.

🤖 Prompt for AI Agents 
In components/core/cmake/Options/options.cmake around lines 86 to 90, the option
description string "Build clp_s::timestamp_parser" is missing a trailing period;
update the description to end with a period so it matches the punctuation style
of other options (e.g., "Build clp_s::timestamp_parser.").


option(
CLP_BUILD_CLP_S_TIMESTAMPPATTERN
"Build clp_s::timestamp_pattern."
Expand Down Expand Up @@ -359,6 +365,19 @@ function(set_clp_s_search_sql_dependencies)
)
endfunction()

function(validate_clp_s_timestamp_parser_dependencies)
validate_clp_dependencies_for_target(CLP_BUILD_CLP_S_TIMESTAMP_PARSER
CLP_BUILD_CLP_STRING_UTILS
)
endfunction()

function(set_clp_s_timestamp_parser_dependencies)
set_clp_need_flags(
CLP_NEED_DATE
CLP_NEED_YSTDLIB
)
endfunction()
Comment on lines +368 to +379
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Missing dependency on CLP_BUILD_CLP_S_TIMESTAMP_PATTERN.

The parser implementation pulls in ../Defs.hpp, which in turn depends on the timestamp pattern library. Please require CLP_BUILD_CLP_S_TIMESTAMPPATTERN here to avoid surprising build breaks when timestamp_pattern is disabled.

 function(validate_clp_s_timestamp_parser_dependencies)
     validate_clp_dependencies_for_target(CLP_BUILD_CLP_S_TIMESTAMP_PARSER
         CLP_BUILD_CLP_STRING_UTILS
+        CLP_BUILD_CLP_S_TIMESTAMPPATTERN
     )
 endfunction()

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In components/core/cmake/Options/options.cmake around lines 368–379, the
timestamp parser functions do not require the timestamp pattern library; add
CLP_BUILD_CLP_S_TIMESTAMP_PATTERN to the validate_clp_dependencies_for_target
call in validate_clp_s_timestamp_parser_dependencies and also add the
corresponding need flag (e.g. CLP_NEED_TIMESTAMP_PATTERN) to the
set_clp_need_flags list in set_clp_s_timestamp_parser_dependencies so the
timestamp_pattern library is enabled when the parser is built.


function(validate_clp_s_timestamppattern_dependencies)
validate_clp_dependencies_for_target(CLP_BUILD_CLP_S_TIMESTAMPPATTERN
CLP_BUILD_CLP_STRING_UTILS
Expand Down Expand Up @@ -443,6 +462,11 @@ function(validate_and_setup_all_clp_dependency_flags)
set_clp_s_search_sql_dependencies()
endif()

if (CLP_BUILD_CLP_S_TIMESTAMP_PARSER)
validate_clp_s_timestamp_parser_dependencies()
set_clp_s_timestamp_parser_dependencies()
endif()
Comment on lines +465 to +468
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Tests need the timestamp parser option enabled.

unitTest links against clp_s::timestamp_parser. If someone disables CLP_BUILD_CLP_S_TIMESTAMP_PARSER but leaves testing on, CMake will fail during configure. Please add CLP_BUILD_CLP_S_TIMESTAMP_PARSER to validate_clp_tests_dependencies so the option gate stays consistent.

 function(validate_clp_tests_dependencies)
     validate_clp_dependencies_for_target(CLP_BUILD_TESTING
         CLP_BUILD_CLP_REGEX_UTILS
         CLP_BUILD_CLP_STRING_UTILS
         CLP_BUILD_CLP_S_SEARCH_AST
         CLP_BUILD_CLP_S_SEARCH_KQL
         CLP_BUILD_CLP_S_SEARCH_SQL
         CLP_BUILD_CLP_S_TIMESTAMPPATTERN
+        CLP_BUILD_CLP_S_TIMESTAMP_PARSER
     )
 endfunction()
🤖 Prompt for AI Agents 
In components/core/cmake/Options/options.cmake around lines 465-468, tests
require the timestamp parser option but the validation function for tests
doesn't enforce CLP_BUILD_CLP_S_TIMESTAMP_PARSER; update
validate_clp_tests_dependencies to include a check that
CLP_BUILD_CLP_S_TIMESTAMP_PARSER is ON (or call the existing
validate_clp_s_timestamp_parser_dependencies) so that when tests are enabled the
timestamp parser option is required, and return a clear error message if it's
disabled.


if (CLP_BUILD_CLP_S_TIMESTAMPPATTERN)
validate_clp_s_timestamppattern_dependencies()
set_clp_s_timestamppattern_dependencies()
Expand Down
1 change: 1 addition & 0 deletions components/core/src/clp_s/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
add_subdirectory(indexer)
add_subdirectory(search)
add_subdirectory(timestamp_parser)

set(
CLP_S_CLP_SOURCES
Expand Down
25 changes: 25 additions & 0 deletions components/core/src/clp_s/timestamp_parser/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
set(
CLP_S_TIMESTAMP_PARSER_SOURCES
../Defs.hpp
TimestampParser.cpp
TimestampParser.hpp
ErrorCode.cpp
ErrorCode.hpp
)

if(CLP_BUILD_CLP_S_TIMESTAMP_PARSER)
add_library(
clp_s_timestamp_parser
${CLP_S_TIMESTAMP_PARSER_SOURCES}
)
add_library(clp_s::timestamp_parser ALIAS clp_s_timestamp_parser)
target_compile_features(clp_s_timestamp_parser PRIVATE cxx_std_20)
target_include_directories(clp_s_timestamp_parser PUBLIC ../../)
target_link_libraries(
clp_s_timestamp_parser
PRIVATE
clp::string_utils
date::date
ystdlib::error_handling
)
endif()
31 changes: 31 additions & 0 deletions components/core/src/clp_s/timestamp_parser/ErrorCode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#include "ErrorCode.hpp"

#include <string>

#include <ystdlib/error_handling/ErrorCode.hpp>

namespace {
using clp_s::timestamp_parser::ErrorCodeEnum;
using ErrorCategory = ystdlib::error_handling::ErrorCategory<ErrorCodeEnum>;
} // namespace

template <>
auto ErrorCategory::name() const noexcept -> char const* {
return "clp_s::timestamp_parser::ErrorCode";
}

template <>
auto ErrorCategory::message(ErrorCodeEnum error_enum) const -> std::string {
switch (error_enum) {
case ErrorCodeEnum::InvalidTimestampPattern:
return "Encountered timestamp pattern with invalid syntax.";
case ErrorCodeEnum::IncompatibleTimestampPattern:
return "Timestamp pattern does not match the format of the given timestamp.";
case ErrorCodeEnum::InvalidDate:
return "Timestamp was parsed successfully, but did not yield a valid date.";
case ErrorCodeEnum::FormatSpecifierNotImplemented:
return "Encountered unimplemented format specifier in timestamp pattern.";
default:
return "Unknown error code enum.";
}
}
21 changes: 21 additions & 0 deletions components/core/src/clp_s/timestamp_parser/ErrorCode.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#ifndef CLP_S_TIMESTAMPPARSERERRORCODE_HPP
#define CLP_S_TIMESTAMPPARSERERRORCODE_HPP
Copy link
Member

Choose a reason for hiding this comment

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

Same, this seems out dated.


#include <cstdint>

#include <ystdlib/error_handling/ErrorCode.hpp>

namespace clp_s::timestamp_parser {
enum class ErrorCodeEnum : uint8_t {
InvalidTimestampPattern = 1,
IncompatibleTimestampPattern,
InvalidDate,
FormatSpecifierNotImplemented
};

using ErrorCode = ystdlib::error_handling::ErrorCode<ErrorCodeEnum>;
} // namespace clp_s::timestamp_parser

YSTDLIB_ERROR_HANDLING_MARK_AS_ERROR_CODE_ENUM(clp_s::timestamp_parser::ErrorCodeEnum);

#endif // CLP_S_TIMESTAMPPARSERERRORCODE_HPP
Loading
Loading