Skip to content
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

Jump to bottom

adds accessor methods for Timestamp #482

Merged
merged 6 commits into from
Mar 17, 2023
Merged

adds accessor methods for Timestamp #482

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
01517e6
adds accessors for Timestamp
desaikd Mar 14, 2023
52ddcd1
Merge branch 'main' of github.com:amzn/ion-rust into desaikd-timestam…
desaikd Mar 17, 2023
249db37
adds suggested changes
desaikd Mar 17, 2023
1e8f911
adds more tests and doc comment changes
desaikd Mar 17, 2023
8e6c891
add more suggested changes
desaikd Mar 17, 2023
f0b9ba7
Merge branch 'main' into desaikd-timestamp-accessor
desaikd Mar 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
168 changes: 168 additions & 0 deletions src/types/timestamp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,66 @@ impl Timestamp {
pub fn precision(&self) -> Precision {
self.precision
}

/// Returns the year that has been specified in the [Timestamp].
desaikd marked this conversation as resolved.
Show resolved Hide resolved
pub fn year(&self) -> i32 {
self.date_time.year()
}

/// Returns the month that has been specified in the [Timestamp].
pub fn month(&self) -> u32 {
self.date_time.month()
}

/// Returns the day that has been specified in the [Timestamp].
pub fn day(&self) -> u32 {
self.date_time.day()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For year, month, and day, I believe the current implementation always returns the UTC date. We'll need to offer local and utc_ versions of each of those as well. (Consider 11PM on New Year's Eve in NYC, when it's already New Year's day in London.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added to_utc for Timestamp, removed all utc_* methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

self.date_time.day() is the UTC day. In order to return the local day, you have to localize the year/month/day fields of self.date_time to accommodate the new year's eve case I described above.

If the Timestamp doesn't have an offset because its precision is too low, you can use the current values.


/// Returns the hour(s) that has been specified in the [Timestamp].
pub fn hour(&self) -> u32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you thought about returning an Option from these functions so that they can reflect the precision of the timestamp?
E.g.

/// Returns the hour(s) that has been specified in the [Timestamp] if the timestamp's precision 
/// includes hours. If you need hour, regardless of precision, use `hour().unwrap_or_default()`,
/// which will correctly return an hour of `0` when the precision does not include hours.
pub fn hour(&self) -> Option<u32> {
    // ...
}

I think it makes it match the data model a little better. It's a little more verbose when you always want some value, but it's a little more ergonomic when you only want to deal with the subfields that are included by the timestamp's precision. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good observation. I'm unsure which way to go on it; Ion Timestamps always have a hour/minutes/second in that they represent a fixed point in time and those fields are 0 when the precision is too low. However, this does represent an opportunity to remind users that the 0 might have been implicit rather than explicitly part of the value.

I suspect that as a generalized datetime value, providing field defaults is probably what users would prefer. @jpschorr, what's your take on this? Would you rather check the timestamp's precision, or hour().is_none()?

Copy link
Contributor Author

@desaikd desaikd Mar 17, 2023 •
edited

Choose a reason for hiding this comment

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

+ 1. I think it would rather simple to check precision instead of hour().is_none().

// verify if the timestamp has an offset
if let Some(offset) = self.offset {
// `NativeDateTime#hours()` returns hours normalized as per UTC
// for local time we need to +/- the difference
return (self.date_time.hour() as i32 + (offset.local_minus_utc() / 3600)) as u32;
desaikd marked this conversation as resolved.
Show resolved Hide resolved
}
self.date_time.hour()
}

/// Returns the minute(s) that has been specified in the [Timestamp].
pub fn minute(&self) -> u32 {
// verify if the timestamp has an offset
if let Some(offset) = self.offset {
// `NativeDateTime#hours()` returns minutes normalized as per UTC
desaikd marked this conversation as resolved.
Show resolved Hide resolved
// for local time we need to +/- the difference
let local_minus_utc_minute = (offset.local_minus_utc() % 3600) / 60;
return (self.date_time.minute() as i32 + local_minus_utc_minute) as u32;
}
self.date_time.minute()
}

/// Returns the second(s) that has been specified in the [Timestamp].
pub fn second(&self) -> u32 {
self.date_time.second()
}

/// Returns the UTC hour(s) that has been specified in the [Timestamp].
pub fn utc_hour(&self) -> u32 {
desaikd marked this conversation as resolved.
Show resolved Hide resolved
self.date_time.hour()
}

/// Returns the UTC minute(s) that has been specified in the [Timestamp].
pub fn utc_minute(&self) -> u32 {
self.date_time.minute()
}

/// Returns the UTC second(s) that has been specified in the [Timestamp].
// this will be similar to `second` method because `Timestamp` creation takes minutes offset
// i.e. we can not put an offset with second precision and hence no conversion for offset would be needed
desaikd marked this conversation as resolved.
Show resolved Hide resolved
pub fn utc_second(&self) -> u32 {
self.date_time.second()
}
}

