u70b3 commented on code in PR #203:
URL: https://github.com/apache/paimon-cpp/pull/203#discussion_r3819248677
##########
src/paimon/core/mergetree/compact/aggregate/field_sum_agg.cpp:
##########
@@ -93,9 +97,10 @@ Result<FieldSumAgg::FieldNegFunc> FieldSumAgg::CreateNegFunc(
arrow::Type::type type = field_type->id();
switch (type) {
case arrow::Type::type::INT8:
+ // As in CreateSumFunc, negate the signed value the plain char
stands for.
return FieldNegFunc([](const VariantType& input_field) ->
VariantType {
- char value = DataDefine::GetVariantValue<char>(input_field);
- return static_cast<char>(-value);
+ auto value =
static_cast<int8_t>(DataDefine::GetVariantValue<char>(input_field));
Review Comment:
We ended up reverting the INT8 change entirely rather than fixing the cast:
modulo-256 addition and negation produce the same stored bits regardless of
plain-`char` signedness, so the original `char` arithmetic is already
ABI-stable here — unlike the min/max comparisons fixed in #181, where
signedness does change results. The PR description documents this audit
conclusion.
This also moots the clang-tidy `modernize-use-auto` failure from the other
thread, since that declaration no longer exists. PTAL.
--
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]
