github-actions[bot] commented on code in PR #67872:
URL: https://github.com/apache/doris/pull/67872#discussion_r4011349066
##########
be/src/exprs/math_functions.cpp:
##########
@@ -133,26 +133,30 @@ StringRef MathFunctions::decimal_to_base(FunctionContext*
ctx, int64_t src_num,
}
bool MathFunctions::decimal_in_base_to_decimal(int64_t src_num, int8_t
src_base, int64_t* result) {
- uint64_t temp_num = std::abs(src_num);
- int32_t place = 1;
- *result = 0;
+ auto magnitude = static_cast<uint64_t>(src_num);
+ if (src_num < 0) {
+ magnitude = 0 - magnitude;
+ }
+ uint64_t divisor = 1;
+ while (magnitude / divisor >= 10) {
+ divisor *= 10;
+ }
+
+ uint64_t value = 0;
do {
- int32_t digit = temp_num % 10;
- // Reset result if digit is not representable in src_base.
+ const int digit = static_cast<int>(magnitude / divisor);
+ // Keep the prefix preceding the first digit not representable in
src_base.
if (digit >= src_base) {
- *result = 0;
- place = 1;
- } else {
- *result += digit * place;
- place *= src_base;
- // Overflow.
- if (UNLIKELY(*result < digit)) {
- return false;
- }
+ break;
}
- temp_num /= 10;
- } while (temp_num > 0);
- *result = (src_num < 0) ? -(*result) : *result;
+ if (UNLIKELY(value > (std::numeric_limits<uint64_t>::max() - digit) /
src_base)) {
+ return false;
+ }
+ value = value * src_base + digit;
+ magnitude %= divisor;
+ divisor /= 10;
+ } while (divisor > 0);
+ *result = static_cast<int64_t>(src_num < 0 ? 0 - value : value);
Review Comment:
[P1] Handle the newly reachable `INT64_MIN` in the formatter
For `conv(8000000000000000, 16, -10)`, this successful conversion stores
`1ULL << 63` as `INT64_MIN`. `ConvInt64Impl::calculate_cell` then
unconditionally passes it to `decimal_to_base`, whose negative-destination
branch evaluates `std::abs(INT64_MIN)`. That magnitude is not representable as
`int64_t`, so this valid SQL input still reaches signed-overflow undefined
behavior (and Doris's normal ASAN BE flags include UBSan). Please compute the
formatter's signed magnitude through unsigned arithmetic, as this helper now
does, and add an end-to-end numeric `conv()` case for the negative destination
base; the helper-only test does not exercise this path.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
