Skip to content

ARROW-14100: [C++] subtract(duration, duration) -> duration kernel #12141

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

Closed
rok wants to merge 3 commits into from
Closed

ARROW-14100: [C++] subtract(duration, duration) -> duration kernel #12141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f38de69
Subtract duration
rok Jan 13, 2022
eaa5c63
Add ReplaceTemporalTypes
rok Jan 19, 2022
4f6675d
Review feedback
rok Feb 15, 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
16 changes: 16 additions & 0 deletions cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2660,6 +2660,13 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
std::move(exec)));
}

// Add subtract(duration, duration) -> duration
for (auto unit : TimeUnit::values()) {
InputType in_type(match::DurationTypeUnit(unit));
auto exec = ArithmeticExecFromOp<ScalarBinaryEqualTypes, Subtract>(Type::DURATION);
DCHECK_OK(subtract->AddKernel({in_type, in_type}, duration(unit), std::move(exec)));
}

// Add subtract(time32, time32) -> duration
for (auto unit : {TimeUnit::SECOND, TimeUnit::MILLI}) {
InputType in_type(match::Time32TypeUnit(unit));
Expand Down Expand Up @@ -2714,6 +2721,15 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
OutputType(FirstType), std::move(exec)));
}

// Add subtract_checked(duration, duration) -> duration
for (auto unit : TimeUnit::values()) {
InputType in_type(match::DurationTypeUnit(unit));
auto exec =
ArithmeticExecFromOp<ScalarBinaryEqualTypes, SubtractChecked>(Type::DURATION);
DCHECK_OK(
subtract_checked->AddKernel({in_type, in_type}, duration(unit), std::move(exec)));
}

// Add subtract_checked(date32, date32) -> duration(TimeUnit::SECOND)
auto exec_date_32_checked =
ScalarBinaryEqualTypes<Int64Type, Date32Type, SubtractCheckedDate32>::Exec;
Expand Down
26 changes: 26 additions & 0 deletions cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1429,6 +1429,32 @@ TEST_F(ScalarTemporalTest, TestTemporalSubtractTimeAndDuration) {
}
}

TEST_F(ScalarTemporalTest, TestTemporalSubtractDuration) {
for (auto op : {"subtract", "subtract_checked"}) {
for (auto u : TimeUnit::values()) {
auto unit = duration(u);
CheckScalarBinary(op, ArrayFromJSON(unit, times_s2), ArrayFromJSON(unit, times_s),
ArrayFromJSON(unit, seconds_between_time));
}

auto seconds_3 = ArrayFromJSON(duration(TimeUnit::SECOND), R"([3, null])");
auto milliseconds_2k = ArrayFromJSON(duration(TimeUnit::MILLI), R"([2000, null])");
auto milliseconds_1k = ArrayFromJSON(duration(TimeUnit::MILLI), R"([1000, null])");
CheckScalarBinary(op, seconds_3, milliseconds_2k, milliseconds_1k);
}

for (auto unit : TimeUnit::values()) {
auto duration_ty = duration(unit);
auto arr1 = ArrayFromJSON(duration_ty, R"([-9223372036854775808, null])");
auto arr2 = ArrayFromJSON(duration_ty, R"([1, null])");
auto arr3 = ArrayFromJSON(duration_ty, R"([9223372036854775807, null])");

CheckScalarBinary("subtract", arr1, arr2, arr3);
EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, ::testing::HasSubstr("overflow"),
CallFunction("subtract_checked", {arr1, arr2}));
}
}
Copy link
Member

@pitrou pitrou Feb 15, 2022

Choose a reason for hiding this comment

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

Can you add tests for overflowing inputs?

Copy link
Member Author

@rok rok Feb 16, 2022

Choose a reason for hiding this comment

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

Added. However it seems as it duration is not treated as int64_t here and overflow is not detected?

Copy link
Member Author

@rok rok Feb 16, 2022

Choose a reason for hiding this comment

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

Never mind. I was using the wrong operator.


Copy link
Contributor

@edponce edponce Jan 18, 2022

Choose a reason for hiding this comment

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

Add tests with valid values.

Copy link
Member Author

@rok rok Jan 18, 2022

Choose a reason for hiding this comment

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

Do you mean like these?

I'd like to enable cases like subtract(duration[ms], duration[us]) -> duration[us]. I'll ping when it's done.

Copy link
Member Author

@rok rok Jan 19, 2022

