Uh oh!
There was an error while loading. Please reload this page.
ARROW-13561: [C++] Implement week kernel that accepts WeekOptions#11026
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -317,12 +317,12 @@ class ARROW_EXPORT MakeStructOptions : public FunctionOptions { | ||
| struct ARROW_EXPORT DayOfWeekOptions : public FunctionOptions { | ||
| public: | ||
| explicit DayOfWeekOptions(bool count_from_zero = true, uint32_t week_start = 1); | ||
| constexpr static char const kTypeName[] = "DayOfWeekOptions"; | ||
| static DayOfWeekOptions Defaults() { return DayOfWeekOptions(); } | ||
| /// Number days from 0 if true and from 1 if false | ||
| bool count_from_zero; | ||
| /// What day does the week start with (Monday=1, Sunday=7) | ||
| uint32_t week_start; | ||
| }; | ||
| @@ -361,6 +361,33 @@ struct ARROW_EXPORT AssumeTimezoneOptions : public FunctionOptions { | ||
| Nonexistent nonexistent; | ||
| }; | ||
| struct ARROW_EXPORT WeekOptions : public FunctionOptions { | ||
| public: | ||
| explicit WeekOptions(bool week_starts_monday = true, bool count_from_zero = false, | ||
| bool first_week_is_fully_in_year = false); | ||
| constexpr static char const kTypeName[] = "WeekOptions"; | ||
| static WeekOptions Defaults() { return WeekOptions{}; } | ||
| static WeekOptions ISODefaults() { | ||
| return WeekOptions{/*week_starts_monday*/ true, | ||
| /*count_from_zero=*/false, | ||
| /*first_week_is_fully_in_year=*/false}; | ||
| } | ||
| static WeekOptions USDefaults() { | ||
| return WeekOptions{/*week_starts_monday*/ false, | ||
| /*count_from_zero=*/false, | ||
| /*first_week_is_fully_in_year=*/false}; | ||
| } | ||
| /// What day does the week start with (Monday=true, Sunday=false) | ||
| bool week_starts_monday; | ||
| /// Dates from current year that fall into last ISO week of the previous year return | ||
| /// 0 if true and 52 or 53 if false. | ||
| bool count_from_zero; | ||
Member There was a problem hiding this comment. Similar question here vs. MemberAuthor There was a problem hiding this comment. There's a difference between the two - Member There was a problem hiding this comment. Er... then what is the difference between MemberAuthor There was a problem hiding this comment.
MemberAuthor There was a problem hiding this comment. See here for all the possibilities: #11026 (comment) Member There was a problem hiding this comment. While the implementation is reasonably simple, my main concern here is a head-scratching API. Supporting the MySQL modes does not sound like an interesting goal in itself, and exposing obscure parameters makes the API more difficult to learn and use. We should look for actual cases. One actual use case is US week numbering (like MemberAuthor There was a problem hiding this comment.
SQL server seems to support week (week 1 starts with 1st day of week in year) and iso_week. It seems to me we can agree on two parameters already ( MemberAuthor There was a problem hiding this comment. From MySQL docs about why the 8 modes:
Member There was a problem hiding this comment. If the concern is primarily a confusing API, could we offer default options for 'common' modes like us_week and iso_week? Then the individual options could be left up to people who need/understand what they want. | ||
| /// Must the first week be 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)? | ||
| bool first_week_is_fully_in_year; | ||
| }; | ||
| /// @} | ||
| /// \brief Get the absolute value of a value. | ||
| @@ -1008,7 +1035,8 @@ Result<Datum> ISOYear(const Datum& values, ExecContext* ctx = NULLPTR); | ||
| /// \brief ISOWeek returns ISO week of year number for each element of `values`. | ||
| /// First ISO week has the majority (4 or more) of its days in January. | ||
| /// ISO week starts on Monday. Year can have 52 or 53 weeks. | ||
| /// Week numbering can start with 1. | ||
| /// | ||
| /// \param[in] values input to extract ISO week of year from | ||
| /// \param[in] ctx the function execution context, optional | ||
| @@ -1018,6 +1046,34 @@ Result<Datum> ISOYear(const Datum& values, ExecContext* ctx = NULLPTR); | ||
| /// \note API not yet finalized | ||
| ARROW_EXPORT Result<Datum> ISOWeek(const Datum& values, ExecContext* ctx = NULLPTR); | ||
| /// \brief USWeek returns US week of year number for each element of `values`. | ||
| /// First US week has the majority (4 or more) of its days in January. | ||
| /// US week starts on Sunday. Year can have 52 or 53 weeks. | ||
| /// Week numbering starts with 1. | ||
| /// | ||
| /// \param[in] values input to extract US week of year from | ||
| /// \param[in] ctx the function execution context, optional | ||
| /// \return the resulting datum | ||
| /// | ||
| /// \since 6.0.0 | ||
| /// \note API not yet finalized | ||
| ARROW_EXPORT Result<Datum> USWeek(const Datum& values, ExecContext* ctx = NULLPTR); | ||
| /// \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. | ||
| /// 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 week of year from | ||
| /// \param[in] options for setting numbering start | ||
| /// \param[in] ctx the function execution context, optional | ||
| /// \return the resulting datum | ||
| /// | ||
| /// \since 6.0.0 | ||
| /// \note API not yet finalized | ||
| ARROW_EXPORT Result<Datum> Week(const Datum& values, WeekOptions options = WeekOptions(), | ||
| ExecContext* ctx = NULLPTR); | ||
Member There was a problem hiding this comment. It appears to be missing its counterpart in api_scalar.cc. | ||
| /// \brief ISOCalendar returns a (ISO year, ISO week, ISO day of week) struct for | ||
| /// each element of `values`. | ||
| /// ISO week starts on Monday denoted by 1 and ends on Sunday denoted by 7. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -55,11 +55,14 @@ using arrow_vendored::date::literals::dec; | ||
| using arrow_vendored::date::literals::jan; | ||
| using arrow_vendored::date::literals::last; | ||
| using arrow_vendored::date::literals::mon; | ||
| using arrow_vendored::date::literals::sun; | ||
| using arrow_vendored::date::literals::thu; | ||
| using arrow_vendored::date::literals::wed; | ||
| using internal::applicator::ScalarUnaryNotNull; | ||
| using internal::applicator::SimpleUnary; | ||
| using DayOfWeekState = OptionsWrapper<DayOfWeekOptions>; | ||
| using WeekState = OptionsWrapper<WeekOptions>; | ||
| using StrftimeState = OptionsWrapper<StrftimeOptions>; | ||
| using AssumeTimezoneState = OptionsWrapper<AssumeTimezoneOptions>; | ||
| @@ -230,6 +233,18 @@ struct AssumeTimezoneExtractor | ||
| } | ||
| }; | ||
| template <template <typename...> class Op, typename Duration, typename InType, | ||
| typename OutType> | ||
| struct TemporalComponentExtractWeek | ||
| : public TemporalComponentExtractBase<Op, Duration, InType, OutType> { | ||
| using Base = TemporalComponentExtractBase<Op, Duration, InType, OutType>; | ||
| static Status Exec(KernelContext* ctx, const ExecBatch& batch, Datum* out) { | ||
| const WeekOptions& options = WeekState::Get(ctx); | ||
| return Base::ExecWithOptions(ctx, &options, batch, out); | ||
| } | ||
| }; | ||
| // ---------------------------------------------------------------------- | ||
| // Extract year from temporal types | ||
| // | ||
| @@ -301,7 +316,7 @@ struct DayOfWeek { | ||
| for (int i = 0; i < 7; i++) { | ||
| lookup_table[i] = i + 8 - options->week_start; | ||
| lookup_table[i] = (lookup_table[i] > 6) ? lookup_table[i] - 7 : lookup_table[i]; | ||
| lookup_table[i] += !options->count_from_zero; | ||
| } | ||
| } | ||
| @@ -362,31 +377,70 @@ struct ISOYear { | ||
| }; | ||
| // ---------------------------------------------------------------------- | ||
| // Extract week from temporal types | ||
| // | ||
| // First week of an ISO year has the majority (4 or more) of its days in January. | ||
| // Last week of an ISO year has the year's last Thursday in it. | ||
| // Based on | ||
| // https://github.com/HowardHinnant/date/blob/6e921e1b1d21e84a5c82416ba7ecd98e33a436d0/include/date/iso_week.h#L1503 | ||
| template <typename Duration, typename Localizer> | ||
| struct Week { | ||
| explicit Week(const WeekOptions* options, Localizer&& localizer) | ||
| : localizer_(std::move(localizer)), | ||
| count_from_zero_(options->count_from_zero), | ||
| first_week_is_fully_in_year_(options->first_week_is_fully_in_year) { | ||
| if (options->week_starts_monday) { | ||
| if (first_week_is_fully_in_year_) { | ||
| wd_ = mon; | ||
| } else { | ||
| wd_ = thu; | ||
| } | ||
| } else { | ||
| if (first_week_is_fully_in_year_) { | ||
| wd_ = sun; | ||
| } else { | ||
| wd_ = wed; | ||
| } | ||
| } | ||
| if (count_from_zero_) { | ||
| days_offset_ = days{0}; | ||
| } else { | ||
| days_offset_ = days{3}; | ||
| } | ||
lidavidm marked this conversation as resolved.
Outdated
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| template <typename T, typename Arg0> | ||
| T Call(KernelContext*, Arg0 arg, Status*) const { | ||
| const auto t = floor<days>(localizer_.template ConvertTimePoint<Duration>(arg)); | ||
| auto y = year_month_day{t + days_offset_}.year(); | ||
| if (first_week_is_fully_in_year_) { | ||
| auto start = localizer_.ConvertDays(y / jan / wd_[1]); | ||
| if (!count_from_zero_) { | ||
| if (t < start) { | ||
| --y; | ||
| start = localizer_.ConvertDays(y / jan / wd_[1]); | ||
| } | ||
| } | ||
| return static_cast<T>(floor<weeks>(t - start).count() + 1); | ||
| } | ||
| auto start = localizer_.ConvertDays((y - years{1}) / dec / wd_[last]) + (mon - thu); | ||
| if (!count_from_zero_) { | ||
| if (t < start) { | ||
| --y; | ||
| start = localizer_.ConvertDays((y - years{1}) / dec / wd_[last]) + (mon - thu); | ||
| } | ||
| } | ||
| return static_cast<T>(floor<weeks>(t - start).count() + 1); | ||
| } | ||
| Localizer localizer_; | ||
| arrow_vendored::date::weekday wd_; | ||
| arrow_vendored::date::days days_offset_; | ||
| const bool count_from_zero_; | ||
| const bool first_week_is_fully_in_year_; | ||
| }; | ||
| // ---------------------------------------------------------------------- | ||
| @@ -979,7 +1033,7 @@ const FunctionDoc day_of_week_doc{ | ||
| "represented by 6.\n" | ||
| "`DayOfWeekOptions.week_start` can be used to set another starting day using\n" | ||
| "the ISO numbering convention (1=start week on Monday, 7=start week on Sunday).\n" | ||
| "Day numbers can start at 0 or 1 based on `DayOfWeekOptions.count_from_zero`.\n" | ||
| "Null values emit null.\n" | ||
| "An error is returned if the timestamps have a defined timezone but it\n" | ||
| "cannot be found in the timezone database."), | ||
| @@ -1004,13 +1058,34 @@ const FunctionDoc iso_year_doc{ | ||
| const FunctionDoc iso_week_doc{ | ||
| "Extract ISO week of year number", | ||
| ("First ISO week has the majority (4 or more) of its days in January." | ||
| "ISO week starts on Monday.\n" | ||
| "Week of the year starts with 1 and can run up to 53.\n" | ||
| "Null values emit null.\n" | ||
| "An error is returned if the timestamps have a defined timezone but it\n" | ||
| "cannot be found in the timezone database."), | ||
| {"values"}}; | ||
| const FunctionDoc us_week_doc{ | ||
| "Extract US week of year number", | ||
| ("First US week has the majority (4 or more) of its days in January." | ||
| "US week starts on Sunday.\n" | ||
| "Week of the year starts with 1 and can run up to 53.\n" | ||
| "Null values emit null.\n" | ||
| "An error is returned if the timestamps have a defined timezone but it\n" | ||
| "cannot be found in the timezone database."), | ||
| {"values"}}; | ||
| const FunctionDoc week_doc{ | ||
| "Extract week of year number", | ||
| ("First week has the majority (4 or more) of its days in January.\n" | ||
| "Year can have 52 or 53 weeks. Week numbering can start with 0 or 1 using " | ||
| "DayOfWeekOptions.count_from_zero.\n" | ||
| "An error is returned if the timestamps have a defined timezone but it\n" | ||
| "cannot be found in the timezone database."), | ||
| {"values"}, | ||
| "WeekOptions"}; | ||
| const FunctionDoc iso_calendar_doc{ | ||
| "Extract (ISO year, ISO week, ISO day of week) struct", | ||
| ("ISO week starts on Monday denoted by 1 and ends on Sunday denoted by 7.\n" | ||
| @@ -1140,10 +1215,24 @@ void RegisterScalarTemporal(FunctionRegistry* registry) { | ||
| "iso_year", {WithDates, WithTimestamps}, int64(), &iso_year_doc); | ||
| DCHECK_OK(registry->AddFunction(std::move(iso_year))); | ||
| static const auto default_iso_week_options = WeekOptions::ISODefaults(); | ||
| auto iso_week = MakeTemporal<Week, TemporalComponentExtractWeek, Int64Type>( | ||
| "iso_week", {WithDates, WithTimestamps}, int64(), &iso_week_doc, | ||
| &default_iso_week_options, WeekState::Init); | ||
| DCHECK_OK(registry->AddFunction(std::move(iso_week))); | ||
| static const auto default_us_week_options = WeekOptions::USDefaults(); | ||
| auto us_week = MakeTemporal<Week, TemporalComponentExtractWeek, Int64Type>( | ||
| "us_week", {WithDates, WithTimestamps}, int64(), &us_week_doc, | ||
| &default_us_week_options, WeekState::Init); | ||
| DCHECK_OK(registry->AddFunction(std::move(us_week))); | ||
| static const auto default_week_options = WeekOptions(); | ||
| auto week = MakeTemporal<Week, TemporalComponentExtractWeek, Int64Type>( | ||
| "week", {WithDates, WithTimestamps}, int64(), &week_doc, &default_week_options, | ||
| WeekState::Init); | ||
| DCHECK_OK(registry->AddFunction(std::move(week))); | ||
| auto iso_calendar = MakeSimpleUnaryTemporal<ISOCalendar>( | ||
| "iso_calendar", {WithDates, WithTimestamps}, IsoCalendarType(), &iso_calendar_doc); | ||
| DCHECK_OK(registry->AddFunction(std::move(iso_calendar))); | ||
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... did you deliberately choose a different option name and semantics compared with
DayOfWeekOptions::week_start?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Week start is an int and can have values from 1 to 7. Here I'd like to limit to two options - Monday/Sunday hence the boolean and different name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you like to limit it? What is the rationale for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not found an example of Week function that offers options other than Monday or Sunday. So I'd not like to offer more options if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit curious why it would be useful for DayOfWeek but not for Week. Ideally we should try to use similar idioms when similar concepts are involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually switch
DayOfWeekOptions::week_starttobool DayOfWeekOptions::week_starts_mondayif everyone's up for it. Then we could just useWeekOptionsfor both and it would be just slightly wrong.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's reasonable.