lidavidm commented on a change in pull request #12124:
URL: https://github.com/apache/arrow/pull/12124#discussion_r797112068
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -1611,6 +1636,23 @@ Result<ValueDescr>
ResolveDecimalDivisionOutput(KernelContext*,
});
}
+Result<ValueDescr> ResolveTemporalOutput(KernelContext*,
+ const std::vector<ValueDescr>& args) {
+ auto left_type = checked_cast<const TimestampType*>(args[0].type.get());
+ auto right_type = checked_cast<const TimestampType*>(args[1].type.get());
+ DCHECK_EQ(left_type->id(), right_type->id());
Review comment:
nit, but the checked_cast should account for this in debug mode since
the dynamic_cast will give us nullptr (or else if we're going to assert, we
should assert before casting)
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -1611,6 +1636,23 @@ Result<ValueDescr>
ResolveDecimalDivisionOutput(KernelContext*,
});
}
+Result<ValueDescr> ResolveTemporalOutput(KernelContext*,
+ const std::vector<ValueDescr>& args) {
+ auto left_type = checked_cast<const TimestampType*>(args[0].type.get());
+ auto right_type = checked_cast<const TimestampType*>(args[1].type.get());
+ DCHECK_EQ(left_type->id(), right_type->id());
+
+ if ((left_type->timezone() == "" || right_type->timezone() == "") &&
+ left_type->timezone() != right_type->timezone()) {
+ RETURN_NOT_OK(
+ Status::Invalid("Subtraction of zoned and non-zoned times is
ambivalent. (",
+ left_type->timezone(), right_type->timezone(), ")."));
+ }
+
+ auto type = duration(std::max(left_type->unit(), right_type->unit()));
Review comment:
(I think return types are computed post-implicit-casts, right? So they
should match here.)
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -1611,6 +1636,23 @@ Result<ValueDescr>
ResolveDecimalDivisionOutput(KernelContext*,
});
}
+Result<ValueDescr> ResolveTemporalOutput(KernelContext*,
+ const std::vector<ValueDescr>& args) {
+ auto left_type = checked_cast<const TimestampType*>(args[0].type.get());
+ auto right_type = checked_cast<const TimestampType*>(args[1].type.get());
+ DCHECK_EQ(left_type->id(), right_type->id());
+
+ if ((left_type->timezone() == "" || right_type->timezone() == "") &&
+ left_type->timezone() != right_type->timezone()) {
+ RETURN_NOT_OK(
+ Status::Invalid("Subtraction of zoned and non-zoned times is
ambivalent. (",
+ left_type->timezone(), right_type->timezone(), ")."));
+ }
+
+ auto type = duration(std::max(left_type->unit(), right_type->unit()));
Review comment:
Ah, I was thinking, since we only register kernels where the units
match, technically this max is redundant, but it doesn't hurt to have it.
(Though perhaps it would be safer to DCHECK or return an error if they don't
match to avoid accidentally doing arithmetic on incompatible types.)
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -1611,6 +1636,23 @@ Result<ValueDescr>
ResolveDecimalDivisionOutput(KernelContext*,
});
}
+Result<ValueDescr> ResolveTemporalOutput(KernelContext*,
+ const std::vector<ValueDescr>& args) {
+ auto left_type = checked_cast<const TimestampType*>(args[0].type.get());
+ auto right_type = checked_cast<const TimestampType*>(args[1].type.get());
+ DCHECK_EQ(left_type->id(), right_type->id());
+
+ if ((left_type->timezone() == "" || right_type->timezone() == "") &&
+ left_type->timezone() != right_type->timezone()) {
+ RETURN_NOT_OK(
+ Status::Invalid("Subtraction of zoned and non-zoned times is
ambiguous. (",
+ left_type->timezone(), right_type->timezone(), ")."));
Review comment:
nit, but just `return Status::Invalid(...);`?
--
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]
