feat(clp-s::timestamp_parser): Add support for marshalling timestamps. #1646
Conversation
WalkthroughAdds timestamp marshaling (date-time and numeric) and an absolute-subsecond helper, exposes Changes
Sequence Diagram(s)%% Accessible, subtle colours for clarity
sequenceDiagram
participant Test as Test Suite / Caller
participant Marshal as marshal_timestamp
participant DateFmt as marshal_date_time_timestamp
participant NumFmt as marshal_numeric_timestamp
participant Buf as Output Buffer
Test->>Marshal: marshal_timestamp(epoch, pattern, buffer)
alt pattern contains date-time specifiers
Marshal->>DateFmt: delegate(epoch, pattern, buffer)
Note right of DateFmt `#DDDDFF`: build Y/M/D, DOW,\n12/24-hour, AM/PM, min/sec,\nsubseconds (3/6/9/T), timezone
DateFmt->>Buf: append formatted string
DateFmt-->>Marshal: Result<void>
else pattern is numeric (E/L/C/N or subsecond)
Marshal->>NumFmt: delegate(epoch, pattern, buffer)
Note right of NumFmt `#DDFFDD`: compute epoch base,\nformat digits and subseconds,\nhandle negative epochs
NumFmt->>Buf: append formatted string
NumFmt-->>Marshal: Result<void>
else invalid pattern
Marshal-->>Test: InvalidTimestampPattern error
end
Marshal-->>Test: Result<void>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp (2)
⏰ 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). (14)
🔇 Additional comments (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/core/src/clp_s/timestamp_parser/TimestampParser.cpp(4 hunks)components/core/src/clp_s/timestamp_parser/TimestampParser.hpp(1 hunks)components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp(3 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/timestamp_parser/TimestampParser.hppcomponents/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cppcomponents/core/src/clp_s/timestamp_parser/TimestampParser.cpp
🧬 Code graph analysis (2)
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp (2)
components/core/src/clp_s/timestamp_parser/TimestampParser.cpp (2)
marshal_timestamp(1549-1557)marshal_timestamp(1549-1550)components/core/src/clp_s/timestamp_parser/TimestampParser.hpp (1)
marshal_timestamp(161-162)
components/core/src/clp_s/timestamp_parser/TimestampParser.cpp (1)
components/core/src/clp_s/timestamp_parser/TimestampParser.hpp (5)
nodiscard(42-42)nodiscard(44-47)nodiscard(49-51)nodiscard(53-53)pattern(29-30)
⏰ 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). (14)
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
🔇 Additional comments (3)
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp (1)
449-450: Good coverage of tricky datetime casesThe added
ExpectedParsingResultentries for 12‑hour midnight/noon and the 1895 pre‑epoch timestamp look consistent with the existing expectations and nicely pin down edge behaviour.Also applies to: 499-500
components/core/src/clp_s/timestamp_parser/TimestampParser.cpp (2)
232-236: Helper for absolute subsecond nanoseconds looks correct
extract_absolute_subsecond_nanoseconds’s use of modulo by 1e9 followed by taking the absolute value yields a non‑negative subsecond component strictly less than one second for both positive and negative timestamps, which is exactly what the numeric marshaler needs.Also applies to: 485-493
691-786: Numeric marshalling correctly mirrors parsing semantics, including negative timestampsThe numeric marshaler cleanly handles
\E,\L,\C,\Nby integer‑dividing by the appropriate power of ten, and usesextract_absolute_subsecond_nanosecondsto produce the correct 3/6/9/Tfractional parts for both positive and negative epochs. The explicitcase '\\'emitting a backslash also matches the documented\\literal behaviour.
| std::string marshalled_timestamp; | ||
| auto const marshal_result{marshal_timestamp( | ||
| expected_result.epoch_timestamp, | ||
| timestamp_pattern_result.value(), | ||
| marshalled_timestamp | ||
| )}; | ||
| REQUIRE_FALSE(marshal_result.has_error()); | ||
| REQUIRE(expected_result.timestamp == marshalled_timestamp); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Round‑trip tests are strong; consider also exercising patterns from search_known_timestamp_patterns
The new marshalling block effectively verifies that known (epoch, pattern) pairs round‑trip through marshal_timestamp. If you expect callers to marshal using patterns returned by search_known_timestamp_patterns (including those originating from CAT patterns), it may be worth adding similar assertions using the discovered pattern to catch divergences between “hand‑written” and discovered patterns.
🤖 Prompt for AI Agents
In components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp
around lines 530 to 537, the test verifies round‑trip marshalling using the
hardcoded expected pattern but does not exercise patterns returned by
search_known_timestamp_patterns; update the test to call
search_known_timestamp_patterns for the same input, obtain the discovered
pattern(s) (or the primary value()), use that discovered pattern to call
marshal_timestamp into a separate marshalled string, assert marshal_timestamp
succeeds, and REQUIRE that the marshalled result equals the expected timestamp
string (optionally iterate over all discovered patterns to assert each one
round‑trips).
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
The implementation lgtm (also I think the unit test should cover the actual functionality correctness)
Some small style changes
| */ | ||
| [[nodiscard]] auto marshal_date_time_timestamp( | ||
| epochtime_t timestamp, | ||
| TimestampPattern const& timestamp_pattern, |
There was a problem hiding this comment.
pattern to match the docstring? lol
(I prefer pattern for clarity
| */ | ||
| [[nodiscard]] auto marshal_numeric_timestamp( | ||
| epochtime_t timestamp, | ||
| TimestampPattern const& timestamp_pattern, |
There was a problem hiding this comment.
Same (also for the implementation)
| R"(\b \d \H:\M:\S UTC\z{+0130})", | ||
| 1'765'602'000'000'000} | ||
| 1'765'602'000'000'000}, | ||
| {"1895-11-20T21:55:46,010", R"(\Y-\m-\dT\H:\M:\S,\3)", -2'338'769'053'990'000'000} |
There was a problem hiding this comment.
Logs before semiconductor, lol
Co-authored-by: Lin Zhihao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/core/src/clp_s/timestamp_parser/TimestampParser.cpp(14 hunks)components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp(4 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/timestamp_parser/test/test_TimestampParser.cppcomponents/core/src/clp_s/timestamp_parser/TimestampParser.cpp
🧬 Code graph analysis (2)
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp (2)
components/core/src/clp_s/timestamp_parser/TimestampParser.cpp (2)
marshal_timestamp(1554-1562)marshal_timestamp(1554-1555)components/core/src/clp_s/timestamp_parser/TimestampParser.hpp (1)
marshal_timestamp(162-163)
components/core/src/clp_s/timestamp_parser/TimestampParser.cpp (1)
components/core/src/clp_s/timestamp_parser/TimestampParser.hpp (5)
nodiscard(42-42)nodiscard(44-47)nodiscard(49-51)nodiscard(53-53)pattern(29-30)
⏰ 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). (14)
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- 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-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (6)
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp (1)
531-538: Marshalling round-trip verification looks good.The marshalling verification correctly validates that timestamps can be round-tripped through
marshal_timestamp. Note that there is a past review comment suggesting to also exercise patterns returned bysearch_known_timestamp_patternsto catch divergences between hand-written and discovered patterns.components/core/src/clp_s/timestamp_parser/TimestampParser.cpp (5)
294-306: Zero-padding safety check is correct.The added validation correctly rejects strings that contain additional zero-padding after stripping the expected padding character. This prevents cases like
" 007"(with space padding) from being incorrectly parsed as valid when only" 7"or" 07"should be accepted.
494-501: Absolute subsecond extraction correctly handles negative timestamps.The implementation correctly extracts the absolute value of the subsecond fractional component for both positive and negative epoch timestamps. For negative timestamps, the modulo operation yields a negative (or zero) result, which is then negated to produce the absolute subsecond value.
503-691: Date-time marshalling implementation is thorough and correct.The implementation correctly handles all date-time format specifiers including:
- Year, month, day variations with proper zero/space padding
- 12-hour and 24-hour clock formats with correct modulo-12 logic for 12-hour clocks
- Subsecond precision (milliseconds, microseconds, nanoseconds) with proper scaling
- Variable-length subsecond formatting that strips trailing zeros
- Timezone offset handling
- Literal backslash escaping
The function adheres to the coding guideline of using
false == <expression>rather than!<expression>.
695-790: Numeric timestamp marshalling correctly handles all epoch precisions.The implementation correctly marshals numeric timestamps including:
- Different epoch precisions (seconds, milliseconds, microseconds, nanoseconds)
- Subsecond formatting with proper zero-padding
- Variable-length subsecond formatting that strips trailing zeros
- Correct handling of negative timestamps by using
extract_absolute_subsecond_nanoseconds- Literal backslash escaping
The function adheres to the coding guideline of using
false == <expression>rather than!<expression>.
1554-1562: Marshalling dispatcher is straightforward and correct.The dispatcher correctly routes marshalling requests to either
marshal_date_time_timestampormarshal_numeric_timestampbased on the pattern type. Error handling is properly propagated using the error handling macros.
components/core/src/clp_s/timestamp_parser/test/test_TimestampParser.cpp
Outdated
Show resolved
Hide resolved
2 participants
Description
This PR follows up on #1626, #1599, #1564, and #1385 by implementing marshalling for numeric and date-time timestamp patterns. The marshalling implementation currently uses
fmtto format some parts of a timestamp, which results in some extra copying of small strings; if this ends up being a performance problem we can switch to a custom implementation that elides these small copies.This PR may be followed up by one more PR to the
clp-s::timestamp_parserlibrary to improve support for parsing leap-seconds before a PR to fully integrateclp-s::timestamp_parserwith clp-s.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.