Repository navigation
[fix](be) Preserve decimal precision in format_round - #68397
Conversation
Issue Number: None
Related PR: None
Problem Summary: format_round(CAST('1.44' AS DECIMAL(38, 2)), 20)
returns 1.00000000000000000044 instead of 1.44000000000000000000.
The decimal formatter uses a 32-bit power-of-ten helper to expand the
fraction and narrows the fraction to int when rounding and rendering it.
This also corrupts existing high-scale fractions. The DECIMALV2 caller
additionally discards all but three fractional digits.
Round with full-width integer arithmetic and append trailing zeroes in
the output buffer instead of scaling the fraction. Pass the complete
DECIMALV2 fraction. Cover decimal widths, constant and column arguments,
NULLs, negative values, rounding carries, and the 1024-place boundary.
Regenerate the two existing incorrect format_round expectations and
verify money_format retains exact two-place rounding.
Fix incorrect format_round results for high-precision decimals and
preserve all fractional digits of DECIMALV2 inputs before rounding.
- Test:
- ASAN BE build via ./build.sh --be -j48
- Unit Test: 7 tests passed via ./run-be-ut.sh --run --filter='FormatRoundTest.*:function_money_format_test.*' -j48
- Regression test: test_format_round, test_format_round_decimal, and test_money_format passed
- Manual test: original SQL reproduced before the fix and returned exact decimal results after it; 59 generated SQL output rows checked against Python Decimal ROUND_HALF_UP
- clang-format v16 and build hygiene passed
- clang-tidy attempted; blocked by the pre-existing unmatched NOLINTEND in be/src/core/types.h:576
- Behavior changed: Yes, decimal formatting preserves the correct value and requested fractional digits
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Request changes: one material correctness gap remains in the advertised DecimalV2 path; see the inline comment.
Critical checkpoint conclusions:
- Goal and scope: the full-width rounding/right-padding rewrite is focused and correct for integers plus Decimal32/64/128V3/256, but the stated DecimalV2 precision fix is not implemented.
- Concurrency, lifecycle, and configuration: this is stateless row-local formatting with no new threads, locks, static lifecycle, or configuration item. The existing Decimal256 and DecimalV2 gates were traced.
- Compatibility and parallel paths: no protocol, symbol, serialization, or storage-format change is introduced. FE/BE dispatch is consistent for the covered types; the registered DecimalV2 parallel path remains lossy.
- Errors, transactions, and data correctness: every execution path validates places in [0,1024] and propagates failure. There is no transaction, persistence, or data-write path, but DecimalV2 can still return an incorrect displayed value.
- Performance and memory: work is output-proportional, integer buffers are sufficient through Decimal256, and no untracked long-lived ownership is added.
- Tests and results: the new BE test is auto-discovered and the DecimalV3/Decimal256 unit/regression expectations are consistent with half-up rounding, including const/column, NULL, sign, carry, extrema, and 1024-place cases. No changed test exercises TYPE_DECIMALV2, which is the missing path.
Review completion: the main scan plus two normal/risk-focused convergence rounds covered all seven changed files and relevant call chains. Round 2 returned NO_NEW_VALUABLE_FINDINGS, every candidate was accepted, dismissed with evidence, or deduplicated, and no additional user focus was supplied. Validation here was static-only because this review environment prohibited builds/tests; the author-reported ASAN build, unit tests, and regression tests were not independently executed.
|
run buildall |
TPC-H: Total hot run time: 28023 ms |
TPC-DS: Total hot run time: 152601 ms |
ClickBench: Total hot run time: 24.09 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve?
Issue Number: N/A
Problem Summary:
**Context.** `run-be-ut.sh` is the only entry point for the BE unit
tests, and on macOS it is also the only way to run them: the `BE UT
(macOS)` workflow runs `build.sh --be`, which configures with
`-DMAKE_TEST=OFF`, so nothing under `be/test` is compiled there (the
workflow says so in its own comment at
`.github/workflows/be-ut-mac.yml:123`). On a clean worktree the script
therefore fails twice before a single test executes, and then stops
seven more times while compiling `be/test`.
**1. The problem, and what it cost**
*The script never initializes two of the submodules it needs.*
`run-be-ut.sh` initializes three submodules (`contrib/datasketches-cpp`,
`contrib/apache-orc`, `contrib/clucene`); `build.sh` initializes five,
and the two it adds — `contrib/faiss` and `contrib/openblas` — are
required before a single file is compiled. `be/CMakeLists.txt:1038` adds
`be/src/storage/index/ann` unconditionally, and that directory's
`cmake-protect/CMakeLists.txt:48,66` calls `add_subdirectory()` on both.
On a fresh clone or a new git worktree the configure step therefore ends
with
CMake Error at src/storage/index/ann/cmake-protect/CMakeLists.txt:48
(add_subdirectory):
The source directory .../contrib/openblas does not contain a
CMakeLists.txt file.
CMake Error at src/storage/index/ann/cmake-protect/CMakeLists.txt:66
(add_subdirectory):
The source directory .../contrib/faiss does not contain a CMakeLists.txt
file.
Running `build.sh` once first hides it, which is why it goes unnoticed.
*Seven `be/test` translation units do not compile on macOS.* The Linux
BE UT build is green for all seven. `run-be-ut.sh` invokes ninja without
`-k`, so the developer fixes one file, waits out another full compile,
and meets the next one — seven round trips, each of them tens of
minutes, for a task whose whole point is to run a test.
| # | TU | Error | Root cause |
|---|---|---|---|
| 1 | `exprs/function/function_map_inner_product_test.cpp` |
`'faiss/impl/platform_macros.h' file not found` | The `APPLE` branch of
`be/test/CMakeLists.txt:170` links `vector_search_test` as
`-Wl,-force_load,$<TARGET_FILE:vector_search_test>`: a file path, which
carries no usage requirements. Every other platform links
`-Wl,--whole-archive vector_search_test`, which is a target in the link
list and does propagate them. |
| 2 | `exprs/function/function_format_round_test.cpp` | `ambiguous
conversion for functional-style cast from 'long' to 'Decimal64'` (11
sites) | `Decimal64` is `Decimal<int64_t>`, so on Darwin the constructor
parameter is `long long` while these literals are `long`; every integral
and floating constructor is then an equally ranked conversion away. |
| 3 | `storage/index/snii/bench/bkd_native_vs_clucene_bench_test.cpp` |
six `-Werror,-Wformat` | `uint64_t` is `unsigned long long` on Darwin
and `unsigned long` on Linux, so `%lu` / `%ld` is right on one and wrong
on the other. |
| 4 | `storage/index/snii/bench/snii_vs_v3_benchmark_test.cpp` | `no
member named 'posix_fadvise'`, `mincore` argument type | `posix_fadvise`
/ `POSIX_FADV_DONTNEED` do not exist on Darwin; `mincore(2)` takes
`char*` there and `unsigned char*` on Linux. |
| 5 | `storage/index/snii/bkd/bkd_corruption_test.cpp` | `deduced
conflicting types ('size_type' vs 'uint64_t')` | An initializer list
mixing `payload.size()` (size_t) with `uint64_t` literals. |
| 6 | `storage/index/snii/writer/spimi_locality_bench_test.cpp` | libc++
`no matching function for call to object of type 'key_equal'` | The
transparent equality functor defines one argument order only. libc++
calls `key_equal` as `(key, lookup key)`, libstdc++ as `(lookup key,
key)`. |
| 7 | `storage/segment/hierarchical_data_iterator_test.cpp` | three `no
matching function for call to 'min'` | `kRows` is `size_t` and
`current_ordinal` is `ordinal_t` (`uint64_t`), which are distinct types
on Darwin, so `std::min` cannot deduce a common type. |
None of the seven is related to a local change; a clean master
reproduces all of them. #2 arrived on 2026-09-24 with #68397 (`[fix](be)
Preserve decimal precision in format_round`, by TengJianPing), which is
the commit that added `function_format_round_test.cpp` — after the last
full macOS sweep. So this class of breakage is still being produced:
"green on Linux, red on macOS" is invisible to every CI job we have, and
the two obstacles above make the one manual path that would find it
expensive enough that nobody walks it.
**2. What this PR does, and why it helps**
- `run-be-ut.sh`: the `update_submodule` list gains `contrib/openblas`
and `contrib/faiss`, with the same archive URLs `build.sh:941` already
uses, plus a comment that the list has to stay in sync with `build.sh`.
- `be/test/CMakeLists.txt`: the `APPLE` branch links `ann_index` in
addition to the `force_load` flag. `ann_index` is the target that
exports the faiss include directories `PUBLIC`, so the APPLE link line
now feeds the same compile and link inputs as every other platform,
instead of being a macOS special case.
- Six test files: the smallest change that removes the platform
difference, each spelled the way the file's neighbours already spell it
— `PRIu64` / `PRId64` (`core/decimal12.h`, `util/histogram.cpp`),
`int64_t(...)` around the literals (`storage/key_coder_test.cpp:392`,
`runtime/jsonb_document_test.cpp:433`), `#if
defined(POSIX_FADV_DONTNEED)` around the call.
Linux behaviour is unchanged: every edit is either inside an
`APPLE`-only path or a strictly more portable spelling of the same
value. On macOS `run-be-ut.sh` becomes a usable test line for the first
time.
One cost worth naming: because the APPLE link line now propagates
include directories, the compile command of every `be/test` TU changes
once, so the first macOS build after this lands rebuilds `be/test`
(about 1230 TUs). Later builds are incremental again.
**Branch pick.** #68397 was never picked to `branch-4.1`, so
`function_format_round_test.cpp` does not exist there. Of the seven
files, only `storage/segment/hierarchical_data_iterator_test.cpp` exists
on `branch-4.1`, with the same three `std::min(*rows, ROWS -
current_ordinal)` sites and the same types (`ROWS` is `size_t`,
`ordinal_t` is `uint64_t`), so it fails to compile on macOS there too;
that branch also has no `APPLE` `force_load` branch in
`be/test/CMakeLists.txt`, so the faiss part is master-only. If 4.1 is
meant to run BE UT on macOS, the pick is that one file's hunk.
**3. The classes, and how they call each other**
- `run-be-ut.sh` (changed): `update_submodule()` calls `git submodule
update --init --recursive <path>` and falls back to the branch tarball;
the two added lines are identical in shape to the three that were
already there.
- `be/test/CMakeLists.txt` (changed): the `APPLE` branch of the
`doris_be_test` link step.
- `be/src/storage/index/ann/cmake-protect/CMakeLists.txt` (unchanged,
the reason the submodules are mandatory): `add_subdirectory()` on
`contrib/openblas` and `contrib/faiss`, reached unconditionally from
`be/CMakeLists.txt:1038`.
- `ann_index` (unchanged target in `be/src/storage/index/ann`): links
`faiss` `PUBLIC`, so its `INTERFACE_INCLUDE_DIRECTORIES` carry the faiss
include path.
- `vector_search_test` (unchanged target in
`be/test/storage/index/ann`): links `ann_index` `PUBLIC`; on APPLE it
reaches `doris_be_test` as forced-in objects rather than as a target.
- The seven test TUs: unchanged in behaviour, three of them compiled in
a portable spelling.
```
run-be-ut.sh
|- update_submodule contrib/{datasketches-cpp, apache-orc, clucene} [already there]
'- update_submodule contrib/{openblas, faiss} [added here]
'- git submodule update --init --recursive
be/CMakeLists.txt:1038 add_subdirectory(be/src/storage/index/ann)
'- storage/index/ann/CMakeLists.txt:18 add_subdirectory(cmake-protect)
'- cmake-protect/CMakeLists.txt:48,66 add_subdirectory(contrib/{openblas, faiss})
'- target faiss --(PUBLIC)--> target ann_index --(PUBLIC)--> vector_search_test
|
be/test/CMakeLists.txt:170 if (APPLE) |
|- before: -Wl,-force_load,<path to libvector_search_test.a> -- propagates nothing
'- after: the same flag + ann_index -- propagates the faiss include dirs
|
function_map_inner_product_test.cpp <--'
(and every other be/test TU, which now
compiles with the same inputs as on Linux)
be/test/CMakeLists.txt add_executable(doris_be_test ${UT_FILES}) <- the other six TUs, no link involvement
```
Issue Number: None
Related PR: None
Problem Summary: format_round(CAST('1.44' AS DECIMAL(38, 2)), 20)
returns 1.00000000000000000044 instead of 1.44000000000000000000. The
decimal formatter uses a 32-bit power-of-ten helper to expand the
fraction and narrows the fraction to int when rounding and rendering it.
This also corrupts existing high-scale fractions. The DECIMALV2 caller
additionally discards all but three fractional digits.
Round with full-width integer arithmetic and append trailing zeroes in
the output buffer instead of scaling the fraction. Pass the complete
DECIMALV2 fraction. Cover decimal widths, constant and column arguments,
NULLs, negative values, rounding carries, and the 1024-place boundary.
Regenerate the two existing incorrect format_round expectations and
verify money_format retains exact two-place rounding.
Fix incorrect format_round results for high-precision decimals and
preserve all fractional digits of DECIMALV2 inputs before rounding.
- Test:
- ASAN BE build via ./build.sh --be -j48
- Unit Test: 7 tests passed via ./run-be-ut.sh --run
--filter='FormatRoundTest.*:function_money_format_test.*' -j48
- Regression test: test_format_round, test_format_round_decimal, and
test_money_format passed
- Manual test: original SQL reproduced before the fix and returned exact
decimal results after it; 59 generated SQL output rows checked against
Python Decimal ROUND_HALF_UP
- clang-format v16 and build hygiene passed
- clang-tidy attempted; blocked by the pre-existing unmatched NOLINTEND
in be/src/core/types.h:576
- Behavior changed: Yes, decimal formatting preserves the correct value
and requested fractional digits
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [ ] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg:
apache/doris-website#1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR
should merge into -->
Issue Number: None
Related PR: None
Problem Summary: format_round(CAST('1.44' AS DECIMAL(38, 2)), 20) returns 1.00000000000000000044 instead of 1.44000000000000000000. The decimal formatter uses a 32-bit power-of-ten helper to expand the fraction and narrows the fraction to int when rounding and rendering it. This also corrupts existing high-scale fractions. The DECIMALV2 caller additionally discards all but three fractional digits.
Round with full-width integer arithmetic and append trailing zeroes in the output buffer instead of scaling the fraction. Pass the complete DECIMALV2 fraction. Cover decimal widths, constant and column arguments, NULLs, negative values, rounding carries, and the 1024-place boundary. Regenerate the two existing incorrect format_round expectations and verify money_format retains exact two-place rounding.
Fix incorrect format_round results for high-precision decimals and preserve all fractional digits of DECIMALV2 inputs before rounding.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)