From bc37fcbc4b144a43a76e8dcde7a0a6dfbe849d12 Mon Sep 17 00:00:00 2001 From: Bradley White <14679271+devbww@users.noreply.github.com> Date: Fri, 3 May 2024 14:42:14 -0400 Subject: [PATCH] fix(spanner): correct use of Interval offset in Timestamp addition (#14121) When adding an `Interval` to a `Timestamp`+time-zone, the offset should be interpreted as an absolute value rather than a civil one. (This is a poor choice in my opinion, but so be it.) Update the test case, which now has to use a "1 day" interval to show the difference between `Add(ts, "1 day", tz)` and `Add(ts, "24 hours", tz)` over a civil-time discontinuity. Add a new test based upon the postgresql.org examples, one of which would have demonstrated the original problem. --- google/cloud/spanner/interval.cc | 7 ++-- google/cloud/spanner/interval_test.cc | 54 +++++++++++++++++++++------ 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/google/cloud/spanner/interval.cc b/google/cloud/spanner/interval.cc index 926b1b4e88b0..448b2685cdea 100644 --- a/google/cloud/spanner/interval.cc +++ b/google/cloud/spanner/interval.cc @@ -404,13 +404,12 @@ StatusOr Add(Timestamp const& ts, Interval const& intvl, auto tz = LoadTimeZone(time_zone); if (!tz) return tz.status(); auto ci = tz->At(*ts.get()); - auto offset = absl::FromChrono(intvl.offset_); - auto seconds = absl::IDivDuration(offset, absl::Seconds(1), &offset); auto cs = absl::CivilSecond( // add the civil-time parts ci.cs.year(), ci.cs.month() + intvl.months_, ci.cs.day() + intvl.days_, - ci.cs.hour(), ci.cs.minute(), ci.cs.second() + seconds); + ci.cs.hour(), ci.cs.minute(), ci.cs.second()); return *MakeTimestamp(internal::ToProtoTimestamp( // overflow saturates - absl::FromCivil(cs, *tz) + ci.subsecond + offset)); + absl::FromCivil(cs, *tz) + ci.subsecond + + absl::FromChrono(intvl.offset_))); } StatusOr Diff(Timestamp const& ts1, Timestamp const& ts2, diff --git a/google/cloud/spanner/interval_test.cc b/google/cloud/spanner/interval_test.cc index 690fe464f6e1..1b51919d8287 100644 --- a/google/cloud/spanner/interval_test.cc +++ b/google/cloud/spanner/interval_test.cc @@ -292,22 +292,52 @@ TEST(Interval, TimestampOperations) { MakeTimestamp("2020-02-03T04:05:06.123456789Z"), utc), IsOkAndHolds(Interval(0, 0, 428, hms(4, 5, 6) + nanoseconds(999)))); - // Over civil-time discontinuities, two civil hours is either one absolute - // hour (skipped) or three absolute hours (repeated). + // Over civil-time discontinuities, one civil day is either 23 absolute + // hours (skipped) or 25 absolute hours (repeated). EXPECT_THAT(Add(MakeTimestamp("2023-03-12T01:02:03.456789-05:00"), - Interval(0, 0, 0, hours(2)), nyc), - IsOkAndHolds(MakeTimestamp("2023-03-12T03:02:03.456789-04:00"))); - auto ts1 = MakeTimestamp("2023-03-12T03:02:03.456789-04:00"); + Interval(0, 0, 1), nyc), + IsOkAndHolds(MakeTimestamp("2023-03-13T01:02:03.456789-04:00"))); + auto ts1 = MakeTimestamp("2023-03-13T01:02:03.456789-04:00"); auto ts2 = MakeTimestamp("2023-03-12T01:02:03.456789-05:00"); - EXPECT_EQ(*ts1.get() - *ts2.get(), absl::Hours(1)); - EXPECT_THAT(Diff(ts1, ts2, nyc), IsOkAndHolds(Interval(hours(2)))); + EXPECT_EQ(*ts1.get() - *ts2.get(), absl::Hours(23)); + EXPECT_THAT(Diff(ts1, ts2, nyc), IsOkAndHolds(Interval(0, 0, 1))); EXPECT_THAT(Add(MakeTimestamp("2023-11-05T01:02:03.456789-04:00"), - Interval(0, 0, 0, hours(2)), nyc), - IsOkAndHolds(MakeTimestamp("2023-11-05T03:02:03.456789-05:00"))); - ts1 = MakeTimestamp("2023-11-05T03:02:03.456789-05:00"); + Interval(0, 0, 1), nyc), + IsOkAndHolds(MakeTimestamp("2023-11-06T01:02:03.456789-05:00"))); + ts1 = MakeTimestamp("2023-11-06T01:02:03.456789-05:00"); ts2 = MakeTimestamp("2023-11-05T01:02:03.456789-04:00"); - EXPECT_EQ(*ts1.get() - *ts2.get(), absl::Hours(3)); - EXPECT_THAT(Diff(ts1, ts2, nyc), IsOkAndHolds(Interval(hours(2)))); + EXPECT_EQ(*ts1.get() - *ts2.get(), absl::Hours(25)); + EXPECT_THAT(Diff(ts1, ts2, nyc), IsOkAndHolds(Interval(0, 0, 1))); +} + +// A miscellaneous bunch of Interval tests that come from the examples +// in https://www.postgresql.org/docs/current/functions-datetime.html. +TEST(Interval, FromPostgreSqlDocs) { + EXPECT_EQ(Interval(0, 0, 1) + Interval(hours(1)), + Interval(0, 0, 1, hours(1))); + EXPECT_EQ(-Interval(hours(23)), Interval(-hours(23))); + EXPECT_EQ(Interval(0, 0, 1) - Interval(hours(1)), + Interval(0, 0, 1, -hours(1))); + EXPECT_EQ(Interval(seconds(1)) * 900, Interval(minutes(15))); + EXPECT_EQ(Interval(0, 0, 1) * 21, Interval(0, 0, 21)); + EXPECT_EQ(Interval(hours(1)) * 3.5, Interval(hours(3) + minutes(30))); + EXPECT_EQ(Interval(hours(1)) / 1.5, Interval(minutes(40))); + if (auto ts = Add(MakeTimestamp("2021-10-31T00:00:00+02:00"), + Interval(0, 0, 1), "Europe/Warsaw")) { + EXPECT_EQ(*ts, MakeTimestamp("2021-10-31T23:00:00+00:00")); + } + if (auto ts = Add(MakeTimestamp("2021-11-01T00:00:00+01:00"), + -Interval(0, 0, 1), "Europe/Warsaw")) { + EXPECT_EQ(*ts, MakeTimestamp("2021-10-30T22:00:00+00:00")); + } + if (auto ts = Add(MakeTimestamp("2005-04-02T12:00:00-07:00"), + Interval(0, 0, 1), "America/Denver")) { + EXPECT_EQ(*ts, MakeTimestamp("2005-04-03T12:00:00-06:00")); + } + if (auto ts = Add(MakeTimestamp("2005-04-02T12:00:00-07:00"), + Interval(hours(24)), "America/Denver")) { + EXPECT_EQ(*ts, MakeTimestamp("2005-04-03T13:00:00-06:00")); + } } } // namespace