fix(clp-s::log_converter): Make timezone offset optional in timezone regex (fixes #1952). by gibber9809 · Pull Request #2134 · y-scope/clp

gibber9809 · 2026-03-26T14:37:57Z

Description

This PR fixes an issue in log-converter where the timezone Z wasn't accepted as a valid timezone on its own since the timezone regex required a timezone offset in matching timezones.

The fix is simply to update the regex to make the offset part of the timezone optional.

Checklist

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

Validation performed

Processed the following dataset into log-converter:

2026-03-26T07:03:14,788Z INFO [main] One test line.
2026-03-26T07:03:17,222Z INFO [main] Another line.

Ingested the resulting converted log into clp-s using the --timestamp-key timestamp option.
Observed that the search query timestamp < timestamp("2026-03-26T07:03:17,222Z") returned the following result, as expected:

{"message":" INFO [main] One test line.\n","timestamp":"2026-03-26T07:03:14,788Z"}

Summary by CodeRabbit

Release Notes

Bug Fixes
- Improved timestamp parsing flexibility in log conversion to better support varying fractional-seconds formats.

coderabbitai · 2026-03-26T14:38:37Z

Walkthrough

Modified the timestamp schema regex pattern in the log converter to make fractional seconds an optional component directly following the seconds field, rather than being conditionally tied to timezone or subsequent segments. This alters which timestamp formats are accepted during log parsing.

Changes

Cohort / File(s)	Summary
Timestamp Schema Pattern `components/core/src/clp_s/log_converter/LogConverter.cpp`	Updated `cTimestampSchema` regex to reposition the optional fractional-seconds component `([,\.:]\d{1,9}){0,1}` from being dependent on timezone/follow-on alternatives to being a direct optional modifier after the seconds field in the non-timezone timestamp pattern.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: making timezone offset optional in the timezone regex to fix issue `#1952`, which aligns with the PR's core objective of accepting standalone 'Z' timezone designators.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@components/core/src/clp_s/log_converter/LogConverter.cpp`:
- Around line 32-33: Add a unit test that verifies the regex in LogConverter
(the multi-line raw string literal that matches timestamps, including the Z
timezone branch) correctly accepts and parses timestamps ending with a bare "Z"
(e.g. "2023-12-01T12:34:56Z"); call the same public method used in production
(e.g., LogConverter::convertLine or LogConverter::parseTimestamp) with a sample
log containing a Z-only timezone and assert the timestamp is
recognized/converted to UTC and no error is produced; place the test alongside
other LogConverter tests so future changes to the regex literal will be caught
by CI.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: dd60f952-fc94-48d8-a9c4-24efc06c7edb

📥 Commits

Reviewing files that changed from the base of the PR and between dd0630e and 953d99f.

📒 Files selected for processing (1)

components/core/src/clp_s/log_converter/LogConverter.cpp

coderabbitai · 2026-03-26T14:41:21Z

+        R"((Dec(ember){0,1}))[ /\-]\d{2,4}))[ T:][ 0-9]{2}:[ 0-9]{2}:[ 0-9]{2}([,\.:]\d{1,9}){0,1})"
+        R"(([ ]{0,1}(UTC){0,1}([\+\-]\d{2}(:{0,1}\d{2}){0,1}){0,1}Z{0,1}){0,1}))"


🧹 Nitpick | 🔵 Trivial

Add a regression test for Z-only timezone timestamps.

The case is manually validated in the PR, but it should be covered by automated tests to avoid future regex regressions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@components/core/src/clp_s/log_converter/LogConverter.cpp` around lines 32 - 33, Add a unit test that verifies the regex in LogConverter (the multi-line raw string literal that matches timestamps, including the Z timezone branch) correctly accepts and parses timestamps ending with a bare "Z" (e.g. "2023-12-01T12:34:56Z"); call the same public method used in production (e.g., LogConverter::convertLine or LogConverter::parseTimestamp) with a sample log containing a Z-only timezone and assert the timestamp is recognized/converted to UTC and no error is produced; place the test alongside other LogConverter tests so future changes to the regex literal will be caught by CI.

LinZhihao-723 · 2026-03-26T14:42:05Z

+        R"((Dec(ember){0,1}))[ /\-]\d{2,4}))[ T:][ 0-9]{2}:[ 0-9]{2}:[ 0-9]{2}([,\.:]\d{1,9}){0,1})"
+        R"(([ ]{0,1}(UTC){0,1}([\+\-]\d{2}(:{0,1}\d{2}){0,1}){0,1}Z{0,1}){0,1}))"


iiuc, Z is a short term for UTC and it should be mutually exclusive to the UTC offset. Does it make more sense to write in this way (or similar)?

Suggested change

R"((Dec(ember){0,1}))[ /\-]\d{2,4}))[ T:][ 0-9]{2}:[ 0-9]{2}:[ 0-9]{2}([,\.:]\d{1,9}){0,1})"

R"(([ ]{0,1}(UTC){0,1}([\+\-]\d{2}(:{0,1}\d{2}){0,1}){0,1}Z{0,1}){0,1}))"

R"((Dec(ember){0,1}))[ /\-]\d{2,4}))[ T:][ 0-9]{2}:[ 0-9]{2}:[ 0-9]{2})"

R"(([,\.:]\d{1,9}){0,1}(([ ]{0,1}(UTC){0,1}[\+\-]\d{2}(:{0,1}\d{2}))|(Z)){0,1}))"

In practice it isn't mutually exclusive. We see timezones like -00Z in the wild. (Edited to a more common case).

…regex (fixes y-scope#1952). (y-scope#2134)

Make timezone offset optional in timezone regex.

953d99f

gibber9809 requested a review from a team as a code owner March 26, 2026 14:37

gibber9809 linked an issue Mar 26, 2026 that may be closed by this pull request

log-converter: Some timezones not extracted correctly. #1952

Closed

coderabbitai Bot reviewed Mar 26, 2026

View reviewed changes

LinZhihao-723 reviewed Mar 26, 2026

View reviewed changes

LinZhihao-723 approved these changes Mar 26, 2026

View reviewed changes

gibber9809 merged commit 7eba9fe into y-scope:main Mar 26, 2026
30 of 32 checks passed

junhaoliao added this to the March 2026 milestone Mar 26, 2026

junhaoliao mentioned this pull request May 11, 2026

docs: Revert removal of PostgreSQL dataset from resources documentation. #2272

Draft

3 tasks

junhaoliao pushed a commit to junhaoliao/clp that referenced this pull request May 17, 2026

fix(clp-s::log_converter): Make timezone offset optional in timezone …

6515a3a

…regex (fixes y-scope#1952). (y-scope#2134)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(clp-s::log_converter): Make timezone offset optional in timezone regex (fixes #1952).#2134

fix(clp-s::log_converter): Make timezone offset optional in timezone regex (fixes #1952).#2134
gibber9809 merged 1 commit into
y-scope:mainfrom
gibber9809:add-logconverter-timestamp-format

gibber9809 commented Mar 26, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Mar 26, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Mar 26, 2026

Uh oh!

LinZhihao-723 Mar 26, 2026

Uh oh!

gibber9809 Mar 26, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		R"((Dec(ember){0,1}))[ /\-]\d{2,4}))[ T:][ 0-9]{2}:[ 0-9]{2}:[ 0-9]{2}([,\.:]\d{1,9}){0,1})"
		R"(([ ]{0,1}(UTC){0,1}([\+\-]\d{2}(:{0,1}\d{2}){0,1}){0,1}Z{0,1}){0,1}))"

Conversation

gibber9809 commented Mar 26, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Validation performed

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai Bot commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

LinZhihao-723 Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

gibber9809 Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gibber9809 commented Mar 26, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 26, 2026 •

edited

Loading

gibber9809 Mar 26, 2026 •

edited

Loading