You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "ZhangHuiGui (via GitHub)" <gi...@apache.org> on 2024/04/04 15:37:58 UTC
[PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
ZhangHuiGui opened a new pull request, #41012:
URL: https://github.com/apache/arrow/pull/41012
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
Fix the problem that decimal types array compare with different scales make error results.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
### What changes are included in this PR?
Add an output type resolver for decimal types in CompareFunction so can be casted correctly
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
### Are these changes tested?
Yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
### Are there any user-facing changes?
Yes, decimal type's comparison operations with different scale will be correct.
Co-authored-by ZhangHuiGui [2689496754@qq.com](mailto:2689496754@qq.com)
Co-authored-by laotan332 [qinpengxiang@outlook.com](mailto:qinpengxiang@outlook.com)
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "ZhangHuiGui (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "ZhangHuiGui (via GitHub)" <gi...@apache.org>.
ZhangHuiGui commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1604569617
##########
cpp/src/arrow/compute/kernels/scalar_compare.cc:
##########
@@ -385,6 +385,55 @@ struct VarArgsCompareFunction : ScalarFunction {
}
};
+class DecimalTypesCompareMatcher : public TypeMatcher {
+ public:
+ explicit DecimalTypesCompareMatcher(std::shared_ptr<TypeMatcher> decimal_type_matcher)
+ : decimal_type_matcher(std::move(decimal_type_matcher)) {}
+
+ bool Matches(const DataType& type) const override {
+ return decimal_type_matcher->Matches(type);
+ }
+
+ bool Matches(const std::vector<TypeHolder>& types) const override {
+ DCHECK_EQ(types.size(), 2);
+ if (!is_decimal(*types[0]) || !is_decimal(*types[1])) {
+ return true;
+ }
+
+ // Below match logic should only be executed when types are both decimal
+ //
+ const auto& left_type = checked_cast<const DecimalType&>(*types[0]);
+ const auto& right_type = checked_cast<const DecimalType&>(*types[1]);
+ assert(is_decimal(left_type) && is_decimal(right_type));
+
+ // check the decimal types' scales according kAdd promotion rule
+ const int32_t s1 = left_type.scale();
+ const int32_t s2 = right_type.scale();
Review Comment:
Yes, Add promotion rule only care about the scale and the implicit cast with this rule only keep the scale same.
https://github.com/apache/arrow/blob/2dbc5e26dcbc6826b4eb7a330fa8090836f6b727/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc#L509-L523
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "ZhangHuiGui (via GitHub)" <gi...@apache.org>.
ZhangHuiGui commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1604537627
##########
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:
> As long as it checks only the types and the code is simple enough that we can reason about the set of combinations handled.
Thanks, it's more concise to use `TypeMatcher` check the input types.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1628457872
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -109,6 +109,9 @@ struct ARROW_EXPORT TypeMatcher {
/// \brief Return true if this matcher accepts the data type.
virtual bool Matches(const DataType& type) const = 0;
+ /// \brief Return true if this matcher accepts the combination types
+ virtual bool Matches(const std::vector<TypeHolder>& types) const { return true; }
+
Review Comment:
This class is for matching a single matcher (`this`) against a single type.
I don't think it's reasonable to extend the type matching this way as it makes type checking Turing-complete (wouldn't be easy/possible to encode the typing rules in a provably-decidable type system).
I'm sorry for this back and forth because I was the one that said "type matching machinery". I still think this logic shouldn't go on the output type resolver so I have a more precise suggestion this time. Since functions already have a way of implementing custom type matching rules -- `DispatchBest` —— you can add this logic in https://github.com/apache/arrow/blob/9ee6ea701e20d1b47934f977d87811624061d597/cpp/src/arrow/compute/kernels/scalar_compare.cc#L343-L345
and return `return arrow::compute::detail::NoMatchingKernel(this, *types);` or a `Status::NotImplemented` with a message that describes that you can't compare two decimals 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
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 problem here is that we need a special cast when the scales don't match because the generic cast can't currently handle that.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "ZhangHuiGui (via GitHub)" <gi...@apache.org>.
ZhangHuiGui commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1628675148
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -109,6 +109,9 @@ struct ARROW_EXPORT TypeMatcher {
/// \brief Return true if this matcher accepts the data type.
virtual bool Matches(const DataType& type) const = 0;
+ /// \brief Return true if this matcher accepts the combination types
+ virtual bool Matches(const std::vector<TypeHolder>& types) const { return true; }
+
Review Comment:
Ah, the root cause of the problem that this PR is trying to solve is actually CompareFunction with two different decimal scales in BindNonRecursive , it is impossible to enter the logic of `DispatchBest`.
https://github.com/apache/arrow/blob/9ee6ea701e20d1b47934f977d87811624061d597/cpp/src/arrow/compute/expression.cc#L556-L563
This is why we did type judgment in the output type Resolver before, so that we can return not-ok in the first `FinishBind` and enter `DispatchBest`.
`DispatchBest` does not need to make additional judgments, because it will cast according to the decimal rules, ensuring that different input scales are cast to the same scales according to `DecimalPromotion::kAdd`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "ZhangHuiGui (via GitHub)" <gi...@apache.org>.
ZhangHuiGui commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1604535032
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -505,9 +510,14 @@ bool KernelSignature::Equals(const KernelSignature& other) const {
}
bool KernelSignature::MatchesInputs(const std::vector<TypeHolder>& types) const {
+ auto is_match_combination_types = [&](const InputType& in_type) {
Review Comment:
Add type matcher acceptable rule for the combination types.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "ZhangHuiGui (via GitHub)" <gi...@apache.org>.
ZhangHuiGui commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1604535032
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -505,9 +510,14 @@ bool KernelSignature::Equals(const KernelSignature& other) const {
}
bool KernelSignature::MatchesInputs(const std::vector<TypeHolder>& types) const {
+ auto is_match_combination_types = [&](const InputType& in_type) {
Review Comment:
Add a simple type matcher accept rule for the combination types matched rules.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 closed pull request #41012: GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly
URL: https://github.com/apache/arrow/pull/41012
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "ZhangHuiGui (via GitHub)" <gi...@apache.org>.
ZhangHuiGui commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1604535032
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -505,9 +510,14 @@ bool KernelSignature::Equals(const KernelSignature& other) const {
}
bool KernelSignature::MatchesInputs(const std::vector<TypeHolder>& types) const {
+ auto is_match_combination_types = [&](const InputType& in_type) {
Review Comment:
Add type matcher accept rule for the combination types.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "ZhangHuiGui (via GitHub)" <gi...@apache.org>.
ZhangHuiGui commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1604571659
##########
cpp/src/arrow/compute/kernels/scalar_compare.cc:
##########
@@ -385,6 +385,55 @@ struct VarArgsCompareFunction : ScalarFunction {
}
};
+class DecimalTypesCompareMatcher : public TypeMatcher {
+ public:
+ explicit DecimalTypesCompareMatcher(std::shared_ptr<TypeMatcher> decimal_type_matcher)
+ : decimal_type_matcher(std::move(decimal_type_matcher)) {}
+
+ bool Matches(const DataType& type) const override {
+ return decimal_type_matcher->Matches(type);
+ }
+
+ bool Matches(const std::vector<TypeHolder>& types) const override {
+ DCHECK_EQ(types.size(), 2);
+ if (!is_decimal(*types[0]) || !is_decimal(*types[1])) {
+ return true;
+ }
+
+ // Below match logic should only be executed when types are both decimal
+ //
+ const auto& left_type = checked_cast<const DecimalType&>(*types[0]);
+ const auto& right_type = checked_cast<const DecimalType&>(*types[1]);
+ assert(is_decimal(left_type) && is_decimal(right_type));
Review Comment:
Ah, yeah. Thanks, the line is for test. :)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "ZhangHuiGui (via GitHub)" <gi...@apache.org>.
ZhangHuiGui commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1604569617
##########
cpp/src/arrow/compute/kernels/scalar_compare.cc:
##########
@@ -385,6 +385,55 @@ struct VarArgsCompareFunction : ScalarFunction {
}
};
+class DecimalTypesCompareMatcher : public TypeMatcher {
+ public:
+ explicit DecimalTypesCompareMatcher(std::shared_ptr<TypeMatcher> decimal_type_matcher)
+ : decimal_type_matcher(std::move(decimal_type_matcher)) {}
+
+ bool Matches(const DataType& type) const override {
+ return decimal_type_matcher->Matches(type);
+ }
+
+ bool Matches(const std::vector<TypeHolder>& types) const override {
+ DCHECK_EQ(types.size(), 2);
+ if (!is_decimal(*types[0]) || !is_decimal(*types[1])) {
+ return true;
+ }
+
+ // Below match logic should only be executed when types are both decimal
+ //
+ const auto& left_type = checked_cast<const DecimalType&>(*types[0]);
+ const auto& right_type = checked_cast<const DecimalType&>(*types[1]);
+ assert(is_decimal(left_type) && is_decimal(right_type));
+
+ // check the decimal types' scales according kAdd promotion rule
+ const int32_t s1 = left_type.scale();
+ const int32_t s2 = right_type.scale();
Review Comment:
Yes, `kAdd` promotion rule only care about the scale and the implicit cast with this rule only keep the scale same.
https://github.com/apache/arrow/blob/2dbc5e26dcbc6826b4eb7a330fa8090836f6b727/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc#L509-L523
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1600296592
##########
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:
> Is this reasonable for the design of TypeMatcher?
As long as it checks only the types and the code is simple enough that we can reason about the set of combinations handled.
If some combinations are invalid, but you want to generate a message, I think you could have the type matcher route bad combinations to these fail kernels that produce an error instead of automatically casting inputs.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41012:
URL: https://github.com/apache/arrow/pull/41012#issuecomment-2037546704
:warning: GitHub issue #41011 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "ZhangHuiGui (via GitHub)" <gi...@apache.org>.
ZhangHuiGui commented on PR #41012:
URL: https://github.com/apache/arrow/pull/41012#issuecomment-2049100708
@bkietz Would mind take a look at this? A simple fix for the decimal type's compare function:)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #41012:
URL: https://github.com/apache/arrow/pull/41012#issuecomment-3252268387
Issue resolved by #47459. I'm closing this now. Thank you @ZhangHuiGui all the same!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1604554818
##########
cpp/src/arrow/compute/kernels/scalar_compare.cc:
##########
@@ -385,6 +385,55 @@ struct VarArgsCompareFunction : ScalarFunction {
}
};
+class DecimalTypesCompareMatcher : public TypeMatcher {
+ public:
+ explicit DecimalTypesCompareMatcher(std::shared_ptr<TypeMatcher> decimal_type_matcher)
+ : decimal_type_matcher(std::move(decimal_type_matcher)) {}
+
+ bool Matches(const DataType& type) const override {
+ return decimal_type_matcher->Matches(type);
+ }
+
+ bool Matches(const std::vector<TypeHolder>& types) const override {
+ DCHECK_EQ(types.size(), 2);
+ if (!is_decimal(*types[0]) || !is_decimal(*types[1])) {
+ return true;
+ }
+
+ // Below match logic should only be executed when types are both decimal
+ //
+ const auto& left_type = checked_cast<const DecimalType&>(*types[0]);
+ const auto& right_type = checked_cast<const DecimalType&>(*types[1]);
+ assert(is_decimal(left_type) && is_decimal(right_type));
+
+ // check the decimal types' scales according kAdd promotion rule
+ const int32_t s1 = left_type.scale();
+ const int32_t s2 = right_type.scale();
Review Comment:
So same scale with different precision can be matched here?
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -505,9 +510,14 @@ bool KernelSignature::Equals(const KernelSignature& other) const {
}
bool KernelSignature::MatchesInputs(const std::vector<TypeHolder>& types) const {
+ auto is_match_combination_types = [&](const InputType& in_type) {
+ return in_type.kind() == InputType::USE_TYPE_MATCHER ? in_type.Matches(types) : true;
+ };
+
if (is_varargs_) {
for (size_t i = 0; i < types.size(); ++i) {
- if (!in_types_[std::min(i, in_types_.size() - 1)].Matches(*types[i])) {
+ auto in_type = in_types_[std::min(i, in_types_.size() - 1)];
Review Comment:
```suggestion
const auto& in_type = in_types_[std::min(i, in_types_.size() - 1)];
```
##########
cpp/src/arrow/compute/kernels/scalar_compare.cc:
##########
@@ -385,6 +385,55 @@ struct VarArgsCompareFunction : ScalarFunction {
}
};
+class DecimalTypesCompareMatcher : public TypeMatcher {
+ public:
+ explicit DecimalTypesCompareMatcher(std::shared_ptr<TypeMatcher> decimal_type_matcher)
+ : decimal_type_matcher(std::move(decimal_type_matcher)) {}
+
+ bool Matches(const DataType& type) const override {
+ return decimal_type_matcher->Matches(type);
+ }
+
+ bool Matches(const std::vector<TypeHolder>& types) const override {
+ DCHECK_EQ(types.size(), 2);
+ if (!is_decimal(*types[0]) || !is_decimal(*types[1])) {
+ return true;
+ }
+
+ // Below match logic should only be executed when types are both decimal
+ //
+ const auto& left_type = checked_cast<const DecimalType&>(*types[0]);
+ const auto& right_type = checked_cast<const DecimalType&>(*types[1]);
+ assert(is_decimal(left_type) && is_decimal(right_type));
Review Comment:
Is this duplicate?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "ZhangHuiGui (via GitHub)" <gi...@apache.org>.
ZhangHuiGui commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1604569617
##########
cpp/src/arrow/compute/kernels/scalar_compare.cc:
##########
@@ -385,6 +385,55 @@ struct VarArgsCompareFunction : ScalarFunction {
}
};
+class DecimalTypesCompareMatcher : public TypeMatcher {
+ public:
+ explicit DecimalTypesCompareMatcher(std::shared_ptr<TypeMatcher> decimal_type_matcher)
+ : decimal_type_matcher(std::move(decimal_type_matcher)) {}
+
+ bool Matches(const DataType& type) const override {
+ return decimal_type_matcher->Matches(type);
+ }
+
+ bool Matches(const std::vector<TypeHolder>& types) const override {
+ DCHECK_EQ(types.size(), 2);
+ if (!is_decimal(*types[0]) || !is_decimal(*types[1])) {
+ return true;
+ }
+
+ // Below match logic should only be executed when types are both decimal
+ //
+ const auto& left_type = checked_cast<const DecimalType&>(*types[0]);
+ const auto& right_type = checked_cast<const DecimalType&>(*types[1]);
+ assert(is_decimal(left_type) && is_decimal(right_type));
+
+ // check the decimal types' scales according kAdd promotion rule
+ const int32_t s1 = left_type.scale();
+ const int32_t s2 = right_type.scale();
Review Comment:
Yes, `kAdd` promotion rule only care about the scale and the implicit cast with this rule only keep scales same.
https://github.com/apache/arrow/blob/2dbc5e26dcbc6826b4eb7a330fa8090836f6b727/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc#L509-L523
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #41012:
URL: https://github.com/apache/arrow/pull/41012#issuecomment-2150915365
> cc @felipecrv , the new changes include the `TypeMatcher` way to return failure for combination types.
I've been away. I will take a look soon.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "ZhangHuiGui (via GitHub)" <gi...@apache.org>.
ZhangHuiGui commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1628675148
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -109,6 +109,9 @@ struct ARROW_EXPORT TypeMatcher {
/// \brief Return true if this matcher accepts the data type.
virtual bool Matches(const DataType& type) const = 0;
+ /// \brief Return true if this matcher accepts the combination types
+ virtual bool Matches(const std::vector<TypeHolder>& types) const { return true; }
+
Review Comment:
Ah, the root cause of the problem that this PR is trying to solve is actually CompareFunction with two different decimal scales in BindNonRecursive , it is impossible to enter the logic of `DispatchBest`.
https://github.com/apache/arrow/blob/9ee6ea701e20d1b47934f977d87811624061d597/cpp/src/arrow/compute/expression.cc#L556-L563
Which means the `DispatchExact ` will always return ok and can't go into `DispatchBest` when `CompareFunction` called by expression system with two different decimal scales.
This is why we did type judgment in the output type Resolver before, so that we can return not-ok in the first `FinishBind` and enter `DispatchBest`.
`DispatchBest` does not need to make additional judgments, because it will do cast according to the decimal rules, ensuring that different input scales are cast to the same scales according to `DecimalPromotion::kAdd`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "ZhangHuiGui (via GitHub)" <gi...@apache.org>.
ZhangHuiGui commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1596947045
##########
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:
Besides, there is a similar case in `IfElse` related kernel functions : https://github.com/apache/arrow/pull/41363.
Do the input types' check in the output `resolver` in `IfElse` case is reasonable?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1596876371
##########
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:
Or maybe we want users to be explicit with casts before comparing. I still think this should be handled by the input matching machinery.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41012:
URL: https://github.com/apache/arrow/pull/41012#discussion_r1600299200
##########
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:
That way, the output type of "compare" remains boolean as it should be. No extra logic needed for output type resolution, but the function is not total — for some input types, it produces a bad `Status`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41011: [C++] Add an output type resolver for decimal types in CompareFunction so can be casted correctly [arrow]
Posted by "ZhangHuiGui (via GitHub)" <gi...@apache.org>.
ZhangHuiGui commented on PR #41012:
URL: https://github.com/apache/arrow/pull/41012#issuecomment-2121493115
cc @felipecrv , the new changes include the `TypeMatcher` way to return failure for combination types.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org