morningman commented on PR #68524: URL: https://github.com/apache/doris/pull/68524#issuecomment-6010799722
<!-- doris-repo-review:report:begin --> ## Code review of PR #68524 — **REQUEST_CHANGES** | | | |---|---| | Reviewed head | `1f90c1be31a4d7ef0df6b08b90a93e0b72075e15` | | Diff range | `0dcd2a31a7bb...1f90c1be31a4` (three-dot, from the merge base) | | Scope | 10 files, +610/-0 lines, 3 commits | | Result | 1 Blocker, 1 Major, 4 Minor, 1 Nit | ### Summary The PR adds the scalar function `human_readable_seconds(DOUBLE)`: a BE vectorized kernel, an FE constant-folding implementation, FE/BE registration, a BE unit test, an FE unit test and a regression suite. The core formatting is right: for every finite double below 2^63 the FE fold and the BE kernel produce byte-identical strings, and the strings match Trino 435's implementation (differential probes over 33 input classes and about 6.2 million values). Two defects block the merge. The regression `.out` file was written by hand — its block tags keep the `qt_`/`order_qt_` prefix and its `order_qt_` rows are in numeric instead of string order — so the new suite fails at its first statement on every run (F-01). The int64 overflow guard is off by one representable double on both sides, so a BIGINT within 512 of BIGINT_MAX/BIGINT_MIN returns an empty string on an x86-64 BE while the FE folds the same literal to a full string, and neither side returns the NULL the PR promises (F-02). The remaining findings are test coverage (no FE-vs-BE check, no edge cases end to end), unreachable code with divergent semantics, a stale and incorrect comment that justifies hand-rolled null handling, and an overstated "Trino 1:1" claim. ### Findings #### F-01 · The regression `.out` is hand-written; the new suite fails at its first statement on every run - **Severity**: Blocker - **Category**: test-coverage (regression baseline written by hand) - **Regression**: no (new suite and new baseline file) - **Where**: [`regression-test/data/query_p0/sql_functions/datetime_functions/test_human_readable_seconds.out:1-3`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/regression-test/data/query_p0/sql_functions/datetime_functions/test_human_readable_seconds.out#L1-L3), [`regression-test/data/query_p0/sql_functions/datetime_functions/test_human_readable_seconds.out:41-44`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/regression-test/data/query_p0/sql_functions/datetime_functions/test_human_readable_seconds.out#L41-L44), [`regression-test/suites/query_p0/sql_functions/datetime_functions/test_human_readable_seconds.groovy:20`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/regression-test/suites/query_p0/sql_functions/datetime_functions/test_human_readable_seconds.groovy#L20) ```text -- This file is automatically generated. Do not edit. -- !qt_sql_const_0 -- 0 seconds ... -- !order_qt_sql_batch_double -- 1 0 seconds 2 1 second 3 1 minute ``` **What is wrong.** All 16 block headers carry the statement prefix (`-- !qt_sql_const_0 --`, `-- !order_qt_sql_batch_double --`), but the framework looks blocks up by the tag *without* the prefix, so no block is ever found. The three `order_qt_` blocks are also in numeric id order, while the framework compares rows in string order. The header line is not the one the generator writes. The file was not produced by `-genOut`, which root `AGENTS.md` requires ("Regression test result files must not be handwritten"). **Why it happens.** `qt_sql_const_0 "..."` is dispatched by `Suite.invokeMethod`, which strips the prefix and runs `quickTest("sql_const_0", ...)` ([`regression-test/framework/src/main/groovy/org/apache/doris/regression/suite/Suite.groovy:2106-2109`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/regression-test/framework/src/main/groovy/org/apache/doris/regression/suite/Suite.groovy#L2106-L2109)). The reader takes the header text verbatim as the tag, so the file's tag is `qt_sql_const_0` ([`regression-test/framework/src/main/groovy/org/apache/doris/regression/util/OutputUtils.groovy:336-339`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/regression-test/framework/src/main/groovy/org/apache/doris/regression/util/OutputUtils.groovy#L336-L339)). `hasNextTagBlock("sql_const_0")` compares with `Objects.equals`, drains every block and returns false ([`regression-test/framework/src/main/groovy/org/apache/doris/regression/util/OutputUtils.groovy:357-370`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/regression-test/framework/src/main/groovy/org/apache/doris/regression/util/OutputUtils.groovy#L357-L370)), and the suite throws `IllegalStateException: Missing output block for tag 'sql_const_0'` ([`regression-test/framework/src/main/groovy/org/apache/doris/regression/suite/Suite.groovy:1946-1947`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/regression-test/framework/src/main/groovy/org/apache/doris/regression/suite/Suite.groovy#L1946-L1947)). Renaming the tags is not enough: for `order_qt_` the real rows are sorted column-wise by `toString()` ([`regression-test/framework/src/main/groovy/org/apache/doris/regression/suite/Suite.groovy:1988-1989`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/regression-test/framework/src/main/groovy/org/apache/doris/regression/suite/Suite.groovy#L1988-L1989), [`regression-test/framework/src/main/groovy/org/apache/doris/regression/util/DataUtils.groovy:25-46`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/regression-test/framework/src/main/groovy/org/apache/doris/regression/util/DataUtils.groovy#L25-L46)), giving ids `1, 10, 11, 2, ..., 9`, and `checkOutput` compares line by line ([`regression-test/framework/src/main/groovy/org/apache/doris/regression/util/OutputUtils.groovy:118-161`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/regression-test/framework/src/main/groovy/org/apache/doris/regression/util/OutputUtils.groovy#L118-L161)), so each `order_qt_` block would still fail at its second row. The generator's header is `-- This file is automatically generated. You should know what you did if you want to edit this` ([`regression-test/framework/src/main/groovy/org/apache/doris/regression/util/OutputUtils.groovy:252`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/regression-test/framework/src/main/groovy/org/apache/doris/regression/util/OutputUtils.groovy#L252)). **When it bites.** On every run of `query_p0`, as soon as this suite executes its first statement. The PR head has no TeamCity status at all (only the `code-review` check), so the suite has never run in CI; it will turn P0 red as soon as the full CI pipeline (buildall) is triggered, and the end-to-end evidence the PR claims ("Regression test") does not exist yet. **Suggested fix.** Delete the hand-written file and regenerate it against a running cluster. `-genOut` skips an existing file ([`regression-test/framework/src/main/groovy/org/apache/doris/regression/suite/SuiteContext.groovy:620-633`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/regression-test/framework/src/main/groovy/org/apache/doris/regression/suite/SuiteContext.groovy#L620-L633)), so use `-forceGenOut`: ```bash ./run-regression-test.sh --run -d query_p0 -s test_human_readable_seconds -forceGenOut ``` Commit the generated file unchanged. Its values for the current inputs should be the same strings, but its tags and `order_qt_` row order will differ from the hand-written file. #### F-02 · The int64 overflow guard lets |x| == 2^63 through: an x86-64 BE returns an empty string, the FE folds a full string, and neither returns NULL - **Severity**: Major - **Category**: data-error (numeric boundary, FE/BE inconsistency) - **Regression**: no (new function) - **Where**: [`be/src/exprs/function/function_human_readable_seconds.cpp:199-205`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_human_readable_seconds.cpp#L199-L205), [`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java:2072-2076`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java#L2072-L2076) ```cpp double abs_val = std::abs(static_cast<double>(val)); if (abs_val > static_cast<double>(std::numeric_limits<int64_t>::max())) { null_data[i] = 1; res_offsets[i] = cast_set<UInt32>(res_data.size()); continue; } int64_t seconds = std::llround(abs_val); ``` ```java double absVal = Math.abs(val); if (absVal > Long.MAX_VALUE) { return new NullLiteral(VarcharType.SYSTEM_DEFAULT); } return formatHumanReadableSeconds(Math.round(absVal)); ``` **What is wrong.** INT64_MAX is not representable as a double; `static_cast<double>(INT64_MAX)` (and Java's implicit promotion of `Long.MAX_VALUE`) rounds up to exactly 2^63. The guard `abs_val > 2^63` therefore lets |x| == 2^63 through, although 2^63 is outside int64. The PR body promises "values exceeding int64 bounds return NULL"; instead the three runtimes return three different things: | Where it is evaluated | Result for \|x\| == 2^63 | |---|---| | FE constant folding (`Math.round` clamps to `Long.MAX_VALUE`) | `15250284452471 weeks, 3 days, 15 hours, 30 minutes, 7 seconds` | | BE on x86-64 Linux (glibc `llround`) | `''` (empty, non-NULL) | | BE on aarch64 (glibc `llround` saturates) | `15250284452471 weeks, 3 days, 15 hours, 30 minutes, 7 seconds` | | PR contract | NULL | **Why it happens.** `std::llround` of an out-of-range value returns an unspecified value. The BE build does not use fast-math, so clang lowers the call to `llround@PLT` into glibc. glibc's generic implementation returns `(long long) x` for |x| >= 2^63, which on x86-64 is `cvttsd2si`'s integer-indefinite value INT64_MIN. With `seconds = INT64_MIN`, every unit in `format_seconds` is negative, no `> 0` branch appends anything, and the function returns length 0 ([`be/src/exprs/function/function_human_readable_seconds.cpp:131-165`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_human_readable_seconds.cpp#L131-L165)). The row is emitted with `null_data[i] = 0` and an empty string. On the FE, `absVal > Long.MAX_VALUE` is `2^63 > 2^63`, which is false, and `Math.round(2^63)` clamps to `Long.MAX_VALUE`. **When it bites.** Any of these values reaching the function: - DOUBLE ±9223372036854775808.0. - BIGINT in [9223372036854775296, 9223372036854775807] or [-9223372036854775808, -9223372036854775296]. These all widen to ±2^63, and `BIGINT_MAX`/`BIGINT_MIN` are common "no limit" sentinels in duration columns. - LARGEINT in [9223372036854775296, 9223372036854776832] or its negation. `SELECT human_readable_seconds(9223372036854775807)` folds on the FE to the full string. The same value read from a BIGINT column returns `''` on every x86-64 BE, so a literal query and a table query disagree, and so do BEs of different architectures. All of this was reproduced by running the PR's own code outside the repository: - glibc 2.17 x86-64 (`docker run --platform linux/amd64 centos:7`, ctypes): `llround(2.0**63)` = -9223372036854775808. - The PR's `format_seconds` copied verbatim into a probe: `format_seconds(INT64_MIN)` returns len=0. - JDK 17.0.19: `absVal > Long.MAX_VALUE` is false and `Math.round(absVal)` = 9223372036854775807. - glibc 2.39 aarch64 (`ubuntu:24.04`): saturates to INT64_MAX. **Suggested fix.** Reject 2^63 on both sides, and pin it with tests on both sides (see F-03): ```diff --- a/be/src/exprs/function/function_human_readable_seconds.cpp +++ b/be/src/exprs/function/function_human_readable_seconds.cpp - if (abs_val > static_cast<double>(std::numeric_limits<int64_t>::max())) { + // (double)INT64_MAX rounds up to 2^63, which llround cannot represent (glibc x86-64 + // returns INT64_MIN), so 2^63 itself must be rejected as well. + if (abs_val >= 0x1p63) { --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java - if (absVal > Long.MAX_VALUE) { + // (double) Long.MAX_VALUE == 2^63; keep 2^63 out of Math.round so FE and BE agree on NULL + if (absVal >= 0x1p63) { ``` The alternative is Trino's behaviour: clamp to INT64_MAX on both sides and drop the overflow-to-NULL rule. That would also remove the deviation noted in F-06. Either choice works as long as FE and BE make the same one. #### F-03 · No test compares FE folding with BE execution, and the edge cases are not covered end to end - **Severity**: Minor - **Category**: test-coverage (FE/BE equivalence) - **Regression**: no (new tests) - **Where**: [`regression-test/suites/query_p0/sql_functions/datetime_functions/test_human_readable_seconds.groovy:19-32`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/regression-test/suites/query_p0/sql_functions/datetime_functions/test_human_readable_seconds.groovy#L19-L32) ```groovy // Constant folding queries matching Trino specification qt_sql_const_0 "SELECT human_readable_seconds(0);" qt_sql_const_1 "SELECT human_readable_seconds(1);" qt_sql_const_60 "SELECT human_readable_seconds(60);" ``` **What is wrong.** The function is implemented twice, once as the FE fold and once as the BE kernel. No test runs both on the same input. - The 13 constant queries are all folded by the FE: the full fold rewriter runs `FoldConstantRuleOnFE` before `FoldConstantRuleOnBE` ([`fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/FoldConstantRule.java:38-42`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/FoldConstantRule.java#L38-L42)), and the FE always folds a `DoubleLiteral` argument. The BE kernel therefore sees only the 11 table rows, which hold ordinary values. - The edge cases the PR body lists (NaN, ±Infinity, |x| beyond int64, the int64 boundary) are not exercised end to end. - The BE UT covers NaN/±Inf/NULL only ([`be/test/exprs/function/function_time_test.cpp:2828-2864`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/test/exprs/function/function_time_test.cpp#L2828-L2864)); the overflow-to-NULL branch at [`be/src/exprs/function/function_human_readable_seconds.cpp:200-204`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_human_readable_seconds.cpp#L200-L204) has no test anywhere. - The FE UT ([`fe/fe-core/src/test/java/org/apache/doris/nereids/rules/expression/FoldConstantTest.java:1716-1728`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/expression/FoldConstantTest.java#L1716-L1728)) has no NaN, Infinity or overflow case. **Why it matters.** F-02 exists precisely because the two implementations were written independently. The repository already has the gate for this: `testFoldConst` runs a statement with folding on and with `debug_skip_fold_constant=true` and compares the two results ([`regression-test/framework/src/main/groovy/org/apache/doris/regression/suite/Suite.groovy:2449-2483`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/regression-test/framework/src/main/groovy/org/apache/doris/regression/suite/Suite.groovy#L2449-L2483)). It is widely used in the neighbouring `datetime_functions` suites. `testFoldConst("SELECT human_readable_seconds(9223372036854775807)")` fails on an x86-64 BE today. **When it bites.** Any future change to one implementation but not the other, including the F-02 fix, lands with every test still green. **Suggested fix.** Add `testFoldConst` cases with DOUBLE/BIGINT inputs: - `9223372036854775807` - `-9223372036854775808` - `9223372036854775295` - `1e19` - `cast('nan' as double)`, `cast('inf' as double)`, `cast('-inf' as double)` - the ties `0.5`, `2.5`, `-2.5` Add the overflow rows (`1e19` -> NULL, `9223372036854775807.0` -> NULL after the F-02 fix) to the BE UT and FE UT. Avoid FLOAT literals >= 2^24 and long-scale DECIMAL near-ties in `testFoldConst`: they diverge in the pre-existing generic casts (D-16, D-17), not in this function. #### F-04 · Unreachable code: four FE integer-literal fold overloads, the BE Float32 branch and a second return-type signature - **Severity**: Minor - **Category**: maintainability (dead code with divergent semantics) - **Regression**: no (new code) - **Where**: [`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java:2082-2117`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java#L2082-L2117), [`be/src/exprs/function/function_human_readable_seconds.cpp:83-95`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_human_readable_seconds.cpp#L83-L95), [`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/HumanReadableSeconds.java:42-45`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/HumanReadableSeconds.java#L42-L45) ```java @ExecFunction(name = "human_readable_seconds") public static Expression humanReadableSeconds(BigIntLiteral seconds) { long val = seconds.getValue(); if (val == Long.MIN_VALUE) { return new NullLiteral(VarcharType.SYSTEM_DEFAULT); } return formatHumanReadableSeconds(Math.abs(val)); } ``` **What is wrong.** - **FE integer-literal overloads.** The only argument type is DOUBLE ([`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/HumanReadableSeconds.java:43-44`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/HumanReadableSeconds.java#L43-L44)), so coercion wraps every argument in `CAST(... AS DOUBLE)`. `FoldConstantRuleOnFE.visitBoundFunction` folds that child first ([`fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/FoldConstantRuleOnFE.java:600-607`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/FoldConstantRuleOnFE.java#L600-L607)), and `ExpressionEvaluator` picks the method by the child's runtime class ([`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/ExpressionEvaluator.java:125-175`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/ExpressionEvaluator.java#L125-L175)). Only `humanReadableSeconds(DoubleLiteral)` can ever run; the BigInt, Integer, SmallInt and TinyInt overloads cannot be selected. - **Their semantics differ from the live path.** `Long.MIN_VALUE` and `Integer.MIN_VALUE` return NULL, while the live DOUBLE path formats them (abs(INT_MIN) = 2147483648 gives `3550 weeks, 5 days, 3 hours, 14 minutes, 8 seconds`). If someone later adds an integer signature, as the first review round suggested, these become live with the wrong rule. - **BE Float32 branch.** It can only be reached from the BE UT ([`be/test/exprs/function/function_time_test.cpp:2851-2863`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/test/exprs/function/function_time_test.cpp#L2851-L2863)). - **InvalidArgument fallback.** The fallback at lines 92-95 has no reachable failure path, which root `AGENTS.md` ("assert correctness only") rules out. Sibling kernels fed the same FE-coerced DOUBLE simply `assert_cast` to `ColumnFloat64`: `format_number` ([`be/src/exprs/function/function_format.cpp:55-57`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_format.cpp#L55-L57)) and `sec_to_time` ([`be/src/exprs/function/function_date_or_datetime_computation.h:1452`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_date_or_datetime_computation.h#L1452)). - **Second signature.** `ret(StringType).args(DoubleType)` has the same argument list as the VARCHAR one before it. `SearchSignature` keeps the first candidate on a full tie ([`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/SearchSignature.java:127-155`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/SearchSignature.java#L127-L155)), so it never wins. - **PR body.** It also describes a `FloatLiteral` fold overload that does not exist. **Why it happens.** They are leftovers of the first PR commit `2a73e15f717`. That commit declared BIGINT/INT signatures, which made the overloads reachable. Commit `eea4291061c` switched the signature to DOUBLE but kept the overloads. **When it bites.** It does not today. The cost is that readers believe the overloads matter, and that the dead overloads become live with the wrong semantics as soon as an integer signature is added. **Suggested fix.** Delete the four integer overloads ([`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java:2079-2117`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java#L2079-L2117)) and the STRING signature. In the BE, replace the Float64/Float32 probing with `assert_cast<const ColumnFloat64*>` and remove the Float32 template instantiation and its UT, or keep a single `DORIS_CHECK`. Correct the PR body. #### F-05 · Stale and incorrect comment justifies hand-rolled null handling; the manual unnest skips the documented Const(Nullable) check - **Severity**: Minor - **Category**: maintainability (null handling, follow context) - **Regression**: no (new code) - **Where**: [`be/src/exprs/function/function_human_readable_seconds.cpp:60-78`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_human_readable_seconds.cpp#L60-L78) ```cpp // We must return false here to manually handle both input nulls and computed nulls // (negative values -> NULL). If default null implementation is used, it strips nulls // before execution, conflicting with our explicit output null map creation. bool use_default_implementation_for_nulls() const override { return false; } ``` **What is wrong.** Both premises of the comment are false at HEAD. - **"negative values -> NULL" is stale.** The first PR commit `2a73e15f717` returned NULL for `val < 0`. Commit `eea4291061c` switched to `std::abs` (line 199) and left the comment behind. - **The default null implementation does not conflict with computed NULLs.** `wrap_in_nullable` explicitly merges the null map of a result that is already nullable ([`be/src/exprs/function/function.cpp:52-55`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function.cpp#L52-L55)). `ipv4_num_to_string` is a direct precedent: it keeps default null handling and returns a `ColumnNullable` with computed NULLs ([`be/src/exprs/function/function_ip.h:105-107`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_ip.h#L105-L107)). - **The hand-rolled unnest has a gap.** It casts after `is_nullable()` (lines 74-75) with no comment saying why a `ColumnConst(ColumnNullable)` cannot arrive, which code-review checklist 1.3.6 asks for. `ColumnConst::is_nullable()` returns true for Const(Nullable) ([`be/src/core/column/column_const.h:280`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/core/column/column_const.h#L280)), so such a column would fail the `assert_cast`. It cannot arrive today only because the single-argument const path unwraps it first ([`be/src/exprs/function/function.cpp:153-155`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function.cpp#L153-L155)). **When it bites.** Not at runtime today. The next maintainer is told that default null handling cannot work here, and inherits the manual unnest along with its undocumented invariant. **Suggested fix.** Keep the default null implementation: - Remove the override and lines 70-78, along with the per-row input-null branch at lines 187-191. - Keep `get_return_type_impl` returning `make_nullable(String)`, like `ipv4_num_to_string`. - Let `execute_impl` return the `ColumnNullable` it builds for NaN/Inf/overflow. If manual handling stays, replace the comment with the real reason. Also use `VectorizedUtils::get_null_map` + `remove_nullable` ([`be/src/exec/common/util.hpp:107-117`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exec/common/util.hpp#L107-L117)) or add a `DORIS_CHECK(!is_column_const(*source_col))`. #### F-06 · "Matching Trino 1:1" is overstated: NaN/Infinity and overflow behave differently from Trino, and the linked doc omits the overflow rule - **Severity**: Minor - **Category**: wording (compatibility claim) - **Regression**: no (new function) - **Where**: [`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java:2069-2075`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java#L2069-L2075) ```java if (Double.isNaN(val) || Double.isInfinite(val)) { return new NullLiteral(VarcharType.SYSTEM_DEFAULT); } double absVal = Math.abs(val); if (absVal > Long.MAX_VALUE) { return new NullLiteral(VarcharType.SYSTEM_DEFAULT); } ``` **What is wrong.** Several user-facing texts claim full Trino compatibility: - The PR summary says "matching Trino's `human_readable_seconds` specification 1:1". - The release note says "Trino-compatible". - The linked user doc (apache/doris-website#4174) says "compatible with Trino's `human_readable_seconds`". The code deliberately returns NULL in two cases where Trino 435 does something else. For NaN and ±Infinity, Trino raises `INVALID_FUNCTION_ARGUMENT`. For |x| at or beyond 2^63, Trino returns the value clamped by `Math.round(Math.abs(x))`, e.g. `1e19` -> `15250284452471 weeks, 3 days, 15 hours, 30 minutes, 7 seconds`. Evidence: `trino-main-435-sources.jar`, `io/trino/operator/scalar/timestamp/HumanReadableSeconds.java` lines 51-55. The PR body's own bullets declare these NULLs, so the behaviour is intended, but the "1:1" claim contradicts them. The linked doc lists NULL/NaN/Infinity and never mentions the overflow rule. The summary also still names the signature `human_readable_seconds(int/bigint)`, while the only signature is DOUBLE. For all finite |x| below 2^63 the FE output equals Trino's (a differential probe over 6.2 million inputs found no difference), so the claim is accurate for normal values. **When it bites.** A user migrating Trino SQL (the motivation in #48203) relies on "compatible" and gets NULL where Trino would raise an error or return a clamped string. **Suggested fix.** Drop "1:1" or qualify it. In the PR body, release note and doc, state that NaN/±Infinity return NULL (Trino raises an error) and that |x| >= 2^63 returns NULL (Trino clamps). Change `(int/bigint)` to `(double)`. #### F-07 · Singular "week" and a weeks-only value are never tested - **Severity**: Nit - **Category**: test-coverage (format branches) - **Regression**: no (new tests) - **Where**: [`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java:2133-2135`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java#L2133-L2135) ```java if (weeks > 0) { sb.append(weeks).append(weeks == 1 ? " week" : " weeks"); } ``` **What is wrong.** Every input in the FE UT, the BE UT and the regression suite is one of 0, 1, ±60, ±61, 3601, 3660, 8003, 56363463 (93 weeks, plural, every unit non-zero) and 535333.95/.25 (no weeks). No test reaches `1 week` or a value of whole weeks followed by omitted zero units. **When it bites.** Swapping `" week" : " weeks"` here, or `"week", "weeks"` at [`be/src/exprs/function/function_human_readable_seconds.cpp:145`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_human_readable_seconds.cpp#L145), leaves all three suites green. **Suggested fix.** Add `604800 -> '1 week'`, `604801 -> '1 week, 1 second'` and `1209600 -> '2 weeks'` to the FE UT and BE UT, and to the regression suite through `testFoldConst`. <details> <summary><b>Critical checkpoints</b></summary> | Checkpoint | Conclusion | |---|---| | Goal achieved, proven by a test | Partly. Formatting is correct and FE/BE-identical for finite \|x\| < 2^63, and matches Trino. The 2^63 cell is wrong on both sides (F-02). The e2e test cannot pass (F-01); BE UT and FE UT cover normal values only (F-03). | | Change is minimal, clear, focused | Mostly. Unreachable FE overloads, a dead Float32 branch and a dead signature (F-04), plus hand-rolled null handling with a wrong rationale (F-05). | | Concurrency: threads, shared state, locks, lock order | n/a: stateless `IFunction` and static FE methods; no shared state. | | Lifecycle / static init order | n/a: only a function-local `static constexpr char[]`; registration runs inside `SimpleFunctionFactory::instance()`'s one-time init. | | New config: dynamic? observed without restart? | n/a: no config added. | | Incompatible change: rolling-upgrade path | n/a: new builtin, no thrift or storage change. A new FE with an old BE fails column calls with "function not found", the normal behaviour of any new function. | | Parallel code paths updated | FE fold, BE kernel and BE constant folding all exist. FE and BE agree except at \|x\| == 2^63 (F-02); BE constant folding runs the same kernel through [`be/src/exprs/function/function.cpp:153-155`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function.cpp#L153-L155). | | Special conditionals explained | No. The overflow guard has no comment and is off by one (F-02). The `Long.MIN_VALUE`/`Integer.MIN_VALUE` -> NULL checks contradict the live path (F-04). The null-handling comment is wrong (F-05). | | Test coverage (e2e, negative, unit) | Insufficient. The e2e suite is broken (F-01). There is no FE-vs-BE comparison and no overflow/boundary case (F-03), and no singular-week case (F-07). | | Test results added/modified and correct | The `.out` was written by hand with wrong tags and row order (F-01). All BE UT and FE UT expected strings were recomputed and are correct. | | Observability (logs, VLOG, metrics) | n/a for a scalar function. | | Transaction / persistence / EditLog replay | n/a. | | Data write atomicity, crash safety | n/a. | | New FE↔BE variables on every send path | n/a: no new variables. Return types are compatible: FE `VARCHAR(65533)` and BE `String` compare equal by `typeid` ([`be/src/core/data_type/data_type_string.cpp:58-60`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/core/data_type/data_type_string.cpp#L58-L60)). Nullability agrees (AlwaysNullable vs `make_nullable`). | | Performance and anti-patterns | No issue. One pass per block, constant divisors, `reserve`; at most 61 bytes per row. | | Anything else | The PR body describes code that is not there (a `FloatLiteral` overload, `(int/bigint)`) and overstates Trino parity (F-04, F-06). | </details> <details> <summary><b>Considered and dismissed (23 items, with evidence)</b></summary> | # | Concern | Where | Why it is not an issue | |---|---|---|---| | D-01 | `ColumnConst(ColumnNullable)` reaches `assert_cast<const ColumnNullable*>` | [`be/src/exprs/function/function_human_readable_seconds.cpp:74-75`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_human_readable_seconds.cpp#L74-L75) | `use_default_implementation_for_constants()` is true ([`be/src/exprs/function/function.h:132`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function.h#L132)) and the function has one argument, so an all-const call is unwrapped to a 1-row column before `execute_impl` ([`be/src/exprs/function/function.cpp:153-155`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function.cpp#L153-L155)). The missing comment is part of F-05. | | D-02 | FE return type `VARCHAR(65533)` vs BE `String` fails the BE return-type check | [`be/src/exprs/function/function.h:338-354`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function.h#L338-L354) | `DataTypeString::equals` compares `typeid` only ([`be/src/core/data_type/data_type_string.cpp:58-60`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/core/data_type/data_type_string.cpp#L58-L60)). | | D-03 | `return ptr - buf;` (signed to unsigned) breaks the `-Werror` build | [`be/src/exprs/function/function_human_readable_seconds.cpp:165`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_human_readable_seconds.cpp#L165) | The BE builds with `-Wconversion -Wno-sign-conversion` ([`be/CMakeLists.txt:459-463`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/CMakeLists.txt#L459-L463)). | | D-04 | The new `.cpp` is not compiled | [`be/src/exprs/CMakeLists.txt:24`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/CMakeLists.txt#L24) | `file(GLOB_RECURSE ... *.cpp)` picks it up. | | D-05 | FE UT assertions fail on literal type/class differences | [`fe/fe-core/src/test/java/org/apache/doris/nereids/rules/expression/ExpressionRewriteTestHelper.java:113`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/expression/ExpressionRewriteTestHelper.java#L113) | The helper compares `toSql()` strings; `'0 seconds'` and `NULL` match. | | D-06 | `SECONDS_PER_*` locals collide with a macro in a unity batch | [`be/src/exprs/function/function_human_readable_seconds.cpp:125-129`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_human_readable_seconds.cpp#L125-L129) | No `#define SECONDS_PER_*` / `DAYS_PER_*` anywhere in `be/src` or `be/test`; the other `SECONDS_PER_DAY` names are class- or function-scoped constants. | | D-07 | Existing thread: literal NULL does not fold | [`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/HumanReadableSeconds.java:40`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/HumanReadableSeconds.java#L40) | Fixed at head: `PropagateNullLiteral` is implemented. Not re-raised. | | D-08 | Existing thread: output format differs from Trino | [`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java:2119-2161`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java#L2119-L2161) | Fixed at head: weeks, long unit names, `", "`, DOUBLE signature; byte-identical to Trino 435 for finite \|x\| < 2^63. The remaining deviations are declared and appear only as wording (F-06). | | D-09 | Existing thread: `to_chars` end pointer past the buffer | [`be/src/exprs/function/function_human_readable_seconds.cpp:117`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_human_readable_seconds.cpp#L117) | Fixed at head: `buf_end` is passed, and `buf[128]` exceeds the 61-byte maximum output. Not re-raised. | | D-10 | `cast_set<UInt32>` per row can throw | [`be/src/exprs/function/function_human_readable_seconds.cpp:212`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_human_readable_seconds.cpp#L212) | It throws only past UINT32_MAX chars per block; output is at most 61 bytes per row. | | D-11 | Performance anti-pattern in the kernel | [`be/src/exprs/function/function_human_readable_seconds.cpp:180-213`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_human_readable_seconds.cpp#L180-L213) | One pass, constant divisors, reserved chars, at most 61-byte memcpy per row; same cost class as `ipv4_num_to_string`. | | D-12 | glibc 2.17 baseline | [`be/src/exprs/function/function_human_readable_seconds.cpp:205`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_human_readable_seconds.cpp#L205) | The only new libm symbol is `llround`, which exists in glibc 2.17 (it was called inside `centos:7`); integral `std::to_chars` is header-only. | | D-13 | Build hygiene / hub-header include | [`be/src/exprs/function/simple_function_factory.h:126`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/simple_function_factory.h#L126) | One declaration and one call, no new `#include` in a header, no gate-table change. The UT includes no src `.cpp`. | | D-14 | Reuse First: an existing template should host the kernel | [`be/src/exprs/function/function_totype.h:42`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/function_totype.h#L42) | `FunctionUnaryToType` handles int/bool to non-nullable string only; a direct `IFunction` is the norm for DOUBLE to Nullable(String) (`format_number`, `ipv4_num_to_string`). Only the null-handling choice deviates (F-05). | | D-15 | `Math.round` vs `llround` on ties | [`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java:2076`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java#L2076) | For x >= 0, round-half-up equals round-half-away-from-zero. JDK 17 and glibc 2.17 agree on 0.5, 1.5, 2.5, 59.5, 2^52-0.5 and 0.49999999999999994. | | D-16 | FLOAT arguments fold differently on FE and BE | [`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/FloatLiteral.java:68-69`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/FloatLiteral.java#L68-L69) | Pre-existing generic cast, not in the diff. FE widens a FLOAT literal through its shortest decimal string, the BE widens exactly ([`be/src/exprs/function/cast/cast_to_float.h:28-39`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/cast/cast_to_float.h#L28-L39)), e.g. `CAST(8589934592 AS FLOAT)`. This affects every DOUBLE-signature function; worth its own issue. | | D-17 | DECIMAL arguments near .5 ties fold differently | [`be/src/exprs/function/cast/cast_to_basic_number_common.h:343-344`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/be/src/exprs/function/cast/cast_to_basic_number_common.h#L343-L344) | Pre-existing generic cast, not in the diff: the BE divides two doubles (two roundings), the FE uses `BigDecimal.doubleValue()`. It needs at least 17 significant digits within one ulp of k+0.5. | | D-18 | `ExplicitlyCastableSignature` lets STRING/BOOLEAN/DATE bind | [`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/HumanReadableSeconds.java:40`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/HumanReadableSeconds.java#L40) | Same trait as `SecToTime`, `MoneyFormat`, `Ipv4NumToString`, `FromUnixtime`: project convention. | | D-19 | BIGINT above 2^53 loses precision through the DOUBLE cast | [`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/HumanReadableSeconds.java:43`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/HumanReadableSeconds.java#L43) | Identical to Trino, which also has a DOUBLE-only signature. | | D-20 | Folded `VarcharLiteral` has its natural length, not `VARCHAR(65533)` | [`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java:2121`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java#L2121) | Established pattern for every string-returning FE fold in this class; values are identical. | | D-21 | NaN/±Infinity and \|x\| beyond int64 return NULL instead of Trino's error/clamp | [`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java:2069-2075`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/executable/DateTimeExtractAndTransform.java#L2069-L2075) | Declared in the PR body as intended Doris semantics; only the "1:1" wording is a finding (F-06). | | D-22 | A Nullable(Nothing) argument reaches the kernel | [`fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:548-551`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java#L548-L551) | The FE turns a NULL argument into `NullLiteral(DOUBLE)`, and `PropagateNullLiteral` folds the call to NULL first. | | D-23 | A function-list snapshot or legacy registry needs updating | [`fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/ExpressionTranslator.java:747-750`](https://github.com/apache/doris/blob/1f90c1be31a4d7ef0df6b08b90a93e0b72075e15/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/ExpressionTranslator.java#L747-L750) | Nereids builds the catalog function directly; no gensrc entry is needed, and no SHOW FUNCTIONS snapshot lists builtins. | </details> <details> <summary><b>Coverage and limits</b></summary> - Read in depth: all 10 changed files, plus the code they depend on: - the regression framework: `Suite.groovy`, `OutputUtils.groovy`, `DataUtils.groovy`, `SuiteContext.groovy` - the BE function framework: `function.h`, `function.cpp` - Nereids folding and binding: `FoldConstantRule*`, `ExpressionEvaluator`, `SearchSignature`, `TypeCoercionUtils` - the FE and BE cast code - sibling functions: `ipv4_num_to_string`, `format_number`, `sec_to_time` - Not verified: nothing was built and no test was run. F-01 follows from the framework code. F-02 was reproduced by running the PR's own code outside the repository: - glibc 2.17 x86-64, in a `centos:7` linux/amd64 container - glibc 2.39 aarch64 - JDK 17.0.19 - Evidence from outside the repository: - trino-main 435 sources, `io/trino/operator/scalar/timestamp/HumanReadableSeconds.java` - apache/doris-website#4174, the linked user doc - Review process: two rounds, converged. The verdict has not changed since round 1. - The three earlier github-actions threads are fixed at this head and are not repeated here (D-07, D-08, D-09). </details> <sub>Reviewed locally with the doris-repo-review workflow against head `1f90c1b`. This is review feedback, not a merge approval.</sub> <!-- doris-repo-review:report:end --> -- 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]
