[opt](exprs) Remove unreachable mixed-width decimal registrations for add/subtract/mod - #66617
Merged
morningman merged 5 commits intoAug 11, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
…ails Thirteen cases through the real SimpleFunctionFactory dispatch, covering multiply, add and subtract: - multiply: int64 vec_vec, Decimal64*Decimal64->Decimal128 (same-width), Decimal32*Decimal64->Decimal128 (mixed-width), vector_constant, and one-row constant_constant; - add/subtract: int64 vec_vec (the integral control case), same-width Decimal32 and Decimal64 vec_vec, vector_constant, and one-row constant_constant. FE casts both children of a decimal add/subtract to exactly the return type, so - unlike multiply - only the same-width shapes are reachable there, and those are exactly the ones the upcoming registration change rewrites: today they are served by the <Type, DECIMAL256> variants that won the collapsed factory keys, so they compute in Int256 regardless of operand width. These pin rows/s baselines for the template refactors of the arithmetic kernels: dropping dead mixed-width registrations, removing custom constant_constant paths, un-templating the bool variants, and the FE width-unification A/B all compare against these numbers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND7L1ZVTJf91TBpLwYSqct
FE casts both children of a decimal Add to exactly the same type as the return type (TypeCoercionUtils#processDecimalV3BinaryArithmetic, since 2.0 via apache#17393), so only same-width pairs can reach BE. The 12 mixed-width registrations were pure dead weight: since apache#52837 the impl carried "PTypeB = TypeA", which made every mixed-width variant register under the same-width factory key anyway (last one overwriting the diagonal), so mixed-width add has been unresolvable on 4.0/4.1/master for over a year with zero field reports. Deleting them removes ~2/3 of the template instantiations of this TU (probe: -31% compile wall, -54% .text on the sibling plus experiment) and fixes a runtime quirk: the surviving same-width entries were the <Type, DECIMAL256> variants, whose plain (non-overflow-check) path promoted Decimal32/64/128 arithmetic to Int256. Collapse the impl to a single type parameter so mixed-width instantiations cannot be registered again. A mixed-width lookup now fails loudly ("Could not find function add, arg ... return ..."). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0121aRtZYjjYdNr2a6z8BLzR
…tions Same rationale as the add change: FE casts both children of a decimal Subtract to exactly the return type (TypeCoercionUtils# processDecimalV3BinaryArithmetic, since 2.0 via apache#17393), so only same-width pairs reach BE, and since apache#52837 the "PTypeB = TypeA" typo made all 12 mixed-width registrations collapse onto same-width factory keys anyway - mixed-width subtract has been unresolvable in production for over a year with zero field reports. Also restores narrow-width arithmetic for same-width inputs (the surviving key winners were the <Type, DECIMAL256> variants) and collapses the impl to a single type parameter so mixed-width instantiations cannot come back. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0121aRtZYjjYdNr2a6z8BLzR
Unlike add/subtract, mod's 16 decimal combos were all live in the factory (no key-collapsing typo here), so this is a real narrowing of the registered signature set. It is still safe across the supported upgrade window (old FE + new BE): - Nereids has cast both children of Mod to exactly the return type since 2.0 (TypeCoercionUtils#processDecimalV3BinaryArithmetic, apache#17393; briefly removed and restored within two days in May 2023, master-only window); - the legacy planner's ArithmeticExpr#analyzeDecimalV3Op cast both children unconditionally for MOD (only ADD/SUBTRACT had the scale-only-comparison hole), and its builtin table only ever registered same-width decimal MOD; - supported upgrade sources for master (4.0/4.1) are Nereids-only: the legacy expression analyzer no longer exists there. A mixed-width lookup now fails loudly with function name, argument types and return type instead of resolving. Impl collapsed to a single type parameter like add/subtract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0121aRtZYjjYdNr2a6z8BLzR
FE casts both children of Add/Subtract/Mod to exactly the return type (TypeCoercionUtils#processDecimalV3BinaryArithmetic), so BE registers only same-width decimal pairs for them; Multiply is exempt and keeps the full width cross product. These tests pin that surface: same-width add/subtract/ mod lookups must resolve, mixed-width ones must return nullptr (no bare-name fallback exists), multiply's 4x4 cross product must stay, and DecimalV2 stays resolvable. If someone re-adds mixed-width registrations (paying ~2/3 of those TUs' template instantiations for unreachable code) or drops a reachable signature, this fails fast. Co-Authored-By: Claude Fable 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND7L1ZVTJf91TBpLwYSqct
morningman
force-pushed
the
be-build-opt-c-arith-dead-reg
branch
from
August 10, 2026 12:33
9e24fdb to
bbd3de2
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28376 ms |
Contributor
TPC-DS: Total hot run time: 159597 ms |
Contributor
ClickBench: Total hot run time: 24.02 s |
Contributor
Author
|
run check_coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Related PR: #66510
Problem Summary:
The mixed-width decimal registrations for
add/subtract/modcannot bereached at runtime, and each one costs a full set of template instantiations in
some of the heaviest TUs in
be/src/exprs/function/.FE casts both children of a decimal
Add/Subtract/Modto exactly thereturn type (
TypeCoercionUtils#processDecimalV3BinaryArithmetic, since 2.0 via#17393), so only same-width pairs can ever reach BE.
For add and subtract the registrations were not merely unreachable, they
were already broken: since #52837 the impl carried
PTypeB = TypeA, which madeevery mixed-width variant register under the same-width factory key, the last
one overwriting the diagonal. Mixed-width add/subtract has therefore been
unresolvable on 4.0 / 4.1 / master for over a year with zero field reports.
That key collapse also had a runtime consequence worth calling out: the entries
that survived on the diagonal were the
<Type, DECIMAL256>variants, whoseplain (non-overflow-checked) path promoted
Decimal32/64/128arithmetic toInt256. Collapsing the impl to a single type parameter puts same-width inputsback on their natural width.
For mod there was no such typo — its 16 decimal combinations were all live in
the factory — so this is a genuine narrowing of the registered signature set.
It is still safe across the supported upgrade window (old FE + new BE):
Modto exactly the return type since 2.0([enhancement](Nereids) support decimalv3 type #17393; briefly removed and restored within two days in May 2023, a
master-only window);
ArithmeticExpr#analyzeDecimalV3Opcast both childrenunconditionally for
MOD(onlyADD/SUBTRACThad thescale-only-comparison hole), and its builtin table only ever registered
same-width decimal
MOD;expression analyzer no longer exists there.
A mixed-width lookup now fails loudly with function name, argument types and
return type instead of silently resolving to the wrong instantiation.
Multiplyis exempt from the FE cast and keeps its full width cross product.Why this is a build-time win
Dropping mixed-width removes roughly two thirds of the decimal instantiations of
these TUs — a whole template family per width pair, not a handful of functions.
Measured with the real build's own compile commands (Release, PCH on, the exact
flags
build.sh --beproduces), recompiling each TU serialized and uncontended,alternating base/PR across three rounds.
multiply.cppis the control: this PRdeliberately does not touch it, so it must not move.
plus.cppminus.cppmodulo.cppmultiply.cpp(control)Run-to-run spread is tight (
plus.cppbase 9.93 / 10.19 / 10.24s, after 6.01 /6.06 / 6.11s) and the control is flat, so these deltas are the change rather than
scheduling noise. The ~350MB drop in peak RSS per TU is worth as much as the wall
time if you build at high
-j.What this does not claim. These are 3 TUs out of 8382 in a cold BE build, and
35.5s out of 7237s of total TU CPU. The ~11s of CPU saved is about 0.15% of a
full build — real, but well below what a single end-to-end run can resolve, so
I am deliberately not quoting an end-to-end percentage for it. The value of this
PR is the per-TU cost of these three files, the peak-memory drop, the removal of
code that cannot be reached, and the contract test that keeps it gone.
Release note
None
Check List (For Author)
pre-change baseline suite for suite (the only failure is a known,
unrelated S3-credential
outfilecase that also fails on the baseline).BinaryArithmeticRegistrationTest(
be/test/exprs/function/binary_arithmetic_registration_test.cpp, 5/5passing) pins the registration surface: same-width add/subtract/mod
lookups must resolve, mixed-width ones must return
nullptr(there is nobare-name fallback), multiply's 4x4 cross product must stay, and DecimalV2
stays resolvable. If someone re-adds mixed-width registrations — paying
~2/3 of those TUs' instantiations for unreachable code — or drops a
reachable signature, this fails fast.
Runtime A/B: no regression on any case
The first commit adds
benchmark_binary_arithmetic(13 cases through the realSimpleFunctionFactorydispatch: multiply int64/same-width/mixed-width/vector_constant/constant_constant, plus add and subtract in int64 vec_vec,
same-width DECIMAL32 and DECIMAL64 vec_vec, vector_constant and one-row
constant_constant). Two
benchmark_testbinaries were built from one treediffering only in
be/src/exprs/function/, then run alternately so both sharethermal and scheduling conditions. Figures are the better of two rounds'
5-repetition medians, CPU ns/iteration, 4096-row blocks, macOS arm64 / clang 20
/ Release.
add_d64_d64_const_constsubtract_d64_d64_const_constmultiply_d64_d64_const_constadd_d64_d64_vec_vecadd_d32_d32_vec_vecsubtract_d64_d64_vec_vecadd_d64_d64_vec_constadd_int64_vec_vecsubtract_int64_vec_vecmultiply_d64_d64_vec_vecmultiply_d32_d64_vec_vecmultiply_d64_d64_vec_constmultiply_int64_vec_vecEvery case is inside the +-3.5% round-to-round noise band; nothing regresses.
Behavior changed:
add/subtract/modlookup now failsloudly (
Could not find function ...with argument and return types)instead of resolving. For add/subtract nothing changes in practice —
those keys were already unreachable. For
modthis is a realnarrowing, argued safe above; FE never emits such a call.
add/subtractnow compute at their natural width instead of beingpromoted to
Int256. Results are unchanged; the intermediate widthis not.
Does this need documentation?
Proactive disclosure
construction. Same-width decimal add/subtract come out flat. The
<Type, DECIMAL256>promotion sits on the plain, non-overflow-checkedbranch of the kernel, and the benchmark runner pins
check_overflow_for_decimalto the production default (true), so it neverexecutes that branch. The restoration is a code-level fact you can read in the
diff; it is not something this A/B measured, and I would rather say so than
let the table imply otherwise.
modis the only place where a signature genuinely disappears. add andsubtract were already unresolvable. If a reviewer disagrees with the mod
argument, that is the single hunk to contest, and the contract UT is what
would need updating.
links system malloc rather than tcmalloc (an arm64 branch-range workaround).
Nothing here is platform-specific — it is registration and template code — but
the numbers are single-platform.
constant_constantpaths wasoriginally part of this PR and has been taken back out: isolating it showed
it carried a reproducible +60-70% per-call regression on the constant-folding
path, while this registration change measured flat. It will be proposed
separately, on its own evidence.