Choose a reason for hiding this comment

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

Ok, this now covers cases like subtract(duration[ms], duration[us]) -> duration[us].

TEST_F(ScalarTemporalTest, TestTemporalDifferenceWeeks) {
auto raw_days = ArrayFromJSON(timestamp(TimeUnit::SECOND), R"([
"2021-08-09", "2021-08-10", "2021-08-11", "2021-08-12", "2021-08-13", "2021-08-14", "2021-08-15",
Expand Down
66 changes: 33 additions & 33 deletions docs/source/cpp/compute.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,39 +423,39 @@ floating-point arguments will cast all arguments to floating-point, while mixed
decimal and integer arguments will cast all arguments to decimals.
Mixed time resolution temporal inputs will be cast to finest input resolution.

+------------------+--------+----------------------------+----------------------------+-------+
| Function name | Arity | Input types | Output type | Notes |
+==================+========+============================+============================+=======+
| abs | Unary | Numeric | Numeric | |
+------------------+--------+----------------------------+----------------------------+-------+
| abs_checked | Unary | Numeric | Numeric | |
+------------------+--------+----------------------------+----------------------------+-------+
| add | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
+------------------+--------+----------------------------+----------------------------+-------+
| add_checked | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
+------------------+--------+----------------------------+----------------------------+-------+
| divide | Binary | Numeric | Numeric | \(1) |
+------------------+--------+----------------------------+----------------------------+-------+
| divide_checked | Binary | Numeric | Numeric | \(1) |
+------------------+--------+----------------------------+----------------------------+-------+
| multiply | Binary | Numeric | Numeric | \(1) |
+------------------+--------+----------------------------+----------------------------+-------+
| multiply_checked | Binary | Numeric | Numeric | \(1) |
+------------------+--------+----------------------------+----------------------------+-------+
| negate | Unary | Numeric | Numeric | |
+------------------+--------+----------------------------+----------------------------+-------+
| negate_checked | Unary | Signed Numeric | Signed Numeric | |
+------------------+--------+----------------------------+----------------------------+-------+
| power | Binary | Numeric | Numeric | |
+------------------+--------+----------------------------+----------------------------+-------+
| power_checked | Binary | Numeric | Numeric | |
+------------------+--------+----------------------------+----------------------------+-------+
| sign | Unary | Numeric | Int8/Float32/Float64 | \(2) |
+------------------+--------+----------------------------+----------------------------+-------+
| subtract | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
+------------------+--------+----------------------------+----------------------------+-------+
| subtract_checked | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
+------------------+--------+----------------------------+----------------------------+-------+
+------------------+--------+------------------+----------------------+-------+
| Function name | Arity | Input types | Output type | Notes |
+==================+========+==================+======================+=======+
| abs | Unary | Numeric | Numeric | |
+------------------+--------+------------------+----------------------+-------+
| abs_checked | Unary | Numeric | Numeric | |
+------------------+--------+------------------+----------------------+-------+
| add | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
+------------------+--------+------------------+----------------------+-------+
| add_checked | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
+------------------+--------+------------------+----------------------+-------+
| divide | Binary | Numeric | Numeric | \(1) |
+------------------+--------+------------------+----------------------+-------+
| divide_checked | Binary | Numeric | Numeric | \(1) |
+------------------+--------+------------------+----------------------+-------+
| multiply | Binary | Numeric | Numeric | \(1) |
+------------------+--------+------------------+----------------------+-------+
| multiply_checked | Binary | Numeric | Numeric | \(1) |
+------------------+--------+------------------+----------------------+-------+
| negate | Unary | Numeric | Numeric | |
+------------------+--------+------------------+----------------------+-------+
| negate_checked | Unary | Signed Numeric | Signed Numeric | |
+------------------+--------+------------------+----------------------+-------+
| power | Binary | Numeric | Numeric | |
+------------------+--------+------------------+----------------------+-------+
| power_checked | Binary | Numeric | Numeric | |
+------------------+--------+------------------+----------------------+-------+
| sign | Unary | Numeric | Int8/Float32/Float64 | \(2) |
+------------------+--------+------------------+----------------------+-------+
| subtract | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
+------------------+--------+------------------+----------------------+-------+
| subtract_checked | Binary | Numeric/Temporal | Numeric/Temporal | \(1) |
+------------------+--------+------------------+----------------------+-------+

* \(1) Precision and scale of computed DECIMAL results

Expand Down