You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Read integer %Y%m%d times without formatting them as text - #223
An integer time column declared with timestamp_format: "%Y%m%d" (values like 20240301) is read in RowAccess::timestamp_in by formatting each integer as a string (a heap allocation) and parsing it with chrono, first as a date-time, which always fails for a date-only format, and then as a date. That costs roughly 300 ns per value on a per-row path. Ingestion runs it for every row of such a column, for example when the column is an edge's sort key time. Temporal queries will also run it for every candidate row once they read declared formats (#205).
String columns with the same format already go through a byte-level fast parser (TimestampFormatParser), but integer columns never reached it.
The fix: before the text round-trip, an eight-digit integer under the %Y%m%d parser is split into year, month and day arithmetically and checked with chrono's NaiveDate::from_ymd_opt. Everything else takes the existing text path unchanged: other formats, values that are not eight digits, and invalid dates such as 20231345 or 20230229. The existing string fast parser parse_compact_ymd_fast is not reused because it does not validate month or day (it accepts month 13).
Synthetic users -> events graph with 10M events and an int64 t_ymd column declared %Y%m%d, on a 64-core box using release wheels. Each figure is a median of 5 runs.
Case
main
this PR
Ingest with t_ymd as the edge sort key time
6.5 s
4.9 s
Temporal queries on main read this column as epoch seconds and never reach this path. With #205 merged locally underneath, the cases that read the column from row data speed up. The sort-key path doesn't parse it, so that case is unchanged. These temporal numbers come from a single round and may be off by a few percent.
query_temporal_lengths, time column not the sort key
3292 ms
1882 ms
query_temporal, time column is the sort key
2036 ms
1577 ms
query_temporal_lengths, time column is the sort key
992 ms
987 ms
Back-compat
Results are identical. chrono's %Y%m%d reads exactly four year digits, then two month digits and two day digits, and accepts the value exactly when from_ymd_opt does. So the new path returns a value exactly when the old one did, with the same seconds. A sweep over all 90,000,000 eight-digit values against chrono 0.4.45 found no mismatches. Shorter values that chrono also reads as dates (for example the seven-digit 2024031, which it reads as 2024-03-01) still take the old path.
Indexes ingested on main and on this branch return the same temporal results for every user at three cutoffs: the same lengths, row ids and times, through the sort-key path. Their files are not byte-comparable because ingest output differs from run to run on main too.
Testing Done
New integer_yyyymmdd_matches_text_parse compares the result against the text parse (falling back to the epoch reading when it fails) over valid and invalid months and days in six years, plus 7-, 9-digit, zero, negative and i64 extreme values.
cargo test -p diskgraph-rowpack (319 tests), the timestamp and date tests of cargo test -p diskgraph-ingest, and cargo clippy -p diskgraph-rowpack --all-targets -- -D warnings pass.
Hotfix Context
Not a hotfix or backport.
Checklist
I listed all the breaking changes introduced by the PR: none.
I made sure to update examples and documentation with my changes: not needed.
I have added and modified unit tests required to cover these changes.
An integer time column declared with timestamp_format "%Y%m%d" was
formatted as a string and parsed by chrono for every row. Eight-digit
values that form a valid date now go straight to the date; everything
else takes the existing text parse, so results are unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Observed failure: reading an integer %Y%m%d time column formats every value as a string and parses it with chrono, about 300 ns per row in ingestion and temporal queries.
Existing behavior:RowAccess::timestamp_in sends integers with a format through value.to_string() and parse_timestamp_text_parts, falling back to an epoch reading when that fails. String columns already use the compiled TimestampFormatParser.
Smallest sufficient change: for the compiled %Y%m%d parser only, read eight-digit integers that form a valid date arithmetically, before the text round-trip. All other values keep the existing path.
Focused verification: a unit test compares against the text parse over valid and invalid dates and edge values; an exhaustive sweep of all eight-digit values found no mismatches; indexes ingested on main and on this branch return identical temporal results.
Stopping condition: integer %Y%m%d values no longer allocate or call chrono's string parser, with results unchanged. Other formats are out of scope.
Reviewing files that changed from the base of the PR and between 9ec4b54 and 8797d26.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
No actionable comments were generated in the recent review.
Walkthrough
When a format hint is present, timestamp_in first tries to parse an eligible eight-digit integer as a %Y%m%d date. Valid dates convert to midnight UTC epoch seconds. Other values continue through the existing formatted-text parsing and epoch-unit fallback. A unit test compares hinted integer parsing with text parsing and parsing without a hint across valid and invalid inputs.
Merge Risk: Minimal · previous review 9ec4b
No merge-blocking issue was identified in the integer date fast path.
Pre-merge checks: 2 · 1
Failed checks: 1 inconclusive
Check name
Status
Explanation
Resolution
Change Map Design Review
The reviewed code is available and shows a focused three-file change: TimestampFormatParser::parse_compact_ymd_int validates eight-digit %Y%m%d integers, RowAccess::timestamp_in uses it before t…
Provide the author-authored change-map conversation comment, including its table and the required failure, existing-behavior, smallest-change, verification, and stopping-condition details. Then reassess each proposed change against the insp…
Passed checks: 2 passed
Check name
Status
Explanation
Pr Size
The pull request changes 3 files. This count does not exceed 40.
Public Resource Access
The pull request adds no URL, repository location, or package source location. The changed lines only add Rust code, comments, and unit tests. No dependency or package manifest changes were made, so t…
Full details: Public Resource Access
Explanation
The pull request adds no URL, repository location, or package source location. The changed lines only add Rust code, comments, and unit tests. No dependency or package manifest changes were made, so there is no external resource to verify.
Full details: Change Map Design Review
Explanation
The reviewed code is available and shows a focused three-file change: TimestampFormatParser::parse_compact_ymd_int validates eight-digit %Y%m%d integers, RowAccess::timestamp_in uses it before the existing text path, and a parity test covers fallback cases. However, the required PR conversation comment beginning <!-- kumo-pr-change-map --> is not present in the supplied context or repository. Therefore, I cannot verify the proposed design's per-change rationale, observed failure, focused verification, optional refinements, or stopping condition in that same comment.
Resolution
Provide the author-authored change-map conversation comment, including its table and the required failure, existing-behavior, smallest-change, verification, and stopping-condition details. Then reassess each proposed change against the inspected diff.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitHub Actions
Inactive
Summary
An integer time column declared with
timestamp_format: "%Y%m%d"(values like20240301) is read inRowAccess::timestamp_inby formatting each integer as a string (a heap allocation) and parsing it with chrono, first as a date-time, which always fails for a date-only format, and then as a date. That costs roughly 300 ns per value on a per-row path. Ingestion runs it for every row of such a column, for example when the column is an edge's sort key time. Temporal queries will also run it for every candidate row once they read declared formats (#205).String columns with the same format already go through a byte-level fast parser (
TimestampFormatParser), but integer columns never reached it.The fix: before the text round-trip, an eight-digit integer under the
%Y%m%dparser is split into year, month and day arithmetically and checked with chrono'sNaiveDate::from_ymd_opt. Everything else takes the existing text path unchanged: other formats, values that are not eight digits, and invalid dates such as20231345or20230229. The existing string fast parserparse_compact_ymd_fastis not reused because it does not validate month or day (it accepts month 13).Change map
#223 (comment)
Measured
Synthetic
users -> eventsgraph with 10M events and an int64t_ymdcolumn declared%Y%m%d, on a 64-core box using release wheels. Each figure is a median of 5 runs.t_ymdas the edge sort key timeTemporal queries on main read this column as epoch seconds and never reach this path. With #205 merged locally underneath, the cases that read the column from row data speed up. The sort-key path doesn't parse it, so that case is unchanged. These temporal numbers come from a single round and may be off by a few percent.
query_temporal, time column not the sort keyquery_temporal_lengths, time column not the sort keyquery_temporal, time column is the sort keyquery_temporal_lengths, time column is the sort keyBack-compat
Results are identical. chrono's
%Y%m%dreads exactly four year digits, then two month digits and two day digits, and accepts the value exactly whenfrom_ymd_optdoes. So the new path returns a value exactly when the old one did, with the same seconds. A sweep over all 90,000,000 eight-digit values against chrono 0.4.45 found no mismatches. Shorter values that chrono also reads as dates (for example the seven-digit2024031, which it reads as 2024-03-01) still take the old path.Indexes ingested on main and on this branch return the same temporal results for every user at three cutoffs: the same lengths, row ids and times, through the sort-key path. Their files are not byte-comparable because ingest output differs from run to run on main too.
Testing Done
integer_yyyymmdd_matches_text_parsecompares the result against the text parse (falling back to the epoch reading when it fails) over valid and invalid months and days in six years, plus 7-, 9-digit, zero, negative andi64extreme values.cargo test -p diskgraph-rowpack(319 tests), the timestamp and date tests ofcargo test -p diskgraph-ingest, andcargo clippy -p diskgraph-rowpack --all-targets -- -D warningspass.Hotfix Context
Not a hotfix or backport.
Checklist
🤖 Generated with Claude Code