-
Notifications
You must be signed in to change notification settings - Fork 4.2k
GH-50395: [C++] Support duration inputs in temporal rounding #50675
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
dc25fa7
61607b8
18e5d1a
a3d83f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3779,6 +3779,126 @@ TEST_F(ScalarTemporalTest, TestCeilFloorRoundTemporalDate) { | |
| CheckScalarUnary("ceil_temporal", arr_ns, arr_ns, &round_to_2_hours); | ||
| } | ||
|
|
||
| TEST_F(ScalarTemporalTest, TestCeilFloorRoundTemporalDuration) { | ||
| auto check_unit = [&](TimeUnit::type time_unit, CalendarUnit calendar_unit) { | ||
| RoundTemporalOptions round_to_2_units(2, calendar_unit); | ||
| auto values = ArrayFromJSON(duration(time_unit), "[0, 1, 2, 3, -1, -2, -3, null]"); | ||
|
|
||
| CheckScalarUnary("ceil_temporal", values, | ||
| ArrayFromJSON(duration(time_unit), "[0, 2, 2, 4, 0, -2, -2, null]"), | ||
| &round_to_2_units); | ||
| CheckScalarUnary("floor_temporal", values, | ||
| ArrayFromJSON(duration(time_unit), "[0, 0, 2, 2, -2, -2, -4, null]"), | ||
| &round_to_2_units); | ||
| CheckScalarUnary("round_temporal", values, | ||
| ArrayFromJSON(duration(time_unit), "[0, 2, 2, 4, 0, -2, -2, null]"), | ||
| &round_to_2_units); | ||
| }; | ||
|
|
||
| check_unit(TimeUnit::SECOND, CalendarUnit::SECOND); | ||
| check_unit(TimeUnit::MILLI, CalendarUnit::MILLISECOND); | ||
| check_unit(TimeUnit::MICRO, CalendarUnit::MICROSECOND); | ||
| check_unit(TimeUnit::NANO, CalendarUnit::NANOSECOND); | ||
|
|
||
| auto check_second_based_unit = [&](CalendarUnit calendar_unit, const char* values_json, | ||
| const char* ceil_round_json, | ||
| const char* floor_json) { | ||
| RoundTemporalOptions round_to_2_units(2, calendar_unit); | ||
| auto values = ArrayFromJSON(duration(TimeUnit::SECOND), values_json); | ||
|
|
||
| CheckScalarUnary("ceil_temporal", values, | ||
| ArrayFromJSON(duration(TimeUnit::SECOND), ceil_round_json), | ||
| &round_to_2_units); | ||
| CheckScalarUnary("floor_temporal", values, | ||
| ArrayFromJSON(duration(TimeUnit::SECOND), floor_json), | ||
| &round_to_2_units); | ||
| CheckScalarUnary("round_temporal", values, | ||
| ArrayFromJSON(duration(TimeUnit::SECOND), ceil_round_json), | ||
| &round_to_2_units); | ||
| }; | ||
|
|
||
| check_second_based_unit(CalendarUnit::MINUTE, | ||
| "[0, 60, 120, 180, -60, -120, -180, null]", | ||
| "[0, 120, 120, 240, 0, -120, -120, null]", | ||
| "[0, 0, 120, 120, -120, -120, -240, null]"); | ||
|
|
||
| check_second_based_unit(CalendarUnit::HOUR, | ||
| "[0, 3600, 7200, 10800, -3600, -7200, -10800, null]", | ||
| "[0, 7200, 7200, 14400, 0, -7200, -7200, null]", | ||
| "[0, 0, 7200, 7200, -7200, -7200, -14400, null]"); | ||
|
|
||
| // A day is treated as a physical 24-hour unit for duration values. | ||
| RoundTemporalOptions round_to_day(1, CalendarUnit::DAY); | ||
| auto day_values = ArrayFromJSON(duration(TimeUnit::SECOND), | ||
| "[0, 43200, 86399, 86400, -1, -43200, null]"); | ||
|
|
||
| CheckScalarUnary( | ||
| "ceil_temporal", day_values, | ||
| ArrayFromJSON(duration(TimeUnit::SECOND), "[0, 86400, 86400, 86400, 0, 0, null]"), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you check values in these tests manually?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I derived them from fixed-duration semantics and the existing tie-breaking behavior, but I’ll independently verify each expected value while updating the tests.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a lot of downstream users that will potentially use this functionality. We want to be as sure about these as possible. So derived + human review + agent review would be great.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I manually re-derived the expected values using fixed-duration boundaries, including negative values and halfway cases. I also added a pandas integration test that independently compares Arrow’s results against pandas for nanoseconds, microseconds, milliseconds, seconds, minutes, hours, days, and weeks. All 8 parameterized integration cases and the full 55-test C++ temporal suite pass locally.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A philosophical question perhaps: how can verify that you, the physical person, did manually verify these and it's not just your agent trying to convince me? |
||
| &round_to_day); | ||
| CheckScalarUnary( | ||
| "floor_temporal", day_values, | ||
| ArrayFromJSON(duration(TimeUnit::SECOND), "[0, 0, 0, 86400, -86400, -86400, null]"), | ||
| &round_to_day); | ||
| CheckScalarUnary( | ||
| "round_temporal", day_values, | ||
| ArrayFromJSON(duration(TimeUnit::SECOND), "[0, 86400, 86400, 86400, 0, 0, null]"), | ||
| &round_to_day); | ||
|
|
||
| // A week is treated as exactly seven physical days for duration values. | ||
| RoundTemporalOptions round_to_week(1, CalendarUnit::WEEK); | ||
| auto week_values = | ||
| ArrayFromJSON(duration(TimeUnit::SECOND), | ||
| "[0, 302400, 604799, 604800, -1, -302400, -604800, null]"); | ||
|
|
||
| CheckScalarUnary("ceil_temporal", week_values, | ||
| ArrayFromJSON(duration(TimeUnit::SECOND), | ||
| "[0, 604800, 604800, 604800, 0, 0, -604800, null]"), | ||
| &round_to_week); | ||
| CheckScalarUnary("floor_temporal", week_values, | ||
| ArrayFromJSON(duration(TimeUnit::SECOND), | ||
| "[0, 0, 0, 604800, -604800, -604800, -604800, null]"), | ||
| &round_to_week); | ||
| CheckScalarUnary("round_temporal", week_values, | ||
| ArrayFromJSON(duration(TimeUnit::SECOND), | ||
| "[0, 604800, 604800, 604800, 0, 0, -604800, null]"), | ||
| &round_to_week); | ||
|
|
||
| auto values = ArrayFromJSON(duration(TimeUnit::SECOND), "[0, 1, -1, null]"); | ||
|
|
||
| RoundTemporalOptions round_to_month(1, CalendarUnit::MONTH); | ||
| for (const auto* function_name : | ||
| {"ceil_temporal", "floor_temporal", "round_temporal"}) { | ||
| EXPECT_RAISES_WITH_MESSAGE_THAT( | ||
| Invalid, ::testing::HasSubstr("Duration values can only be rounded using units"), | ||
| CallFunction(function_name, {values}, &round_to_month)); | ||
| } | ||
|
|
||
| RoundTemporalOptions calendar_origin(2, CalendarUnit::SECOND); | ||
| calendar_origin.calendar_based_origin = true; | ||
| for (const auto* function_name : | ||
| {"ceil_temporal", "floor_temporal", "round_temporal"}) { | ||
| EXPECT_RAISES_WITH_MESSAGE_THAT( | ||
| Invalid, | ||
| ::testing::HasSubstr( | ||
| "calendar_based_origin is not supported for duration inputs"), | ||
| CallFunction(function_name, {values}, &calendar_origin)); | ||
| } | ||
| } | ||
|
|
||
| TEST_F(ScalarTemporalTest, TestDurationTemporalWeekMultipleOverflow) { | ||
| auto values = ArrayFromJSON(duration(TimeUnit::SECOND), "[0]"); | ||
| RoundTemporalOptions options(std::numeric_limits<int>::max(), CalendarUnit::WEEK); | ||
|
|
||
| for (const auto* function_name : | ||
| {"ceil_temporal", "floor_temporal", "round_temporal"}) { | ||
| EXPECT_RAISES_WITH_MESSAGE_THAT( | ||
| Invalid, | ||
| ::testing::HasSubstr("Duration week multiple would not fit in 32-bit integer"), | ||
| CallFunction(function_name, {values}, &options)); | ||
| } | ||
| } | ||
|
|
||
| TEST_F(ScalarTemporalTest, DurationUnaryArithmetics) { | ||
| auto arr = ArrayFromJSON(duration(TimeUnit::SECOND), "[2, -1, null, 3, 0]"); | ||
| CheckScalarUnary("negate", arr, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.