Conversation
RH211-sys
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
September 15, 2026 15:15
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
10 tasks
### What problem does this PR solve? Related issue: [apache#48203](apache#48203) Problem Summary: Add the `gamma` scalar function, which generalizes the factorial to real numbers: `gamma(n)` is `(n - 1)!` for a positive integer `n`, and `gamma(0.5)` is `sqrt(pi)`. - BE: `gamma` is registered in `be/src/exprs/function/math.cpp` on top of `std::tgamma`. The poles are mapped to NULL instead of the value the C library produces: `gamma(0)` and every negative integer return NULL, and so does negative infinity, which the BE classifies as a negative integer pole. A NaN argument returns NaN, and positive infinity or an argument large enough to overflow a double (`gamma(172)` and above) returns Infinity. - FE: `Gamma` (unary, `ExplicitlyCastableSignature`, `AlwaysNullable`, `PropagateNullLiteral`), the Nereids visitor entry, and the builtin scalar function registration. - FE constant folding: `NumericArithmetic.gamma`, so that a folded `gamma(<literal>)` produces the same value as the BE. commons-math3's `Gamma.gamma` saturates to Infinity at 165, where `std::tgamma` still returns a finite 3.29e293 (it stays finite up to 171), so positive inputs are evaluated as `exp(logGamma(x))`. Negative non-integers have no overflow problem and use `Gamma.gamma`. ### Release note Add the `gamma` scalar function. `gamma(n)` is `(n - 1)!` for a positive integer `n`; `gamma(0)` and the negative integers return NULL. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [x] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - Behavior changed: - [x] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [x] Yes. <!-- REPLACE THIS WITH THE DOCS PR LINK, e.g. https://github.com/apache/doris-website/pull/XXXX --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label Manual test: - `select gamma(5), gamma(0.5), gamma(-1.5), gamma(0), gamma(-1), gamma(-3);` returns `24.000000000000004, 1.772453850905516, 2.3632718012073544, NULL, NULL, NULL`, and the same values are returned with `set debug_skip_fold_constant=true`, i.e. constant folding and BE execution agree. - `gamma(cast('nan' as double))` is NaN, `gamma(cast('inf' as double))` is Infinity, `gamma(cast('-inf' as double))` is NULL, `gamma(165)` is 3.287218585534318E293, `gamma(171)` is 7.257415615308056E306 and `gamma(172)` is Infinity. - `explain select gamma(v) from t where gamma(v) > 10;` still plans an olap scan with the predicate and the projection applied.
RH211-sys
force-pushed
the
feat-gamma-function
branch
from
September 16, 2026 11:42
7cb9056 to
6ef21f0
Compare
Author
|
Friendly ping @morrySnow @924060929 @englefly @starocean999 (Code Owners of This PR adds the Local validation: full Docs PR: apache/doris-website#4140 Thanks! |
Contributor
|
/review |
924060929
previously approved these changes
Sep 21, 2026
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27904 ms |
Contributor
|
/review Check whether the calculation logic of fe and be is strictly consistent, especially paying attention to boundary values, positive and negative zero, positive and negative infinity, nan, and handling precision overflow. |
Contributor
TPC-DS: Total hot run time: 152298 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes. The gamma registration, signature, visitor hook, and shared nullable/const execution wrapper are structurally consistent with analogous functions, but the FE constant-folding path is not equivalent to the BE implementation, and the new exact fixtures encode the wrong implementation's values.
Critical checkpoints:
- Task goal: the PR adds gamma and maps zero/negative-integer poles to NULL. That common-path behavior is implemented, but FE folding can change finite values and even NaN/Infinity classification relative to BE execution.
- Correctness: MAIN-1 shows concrete Commons Math 3.6.1 versus std::tgamma divergences for ordinary values, large negative non-integers, and the positive subnormal overflow boundary.
- Tests: MAIN-2 shows the BE unit data and generated regression output use FE exp(logGamma) values while the BE test compares std::tgamma results exactly; these expectations are platform-dependent and fail on this runner's libm.
- Scope/safety: no new concurrency, persistence, configuration, FE-BE protocol, transaction, or data-write lifecycle was introduced. The wrapper's nullable/const null-map flow and Nereids registration were independently traced with no additional issue.
- Validation: this was a static review per the supplied prompt; no repository builds or tests were run. The numerical findings were independently reproduced against the declared Commons Math 3.6.1 dependency and the runner's libm. No user-provided focus points were supplied.
Please resolve the two inline findings, then regenerate/extend parity coverage for positive, negative, and subnormal boundary cases before re-review.
| } | ||
| if (Double.isInfinite(x)) { | ||
| // +inf overflows to itself; -inf is treated as a negative integer, i.e. a pole. | ||
| return x > 0 ? new DoubleLiteral(Double.POSITIVE_INFINITY) |
Contributor
There was a problem hiding this comment.
gamma is folded here with Commons Math, but execution uses std::tgamma in BE. These implementations do not agree for all finite doubles: Commons Math 3.6.1 gives gamma(-150.5) = -0.0 while BE returns about -4.478447658150641e-264, and gamma(-1000.5) = NaN while BE returns signed zero; near the positive subnormal overflow boundary (0x0.3ffffffffffffp-1022) FE produces a finite max-double value while BE returns Infinity. Even ordinary values differ (gamma(10) folds to 362879.9999999998 versus BE 362880.0). A literal query can therefore change rows, comparisons, and overflow/NaN classification when constant folding is toggled. Please use a numerically equivalent implementation/shared boundary or disable folding for inputs that cannot be proven equivalent, and add parity coverage for these domains.
Contributor
There was a problem hiding this comment.
If this function does not contribute to partition pruning and bucket pruning, and cannot simply align the implementations of fe and be, then not implementing the version of fe might be a better choice.
| {{2.5}, 1.329340388179137}, | ||
| {{-0.5}, -3.5449077018110318}, | ||
| {{-1.5}, 2.3632718012073544}, | ||
| {{-2.5}, -0.94530872048294179}, |
Contributor
There was a problem hiding this comment.
These exact expected values are from the FE exp(logGamma) route, not the registered BE call. On this runner std::tgamma(5) is 24.0, tgamma(10) is 362880.0, tgamma(-1.5) is 2.363271801207355, tgamma(-2.5) is -0.9453087204829419, and tgamma(171) is 7.257415615307999e306, while this test expects different doubles. check_function compares ColumnFloat64 values exactly, so the unit test (and the generated regression fixture) is platform-dependent and can fail. Please regenerate expectations from the BE implementation or use a documented tolerance/portable oracle.
Contributor
ClickBench: Total hot run time: 23.99 s |
Contributor
There was a problem hiding this comment.
Request changes.
The gamma registration, signature, visitor dispatch, nullable typing, constant/null adapters, and BE registration are structurally consistent. This exact-head review converged after two full normal and risk-focused passes.
Findings:
- Existing inline comments 4058837042 and 4058837046 already cover the blocking FE/BE numerical divergence and the nonportable exact libm unit oracle, so I did not duplicate them.
- This review adds one distinct test defect: the supposed negative-zero row stores positive zero, leaving the requested signed-zero boundary untested.
Checkpoint conclusions:
- Goal and scope: the patch is focused and structurally wires gamma(DOUBLE) to Nullable(DOUBLE), but strict FE/BE numerical equivalence is not met per the existing thread.
- Parallel paths and conditions: FE folding and BE execution were traced through their full call chains. Pole handling for signed zero and finite negative integers, plus explicit NaN and infinity branches, is structurally aligned; the remaining numerical boundary failures are already covered by the existing thread.
- Tests: result cardinalities, ordering, nullable/constant shapes, and generated output are internally consistent. The exact libm oracle is covered by an existing thread; the new inline identifies the ineffective signed-zero input.
- Concurrency, lifecycle, configuration, compatibility, persistence, transactions/data writes, protocol/storage format, and observability: no new mechanism requiring a separate finding was introduced. No separate performance or error-handling issue was substantiated.
- Validation: static review only, as required; no Doris build or test suite was run. Numerical behavior was independently probed with temporary review-only programs, not a Doris execution.
| sql "truncate table test_gamma" | ||
| sql """ insert into test_gamma values | ||
| (1, 1, 1), (2, 2, 2), (3, 3, 3), (4, 4, 4), (5, 5, 5), (6, 10, 10), | ||
| (7, 0, 0), (8, -0, -0), (9, -1, -1), (10, -2, -2), (11, -3, -3), |
Contributor
There was a problem hiding this comment.
[P2] Construct an actual negative-zero input. Both occurrences of -0 here are parsed as integer unary minus (0 - 0) before the DOUBLE column cast, so this row stores +0.0 and duplicates row 7. Please use an explicit DOUBLE construction such as cast('-0.0' as double) and add a sign-sensitive check (for example signbit(b)) so the requested negative-zero boundary is genuinely exercised before relying on the gamma result.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Related issue: [apache#48203](apache#48203) Problem Summary: The FE folded `gamma(<literal>)` through commons-math3 while the BE computes it with `std::tgamma`. The two are independent implementations that cannot be kept aligned, so a folded constant could differ from the value the BE produces. Measured on the Doris build environment: - `gamma(-1000.5)` is NaN from the FE and -0.0 from the BE, a different classification; - `gamma(-150.5)` is 0.0 from the FE and -4.4784476581511713e-264 from the BE; - `gamma(10)` is 362879.9999999998 from the FE and 362880.00000000047 from the BE, about 11 ulp. `gamma` feeds neither partition nor bucket pruning, so the folding buys nothing. Remove it (`NumericArithmetic.gamma` and the commons-math3 import it needed) and let the BE stay the only implementation. There is then a single evaluation path: the default session, a session with `debug_skip_fold_constant=true`, a session with `enable_fold_constant_by_be=true` and a query that reads `gamma` of a table column all return identical values for the boundary inputs below (0, -0.0, negative integers, +/-infinity, NaN, 165/170/171/171.5/171.8/172/1e308, the smallest subnormal and the smallest normal, 1e-300, and large negative non-integers). Also in this commit: - The negative-zero row of the regression suite was written as the integer `-0`, which is stored as +0.0 and silently duplicated the row holding 0.0. It now uses `cast('-0.0' as double)` and is guarded by a `signbit` assertion, so a wrong test value fails instead of being recorded in the generated .out file. The suite also gains the boundary classes it was missing: inputs whose reciprocal overflows (1e-300, 5e-324, 2.2250738585072014e-308), the overflow onset (171.5 is finite, 171.8 is Infinity) and large negative non-integers that underflow to -0.0 (-1000.5) or to a subnormal (-171.5). - The near-zero rows hold what libm's tgamma returns, about 1e-14 relative away from the correctly rounded value, which is inside the 1e-8 relative tolerance the framework applies to DOUBLE cells. The two properties that tolerance cannot see are asserted directly: the sign of a result that underflows to zero, and a subnormal result that must not collapse to zero. - `testFoldConst` passes by construction now that nothing is folded, so it is kept as a guard for a folding that might come back, and its last two columns are BOOLEAN (`signbit(gamma(-1000.5))`, `gamma(-171.5) > 0`). checkCell compares BOOLEAN cells exactly, while its double path would not notice a folding that flipped the sign of an underflowed zero (0.0 against -0.0 divides by a zero magnitude) or let a subnormal collapse to zero (the decimal-place fallback accepts it). - `math.cpp` cited a MySQL `gamma` anchor, but MySQL has no `gamma` function. The comment now states the convention actually followed: a domain error such as a pole returns NULL, as `sqrt(-1)` and `ln(0)` do, and only overflow returns Infinity. - The BE unit test also pins -0.0 as a pole. - The new FE statements are covered by `GammaTest`, which exercises the signature, the nullability, the implicit cast of the argument and the visitor dispatch through `withChildren`. This is what `check_coverage_fe` reports as uncovered. ### Release note None ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [x] Manual test (the boundary matrix described above) - [ ] No need to test or manual test. Explain why: - Behavior changed: - [x] Yes: `gamma(<literal>)` is no longer constant folded in the FE, it is computed by the BE like every other invocation. The values returned to the client are unchanged. - Does this need documentation? - [x] Yes: apache/doris-website#4140
RH211-sys
force-pushed
the
feat-gamma-function
branch
from
September 21, 2026 12:48
8190c82 to
855f0c9
Compare
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27872 ms |
Contributor
TPC-DS: Total hot run time: 152150 ms |
Contributor
ClickBench: Total hot run time: 24 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
|
@RH211-sys please also add document for this function |
…ge gate ### What problem does this PR solve? Related issue: [apache#48203](apache#48203) Problem Summary: `check_coverage_fe` requires the increment line coverage to be 100% and failed with `Gamma.java 90.00% (9/10)`, even though every executable line of the class was reached. The `Preconditions.checkArgument(children.size() == 1)` in `withChildren` is the only branch in the class. The line itself executes, but its failing direction never does, and the coverage portal reports a partially covered line as uncovered - locally the same run reports `LINE_MISSED=0` while `BRANCH_MISSED=1` and `INSTRUCTION_MISSED=1`. Exercise the failing direction too, so the line is fully covered. Verified with `run-fe-ut.sh --run --coverage org.apache.doris.nereids.trees.expressions.functions.scalar.GammaTest`: Gamma now reports instruction 0/48 missed, branch 0/2 missed, line 0/10 missed, and the four tests of GammaTest pass. ### Release note None
Author
|
@yiguolei The documentation is in apache/doris-website#4140 — it was |
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27943 ms |
Contributor
TPC-DS: Total hot run time: 152466 ms |
Contributor
ClickBench: Total hot run time: 24.01 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Author
|
run buildall |
Author
|
/review |
Contributor
FE UT Coverage ReportIncrement line 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
5 participants
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 issue: #48203
Related PR: #51576, #60111 — earlier attempts at the same function, both closed without merging.
Problem Summary:
Add the
gammascalar function, which generalizes the factorial to real numbers:gamma(n)is(n - 1)!for a positive integern, andgamma(0.5)issqrt(pi).gammais registered inbe/src/exprs/function/math.cppon top ofstd::tgamma. The polesare mapped to NULL instead of the value the C library returns:
gamma(0)(including-0.0) andevery negative integer return NULL, and so does negative infinity, which the BE classifies as a
negative integer pole. A NaN argument returns NaN. Positive infinity, and any argument whose value
overflows a double (
171.5is still finite,171.8and172are Infinity), return Infinity, andso does a very small positive argument, where
gamma(a)is about1 / a. This follows theconvention of the other math functions here: a domain error such as
sqrt(-1)orln(0)is NULLand only overflow is Infinity.
Gamma(unary,ExplicitlyCastableSignature,AlwaysNullable,PropagateNullLiteral), theNereids visitor entry, and the builtin scalar function registration. The FE does not compute
gammaitself. An earlier revision foldedgamma(<literal>)through commons-math3, but thatlibrary and
std::tgammaclassify the boundaries differently —gamma(-1000.5)is NaN fromcommons-math3 and
-0.0from the BE,gamma(-150.5)is0.0against-4.4784476581511713e-264,and
gamma(10)differs by about 11 ulp — so the folding was removed instead of trying to keep thetwo in step.
gammafeeds neither partition nor bucket pruning, so the folding had no benefit, andthe BE is now the single implementation.
test_gamma.groovy) covering the domain — the poles,-0.0, negativenon-integers, ±infinity, NaN, the overflow onset, subnormal inputs, and large negative arguments
that underflow to a signed zero — plus a BE unit test and a
GammaTestFE unit test for the new FEstatements. The suite evaluates
gamma(<literal>),gammaof a table column, and both foldingswitches, so every evaluation path is covered.
Release note
Add the
gammascalar function.gamma(n)is(n - 1)!for a positive integern;gamma(0)and the negative integers return NULL.
Check List (For Author)
gamma(<literal>)is not constant folded in the FE any more, it is computed by the BElike every other invocation. The values returned to the client are unchanged.
Check List (For Reviewer who merge this PR)
Manual test:
select gamma(5), gamma(0.5), gamma(-1.5), gamma(0), gamma(-1), gamma(-3);returns24.000000000000004, 1.772453850905516, 2.3632718012073544, NULL, NULL, NULL; the same valuescome back with
set debug_skip_fold_constant=trueand withset enable_fold_constant_by_be=true, i.e. every evaluation path agrees.gamma(cast('nan' as double))is NaN,gamma(cast('inf' as double))is Infinity,gamma(cast('-inf' as double))is NULL,gamma(cast('-0.0' as double))is NULL,gamma(171)is7.257415615308056E306,
gamma(171.5)is 9.483367566824735E307,gamma(171.8)is Infinity,gamma(1e-300)is 9.999999999999763E299,gamma(5e-324)is Infinity, andgamma(-1000.5)is-0.0.explain select gamma(v) from t where gamma(v) > 10;still plans an olap scan with the predicateand the projection applied.