SteNicholas commented on code in PR #203:
URL: https://github.com/apache/paimon-cpp/pull/203#discussion_r3780769008
##########
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:
This still relies on an implementation-defined C++17 narrowing conversion.
On an unsigned-`char` ABI, bytes `0x80`–`0xff` arrive here as 128–255, which
are outside `int8_t`; the sum path has the same issue when narrowing values up
to 510. The new test also casts back to `int8_t`, so it repeats rather than
detects this assumption. Please decode and wrap through `uint8_t` plus explicit
in-range arithmetic (for example, map bits >= 128 to `int16_t(bits) - 256`)
instead of an out-of-range signed 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
