felipecrv commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1596873928
##########
cpp/src/arrow/compute/kernels/scalar_compare.cc:
##########
@@ -385,6 +385,23 @@ struct VarArgsCompareFunction : ScalarFunction {
}
};
+Result<TypeHolder> ResolveDecimalCompareOutputType(KernelContext*,
+ const
std::vector<TypeHolder>& types) {
+ // casted types should be same size decimals
+ const auto& left_type = checked_cast<const DecimalType&>(*types[0]);
+ const auto& right_type = checked_cast<const DecimalType&>(*types[1]);
+ DCHECK_EQ(left_type.id(), right_type.id());
+
+ // check the casted decimal scales according kAdd promotion rule
+ const int32_t s1 = left_type.scale();
+ const int32_t s2 = right_type.scale();
+ if (s1 != s2) {
+ return Status::Invalid("Comparison of two decimal ", "types s1 != s2. (",
s1, s2,
+ ").");
+ }
+ return boolean();
+}
+
Review Comment:
I've read the issue and it seems that the issue here is that we need a
special cast when the scales don't match because the generic cast can't
currently handle that.
##########
cpp/src/arrow/compute/kernels/scalar_compare.cc:
##########
@@ -385,6 +385,23 @@ struct VarArgsCompareFunction : ScalarFunction {
}
};
+Result<TypeHolder> ResolveDecimalCompareOutputType(KernelContext*,
+ const
std::vector<TypeHolder>& types) {
+ // casted types should be same size decimals
+ const auto& left_type = checked_cast<const DecimalType&>(*types[0]);
+ const auto& right_type = checked_cast<const DecimalType&>(*types[1]);
+ DCHECK_EQ(left_type.id(), right_type.id());
+
+ // check the casted decimal scales according kAdd promotion rule
+ const int32_t s1 = left_type.scale();
+ const int32_t s2 = right_type.scale();
+ if (s1 != s2) {
+ return Status::Invalid("Comparison of two decimal ", "types s1 != s2. (",
s1, s2,
+ ").");
+ }
+ return boolean();
+}
+
Review Comment:
It's an interesting approach to use the output-type resolver to assert on
the input types, but what you really doing here is asserting on the inputs. I
think you should write a custom matcher instead. Compare always returns
`boolean()`, there is nothing to resolve.
That would also help (in the future) dispatching to kernels that can deal
with different scales dynamically. Values can still be logically equal even
though they are represented physically in memory with different scales.
--
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]
