HappenLee commented on code in PR #67872:
URL: https://github.com/apache/doris/pull/67872#discussion_r3999387198
##########
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)) {
Review Comment:
The overflow check recomputes `(UINT64_MAX - digit) / src_base` for every
valid digit. We can precompute the quotient and remainder once per call and
reuse the threshold pattern already used by
[StringParser](https://github.com/apache/doris/blob/00ea392459a79ae704c79f28e4447e85c2c56063/be/src/util/string_parser.hpp#L620-L649):
```cpp
// Before the digit-processing loop:
constexpr uint64_t max_value = std::numeric_limits<uint64_t>::max();
const uint64_t max_div_base = max_value / src_base;
const uint64_t max_mod_base = max_value % src_base;
// Inside the loop, after checking digit < src_base:
if (UNLIKELY(value > max_div_base - (digit > max_mod_base))) {
return false;
}
```
For a valid positive base, let `M = q * base + r`. Since `0 <= digit <
base`, `floor((M - digit) / base)` is exactly `q` when `digit <= r`, and `q -
1` otherwise. This preserves the current pre-operation overflow check.
I compiled extracted copies of the complete helper, changing only this
check, with Clang 16.0.6, Clang 20.1.8, and GCC 15.1.0 using `-std=c++20 -O3
-DNDEBUG -msse4.2 -mavx2`. All three retain a division inside the original
digit loop. With the precomputed thresholds, they use a single division before
the loop to obtain both quotient and remainder. For example, Clang 16 emits:
```asm
# Before the loop: rbx = src_base
mov rax, -1
xor edx, edx
div rbx
mov r9, rax # quotient
mov r10, rdx # remainder
# Inside the loop: rax = digit, rsi = value
cmp r10, rax
mov rdx, r9
sbb rdx, 0 # quotient - (digit > remainder)
cmp rsi, rdx
ja overflow
```
Thus, for an input that processes all d valid digits without overflowing,
the threshold calculation needs one hardware division per call instead of d.
The division used to extract each digit remains unchanged. I also checked the
threshold equivalence for every base from 2 through 36 and every valid decimal
digit, including boundary comparisons.
This is an assembly-confirmed optimization opportunity, not a measured SQL
speedup. Inputs rejected at the first digit would incur an extra
precomputation, so short inputs and invalid prefixes should also be included in
any benchmark.
--
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]
