Adds lazy reader support for timestamps #623
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #623 +/- ##
==========================================
+ Coverage 81.25% 81.54% +0.28%
==========================================
Files 123 123
Lines 22715 23158 +443
Branches 22715 23158 +443
==========================================
+ Hits 18458 18885 +427
+ Misses 2554 2549 -5
- Partials 1703 1724 +21
☔ View full report in Codecov by Sentry. |
| assert_eq!( | ||
| reader.next()?.expect_value()?.read()?, | ||
| RawValueRef::Timestamp(Timestamp::with_year(2023).with_month(8).build()?) | ||
| ); |
There was a problem hiding this comment.
🗺️ This simple test confirms that the LazyRawAnyReader can surface timestamps as expected. More rigorous correctness testing happens later in the diff.
| /// Matches a timestamp offset of any format. | ||
| fn match_timestamp_offset(self) -> IonParseResult<'data, MatchedTimestampOffset> { | ||
| alt(( | ||
| value(MatchedTimestampOffset::Zulu, tag("Z")), |
There was a problem hiding this comment.
Would it have any affect on memory allocation or other operations if we added another branch here for +00:00? I.e.:
value(MatchedTimestampOffset::Zulu, tag("+00:00"))
There was a problem hiding this comment.
It might be a wash; it makes reading +00:00 cheaper at the expense of an extra branch in the general case. I've added the change for now, we can always revisit it later with benchmarks.
| let good_inputs = &[ | ||
| "2023T", | ||
| "2023-08T", | ||
| "2023-08-13", // T is optional for ymd |
There was a problem hiding this comment.
Wait, what? 🤯
How did I go so long without knowing this?
| // throughout. | ||
| let year_text = matched_input.slice(0, 4).as_text().unwrap(); | ||
| let year = u32::from_str(year_text).unwrap(); | ||
| let timestamp = Timestamp::with_year(year); |
There was a problem hiding this comment.
Nice timestamp builder you have there. 😉
src/lazy/text/buffer.rs
Outdated
| "2023-08-18T14:35:52.+24:30", // Out of bounds offset hour | ||
| "2023-08-18T14:35:52.+00:60", // Out of bounds offset minute |
There was a problem hiding this comment.
These both have a dot but no fractional digits, so the test case can pass even if the out of bounds offset is not correctly rejected.
Builds on outstanding PRs #609, #612, #613, #614, #616, #617, #619, #620, #621, and #622.
Adds support for reading text timestamps in the lazy reader.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.