/// Formats an ISO-8601 timestamp of appropriate precision and offset.
Expand Down Expand Up @@ -1584,6 +1644,114 @@ mod timestamp_tests {
Ok(())
}

#[test]
fn test_timestamp_year() -> IonResult<()> {
let timestamp = Timestamp::with_year(2021).with_month(2).build()?;
assert_eq!(timestamp.year(), 2021);
Ok(())
}

#[test]
fn test_timestamp_month() -> IonResult<()> {
let timestamp = Timestamp::with_year(2021).with_month(2).build()?;
assert_eq!(timestamp.month(), 2);
Ok(())
}

#[test]
fn test_timestamp_day() -> IonResult<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need some tests for the year/month/day accessors where the Timestamp has high precision and an offset.

let timestamp_1 = Timestamp::with_year(2021).with_month(2).build()?;
assert_eq!(timestamp_1.day(), 1);

let timestamp_2 = Timestamp::with_year(2021)
.with_month(2)
.with_day(4)
.build()?;

assert_eq!(timestamp_2.day(), 4);
Ok(())
}

#[test]
fn test_timestamp_hour() -> IonResult<()> {
desaikd marked this conversation as resolved.
Show resolved Hide resolved
let timestamp_1 =
Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(-5 * 60)?;
assert_eq!(timestamp_1.hour(), 10);

let timestamp_2 =
Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(5 * 60)?;
assert_eq!(timestamp_2.hour(), 10);

let timestamp_3 = Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(0)?;
assert_eq!(timestamp_3.hour(), 10);
Ok(())
}

#[test]
fn test_timestamp_minute() -> IonResult<()> {
let timestamp_1 = Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(-90)?;
assert_eq!(timestamp_1.minute(), 15);

let timestamp_2 =
Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(-5 * 60)?;
assert_eq!(timestamp_2.minute(), 15);

let timestamp_3 =
Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(5 * 60)?;
assert_eq!(timestamp_3.minute(), 15);

let timestamp_4 = Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(0)?;
assert_eq!(timestamp_4.minute(), 15);
Ok(())
}

#[test]
fn test_timestamp_second() -> IonResult<()> {
let timestamp = Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(-5 * 60)?;
assert_eq!(timestamp.second(), 30);
Ok(())
}

#[test]
fn test_timestamp_utc_hour() -> IonResult<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

For each unit test, please include examples that test the unit's boundary across timezone offsets. (For example, in this hour test we see 5/10/15 -- we also need to verify the behavior when the result goes backwards or forwards across the day boundaries at 0/24.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unresolving this thread because I don't see tests that cross the day boundary. If the hour is 10, what happens when the offset is -11:00? Or if it's 15 and the offset is 10:00?

let timestamp_1 =
Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(-5 * 60)?;
assert_eq!(timestamp_1.utc_hour(), 15);

let timestamp_2 =
Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(5 * 60)?;
assert_eq!(timestamp_2.utc_hour(), 5);

let timestamp_3 = Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(0)?;
assert_eq!(timestamp_3.utc_hour(), 10);
Ok(())
}

#[test]
fn test_timestamp_utc_minute() -> IonResult<()> {
let timestamp_1 = Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(-90)?;
assert_eq!(timestamp_1.utc_minute(), 45);

let timestamp_2 =
Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(-5 * 60)?;
assert_eq!(timestamp_2.utc_minute(), 15);

let timestamp_3 =
Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(5 * 60)?;
assert_eq!(timestamp_3.utc_minute(), 15);

let timestamp_4 = Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(0)?;
assert_eq!(timestamp_4.utc_minute(), 15);
Ok(())
}

#[test]
fn test_timestamp_utc_second() -> IonResult<()> {
desaikd marked this conversation as resolved.
Show resolved Hide resolved
let timestamp = Timestamp::with_ymd_hms(2021, 4, 6, 10, 15, 30).build_at_offset(-5 * 60)?;
assert_eq!(timestamp.utc_second(), 30);
Ok(())
}

#[test]
fn test_timestamp_fractional_seconds_scale() -> IonResult<()> {
// Set fractional seconds as Decimal
Expand Down