lxy-9602 commented on code in PR #349:
URL: https://github.com/apache/paimon-cpp/pull/349#discussion_r4015669613
##########
src/paimon/core/mergetree/compact/aggregate/field_aggregate_utils.cpp:
##########
@@ -123,6 +128,25 @@ Result<bool> EqualMaps(const std::shared_ptr<InternalMap>&
lhs,
return true;
}
+size_t CombineHash(size_t lhs, size_t rhs) {
+ return lhs ^ (rhs + static_cast<size_t>(0x9e3779b97f4a7c15ULL) + (lhs <<
6) + (lhs >> 2));
+}
+
+template <typename T>
+size_t HashFloatingPoint(T value) {
+ using Bits = std::conditional_t<sizeof(T) == sizeof(uint32_t), uint32_t,
uint64_t>;
+ Bits bits;
+ std::memcpy(&bits, &value, sizeof(value));
+ if (std::isnan(value)) {
+ if constexpr (sizeof(T) == sizeof(uint32_t)) {
+ bits = static_cast<Bits>(0x7fc00000U);
+ } else {
+ bits = static_cast<Bits>(0x7ff8000000000000ULL);
+ }
+ }
+ return std::hash<Bits>{}(bits);
Review Comment:
Consider using the floating-point utilities in `math.h` (for example,
`CanonicalizeFloatingPoint` or `CanonicalizeFloatToIntBits`) instead of
reimplementing similar logic.
##########
src/paimon/core/mergetree/compact/aggregate/field_collect_agg_test.cpp:
##########
@@ -60,6 +63,11 @@ Result<std::unique_ptr<FieldCollectAgg>> MakeCollectAgg(bool
distinct) {
return FieldCollectAgg::Create(arrow::list(arrow::int32()), options, "f",
GetDefaultPool());
}
+Result<std::unique_ptr<FieldCollectAgg>> MakeDistinctAgg(
+ const std::shared_ptr<arrow::DataType>& element_type);
+
+VariantType Array(std::vector<VariantType> values);
+
Review Comment:
It seems you could move the function definition earlier, or rearrange the
tests, to reduce the need for forward declarations.
##########
src/paimon/core/mergetree/compact/aggregate/field_aggregate_utils.cpp:
##########
@@ -244,4 +268,67 @@ Result<bool> FieldAggregateUtils::Equals(const
VariantType& lhs, const VariantTy
}
}
+bool FieldAggregateUtils::IsHashableType(const
std::shared_ptr<arrow::DataType>& type) {
+ switch (type->id()) {
+ case arrow::Type::BOOL:
+ case arrow::Type::INT8:
+ case arrow::Type::INT16:
+ case arrow::Type::INT32:
+ case arrow::Type::DATE32:
+ case arrow::Type::INT64:
+ case arrow::Type::FLOAT:
+ case arrow::Type::DOUBLE:
+ case arrow::Type::STRING:
+ case arrow::Type::BINARY:
+ case arrow::Type::TIMESTAMP:
+ return true;
+ default:
+ return false;
+ }
+}
+
+size_t FieldAggregateUtils::Hash(const VariantType& value,
+ const std::shared_ptr<arrow::DataType>& type)
{
+ assert(IsHashableType(type));
+ size_t result = std::hash<int>{}(static_cast<int>(type->id()));
+ if (DataDefine::IsVariantNull(value)) {
Review Comment:
For consistency across the codebase, please use `int32_t` instead of `int`.
##########
src/paimon/core/mergetree/compact/aggregate/field_merge_map_agg.cpp:
##########
Review Comment:
I’m a bit curious: are there any plans to optimize the `Retract` method for
`merge_map` aggregation using hashing?
--
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]
