lidavidm commented on a change in pull request #11026:
URL: https://github.com/apache/arrow/pull/11026#discussion_r714789901
##########
File path: r/src/compute.cpp
##########
@@ -318,14 +318,42 @@ std::shared_ptr<arrow::compute::FunctionOptions>
make_compute_options(
if (func_name == "day_of_week") {
using Options = arrow::compute::DayOfWeekOptions;
- bool one_based_numbering = true;
- if (!Rf_isNull(options["one_based_numbering"])) {
- one_based_numbering =
cpp11::as_cpp<bool>(options["one_based_numbering"]);
+ bool count_from_zero = false;
+ if (!Rf_isNull(options["count_from_zero"])) {
+ count_from_zero = cpp11::as_cpp<bool>(options["count_from_zero"]);
}
- return std::make_shared<Options>(one_based_numbering,
+ return std::make_shared<Options>(count_from_zero,
cpp11::as_cpp<uint32_t>(options["week_start"]));
}
+ if (func_name == "iso_week") {
+ using Options = arrow::compute::WeekOptions;
+ return std::make_shared<Options>(true, false, false);
Review comment:
nit: place `/*count_from_zero=*/`, etc. comments here to make it clear?
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal.cc
##########
@@ -1135,10 +1210,24 @@ void RegisterScalarTemporal(FunctionRegistry* registry)
{
"iso_year", {WithDates, WithTimestamps}, int64(), &iso_year_doc);
DCHECK_OK(registry->AddFunction(std::move(iso_year)));
- auto iso_week = MakeTemporal<ISOWeek, TemporalComponentExtract, Int64Type>(
- "iso_week", {WithDates, WithTimestamps}, int64(), &iso_week_doc);
+ static const auto default_iso_week_options = WeekOptions(true, false, false);
Review comment:
Similarly here, can we put comments for the parameter literals? Or
actually, it might be nice to define WeekOptions::IsoDefaults and
WeekOptions::UsDefaults which could then be used here and in the R bindings.
##########
File path: docs/source/cpp/compute.rst
##########
@@ -1356,11 +1360,19 @@ For timestamps inputs with non-empty timezone,
localized timestamp components wi
* \(1) Outputs the number of the day of the week. By default week begins on
Monday
represented by 0 and ends on Sunday represented by 6.
:member:`DayOfWeekOptions::week_start` can be used to set
the starting day of the week using ISO convention (Monday=1, Sunday=7). Day
numbering can start with 0 or 1
- using :member:`DayOfWeekOptions::one_based_numbering` parameter.
+ using :member:`DayOfWeekOptions::count_from_zero` parameter.
* \(2) First ISO week has the majority (4 or more) of it's days in January.
ISO year
- starts with the first ISO week.
+ starts with the first ISO week. ISO week starts on Monday.
See `ISO 8601 week date definition`_ for more details.
-* \(3) Output is a ``{"iso_year": output type, "iso_week": output type,
"iso_day_of_week": output type}`` Struct.
+* \(3) First US week has the majority (4 or more) of it's days in January. US
year
+ starts with the first US week. US week starts on Sunday.
+* \(4) Returns week number allowing for setting several parameters.
+ :member:`WeekOptions::week_starts_monday` sets what day does the week start
with (Monday=true, Sunday=false).
+ :member:`WeekOptions::count_from_zero` sets that dates from current year
that fall into last ISO week of
+ the previous year return 0 if true and 52 or 53 if false.
+ :member:`WeekOptions::first_week_is_fully_in_year` sets if the first week is
fully in January (true), or is a
+ week that begins on December 29, 30, or 31 considered to be the first week
of the new year (false)?
+* \(5) Output is a ``{"iso_year": output type, "iso_week": output type,
"iso_day_of_week": output type}`` Struct.
Review comment:
```suggestion
* \(3) First US week has the majority (4 or more) of its days in January. US
year
starts with the first US week. US week starts on Sunday.
* \(4) Returns week number allowing for setting several parameters.
If :member:`WeekOptions::week_starts_monday` is true, the week starts with
Monday, else Sunday if false.
If :member:`WeekOptions::count_from_zero` is true, dates from the current
year that fall into the last ISO week
of the previous year are numbered as week 0, else week 52 or 53 if false.
If :member:`WeekOptions::first_week_is_fully_in_year` is true, the first
week (week 1) must fully be in January;
else if false, a week that begins on December 29, 30, or 31 is considered
the first week of the new year.
* \(5) Output is a ``{"iso_year": output type, "iso_week": output type,
"iso_day_of_week": output type}`` Struct.
```
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -1006,17 +1023,20 @@ ARROW_EXPORT Result<Datum> DayOfYear(const Datum&
values, ExecContext* ctx = NUL
ARROW_EXPORT
Result<Datum> ISOYear(const Datum& values, ExecContext* ctx = NULLPTR);
-/// \brief ISOWeek returns ISO week of year number for each element of
`values`.
+/// \brief Week returns week of year number for each element of `values`.
/// First ISO week has the majority (4 or more) of its days in January.
-/// Week of the year starts with 1 and can run up to 53.
+/// Year can have 52 or 53 weeks. Week numbering can start with 0 or 1
+/// depending on DayOfWeekOptions.count_from_zero.
///
-/// \param[in] values input to extract ISO week of year from
+/// \param[in] values input to extract week of year from
+/// \param[in] options for setting numbering start
/// \param[in] ctx the function execution context, optional
/// \return the resulting datum
///
-/// \since 5.0.0
+/// \since 6.0.0
/// \note API not yet finalized
-ARROW_EXPORT Result<Datum> ISOWeek(const Datum& values, ExecContext* ctx =
NULLPTR);
+ARROW_EXPORT Result<Datum> Week(const Datum& values, WeekOptions options =
WeekOptions(),
+ ExecContext* ctx = NULLPTR);
Review comment:
Also add a helper for USWeek?
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal.cc
##########
@@ -364,29 +379,68 @@ struct ISOYear {
// ----------------------------------------------------------------------
// Extract ISO week from temporal types
Review comment:
These comments might need updating (at least 'Extract week from temporal
types', the definition of ISO week can be left here)
##########
File path: docs/source/cpp/compute.rst
##########
@@ -1356,11 +1360,19 @@ For timestamps inputs with non-empty timezone,
localized timestamp components wi
* \(1) Outputs the number of the day of the week. By default week begins on
Monday
represented by 0 and ends on Sunday represented by 6.
:member:`DayOfWeekOptions::week_start` can be used to set
the starting day of the week using ISO convention (Monday=1, Sunday=7). Day
numbering can start with 0 or 1
- using :member:`DayOfWeekOptions::one_based_numbering` parameter.
+ using :member:`DayOfWeekOptions::count_from_zero` parameter.
* \(2) First ISO week has the majority (4 or more) of it's days in January.
ISO year
- starts with the first ISO week.
+ starts with the first ISO week. ISO week starts on Monday.
See `ISO 8601 week date definition`_ for more details.
-* \(3) Output is a ``{"iso_year": output type, "iso_week": output type,
"iso_day_of_week": output type}`` Struct.
+* \(3) First US week has the majority (4 or more) of it's days in January. US
year
+ starts with the first US week. US week starts on Sunday.
+* \(4) Returns week number allowing for setting several parameters.
+ :member:`WeekOptions::week_starts_monday` sets what day does the week start
with (Monday=true, Sunday=false).
+ :member:`WeekOptions::count_from_zero` sets that dates from current year
that fall into last ISO week of
+ the previous year return 0 if true and 52 or 53 if false.
+ :member:`WeekOptions::first_week_is_fully_in_year` sets if the first week is
fully in January (true), or is a
+ week that begins on December 29, 30, or 31 considered to be the first week
of the new year (false)?
+* \(5) Output is a ``{"iso_year": output type, "iso_week": output type,
"iso_day_of_week": output type}`` Struct.
Review comment:
Some wording tweaks just to make this flow a little better to me (feel
free to accept/reject/reword as needed).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
