morningman opened a new pull request, #66617: URL: https://github.com/apache/doris/pull/66617
> Split out of **https://github.com/apache/doris/pull/66510**. That PR carries the > whole BE build-time batch and its end-to-end measurements — **please refer to > #66510 for the complete benefit numbers**. This is the only piece of that batch > that touches the **function registration surface**, so it is broken out here to > get a proper review from the expression owners. It has no dependency on the > rest of the batch. ### What problem does this PR solve? Related PR: #66510 Problem Summary: Two independent template-instantiation cuts in the binary-arithmetic kernels. Both delete code that cannot be reached at runtime; neither changes what a reachable query computes. #### 1. Mixed-width decimal registrations for `add` / `subtract` / `mod` are unreachable FE casts *both* children of a decimal `Add` / `Subtract` / `Mod` to exactly the return 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 made every 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, whose plain (non-overflow-check) path promoted `Decimal32/64/128` arithmetic to `Int256`. Collapsing the impl to a single type parameter **restores narrow-width arithmetic for same-width inputs**. 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): - Nereids has cast both children of `Mod` to exactly the return type since 2.0 (#17393; briefly removed and restored within two days in May 2023, a 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 silently resolving to the wrong instantiation. `Multiply` is exempt and keeps its full width cross product. #### 2. Hand-written `constant_constant` paths duplicate the default implementation All binary arithmetic functions (`add`/`subtract` via `FunctionPlusMinus`, `multiply`, `divide`, `int_div`, `mod`/`pmod`, `bit and`/`or`/`xor`, bit shifts) disabled `use_default_implementation_for_constants` and carried a hand-written `constant_constant` specialization per `Impl`, plus a dispatch branch in `execute_impl`. For these pure functions the default implementation in `function.cpp` is step-for-step equivalent: when every argument is const it unpacks each to a 1-row column, runs `execute_impl` — the same scalar apply the hand-written path called, including the divide-by-zero null-map and the `INT64_MIN / -1` FPE handling — and wraps the result back into a `ColumnConst` of the original row count. The override is only load-bearing for non-deterministic functions (`random`/`uuid`/`uniform`/`random_bytes`) or ones with execution-timing semantics (`sleep`); **all 12 of those keep it**. The 19 deleted definitions (~550 lines) are the pure ones. The both-const branch in `execute_impl` becomes a `DCHECK`: every caller reaches these functions through `PreparedFunctionImpl::execute`, which unpacks all-const blocks before `execute_impl` (verified — no direct `execute_impl` callers exist outside the cast-family internals). ### Why this is a build-time win Both cuts remove whole template *families*, not individual functions: - dropping mixed-width removes ~2/3 of the decimal instantiations of those TUs; - dropping `constant_constant` removes one `ResultType`-templated body plus its `cast_type_to_either` dispatch block per decimal `Impl`. | measurement | before | after | |---|---|---| | `plus.cpp` single-TU wall (`tu-bench.sh`) | 56.5s | **34.4s (-39%)** | | `plus.cpp` weak definitions | 3171 | **1108 (-65%)** | | `plus.cpp` `.text` | — | **-67%** | | `multiply.cpp` `.text` | 1.47MB | **1.06MB (-28%)**, weak -21% | | end-to-end cold build, `-j6`, same-session A/B | 11m23s | **10m45s (-38s / -5.6%)** | The end-to-end pair ran back to back with identical effective parallelism (5.6×), so the -38s is attributable to these deletions rather than to scheduling noise. ### Release note None ### Check List (For Author) - Test - [x] Regression test — the 56 decimal regression suites match the pre-change baseline suite for suite (the only failure is a known, unrelated S3-credential `outfile` case that also fails on the baseline). - [x] Unit Test — new `BinaryArithmeticRegistrationTest` (`be/test/exprs/function/binary_arithmetic_registration_test.cpp`, 5/5 passing) pins the registration surface: same-width add/subtract/mod lookups must resolve, mixed-width ones must return `nullptr` (there is no bare-name fallback), multiply's 4×4 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. - [x] Manual test - **Constant-folding parity harness**: `enable_fold_constant_by_be=true` (BE fold path, all-const columns) vs `false` (FE fold) over add/subtract/multiply/divide/int_div/mod/pmod/bit/shift — **19 groups, all identical**, including divide-by-zero → NULL, `INT64_MIN DIV -1` → NULL, negative shifts, `pmod`, and DECIMAL32/64/128/256 same-width plus multiply mixed-width. - **Symbol check**: zero `constant_constant` instantiations remain in `doris_be`. - **Microbenchmarks**: `benchmark_binary_arithmetic` — thirteen cases added in the first commit of this PR precisely as a guardrail, five for multiply and eight covering add/subtract (int64 vec_vec as the integral control, same-width DECIMAL32 and DECIMAL64 vec_vec, vector_constant, and one-row constant_constant). Full before/after table below. #### Microbenchmark A/B Two `benchmark_test` binaries built from the same source tree, differing only in the nine `be/src/exprs/function/` kernel files (before = those nine reverted to `02cb462eb25`). Run alternately — after, before, after, before — so both states share thermal and scheduling conditions rather than being separated by a rebuild. Each figure is the better of the two rounds' 5-repetition medians, CPU ns/iteration, macOS arm64 / clang 20 / Release, 4096-row blocks. | case | before | after | delta | |---|---:|---:|---:| | `add_int64_vec_vec` | 556.9 | 554.1 | -0.5% | | `subtract_int64_vec_vec` | 546.8 | 557.0 | +1.9% | | `add_d32_d32_vec_vec` | 1467.5 | 1470.6 | +0.2% | | `add_d64_d64_vec_vec` | 1533.6 | 1540.1 | +0.4% | | `subtract_d64_d64_vec_vec` | 1526.4 | 1531.2 | +0.3% | | `add_d64_d64_vec_const` | 5755.4 | 5797.6 | +0.7% | | `multiply_int64_vec_vec` | 1186.8 | 1181.5 | -0.4% | | `multiply_d64_d64_vec_vec` | 14695.0 | 14465.5 | -1.6% | | `multiply_d32_d64_vec_vec` | 14579.5 | 14394.6 | -1.3% | | `multiply_d64_d64_vec_const` | 14551.3 | 14440.7 | -0.8% | | **`add_d64_d64_const_const`** | **98.3** | **165.8** | **+68.6%** | | **`subtract_d64_d64_const_const`** | **97.5** | **165.7** | **+69.9%** | | **`multiply_d64_d64_const_const`** | **108.1** | **170.4** | **+57.5%** | Every vectorized case is flat — inside the ±2% round-to-round spread. The three `constant_constant` cases are a real, reproducible ~60-70% per-call regression, discussed under disclosure below. The stripped binary also shrinks: `benchmark_test` **288M → 284M**. - Behavior changed: - [x] Yes. 1. A **mixed-width** decimal `add`/`subtract`/`mod` lookup now fails loudly (`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 `mod` this is a real narrowing, argued safe above; FE never emits such a call. 2. On the **plain, non-overflow-checked** branch, same-width decimal `add`/`subtract` now compute at their natural width instead of being promoted to `Int256` by the `<Type, DECIMAL256>` variants that were winning the collapsed factory keys. Results are unchanged; the intermediate width is not. This is a code-level change the A/B below does not exercise — see disclosure. - Does this need documentation? - [x] No. ### Proactive disclosure - **This is the highest-risk PR of the #66510 batch** — it is the only one that deletes registrations, i.e. the only one where "you removed something someone depends on" is a fair question. The contract unit test exists specifically so that question has a machine-checkable answer, and the mod argument above is laid out per-planner and per-upgrade-path rather than asserted. - **The `constant_constant` path is measurably slower**: ~98 → ~166 ns/call for add and subtract, ~108 → ~170 ns for multiply, i.e. **+60-70%**, reproducible across both rounds. This is the cost of routing through `default_implementation_for_constants` (unpack each argument to a 1-row column, run `execute_impl`, rewrap in a `ColumnConst`) instead of a hand-written scalar path. It runs **once per query** on a 1-row column and does not scale with data volume, so ~68 ns there buys ~550 lines and a large slice of the instantiation surface. I think that trade is right, but it is a regression and it is the number a reviewer should push back on if they disagree. - **The claimed narrow-width restoration is not visible in these numbers, by construction.** Same-width decimal add/subtract come out flat (+0.2% to +0.4%). The `<Type, DECIMAL256>` promotion described above sits on the *plain*, non-overflow-checked branch of the kernel, and the benchmark runner pins `check_overflow_for_decimal` to the production default (`true`), so it never executes 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. - Absolute values are not comparable against numbers from earlier runs in #66510: `benchmark_test` links system malloc on macOS arm64 (see the sibling macOS PR) and P/E-core scheduling is not pinned. Within this table the comparison is sound — both binaries were built from one tree and run alternately in one session — but treat it as a same-session A/B, not an absolute benchmark. - Measured on macOS arm64 / clang 20 only. Nothing here is platform-specific (it is registration and template code), but the numbers are single-platform. -- 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]
