Skip to content

ARROW-14821: [C++] Add ceil_is_strictly_greater and calendar_based_origin temporal round options (to mimic lubridate's date rounding) #12657

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.

Sign up for GitHub

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

Merged
pitrou merged 19 commits into from
Jun 2, 2022
Merged

ARROW-14821: [C++] Add ceil_is_strictly_greater and calendar_based_origin temporal round options (to mimic lubridate's date rounding) #12657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
7f4ec7b
Add change_on_boundary to RoundTemporalOptions
rok Mar 17, 2022
c4ab6e2
Apply suggestions from code review
rok Apr 21, 2022
88c8646
Review feedback.
rok Apr 21, 2022
29fdbf2
Review feedback.
rok Apr 28, 2022
c240ebb
Review feedback
rok May 6, 2022
4d816b7
strict_ceil to ceil_on_boundary
rok May 6, 2022
4160a40
Apply suggestions from code review
rok May 10, 2022
177b233
Apply suggestions from code review
rok May 10, 2022
9fe7a06
Apply suggestions from code review
rok May 10, 2022
e9f36fb
ceil_on_boundary -> ceil_is_strictly_greater
rok May 11, 2022
af2a5e3
Review feedback.
rok May 12, 2022
c5c7cc0
Update python/pyarrow/_compute.pyx
rok May 16, 2022
b2ff7a2
Update python/pyarrow/_compute.pyx
rok May 16, 2022
65807b7
review feedback
rok May 17, 2022
167236d
multiple_since_greater_unit->calendar_based_origin
rok May 17, 2022
b2f9226
Update python/pyarrow/_compute.pyx
rok May 17, 2022
38696a6
review feedback
rok May 17, 2022
422b022
Comments for RoundTemporalOptions.calendar_based_origin
rok May 30, 2022
de82528
Apply suggestions from code review
rok May 31, 2022
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
13 changes: 10 additions & 3 deletions cpp/src/arrow/compute/api_scalar.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,10 @@ static auto kRoundOptionsType = GetFunctionOptionsType<RoundOptions>(
static auto kRoundTemporalOptionsType = GetFunctionOptionsType<RoundTemporalOptions>(
DataMember("multiple", &RoundTemporalOptions::multiple),
DataMember("unit", &RoundTemporalOptions::unit),
DataMember("week_starts_monday", &RoundTemporalOptions::week_starts_monday));
DataMember("week_starts_monday", &RoundTemporalOptions::week_starts_monday),
DataMember("ceil_is_strictly_greater",
&RoundTemporalOptions::ceil_is_strictly_greater),
DataMember("calendar_based_origin", &RoundTemporalOptions::calendar_based_origin));
static auto kRoundToMultipleOptionsType = GetFunctionOptionsType<RoundToMultipleOptions>(
DataMember("multiple", &RoundToMultipleOptions::multiple),
DataMember("round_mode", &RoundToMultipleOptions::round_mode));
Expand Down Expand Up @@ -491,11 +494,15 @@ RoundOptions::RoundOptions(int64_t ndigits, RoundMode round_mode)
constexpr char RoundOptions::kTypeName[];

RoundTemporalOptions::RoundTemporalOptions(int multiple, CalendarUnit unit,
bool week_starts_monday)
bool week_starts_monday,
bool ceil_is_strictly_greater,
bool calendar_based_origin)
: FunctionOptions(internal::kRoundTemporalOptionsType),
multiple(std::move(multiple)),
unit(unit),
week_starts_monday(week_starts_monday) {}
week_starts_monday(week_starts_monday),
ceil_is_strictly_greater(ceil_is_strictly_greater),
calendar_based_origin(calendar_based_origin) {}
constexpr char RoundTemporalOptions::kTypeName[];

RoundToMultipleOptions::RoundToMultipleOptions(double multiple, RoundMode round_mode)
Expand Down
23 changes: 22 additions & 1 deletion cpp/src/arrow/compute/api_scalar.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,9 @@ enum class CalendarUnit : int8_t {
class ARROW_EXPORT RoundTemporalOptions : public FunctionOptions {
public:
explicit RoundTemporalOptions(int multiple = 1, CalendarUnit unit = CalendarUnit::DAY,
bool week_starts_monday = true);
bool week_starts_monday = true,
bool ceil_is_strictly_greater = false,
bool calendar_based_origin = false);
static constexpr char const kTypeName[] = "RoundTemporalOptions";
static RoundTemporalOptions Defaults() { return RoundTemporalOptions(); }

Expand All @@ -117,6 +119,25 @@ class ARROW_EXPORT RoundTemporalOptions : public FunctionOptions {
CalendarUnit unit;
/// What day does the week start with (Monday=true, Sunday=false)
bool week_starts_monday;
/// Enable this flag to return a rounded value that is strictly greater than the input.
/// For example: ceiling 1970-01-01T00:00:00 to 3 hours would yield 1970-01-01T03:00:00
/// if set to true and 1970-01-01T00:00:00 if set to false.
/// This applies for ceiling only.
bool ceil_is_strictly_greater;
/// By default time is rounded to a multiple of units since 1970-01-01T00:00:00.
/// By setting calendar_based_origin to true, time will be rounded to a number
/// of units since the last greater calendar unit.
/// For example: rounding to a multiple of days since the beginning of the month or
/// to hours since the beginning of the day.
/// Exceptions: week and quarter are not used as greater units, therefore days will
/// will be rounded to the beginning of the month not week. Greater unit of week
/// is year.
/// Note that ceiling and rounding might change sorting order of an array near greater
/// unit change. For example rounding YYYY-mm-dd 23:00:00 to 5 hours will ceil and
/// round to YYYY-mm-dd+1 01:00:00 and floor to YYYY-mm-dd 20:00:00. On the other hand
/// YYYY-mm-dd+1 00:00:00 will ceil, round and floor to YYYY-mm-dd+1 00:00:00. This
/// can break the order of an already ordered array.
bool calendar_based_origin;
Copy link
Member

@jorisvandenbossche jorisvandenbossche May 5, 2022

Choose a reason for hiding this comment

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

To be honest, (before looking at the implementation) this explanation is not really clear to me what the effect exactly is.

Copy link
Member

@jorisvandenbossche jorisvandenbossche May 5, 2022

Choose a reason for hiding this comment

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

By looking at the code and the inline comments, it became clear. One of the comments is

    // Round to a multiple of units since the last greater unit.
    // For example: round to multiple of days since the beginning of the month or
    // to hours since the beginning of the day.

I wanted to suggest using that. Although reading the above again, that's actually already quite similar. But the additional example might help to clarify.

One question I have is how it is exactly defined what the "one less precise calendar unit" / "last greater unit" is? Eg we have "week" between "day" and "month", but we are still using "month" as the greater unit for "day"? (which probably makes sense in practice, but we should maybe document this somewhere more explicitly)

Copy link
Member Author

@rok rok May 5, 2022

Choose a reason for hiding this comment

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

Makes sense. I'll move the comments around.
Regarding the unit ordering I'm basically implementing for lubridate parity (and dropping bimonth, season, halfyear units). I suppose we can either document behaviour or let user choose the "origin unit". I would prefer to just document for now.

Copy link
Member Author

@rok rok May 6, 2022

Choose a reason for hiding this comment

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

How about round_from_greater_unit ?

Copy link
Member

@jorisvandenbossche jorisvandenbossche May 6, 2022

Choose a reason for hiding this comment

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

This is only relevant if multiple is not 1, right? Because in that case it could also be something like multiple_since_greater_unit

Copy link
Member Author

@rok rok May 6, 2022

Choose a reason for hiding this comment

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

Yeah, only for multiple != 1. multiple_since_greater_unit sounds good.

Copy link
Member Author

@rok rok May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Changed to multiple_since_greater_unit.

};

class ARROW_EXPORT RoundToMultipleOptions : public FunctionOptions {
Expand Down
Loading