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.

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

Temporal duration compare #186

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 72 additions & 2 deletions src/builtins/core/duration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use alloc::format;
use alloc::string::String;
use alloc::vec;
use alloc::vec::Vec;
use core::str::FromStr;
use core::{cmp::Ordering, str::FromStr};
use ixdtf::parsers::{
records::{DateDurationRecord, DurationParseRecord, Sign as IxdtfSign, TimeDurationRecord},
IsoDurationParser,
Expand Down Expand Up @@ -277,6 +277,75 @@ impl Duration {
pub fn is_time_within_range(&self) -> bool {
self.time.is_within_range()
}

/// Compares self with other on a field by field basis.
fn cmp_raw(&self, other: &Self) -> Option<Ordering> {
let date_ordering = self.date.partial_cmp(&other.date)?;
if date_ordering != Ordering::Equal {
return Some(date_ordering);
}
let time_ordering = self.time.partial_cmp(&other.time)?;
Some(time_ordering)
}

/// Returns the ordering between the two durations.
///
/// `7.2.3 Temporal.Duration.compare ( one, two [ , options ] )`
#[must_use]
pub fn compare(
Copy link
Member

Choose a reason for hiding this comment

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

question: Should compare take &self for the first Duration parameter or mimic the specification and have it be a standalone function on Duration?

This is more a question of what is the best way to translate compare in Rust vs. JavaScript. In the literal sense to the specification, this is implemented exactly as it should be. But in Rust where PartialOrd exists as a trait, it's common to have one.cmp(&two) conceptually. So is it best for this to be a method similar to PartialOrd, e.g. one.compare(&two, None)??

I've tended to prefer this approach, mostly, because I think it's nice to adhere to similarly structured syntax. But I could be convinced to this current implementation if others think it's bad to overload the concept of one.cmp(&two). The most important thing to be consistent across similar methods and functions in temporal_rs for the builtins.

one: &Duration,
two: &Duration,
relative_to: Option<RelativeTo>,
) -> TemporalResult<Ordering> {
if let Some(Ordering::Equal) = one.cmp_raw(two) {
return Ok(Ordering::Equal);
}
// 8. Let largestUnit1 be DefaultTemporalLargestUnit(one).
// 9. Let largestUnit2 be DefaultTemporalLargestUnit(two).
let largest_unit_1 = one.default_largest_unit();
let largest_unit_2 = two.default_largest_unit();
// 10. Let duration1 be ToInternalDurationRecord(one).
// 11. Let duration2 be ToInternalDurationRecord(two).
// 12. If zonedRelativeTo is not undefined, and either TemporalUnitCategory(largestUnit1) or TemporalUnitCategory(largestUnit2) is date, then
if let Some(RelativeTo::ZonedDateTime(zdt)) = relative_to.as_ref() {
if largest_unit_1.is_date_unit() || largest_unit_2.is_date_unit() {
// a. Let timeZone be zonedRelativeTo.[[TimeZone]].
// b. Let calendar be zonedRelativeTo.[[Calendar]].
// c. Let after1 be ? AddZonedDateTime(zonedRelativeTo.[[EpochNanoseconds]], timeZone, calendar, duration1, constrain).
// d. Let after2 be ? AddZonedDateTime(zonedRelativeTo.[[EpochNanoseconds]], timeZone, calendar, duration2, constrain).
let after1 = zdt.add(one, Some(ArithmeticOverflow::Constrain))?;
let after2 = zdt.add(two, Some(ArithmeticOverflow::Constrain))?;
// e. If after1 > after2, return 1𝔽.
// f. If after1 < after2, return -1𝔽.
// g. Return +0𝔽.
return Ok(after1.cmp(&after2));
Copy link
Member

Choose a reason for hiding this comment

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

note: this will probably need to be updated on a rebase / merge to main due to #175.

Copy link
Member

Choose a reason for hiding this comment

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

}
}
// 13. If IsCalendarUnit(largestUnit1) is true or IsCalendarUnit(largestUnit2) is true, then
let (days1, days2) =
if largest_unit_1.is_calendar_unit() || largest_unit_2.is_calendar_unit() {
// a. If plainRelativeTo is undefined, throw a RangeError exception.
// b. Let days1 be ? DateDurationDays(duration1.[[Date]], plainRelativeTo).
// c. Let days2 be ? DateDurationDays(duration2.[[Date]], plainRelativeTo).
let Some(RelativeTo::PlainDate(pdt)) = relative_to.as_ref() else {
return Err(TemporalError::range());
};
let days1 = one.date.days(pdt)?;
let days2 = two.date.days(pdt)?;
(days1, days2)
} else {
(
one.date.days.as_integer_if_integral()?,
two.date.days.as_integer_if_integral()?,
)
};
// 15. Let timeDuration1 be ? Add24HourDaysToTimeDuration(duration1.[[Time]], days1).
let time_duration_1 = one.time.to_normalized().add_days(days1)?;
// 16. Let timeDuration2 be ? Add24HourDaysToTimeDuration(duration2.[[Time]], days2).
let time_duration_2 = two.time.to_normalized().add_days(days2)?;
// 17. Return 𝔽(CompareTimeDuration(timeDuration1, timeDuration2)).
Ok(time_duration_1.cmp(&time_duration_2))
}
}

// ==== Public `Duration` Getters/Setters ====
Expand Down Expand Up @@ -323,7 +392,7 @@ impl Duration {
self.date.weeks
}

/// Returns the `weeks` field of duration.
/// Returns the `days` field of duration.
#[inline]
#[must_use]
pub const fn days(&self) -> FiniteF64 {
Expand Down Expand Up @@ -572,6 +641,7 @@ impl Duration {
self.weeks(),
self.days().checked_add(&FiniteF64::from(balanced_days))?,
)?;
// TODO: Should this be using AdjustDateDurationRecord?
Copy link
Member

Choose a reason for hiding this comment

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

Good catch! This implementation is from before some of the more recent specification changes. There's a good argument to be made that round should be updated to take advantage of the methods implemented in this PR.

That may be best scoped to another PR as that may include a a couple more changes in the code then specifically in round. I'll leave that up to you.


// c. Let targetDate be ? AddDate(calendarRec, plainRelativeTo, dateDuration).
let target_date = plain_date.add_date(&Duration::from(date_duration), None)?;
Expand Down
63 changes: 61 additions & 2 deletions src/builtins/core/duration/date.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
//! Implementation of a `DateDuration`

use crate::{primitive::FiniteF64, Sign, TemporalError, TemporalResult};
use crate::{
iso::iso_date_to_epoch_days, options::ArithmeticOverflow, primitive::FiniteF64, Duration,
PlainDate, Sign, TemporalError, TemporalResult,
};
use alloc::vec::Vec;

/// `DateDuration` represents the [date duration record][spec] of the `Duration.`
Expand All @@ -10,7 +13,7 @@ use alloc::vec::Vec;
/// [spec]: https://tc39.es/proposal-temporal/#sec-temporal-date-duration-records
/// [field spec]: https://tc39.es/proposal-temporal/#sec-properties-of-temporal-duration-instances
#[non_exhaustive]
#[derive(Debug, Default, Clone, Copy)]
#[derive(Debug, Default, Clone, Copy, PartialEq, PartialOrd)]
pub struct DateDuration {
/// `DateDuration`'s internal year value.
pub years: FiniteF64,
Expand Down Expand Up @@ -105,4 +108,60 @@ impl DateDuration {
pub fn sign(&self) -> Sign {
super::duration_sign(&self.fields())
}

/// DateDurationDays
/// TODO(review): Question: what the correct return type?
pub(crate) fn days(&self, relative_to: &PlainDate) -> TemporalResult<i64> {
// 1. Let yearsMonthsWeeksDuration be ! AdjustDateDurationRecord(dateDuration, 0).
let ymw_duration = self.adjust(FiniteF64(0.0), None, None)?;
// 2. If DateDurationSign(yearsMonthsWeeksDuration) = 0, return dateDuration.[[Days]].
if ymw_duration.sign() == Sign::Zero {
return self.days.as_integer_if_integral();
}
// 3. Let later be ? CalendarDateAdd(plainRelativeTo.[[Calendar]], plainRelativeTo.[[ISODate]], yearsMonthsWeeksDuration, constrain).
let later = relative_to.add(
&Duration {
date: *self,
time: Default::default(),
},
Some(ArithmeticOverflow::Constrain),
)?;
// 4. Let epochDays1 be ISODateToEpochDays(plainRelativeTo.[[ISODate]].[[Year]], plainRelativeTo.[[ISODate]].[[Month]] - 1, plainRelativeTo.[[ISODate]].[[Day]]).
let epoch_days_1 = iso_date_to_epoch_days(
Copy link
Member

Choose a reason for hiding this comment

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

issue: iso_date_to_epoch_days deviates from the specification here and takes an exact month vs. month - 1

This is sort of a mea culpa, I should have documented this change better (and thought that I had!). We don't have to subtract 1 from month here because the neri-schneider calculations take the month exactly vs month - 1. For reference, here are the tests.

relative_to.iso_year(),
i32::from(relative_to.iso_month()), // this function takes 1 based month number
i32::from(relative_to.iso_day()),
);
// 5. Let epochDays2 be ISODateToEpochDays(later.[[Year]], later.[[Month]] - 1, later.[[Day]]).
let epoch_days_2 = iso_date_to_epoch_days(
later.iso_year(),
i32::from(later.iso_month()), // this function takes 1 based month number
i32::from(later.iso_day()),
);
// 6. Let yearsMonthsWeeksInDays be epochDays2 - epochDays1.
let ymd_in_days = epoch_days_2 - epoch_days_1;
// 7. Return dateDuration.[[Days]] + yearsMonthsWeeksInDays.
Ok(self.days.as_integer_if_integral::<i64>()?
+ i64::try_from(ymd_in_days).map_err(|_| TemporalError::range())?)
}

/// AdjustDateDurationRecord
pub(crate) fn adjust(
&self,
days: FiniteF64,
weeks: Option<FiniteF64>,
months: Option<FiniteF64>,
) -> TemporalResult<Self> {
// 1. If weeks is not present, set weeks to dateDuration.[[Weeks]].
// 2. If months is not present, set months to dateDuration.[[Months]].
// 3. Return ? CreateDateDurationRecord(dateDuration.[[Years]], months, weeks, days).
let weeks = weeks.unwrap_or(self.weeks);
let months = months.unwrap_or(self.months);
Ok(Self {
years: self.years,
months,
weeks,
days,
})
}
}
3 changes: 2 additions & 1 deletion src/builtins/core/duration/normalized.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const NANOSECONDS_PER_HOUR: i128 = 60 * NANOSECONDS_PER_MINUTE;
// nanoseconds.abs() <= MAX_TIME_DURATION

/// A Normalized `TimeDuration` that represents the current `TimeDuration` in nanoseconds.
#[derive(Debug, Clone, Copy, Default, PartialEq, PartialOrd)]
#[derive(Debug, Clone, Copy, Default, PartialEq, PartialOrd, Eq, Ord)]
pub(crate) struct NormalizedTimeDuration(pub(crate) i128);

impl NormalizedTimeDuration {
Expand Down Expand Up @@ -66,6 +66,7 @@ impl NormalizedTimeDuration {

// NOTE: `days: f64` should be an integer -> `i64`.
/// Equivalent: 7.5.23 Add24HourDaysToNormalizedTimeDuration ( d, days )
/// Add24HourDaysToTimeDuration??
Copy link
Member

Choose a reason for hiding this comment

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

note: looks like the abstract op name changed in the docs. This can be updated.

pub(crate) fn add_days(&self, days: i64) -> TemporalResult<Self> {
let result = self.0 + i128::from(days) * i128::from(NS_PER_DAY);
if result.abs() > MAX_TIME_DURATION {
Expand Down
53 changes: 52 additions & 1 deletion src/builtins/core/duration/tests.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
use std::string::ToString;

use crate::{
options::ToStringRoundingOptions, parsers::Precision, partial::PartialDuration,
options::{OffsetDisambiguation, RelativeTo, ToStringRoundingOptions},
parsers::Precision,
partial::PartialDuration,
primitive::FiniteF64,
ZonedDateTime,
};

use super::Duration;
Expand Down Expand Up @@ -186,3 +191,49 @@ fn preserve_precision_loss() {

assert_eq!(&result, "PT9016206453995.731991S");
}

#[test]
fn test_duration_compare() {
let one = Duration::from_partial_duration(PartialDuration {
hours: Some(FiniteF64::try_from(79).unwrap()),
minutes: Some(FiniteF64::try_from(10).unwrap()),
..Default::default()
})
.unwrap();
let two = Duration::from_partial_duration(PartialDuration {
days: Some(FiniteF64::try_from(3).unwrap()),
hours: Some(FiniteF64::try_from(7).unwrap()),
seconds: Some(FiniteF64::try_from(630).unwrap()),
..Default::default()
})
.unwrap();
let three = Duration::from_partial_duration(PartialDuration {
days: Some(FiniteF64::try_from(3).unwrap()),
hours: Some(FiniteF64::try_from(6).unwrap()),
minutes: Some(FiniteF64::try_from(50).unwrap()),
..Default::default()
})
.unwrap();

let mut arr = [&one, &two, &three];
arr.sort_by(|a, b| Duration::compare(a, b, None).unwrap());
assert_eq!(
arr.map(ToString::to_string),
[&three, &one, &two].map(ToString::to_string)
);

// Sorting relative to a date, taking DST changes into account:
let zdt = ZonedDateTime::from_str(
"2020-11-01T00:00-07:00[America/Los_Angeles]",
Default::default(),
OffsetDisambiguation::Reject,
)
.unwrap();
arr.sort_by(|a, b| {
Duration::compare(a, b, Some(RelativeTo::ZonedDateTime(zdt.clone()))).unwrap()
});
assert_eq!(
arr.map(ToString::to_string),
[&one, &three, &two].map(ToString::to_string)
)
}
3 changes: 2 additions & 1 deletion src/iso.rs
Original file line number Diff line number Diff line change
Expand Up @@ -930,8 +930,9 @@ fn to_unchecked_epoch_nanoseconds(date: IsoDate, time: &IsoTime) -> i128 {
// ==== `IsoDate` specific utiltiy functions ====

/// Returns the Epoch days based off the given year, month, and day.
/// Note: Month should be 1 indexed
#[inline]
fn iso_date_to_epoch_days(year: i32, month: i32, day: i32) -> i32 {
pub(crate) fn iso_date_to_epoch_days(year: i32, month: i32, day: i32) -> i32 {
// 1. Let resolvedYear be year + floor(month / 12).
let resolved_year = year + month.div_euclid(12);
// 2. Let resolvedMonth be month modulo 12.
Expand Down
12 changes: 11 additions & 1 deletion src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,13 @@ impl TemporalUnit {
matches!(self, Year | Month | Week)
}

#[inline]
#[must_use]
pub fn is_date_unit(&self) -> bool {
use TemporalUnit::{Day, Month, Week, Year};
matches!(self, Day | Year | Month | Week)
}

#[inline]
#[must_use]
pub fn is_time_unit(&self) -> bool {
Expand Down Expand Up @@ -597,9 +604,12 @@ impl fmt::Display for DurationOverflow {
}

/// The disambiguation options for an instant.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
pub enum Disambiguation {
/// Compatible option
///
/// This is the default according to GetTemporalDisambiguationOption
#[default]
Compatible,
/// Earlier option
Earlier,
Expand Down
Loading