From 76c8acda14a85d09e002141adacfc8fb3a1d092f Mon Sep 17 00:00:00 2001 From: Rok Mihevc Date: Mon, 20 Feb 2023 17:37:08 +0100 Subject: [PATCH 1/2] Review feedback --- cpp/src/arrow/compute/api_scalar.cc | 2 +- .../compute/kernels/scalar_temporal_test.cc | 12 +++---- .../compute/kernels/scalar_temporal_unary.cc | 35 ++++++++++--------- docs/source/cpp/compute.rst | 18 +++++----- python/pyarrow/tests/test_compute.py | 2 +- 5 files changed, 35 insertions(+), 34 deletions(-) diff --git a/cpp/src/arrow/compute/api_scalar.cc b/cpp/src/arrow/compute/api_scalar.cc index ace0fcdb58e..c0978eb87ec 100644 --- a/cpp/src/arrow/compute/api_scalar.cc +++ b/cpp/src/arrow/compute/api_scalar.cc @@ -834,7 +834,7 @@ SCALAR_EAGER_UNARY(DayOfYear, "day_of_year") SCALAR_EAGER_UNARY(Hour, "hour") SCALAR_EAGER_UNARY(YearMonthDay, "year_month_day") SCALAR_EAGER_UNARY(IsDaylightSavings, "is_dst") -SCALAR_EAGER_UNARY(LocalTime, "local_time") +SCALAR_EAGER_UNARY(LocalTime, "local_timestamp") SCALAR_EAGER_UNARY(IsLeapYear, "is_leap_year") SCALAR_EAGER_UNARY(ISOCalendar, "iso_calendar") SCALAR_EAGER_UNARY(ISOWeek, "iso_week") diff --git a/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc b/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc index 41408b94797..d922552cf2d 100644 --- a/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc @@ -1833,13 +1833,13 @@ TEST_F(ScalarTemporalTest, TestLocalTime) { "2008-12-27 14:30:00", "2008-12-28 14:30:00", "2011-12-31 15:32:03", null])"; for (auto u : TimeUnit::values()) { - CheckScalarUnary("local_time", timestamp(u), times_seconds_precision, timestamp(u), - times_seconds_precision); - CheckScalarUnary("local_time", timestamp(u, "UTC"), times_seconds_precision, + CheckScalarUnary("local_timestamp", timestamp(u), times_seconds_precision, timestamp(u), times_seconds_precision); - CheckScalarUnary("local_time", timestamp(u, "Asia/Kolkata"), times_seconds_precision, - timestamp(u), expected_local_kolkata); - CheckScalarUnary("local_time", timestamp(u, "Pacific/Marquesas"), + CheckScalarUnary("local_timestamp", timestamp(u, "UTC"), times_seconds_precision, + timestamp(u), times_seconds_precision); + CheckScalarUnary("local_timestamp", timestamp(u, "Asia/Kolkata"), + times_seconds_precision, timestamp(u), expected_local_kolkata); + CheckScalarUnary("local_timestamp", timestamp(u, "Pacific/Marquesas"), times_seconds_precision, timestamp(u), expected_local_marquesas); } } diff --git a/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc b/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc index b69bdc2719e..270aa498d97 100644 --- a/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc +++ b/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc @@ -673,11 +673,11 @@ struct IsDaylightSavings { }; // ---------------------------------------------------------------------- -// Extract local time of a given timestamp given its timezone +// Extract local timestamp of a given timestamp given its timezone template -struct LocalTime { - explicit LocalTime(const FunctionOptions* options, Localizer&& localizer) +struct LocalTimestamp { + explicit LocalTimestamp(const FunctionOptions* options, Localizer&& localizer) : localizer_(std::move(localizer)) {} template @@ -1344,8 +1344,8 @@ Result ResolveAssumeTimezoneOutput(KernelContext* ctx, return timestamp(in_type.unit(), AssumeTimezoneState::Get(ctx).timezone); } -Result ResolveLocalTimeOutput(KernelContext* ctx, - const std::vector& args) { +Result ResolveLocalTimestampOutput(KernelContext* ctx, + const std::vector& args) { const auto& in_type = checked_cast(*args[0]); return timestamp(in_type.unit()); } @@ -1807,11 +1807,12 @@ const FunctionDoc is_dst_doc{ "An error is returned if the values do not have a defined timezone."), {"values"}}; -const FunctionDoc local_time_doc{ +const FunctionDoc local_timestamp_doc{ "Convert timestamp to a timezone-naive local time timestamp", - ("LocalTime converts a timestamp to a local time of timestamps timezone\n" - "and removes timezone metadata. If input is in UTC or doesn't have\n" - "timezone metadata, it is returned as is.\n" + ("LocalTimestamp converts timezone-aware timestamp to local timestamp\n" + "of the given timestamps timezone and removes timezone metadata.\n" + "If input is in UTC or without timezone, then unchanged input values\n" + "without timezone metadata are returned.\n" "Null values emit null."), {"values"}}; const FunctionDoc floor_temporal_doc{ @@ -1831,9 +1832,8 @@ const FunctionDoc ceil_temporal_doc{ const FunctionDoc round_temporal_doc{ "Round temporal values to the nearest multiple of specified time unit", ("Null values emit null.\n" - "If timezone is not given then timezone naive timestamp in UTC are\n" - "returned. An error is returned if the values have a defined timezone\n" - "but it cannot be found in the timezone database."), + "An error is returned if the values have a defined timezone but it\n" + "cannot be found in the timezone database."), {"timestamps"}, "RoundTemporalOptions"}; @@ -2000,11 +2000,12 @@ void RegisterScalarTemporalUnary(FunctionRegistry* registry) { is_dst_doc); DCHECK_OK(registry->AddFunction(std::move(is_dst))); - auto local_time = - UnaryTemporalFactory::Make< - WithTimestamps>("local_time", OutputType::Resolver(ResolveLocalTimeOutput), - local_time_doc); - DCHECK_OK(registry->AddFunction(std::move(local_time))); + auto local_timestamp = + UnaryTemporalFactory::Make< + WithTimestamps>("local_timestamp", + OutputType::Resolver(ResolveLocalTimestampOutput), + local_timestamp_doc); + DCHECK_OK(registry->AddFunction(std::move(local_timestamp))); // Temporal rounding functions // Note: UnaryTemporalFactory will not correctly resolve OutputType(FirstType) to diff --git a/docs/source/cpp/compute.rst b/docs/source/cpp/compute.rst index 87411fbe77a..725697dc8e3 100644 --- a/docs/source/cpp/compute.rst +++ b/docs/source/cpp/compute.rst @@ -1556,20 +1556,20 @@ Input timestamps are assumed to be relative to the timezone given in UTC-relative timestamps with the timezone metadata set to the above value. An error is returned if the timestamps already have the timezone metadata set. -`local_time` function converts UTC-relative timestamps to local "timezone-naive" +`local_timestamp` function converts UTC-relative timestamps to local "timezone-naive" timestamps. The timezone is taken from the timezone metadata of the input timestamps. This function is the inverse of `assume_timezone`. Please note: **all temporal functions already operate on timestamps as if they were in local -time of the metadata provided timezone**. Using `local_time` is only meant to be +time of the metadata provided timezone**. Using `local_timestamp` is only meant to be used when an external system expects local timestamps. -+--------------------+------------+-------------------+---------------+----------------------------------+-------+ -| Function name | Arity | Input types | Output type | Options class | Notes | -+====================+============+===================+===============+==================================+=======+ -| assume_timezone | Unary | Timestamp | Timestamp | :struct:`AssumeTimezoneOptions` | \(1) | -+--------------------+------------+-------------------+---------------+----------------------------------+-------+ -| local_time | Unary | Timestamp | Timestamp | | \(2) | -+--------------------+------------+-------------------+---------------+----------------------------------+-------+ ++-----------------+-------+-------------+---------------+---------------------------------+-------+ +| Function name | Arity | Input types | Output type | Options class | Notes | ++=================+=======+=============+===============+=================================+=======+ +| assume_timezone | Unary | Timestamp | Timestamp | :struct:`AssumeTimezoneOptions` | \(1) | ++-----------------+-------+-------------+---------------+---------------------------------+-------+ +| local_timestamp | Unary | Timestamp | Timestamp | | \(2) | ++-----------------+-------+-------------+---------------+---------------------------------+-------+ * \(1) In addition to the timezone value, :struct:`AssumeTimezoneOptions` allows choosing the behaviour when a timestamp is ambiguous or nonexistent diff --git a/python/pyarrow/tests/test_compute.py b/python/pyarrow/tests/test_compute.py index 75bf70409a8..24e9950f217 100644 --- a/python/pyarrow/tests/test_compute.py +++ b/python/pyarrow/tests/test_compute.py @@ -1951,7 +1951,7 @@ def _check_datetime_components(timestamps, timezone=None): assert pc.microsecond(tsa).equals(pa.array(ts.dt.microsecond % 10 ** 3)) assert pc.nanosecond(tsa).equals(pa.array(ts.dt.nanosecond)) assert pc.subsecond(tsa).equals(pa.array(subseconds)) - assert pc.local_time(tsa).equals(pa.array(ts.dt.tz_localize(None))) + assert pc.local_timestamp(tsa).equals(pa.array(ts.dt.tz_localize(None))) if ts.dt.tz: if ts.dt.tz is datetime.timezone.utc: From 36f4d7337b3cf6ae526beecc60dd88f52eab715f Mon Sep 17 00:00:00 2001 From: Rok Mihevc Date: Wed, 22 Feb 2023 00:05:42 +0100 Subject: [PATCH 2/2] Review feedback --- cpp/src/arrow/compute/api_scalar.cc | 2 +- cpp/src/arrow/compute/api_scalar.h | 5 +++-- cpp/src/arrow/compute/kernels/scalar_temporal_test.cc | 2 +- cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc | 3 ++- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/cpp/src/arrow/compute/api_scalar.cc b/cpp/src/arrow/compute/api_scalar.cc index c0978eb87ec..ae7e82fb2f9 100644 --- a/cpp/src/arrow/compute/api_scalar.cc +++ b/cpp/src/arrow/compute/api_scalar.cc @@ -834,7 +834,7 @@ SCALAR_EAGER_UNARY(DayOfYear, "day_of_year") SCALAR_EAGER_UNARY(Hour, "hour") SCALAR_EAGER_UNARY(YearMonthDay, "year_month_day") SCALAR_EAGER_UNARY(IsDaylightSavings, "is_dst") -SCALAR_EAGER_UNARY(LocalTime, "local_timestamp") +SCALAR_EAGER_UNARY(LocalTimestamp, "local_timestamp") SCALAR_EAGER_UNARY(IsLeapYear, "is_leap_year") SCALAR_EAGER_UNARY(ISOCalendar, "iso_calendar") SCALAR_EAGER_UNARY(ISOWeek, "iso_week") diff --git a/cpp/src/arrow/compute/api_scalar.h b/cpp/src/arrow/compute/api_scalar.h index 3e55e2c8892..10a2b4bffde 100644 --- a/cpp/src/arrow/compute/api_scalar.h +++ b/cpp/src/arrow/compute/api_scalar.h @@ -1518,7 +1518,7 @@ ARROW_EXPORT Result AssumeTimezone(const Datum& values, ARROW_EXPORT Result IsDaylightSavings(const Datum& values, ExecContext* ctx = NULLPTR); -/// \brief LocalTime converts timestamp to timezone naive local timestamp +/// \brief LocalTimestamp converts timestamp to timezone naive local timestamp /// /// \param[in] values input to convert to local time /// \param[in] ctx the function execution context, optional @@ -1526,7 +1526,8 @@ ARROW_EXPORT Result IsDaylightSavings(const Datum& values, /// /// \since 12.0.0 /// \note API not yet finalized -ARROW_EXPORT Result LocalTime(const Datum& values, ExecContext* ctx = NULLPTR); +ARROW_EXPORT Result LocalTimestamp(const Datum& values, + ExecContext* ctx = NULLPTR); /// \brief Years Between finds the number of years between two values /// diff --git a/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc b/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc index d922552cf2d..5cdf6f2bcf1 100644 --- a/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc @@ -1811,7 +1811,7 @@ TEST_F(ScalarTemporalTest, TestTemporalDifferenceErrors) { CallFunction("weeks_between", {arr1, arr1}, &options)); } -TEST_F(ScalarTemporalTest, TestLocalTime) { +TEST_F(ScalarTemporalTest, TestLocalTimestamp) { const char* times_seconds_precision = R"(["1970-01-01T00:00:59", "2000-02-29T23:23:23", "2033-05-18T03:33:20", "2020-01-01T01:05:05", "2019-12-31T02:10:10", "2019-12-30T03:15:15", diff --git a/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc b/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc index 270aa498d97..3addaf68630 100644 --- a/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc +++ b/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc @@ -1810,7 +1810,8 @@ const FunctionDoc is_dst_doc{ const FunctionDoc local_timestamp_doc{ "Convert timestamp to a timezone-naive local time timestamp", ("LocalTimestamp converts timezone-aware timestamp to local timestamp\n" - "of the given timestamps timezone and removes timezone metadata.\n" + "of the given timestamp's timezone and removes timezone metadata.\n" + "Alternative name for this timestamp is also wall clock time.\n" "If input is in UTC or without timezone, then unchanged input values\n" "without timezone metadata are returned.\n" "Null values emit null."),