bkietz commented on code in PR #43957:
URL: https://github.com/apache/arrow/pull/43957#discussion_r1754715223
##########
cpp/src/arrow/engine/substrait/expression_internal.cc:
##########
@@ -951,25 +951,28 @@ struct ScalarToProtoImpl {
Status Visit(const MonthIntervalScalar& s) { return NotImplemented(s); }
Status Visit(const DayTimeIntervalScalar& s) { return NotImplemented(s); }
- Status Visit(const Decimal128Scalar& s) {
+ template <typename T,
+ typename = internal::EnableIfIsOneOf<T, Decimal32Scalar,
Decimal64Scalar,
+ Decimal128Scalar,
Decimal256Scalar>>
+ Status Visit(const T& s) {
+ using TypeClass = typename T::TypeClass;
Review Comment:
```suggestion
template <typename T, typename TypeClass = typename T::TypeClass>
enable_if_decimal<TypeClass, Status> Visit(const T& s) {
```
##########
cpp/src/arrow/engine/substrait/expression_internal.cc:
##########
@@ -951,25 +951,28 @@ struct ScalarToProtoImpl {
Status Visit(const MonthIntervalScalar& s) { return NotImplemented(s); }
Status Visit(const DayTimeIntervalScalar& s) { return NotImplemented(s); }
- Status Visit(const Decimal128Scalar& s) {
+ template <typename T,
+ typename = internal::EnableIfIsOneOf<T, Decimal32Scalar,
Decimal64Scalar,
+ Decimal128Scalar,
Decimal256Scalar>>
+ Status Visit(const T& s) {
+ using TypeClass = typename T::TypeClass;
+ using ValueType = typename T::ValueType;
+
auto decimal = std::make_unique<Lit::Decimal>();
- auto decimal_type = checked_cast<const Decimal128Type*>(s.type.get());
+ auto decimal_type = checked_cast<const TypeClass*>(s.type.get());
decimal->set_precision(decimal_type->precision());
decimal->set_scale(decimal_type->scale());
decimal->set_value(reinterpret_cast<const
char*>(s.value.native_endian_bytes()),
- sizeof(Decimal128));
+ sizeof(ValueType));
#if !ARROW_LITTLE_ENDIAN
std::reverse(decimal->mutable_value()->begin(),
decimal->mutable_value()->end());
#endif
lit_->set_allocated_decimal(decimal.release());
return Status::OK();
}
- // Need support for parameterized UDTs
- Status Visit(const Decimal256Scalar& s) { return NotImplemented(s); }
Review Comment:
:+1:
##########
cpp/src/arrow/compare.cc:
##########
@@ -750,6 +750,18 @@ class TypeEqualsVisitor {
return Status::OK();
}
+ Status Visit(const Decimal32Type& left) {
+ const auto& right = checked_cast<const Decimal32Type&>(right_);
+ result_ = left.precision() == right.precision() && left.scale() ==
right.scale();
+ return Status::OK();
+ }
Review Comment:
```suggestion
Status Visit(const DecimalType& left) {
const auto& right = checked_cast<const DecimalType&>(right_);
result_ = left.byte_width() == right.byte_width() && left.precision() ==
right.precision() && left.scale() == right.scale();
return Status::OK();
}
```
##########
cpp/src/arrow/array/diff.cc:
##########
@@ -707,7 +707,11 @@ class MakeFormatterImpl {
template <typename T>
enable_if_decimal<T, Status> Visit(const T&) {
impl_ = [](const Array& array, int64_t index, std::ostream* os) {
- if constexpr (T::type_id == Type::DECIMAL128) {
+ if constexpr (T::type_id == Type::DECIMAL32) {
Review Comment:
```suggestion
const auto& decimal_array =
checked_cast<const typename TypeTraits<T>::ArrayType&>(array);
*os << decimal_array.FormatValue(index);
```
##########
cpp/src/arrow/integration/json_internal.cc:
##########
@@ -314,6 +314,20 @@ class SchemaWriter {
writer_->Int(type.list_size());
}
+ void WriteTypeMetadata(const Decimal32Type& type) {
Review Comment:
I think we don't need to unpack to specific widths
```suggestion
void WriteTypeMetadata(const DecimalType& type) {
```
##########
cpp/src/arrow/compute/kernels/hash_aggregate.cc:
##########
@@ -892,8 +894,10 @@ struct GroupedVarStdImpl : public GroupedAggregator {
// float/double/int64/decimal: calculate `m2` (sum((X-mean)^2)) with
// `two pass algorithm` (see aggregate_var_std.cc)
template <typename T = Type>
- enable_if_t<is_floating_type<T>::value || (sizeof(CType) > 4), Status>
ConsumeImpl(
- const ExecSpan& batch) {
+ enable_if_t<is_floating_type<T>::value || (sizeof(CType) > 4) ||
+ (!is_integer_type<T>::value && sizeof(CType) == 4),
Review Comment:
I'd prefer to see this last clause be more explicit, IIUC you could also
write
```suggestion
std::is_same_v<CType, Decimal32>,
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
