You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zanmato1984 (via GitHub)" <gi...@apache.org> on 2025/08/08 17:43:05 UTC
[PR] GH-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
zanmato1984 opened a new pull request, #47297:
URL: https://github.com/apache/arrow/pull/47297
### Rationale for this change
A rework of #40223 using a more systematic alternative.
### What changes are included in this PR?
Introduce a structure `MatchConstraint` for applying extra (and optional) matching constraint for kernel signature matching, in additional to simply input type checks.
Also implement two concrete `MatchConstraint`s for binary decimal arithmetic kernels, to suppress exact match even if the input types are OK, for example, by requiring all decimal must be of the same scale for `add` and `subtract`, and s1 >= s2 for `divide`.
This should also be a fundamental enhancement to further resolve similar issues like:
* #35843
* #39875
* #40911
* #41011
* #41336
### Are these changes tested?
UT included.
### Are there any user-facing changes?
New public class `MatchConstraint`.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2284730503
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -669,13 +669,16 @@ void AddDecimalUnaryKernels(ScalarFunction* func) {
template <typename Op>
void AddDecimalBinaryKernels(const std::string& name, ScalarFunction* func) {
OutputType out_type(null());
+ std::shared_ptr<MatchConstraint> constraint = nullptr;
const std::string op = name.substr(0, name.find("_"));
if (op == "add" || op == "subtract") {
out_type = OutputType(ResolveDecimalAdditionOrSubtractionOutput);
+ constraint = DecimalsHaveSameScale();
} else if (op == "multiply") {
out_type = OutputType(ResolveDecimalMultiplicationOutput);
} else if (op == "divide") {
out_type = OutputType(ResolveDecimalDivisionOutput);
+ constraint = BinaryDecimalScaleComparisonGE();
Review Comment:
Since we add some constraints here, does it mean that we can remove some error return paths from the various `ResolveDecimal` functions?
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47297:
URL: https://github.com/apache/arrow/pull/47297#issuecomment-3168845195
Looping in some people who might be interested. @pitrou @kou @westonpace @bkietz @felipecrv @ZhangHuiGui
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47297:
URL: https://github.com/apache/arrow/pull/47297#issuecomment-3168842310
@github-actions crossbow submit -g cpp
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2266076577
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Validate(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = dynamic_cast<const DecimalType*>(types[0].type);
Review Comment:
Can we use `checked_cast` here?
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -348,7 +348,24 @@ class ARROW_EXPORT OutputType {
Resolver resolver_ = NULLPTR;
};
-/// \brief Holds the input types and output type of the kernel.
+/// \brief Additional constraints to apply to the input types of a kernel when matching a
+/// specific kernel signature.
+class ARROW_EXPORT MatchConstraint {
+ public:
+ virtual ~MatchConstraint() = default;
+
+ /// \brief Return true if the input types satisfy the constraint.
+ virtual bool Validate(const std::vector<TypeHolder>& types) const = 0;
Review Comment:
`Validate()` -> `Matches()`?
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2285143249
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -348,7 +348,24 @@ class ARROW_EXPORT OutputType {
Resolver resolver_ = NULLPTR;
};
-/// \brief Holds the input types and output type of the kernel.
+/// \brief Additional constraints to apply to the input types of a kernel when matching a
+/// specific kernel signature.
+class ARROW_EXPORT MatchConstraint {
+ public:
+ virtual ~MatchConstraint() = default;
+
+ /// \brief Return true if the input types satisfy the constraint.
+ virtual bool Matches(const std::vector<TypeHolder>& types) const = 0;
+};
+
+/// \brief Constraint that all input types are decimal types and have the same scale.
+ARROW_EXPORT std::shared_ptr<MatchConstraint> DecimalsHaveSameScale();
+
+/// \brief Constraint that all binary input types are decimal types and the first type's
+/// scale >= the second type's.
+ARROW_EXPORT std::shared_ptr<MatchConstraint> BinaryDecimalScaleComparisonGE();
Review Comment:
> Is `BinaryDecimalScale1GeScale2` better?
Yes :) Not pretty, but more explicit anyway.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2263675377
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Validate(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = dynamic_cast<const DecimalType*>(types[0].type);
+ DCHECK_NE(ty0, nullptr);
+ auto s0 = ty0->scale();
+ for (size_t i = 1; i < types.size(); ++i) {
+ auto ty = dynamic_cast<const DecimalType*>(types[i].type);
+ DCHECK_NE(ty, nullptr);
+ if (ty->scale() != s0) {
+ return false;
+ }
+ }
+ return true;
+ }
+ };
+ static auto instance = std::make_shared<DecimalsHaveSameScaleConstraint>();
+ return instance;
+}
+
+namespace {
+
+template <typename Op>
+class BinaryDecimalScaleComparisonConstraint : public MatchConstraint {
+ public:
+ bool Validate(const std::vector<TypeHolder>& types) const override {
+ DCHECK_EQ(types.size(), 2);
+ DCHECK(is_decimal(types[0].id()));
+ DCHECK(is_decimal(types[1].id()));
+ auto ty0 = dynamic_cast<const DecimalType*>(types[0].type);
+ DCHECK_NE(ty0, nullptr);
+ auto ty1 = dynamic_cast<const DecimalType*>(types[1].type);
+ DCHECK_NE(ty1, nullptr);
+ return Op{}(ty0->scale(), ty1->scale());
+ }
+};
+
+} // namespace
+
+std::shared_ptr<MatchConstraint> BinaryDecimalScaleComparisonGE() {
+ using BinaryDecimalScaleComparisonGEConstraint =
+ BinaryDecimalScaleComparisonConstraint<std::greater_equal<>>;
+ static auto instance = std::make_shared<BinaryDecimalScaleComparisonGEConstraint>();
Review Comment:
The static instance may not be thread-safe during initialization. Consider using std::call_once or similar synchronization mechanism to ensure thread-safe initialization.
```suggestion
static std::shared_ptr<BinaryDecimalScaleComparisonGEConstraint> instance;
static std::once_flag flag;
std::call_once(flag, []() {
instance = std::make_shared<BinaryDecimalScaleComparisonGEConstraint>();
});
```
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Validate(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = dynamic_cast<const DecimalType*>(types[0].type);
+ DCHECK_NE(ty0, nullptr);
+ auto s0 = ty0->scale();
+ for (size_t i = 1; i < types.size(); ++i) {
+ auto ty = dynamic_cast<const DecimalType*>(types[i].type);
+ DCHECK_NE(ty, nullptr);
+ if (ty->scale() != s0) {
+ return false;
+ }
+ }
+ return true;
+ }
+ };
+ static auto instance = std::make_shared<DecimalsHaveSameScaleConstraint>();
Review Comment:
The static instance may not be thread-safe during initialization. Consider using std::call_once or similar synchronization mechanism to ensure thread-safe initialization.
```suggestion
static std::once_flag once;
static std::shared_ptr<DecimalsHaveSameScaleConstraint> instance;
std::call_once(once, []() {
instance = std::make_shared<DecimalsHaveSameScaleConstraint>();
});
```
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2288519961
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -348,7 +348,24 @@ class ARROW_EXPORT OutputType {
Resolver resolver_ = NULLPTR;
};
-/// \brief Holds the input types and output type of the kernel.
+/// \brief Additional constraints to apply to the input types of a kernel when matching a
+/// specific kernel signature.
+class ARROW_EXPORT MatchConstraint {
+ public:
+ virtual ~MatchConstraint() = default;
+
+ /// \brief Return true if the input types satisfy the constraint.
+ virtual bool Matches(const std::vector<TypeHolder>& types) const = 0;
Review Comment:
Thanks for pointing this out. I think we can do that in the future, along with other occurrences of `std::vector` in kernel matching calling chain.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2268280783
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Matches(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = checked_cast<const DecimalType*>(types[0].type);
+ DCHECK_NE(ty0, nullptr);
Review Comment:
For debug build, `checked_cast` leverages `dynamic_cast` and may return `nullptr` for pointer type.
Maybe what you really meant is that we use `checked_cast` on reference type? That does eliminate the need for doing null check.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2284976159
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -348,7 +348,24 @@ class ARROW_EXPORT OutputType {
Resolver resolver_ = NULLPTR;
};
-/// \brief Holds the input types and output type of the kernel.
+/// \brief Additional constraints to apply to the input types of a kernel when matching a
+/// specific kernel signature.
+class ARROW_EXPORT MatchConstraint {
+ public:
+ virtual ~MatchConstraint() = default;
+
+ /// \brief Return true if the input types satisfy the constraint.
+ virtual bool Matches(const std::vector<TypeHolder>& types) const = 0;
Review Comment:
Not any in particular other than making it the same level of abstraction as other kernel staff, e.g. `TypeMatcher`.
> Of course we can also have an API that creates a `MatchConstraint` from a `std::function`, for convenience.
Yes, that makes sense.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2266428442
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -348,7 +348,24 @@ class ARROW_EXPORT OutputType {
Resolver resolver_ = NULLPTR;
};
-/// \brief Holds the input types and output type of the kernel.
+/// \brief Additional constraints to apply to the input types of a kernel when matching a
+/// specific kernel signature.
+class ARROW_EXPORT MatchConstraint {
+ public:
+ virtual ~MatchConstraint() = default;
+
+ /// \brief Return true if the input types satisfy the constraint.
+ virtual bool Validate(const std::vector<TypeHolder>& types) const = 0;
Review Comment:
Hmm, I don't think it's natural to say a constraint "matches" the given list of 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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2268403021
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Matches(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = checked_cast<const DecimalType*>(types[0].type);
+ DCHECK_NE(ty0, nullptr);
Review Comment:
> Sorry I'm not following. Those two `static_assert`s only assert the from and to types are classes. There can be cases that both are classes but without inheritance, that can still result in `nullptr`?
Oh, sorry. I haven't looked at these `static_assert`s carefully... You're right.
> This `is_decimal` merely checks the internal `type_id`. I'm not sure if we can assume `ty.type_id == Type::DECIMAL` implies `typeof(ty) == class DecimalType`.
I think that we can assume it based on the current implementation but I'm OK with keeping this `DCHECK_NE()` now.
https://github.com/apache/arrow/blob/2b8db0094baed09875d44ebe33b78dd7ce859ab5/cpp/src/arrow/type_traits.h#L1109-L1120
https://github.com/apache/arrow/blob/2b8db0094baed09875d44ebe33b78dd7ce859ab5/cpp/src/arrow/type.h#L1047-L1049
https://github.com/apache/arrow/blob/2b8db0094baed09875d44ebe33b78dd7ce859ab5/cpp/src/arrow/type.h#L1082-L1084
https://github.com/apache/arrow/blob/2b8db0094baed09875d44ebe33b78dd7ce859ab5/cpp/src/arrow/type.h#L1116-L1118
https://github.com/apache/arrow/blob/2b8db0094baed09875d44ebe33b78dd7ce859ab5/cpp/src/arrow/type.h#L1149-L1151
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2285023001
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -669,13 +669,16 @@ void AddDecimalUnaryKernels(ScalarFunction* func) {
template <typename Op>
void AddDecimalBinaryKernels(const std::string& name, ScalarFunction* func) {
OutputType out_type(null());
+ std::shared_ptr<MatchConstraint> constraint = nullptr;
const std::string op = name.substr(0, name.find("_"));
if (op == "add" || op == "subtract") {
out_type = OutputType(ResolveDecimalAdditionOrSubtractionOutput);
+ constraint = DecimalsHaveSameScale();
} else if (op == "multiply") {
out_type = OutputType(ResolveDecimalMultiplicationOutput);
} else if (op == "divide") {
out_type = OutputType(ResolveDecimalDivisionOutput);
+ constraint = BinaryDecimalScaleComparisonGE();
Review Comment:
Good point. Yes that's possible, as we are now suppressing the match for decimal arguments not qualifying the constraints. The explicit error returning could be a simple `DCHECK` now.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #47297:
URL: https://github.com/apache/arrow/pull/47297#issuecomment-3168847761
Revision: 489796020ea9ee4cf52d0dda9e8f64c91d9049ee
Submitted crossbow builds: [ursacomputing/crossbow @ actions-fc605b322e](https://github.com/ursacomputing/crossbow/branches/all?query=actions-fc605b322e)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977440/job/47698725964)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977899/job/47698727158)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977826/job/47698726923)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977866/job/47698727146)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977269/job/47698725590)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977693/job/47698726563)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977451/job/47698725986)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977984/job/47698727438)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977913/job/47698727217)|
|test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977801/job/47698726796)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977271/job/47698725620)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977305/job/47698725569)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977874/job/47698727079)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977548/job/47698726245)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977248/job/47698725522)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977582/job/47698726331)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977568/job/47698726255)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977583/job/47698726413)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977498/job/47698726064)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977571/job/47698726302)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/16836977593/job/47698726381)|
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2268383523
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Matches(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = checked_cast<const DecimalType*>(types[0].type);
+ DCHECK_NE(ty0, nullptr);
Review Comment:
> I thought that `checked_cast` never return `nullptr` because there are `static_assert()`s before `dynamic_cast`:
>
> https://github.com/apache/arrow/blob/db1a8a9bf42bec2b643fc19d8e174dd5166cd621/cpp/src/arrow/util/checked_cast.h#L29-L34
Sorry I'm not following. Those two `static_assert`s only assert the from and to types are classes. There can be cases that both are classes but without inheritance, that can still result in `nullptr`?
>
> BTW, there is
>
> ```c++
> DCHECK(std::all_of(types.begin(), types.end(),
> [](const TypeHolder& type) { return is_decimal(type.id()); }));
> ```
>
> before this `checked_cast`. It seems that this `DCHECK()` will be failed when `checked_cast` return `nullptr`.
This `is_decimal` merely checks the internal `type_id`. I'm not sure if we can assume `ty.type_id == Type::DECIMAL` implies `typeof(ty) == class DecimalType`.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47297:
URL: https://github.com/apache/arrow/pull/47297#issuecomment-3177606089
> Can we merge this now? Should we wait for a few days before we merge this?
I think we can keep it open for another two or three days. And thanks for reviewing!
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 merged PR #47297:
URL: https://github.com/apache/arrow/pull/47297
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #47297:
URL: https://github.com/apache/arrow/pull/47297#issuecomment-3208246177
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit dadc21f31b1d43cab4eb763f5e65691ce1f08fd5.
There weren't enough matching historic benchmark results to make a call on whether there were regressions.
The [full Conbench report](https://github.com/apache/arrow/runs/48534284746) has more details.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2284719337
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -348,7 +348,24 @@ class ARROW_EXPORT OutputType {
Resolver resolver_ = NULLPTR;
};
-/// \brief Holds the input types and output type of the kernel.
+/// \brief Additional constraints to apply to the input types of a kernel when matching a
+/// specific kernel signature.
+class ARROW_EXPORT MatchConstraint {
+ public:
+ virtual ~MatchConstraint() = default;
+
+ /// \brief Return true if the input types satisfy the constraint.
+ virtual bool Matches(const std::vector<TypeHolder>& types) const = 0;
Review Comment:
I'm curious: is there a particular need for making this a full-blown abstract class, or can we be content with a simpler `std::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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2285174210
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -348,7 +348,24 @@ class ARROW_EXPORT OutputType {
Resolver resolver_ = NULLPTR;
};
-/// \brief Holds the input types and output type of the kernel.
+/// \brief Additional constraints to apply to the input types of a kernel when matching a
+/// specific kernel signature.
+class ARROW_EXPORT MatchConstraint {
+ public:
+ virtual ~MatchConstraint() = default;
+
+ /// \brief Return true if the input types satisfy the constraint.
+ virtual bool Matches(const std::vector<TypeHolder>& types) const = 0;
+};
+
+/// \brief Constraint that all input types are decimal types and have the same scale.
+ARROW_EXPORT std::shared_ptr<MatchConstraint> DecimalsHaveSameScale();
+
+/// \brief Constraint that all binary input types are decimal types and the first type's
+/// scale >= the second type's.
+ARROW_EXPORT std::shared_ptr<MatchConstraint> BinaryDecimalScaleComparisonGE();
Review Comment:
Thanks. Updated.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2266424491
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Validate(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = dynamic_cast<const DecimalType*>(types[0].type);
Review Comment:
Oh yes, forgot 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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2268112664
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Matches(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = checked_cast<const DecimalType*>(types[0].type);
+ DCHECK_NE(ty0, nullptr);
Review Comment:
Can we remove this with `checked_cast`?
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47297:
URL: https://github.com/apache/arrow/pull/47297#issuecomment-3179933913
I can take a look on Monday.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2285102730
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -348,7 +348,24 @@ class ARROW_EXPORT OutputType {
Resolver resolver_ = NULLPTR;
};
-/// \brief Holds the input types and output type of the kernel.
+/// \brief Additional constraints to apply to the input types of a kernel when matching a
+/// specific kernel signature.
+class ARROW_EXPORT MatchConstraint {
+ public:
+ virtual ~MatchConstraint() = default;
+
+ /// \brief Return true if the input types satisfy the constraint.
+ virtual bool Matches(const std::vector<TypeHolder>& types) const = 0;
Review Comment:
Updated.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #47297:
URL: https://github.com/apache/arrow/pull/47297#issuecomment-3168836621
:warning: GitHub issue #47287 **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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2266889016
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -348,7 +348,24 @@ class ARROW_EXPORT OutputType {
Resolver resolver_ = NULLPTR;
};
-/// \brief Holds the input types and output type of the kernel.
+/// \brief Additional constraints to apply to the input types of a kernel when matching a
+/// specific kernel signature.
+class ARROW_EXPORT MatchConstraint {
+ public:
+ virtual ~MatchConstraint() = default;
+
+ /// \brief Return true if the input types satisfy the constraint.
+ virtual bool Validate(const std::vector<TypeHolder>& types) const = 0;
Review Comment:
OK, after a second thought, satisfying a constraint is actually a "match". Besides, "matches" is widely used in the rest part of the kernel matching logic. So this makes sense. Will update. Thanks.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2268505955
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Matches(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = checked_cast<const DecimalType*>(types[0].type);
+ DCHECK_NE(ty0, nullptr);
Review Comment:
> I think that we can assume it based on the current implementation but I'm OK with keeping this `DCHECK_NE()` now.
Thank you, that's very helpful. I can remove the subsequent `DCHECK`s given this assumption.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2266894589
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Validate(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = dynamic_cast<const DecimalType*>(types[0].type);
Review Comment:
Updated.
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -348,7 +348,24 @@ class ARROW_EXPORT OutputType {
Resolver resolver_ = NULLPTR;
};
-/// \brief Holds the input types and output type of the kernel.
+/// \brief Additional constraints to apply to the input types of a kernel when matching a
+/// specific kernel signature.
+class ARROW_EXPORT MatchConstraint {
+ public:
+ virtual ~MatchConstraint() = default;
+
+ /// \brief Return true if the input types satisfy the constraint.
+ virtual bool Validate(const std::vector<TypeHolder>& types) const = 0;
Review Comment:
Updated.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47297:
URL: https://github.com/apache/arrow/pull/47297#issuecomment-3205487264
Hi @pitrou , can we merge it now?
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2284715164
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Validate(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = dynamic_cast<const DecimalType*>(types[0].type);
+ DCHECK_NE(ty0, nullptr);
+ auto s0 = ty0->scale();
+ for (size_t i = 1; i < types.size(); ++i) {
+ auto ty = dynamic_cast<const DecimalType*>(types[i].type);
+ DCHECK_NE(ty, nullptr);
+ if (ty->scale() != s0) {
+ return false;
+ }
+ }
+ return true;
+ }
+ };
+ static auto instance = std::make_shared<DecimalsHaveSameScaleConstraint>();
Review Comment:
Hmm, did someone decide to add Copilot reviews to the Arrow repo, or is that up to each PR author?
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2285003633
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -348,7 +348,24 @@ class ARROW_EXPORT OutputType {
Resolver resolver_ = NULLPTR;
};
-/// \brief Holds the input types and output type of the kernel.
+/// \brief Additional constraints to apply to the input types of a kernel when matching a
+/// specific kernel signature.
+class ARROW_EXPORT MatchConstraint {
+ public:
+ virtual ~MatchConstraint() = default;
+
+ /// \brief Return true if the input types satisfy the constraint.
+ virtual bool Matches(const std::vector<TypeHolder>& types) const = 0;
+};
+
+/// \brief Constraint that all input types are decimal types and have the same scale.
+ARROW_EXPORT std::shared_ptr<MatchConstraint> DecimalsHaveSameScale();
+
+/// \brief Constraint that all binary input types are decimal types and the first type's
+/// scale >= the second type's.
+ARROW_EXPORT std::shared_ptr<MatchConstraint> BinaryDecimalScaleComparisonGE();
Review Comment:
Is `BinaryDecimalScale1GeScale2` better?
> Also, it's not obvious to me why we would require the first decimal to have a larger scale...
It's required by `divide(decimal, decimal)` kernels:
https://github.com/apache/arrow/blob/69d24870538a82dc9fd7e32ba799aaa68f7341d4/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc#L624-L639
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2285031746
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -684,8 +687,10 @@ void AddDecimalBinaryKernels(const std::string& name, ScalarFunction* func) {
auto in_type256 = InputType(Type::DECIMAL256);
auto exec128 = ScalarBinaryNotNullEqualTypes<Decimal128Type, Decimal128Type, Op>::Exec;
auto exec256 = ScalarBinaryNotNullEqualTypes<Decimal256Type, Decimal256Type, Op>::Exec;
- DCHECK_OK(func->AddKernel({in_type128, in_type128}, out_type, exec128));
- DCHECK_OK(func->AddKernel({in_type256, in_type256}, out_type, exec256));
+ DCHECK_OK(func->AddKernel({in_type128, in_type128}, out_type, exec128, /*init=*/nullptr,
+ constraint));
+ DCHECK_OK(func->AddKernel({in_type256, in_type256}, out_type, exec256, /*init=*/nullptr,
+ constraint));
Review Comment:
There is a TODO in https://github.com/apache/arrow/issues/43956
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47297:
URL: https://github.com/apache/arrow/pull/47297#issuecomment-3200476451
Thanks for looking @pitrou . I'll update 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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #47297:
URL: https://github.com/apache/arrow/pull/47297#issuecomment-3177454079
Can we merge this now?
Should we wait for a few days before we merge this?
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2268510506
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Matches(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = checked_cast<const DecimalType*>(types[0].type);
+ DCHECK_NE(ty0, nullptr);
Review Comment:
Updated.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2263684746
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Validate(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = dynamic_cast<const DecimalType*>(types[0].type);
+ DCHECK_NE(ty0, nullptr);
+ auto s0 = ty0->scale();
+ for (size_t i = 1; i < types.size(); ++i) {
+ auto ty = dynamic_cast<const DecimalType*>(types[i].type);
+ DCHECK_NE(ty, nullptr);
+ if (ty->scale() != s0) {
+ return false;
+ }
+ }
+ return true;
+ }
+ };
+ static auto instance = std::make_shared<DecimalsHaveSameScaleConstraint>();
Review Comment:
Seriously?
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2284732810
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -684,8 +687,10 @@ void AddDecimalBinaryKernels(const std::string& name, ScalarFunction* func) {
auto in_type256 = InputType(Type::DECIMAL256);
auto exec128 = ScalarBinaryNotNullEqualTypes<Decimal128Type, Decimal128Type, Op>::Exec;
auto exec256 = ScalarBinaryNotNullEqualTypes<Decimal256Type, Decimal256Type, Op>::Exec;
- DCHECK_OK(func->AddKernel({in_type128, in_type128}, out_type, exec128));
- DCHECK_OK(func->AddKernel({in_type256, in_type256}, out_type, exec256));
+ DCHECK_OK(func->AddKernel({in_type128, in_type128}, out_type, exec128, /*init=*/nullptr,
+ constraint));
+ DCHECK_OK(func->AddKernel({in_type256, in_type256}, out_type, exec256, /*init=*/nullptr,
+ constraint));
Review Comment:
Side note: do we have an issue open to extend this to Decimal32 and Decimal64, or are these kernels already registered elsewhere?
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2284721953
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -348,7 +348,24 @@ class ARROW_EXPORT OutputType {
Resolver resolver_ = NULLPTR;
};
-/// \brief Holds the input types and output type of the kernel.
+/// \brief Additional constraints to apply to the input types of a kernel when matching a
+/// specific kernel signature.
+class ARROW_EXPORT MatchConstraint {
+ public:
+ virtual ~MatchConstraint() = default;
+
+ /// \brief Return true if the input types satisfy the constraint.
+ virtual bool Matches(const std::vector<TypeHolder>& types) const = 0;
+};
+
+/// \brief Constraint that all input types are decimal types and have the same scale.
+ARROW_EXPORT std::shared_ptr<MatchConstraint> DecimalsHaveSameScale();
+
+/// \brief Constraint that all binary input types are decimal types and the first type's
+/// scale >= the second type's.
+ARROW_EXPORT std::shared_ptr<MatchConstraint> BinaryDecimalScaleComparisonGE();
Review Comment:
The name is unfortunately a bit cryptic. Also, it's not obvious to me why we would require the first decimal to have a larger scale...
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47297:
URL: https://github.com/apache/arrow/pull/47297#issuecomment-3206870916
I'll later follow up on validating the related issues listed in the PR description. Hopefully we can close or fix most of them.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47297:
URL: https://github.com/apache/arrow/pull/47297#issuecomment-3199101273
Kindly ping @pitrou . Did you get a chance to take a look? Thanks.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2286456326
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Validate(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = dynamic_cast<const DecimalType*>(types[0].type);
+ DCHECK_NE(ty0, nullptr);
+ auto s0 = ty0->scale();
+ for (size_t i = 1; i < types.size(); ++i) {
+ auto ty = dynamic_cast<const DecimalType*>(types[i].type);
+ DCHECK_NE(ty, nullptr);
+ if (ty->scale() != s0) {
+ return false;
+ }
+ }
+ return true;
+ }
+ };
+ static auto instance = std::make_shared<DecimalsHaveSameScaleConstraint>();
Review Comment:
It's not a apache/arrow configuration.
PR authors and apache/arrow committers can set Copilot as a reviewer manually.
To use Copilot, they need to enable Copilot explicitly for their account: https://docs.github.com/en/copilot/get-started/quickstart#sign-up-for-github-copilot
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2288089556
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -348,7 +348,24 @@ class ARROW_EXPORT OutputType {
Resolver resolver_ = NULLPTR;
};
-/// \brief Holds the input types and output type of the kernel.
+/// \brief Additional constraints to apply to the input types of a kernel when matching a
+/// specific kernel signature.
+class ARROW_EXPORT MatchConstraint {
+ public:
+ virtual ~MatchConstraint() = default;
+
+ /// \brief Return true if the input types satisfy the constraint.
+ virtual bool Matches(const std::vector<TypeHolder>& types) const = 0;
Review Comment:
For the record, this could also have take a `util::span`, but I'm not sure that would be useful (except if people want to micro-optimize their containers?).
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2285102150
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -669,13 +669,16 @@ void AddDecimalUnaryKernels(ScalarFunction* func) {
template <typename Op>
void AddDecimalBinaryKernels(const std::string& name, ScalarFunction* func) {
OutputType out_type(null());
+ std::shared_ptr<MatchConstraint> constraint = nullptr;
const std::string op = name.substr(0, name.find("_"));
if (op == "add" || op == "subtract") {
out_type = OutputType(ResolveDecimalAdditionOrSubtractionOutput);
+ constraint = DecimalsHaveSameScale();
} else if (op == "multiply") {
out_type = OutputType(ResolveDecimalMultiplicationOutput);
} else if (op == "divide") {
out_type = OutputType(ResolveDecimalDivisionOutput);
+ constraint = BinaryDecimalScaleComparisonGE();
Review Comment:
Updated.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2284725340
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -348,7 +348,24 @@ class ARROW_EXPORT OutputType {
Resolver resolver_ = NULLPTR;
};
-/// \brief Holds the input types and output type of the kernel.
+/// \brief Additional constraints to apply to the input types of a kernel when matching a
+/// specific kernel signature.
+class ARROW_EXPORT MatchConstraint {
+ public:
+ virtual ~MatchConstraint() = default;
+
+ /// \brief Return true if the input types satisfy the constraint.
+ virtual bool Matches(const std::vector<TypeHolder>& types) const = 0;
Review Comment:
Of course we can also have an API that creates a `MatchConstraint` from a `std::function`, for convenience.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2284968937
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Validate(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = dynamic_cast<const DecimalType*>(types[0].type);
+ DCHECK_NE(ty0, nullptr);
+ auto s0 = ty0->scale();
+ for (size_t i = 1; i < types.size(); ++i) {
+ auto ty = dynamic_cast<const DecimalType*>(types[i].type);
+ DCHECK_NE(ty, nullptr);
+ if (ty->scale() != s0) {
+ return false;
+ }
+ }
+ return true;
+ }
+ };
+ static auto instance = std::make_shared<DecimalsHaveSameScaleConstraint>();
Review Comment:
IIRC it was in the "suggested reviewer" list, and I clicked "request" then.
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #47297:
URL: https://github.com/apache/arrow/pull/47297#issuecomment-3173363978
also cc @lidavidm
--
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-47287: [C++][Compute] Add constraint for kernel signature matching and use it for binary decimal arithmetic kernels [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #47297:
URL: https://github.com/apache/arrow/pull/47297#discussion_r2268307177
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -475,23 +475,78 @@ std::string OutputType::ToString() const {
return "computed";
}
+// ----------------------------------------------------------------------
+// MatchConstraint
+
+std::shared_ptr<MatchConstraint> DecimalsHaveSameScale() {
+ class DecimalsHaveSameScaleConstraint : public MatchConstraint {
+ public:
+ bool Matches(const std::vector<TypeHolder>& types) const override {
+ DCHECK_GE(types.size(), 2);
+ DCHECK(std::all_of(types.begin(), types.end(),
+ [](const TypeHolder& type) { return is_decimal(type.id()); }));
+ auto ty0 = checked_cast<const DecimalType*>(types[0].type);
+ DCHECK_NE(ty0, nullptr);
Review Comment:
I thought that `checked_cast` never return `nullptr` because there are `static_assert()`s before `dynamic_cast`:
https://github.com/apache/arrow/blob/db1a8a9bf42bec2b643fc19d8e174dd5166cd621/cpp/src/arrow/util/checked_cast.h#L29-L34
BTW, there is
```cpp
DCHECK(std::all_of(types.begin(), types.end(),
[](const TypeHolder& type) { return is_decimal(type.id()); }));
```
before this `checked_cast`. It seems that this `DCHECK()` will be failed when `checked_cast` return `nullptr`.
--
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