ZhangHuiGui commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1596938290
##########
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:
> but what you really doing here is asserting on the inputs.
Yes, this line of code `DCHECK_EQ(left_type.id(), right_type.id());` in
`ResolveDecimalCompareOutputType` is problematic because we might be comparing
`float and decimal` or `decimal128 and decimal256`...
> Or maybe we want users to be explicit with casts before comparing. I still
think this should be handled by the input matching machinery.
The current matcher system only works on matching of builtin types, and what
the compare kernel function needs is whether the dependencies between builtin
types meet the requirements. This requires adding a `bool Matches(const
std::vector<TypeHolder>& types) const;` interface to `TypeMatcher`.
That means the previous semantics of `TypeMatcher` was to check the validity
of builtin types. Now it is necessary to check the legality of dependencies
generated when functions use builtin types. Is this reasonable for the design
of TypeMatcher?
But in fact, it is indeed more reasonable to use matcher to determine
whether the comparison operation of decimal types is legal.
--
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]
