Skip to content

[fix](nereids) Accept foldable constant level in percentile_reservoir - #68488

Open
mrhhsg wants to merge 9 commits into
apache:masterfrom
mrhhsg:fix/percentile-reservoir-constant-level
Open

mrhhsg wants to merge 9 commits into
apache:masterfrom
mrhhsg:fix/percentile-reservoir-constant-level

Conversation

@mrhhsg

@mrhhsg mrhhsg commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: None

Problem Summary:

percentile_reservoir validated its level argument only in
checkLegalityBeforeTypeCoercion, which runs during analysis before
constant folding. Besides requiring the argument to be constant, it also
required it to already be a Literal, so any constant expression that
only becomes a literal after folding was rejected:

SELECT percentile_reservoir(number, 0.25 + 0.25) FROM numbers('number' = '10');
-- ERROR: percentile_reservoir requires second parameter must be a constant

The equivalent literal 0.5 works, and sibling functions such as
percentile_approx accept the same foldable expression, so the
restriction was inconsistent and unnecessary.

Both checkLegalityBeforeTypeCoercion and checkLegalityAfterRewrite
now share one check that folds the level itself with
FoldConstantRuleOnFE.evaluateWithoutContext (as stack already does
for its row count), casts the folded literal to DOUBLE and range checks
it. Folding inside the check instead of relying on the rewrite phase
matters because:

  • a constant expression such as 0.25 + 0.25 is only a literal after
    folding,
  • INSERT ... VALUES and load column mappings never run the rewrite
    phase, so an analysis-time check is the only one on those paths,
  • debug_skip_fold_constant turns off the regular constant folding, and
    a plain literal 0.5 would otherwise stay an unfolded cast.

Foldable constants such as 0.25 + 0.25, cast('0.5' as double) or
1 - 0.75 are now accepted for the plain aggregate, the window form,
the _state combinator and INSERT ... VALUES. A constant outside
[0, 1], a non-constant argument and a constant that FE cannot fold
(for example pow(0.5, 1)) are still rejected with the same error
messages as before; cast('NaN' as double) is now rejected as out of
range instead of as a non-constant.

The check validates a FE-folded copy of the level, while BE used to get
the original expression and evaluate it itself wherever it was not folded:
load planning always skips constant folding, and DISTINCT or
debug_skip_fold_constant can keep the expression too. The validated and
the executed level could then differ, for example cast(0.1 as float)
folds to 0.1 on FE but BE widens it to 0.10000000149011612, so a stream
load mapping s = percentile_reservoir_state(v, cast(0.1 as float)) wrote
a state that failed with "incompatible quantiles" when merged with a state
built by a query. percentile_reservoir (and its _state / _combine
combinators) therefore replaces the level with the validated literal once
it is analyzed (RewriteWhenAnalyze), so BE always executes the checked
value. Along the way FE folding is aligned with BE where a level could
observe the difference: a string cast to FLOAT/DOUBLE follows the BE
grammar (NaN payloads, ASCII whitespace only), '-nan' folds to a negative
NaN as fast_float does, a string is cast to FLOAT through a double, a
DOUBLE is narrowed to FLOAT with a single rounding, FLOAT/DOUBLE casts keep
the sign of a NaN, and If / CASE branches that hold a NaN literal are no
longer merged as equal (Literal.equals takes NaNs of both signs as
equal, but signbit() tells them apart).

An explicit cast of a percentile_reservoir state to another agg_state
layout, e.g. CAST(percentile_reservoir_state(v, 0.25) AS AGG_STATE<percentile_reservoir(DOUBLE NOT NULL, DOUBLE NULL)>), including
chained casts, makes ConvertAggStateCast wrap the level in Nullable /
NonNullable; the post-rewrite check validates the level beneath those
wrappers, so such casts keep working as before this PR.

Compatibility note: a high-precision DECIMAL level (more than about 15
significant digits, e.g. 0.12345678901234567) is converted to DOUBLE by
FE (BigDecimal.doubleValue()), as queries and INSERT ... VALUES already
did before this PR, while load planning used to let BE compute it
((double)unscaled / (double)10^scale), which can differ in the last bit.
Load-written and query-written states with such a level could therefore
not be merged even before this PR; now every writer stores the FE value,
so a state loaded before the upgrade with such a level cannot be merged
with one written after it. Making BE's DECIMAL to DOUBLE cast correctly
rounded is left to a separate PR.

Release note

percentile_reservoir now accepts any constant expression that folds to a level in [0, 1] (for example 0.25 + 0.25 or cast('0.5' as double)), including in INSERT ... VALUES and with debug_skip_fold_constant = true. A string level is converted to DOUBLE with the session cast mode before the range check: '0.5' is accepted, '5' is rejected as out of range, and a non-numeric string such as '' yields a NULL level under the default non-strict cast (an error under enable_strict_cast = true), the same as cast('' as double). FE constant folding of a DECIMAL division now matches BE: a DECIMALV2 quotient is NULL only for a zero divisor, keeps scale 9 and rounds like BE, and a DECIMALV3 quotient is truncated at the result scale, so constants such as cast(1 as decimalv2(27, 9)) / cast(3 as decimalv2(27, 9)) or 2.0 / 3 fold on FE and are accepted as levels. A constant level is executed as the literal FE validated on every path, including load column mappings, DISTINCT and debug_skip_fold_constant. FE constant folding of string to FLOAT/DOUBLE casts and of DOUBLE to FLOAT casts now produces the same bits as BE (signed NaN, midpoint rounding), and If / CASE branches that hold a NaN literal are no longer merged by FE constant folding.

Check List (For Author)

  • Test:
    • Unit Test: PercentileReservoirParameterTest updated to cover
      literal, foldable, unfoldable, non-constant, string, NULL and
      DECIMALV2/DECIMALV3 quotient levels through both check phases;
      FoldConstantTest pins DECIMALV2/DECIMALV3 division folds against
      BE-computed values.
    • Regression test: new test_percentile_reservoir_constant_level
      (query_p0/sql_functions/aggregate_functions) including the
      INSERT ... VALUES path and debug_skip_fold_constant = true;
      existing test_aggregate_all_functions2, agg_distinct_function,
      test_agg_state_parameters, test_agg_state_nullable_rewrite,
      datatype_p0/decimalv2, fold_constant_numeric_arithmatic and
      test_int128_unaligned_access re-run locally. Later follow-ups add
      fold-on / skip-fold / DISTINCT / window / state-merge / stream load
      cases, StringLikeLiteralTest, DoubleLiteralTest and
      FoldConstantTest raw-bit and NaN-branch cases, and re-run
      function_p0/cast/to_float, datatype_p0/agg_state,
      mv_p0/agg_state, query_p0/expression/fold_constant,
      nereids_rules_p0/fold_constant and nereids_rules_p0/case_when_rules;
      nullable / chained agg_state cast cases in PercentileReservoirParameterTest
      and the regression test.
  • Behavior changed: Yes. Constant expressions that fold to a valid level
    are accepted instead of raising an analysis error. A string literal
    level is cast to DOUBLE before the range check ('0.5' is accepted as
    0.5, '5' is rejected as out of range) instead of being compared as a
    meaningless hash value. A non-numeric string level ('', 'abc')
    follows the session cast mode exactly like the explicit
    cast('' as double): NULL level under the default non-strict cast, a
    cast error under enable_strict_cast = true. A constant level whose
    type cannot be cast to DOUBLE at all (for example an ARRAY literal)
    now fails with the generic "can not cast from origin type ... to
    target type=DOUBLE" error instead of a percentile_reservoir-scoped
    message; the statement failed before as well. FE folding of a
    DECIMALV2 constant division (NumericArithmetic.divideDecimal) now
    returns NULL only for a zero divisor instead of a zero dividend, so
    cast(0 as decimalv2(27, 9)) / cast(2 as decimalv2(27, 9)) is 0 as
    on BE. FE folding of a nonzero DECIMAL quotient now uses BE's scale and
    rounding instead of an exact BigDecimal.divide, which threw for a
    recurring quotient (1 / 3) and produced a value too precise for the
    literal (1 / 1024), leaving the division unfolded: DECIMALV2 keeps
    scale 9 with the analyzed DECIMALV2(27, 9) type and rounds up once the
    remainder reaches divisor >> 1 (DecimalV2Value::operator/), DECIMALV3
    truncates toward zero at the analyzed result scale (DivideDecimalImpl).
    A folded DECIMALV2 quotient therefore prints with scale 9 like the
    BE-computed value. A constant level is replaced by the validated
    literal during analysis, so EXPLAIN shows the literal and a level such as
    cast(0.1 as float) is 0.1 on every path instead of
    0.10000000149011612 where BE evaluated it. FE folding of a string to
    FLOAT/DOUBLE follows the BE grammar and bits ('nan(foo)' is NaN,
    '-nan' is a negative NaN, only ASCII whitespace is skipped, FLOAT is
    parsed through a double), a DOUBLE is narrowed to FLOAT with a single
    rounding, and If / CASE branches holding a NaN literal are not merged.
  • Does this need documentation: No

### What problem does this PR solve?

Issue Number: None

Problem Summary:

`percentile_reservoir` validated its level argument only in
`checkLegalityBeforeTypeCoercion`, which runs during analysis before
constant folding. Besides requiring the argument to be constant, it also
required it to already be a `Literal`, so any constant expression that
only becomes a literal after folding was rejected:

```sql
SELECT percentile_reservoir(number, 0.25 + 0.25) FROM numbers('number' = '10');
-- ERROR: percentile_reservoir requires second parameter must be a constant
```

The equivalent literal `0.5` works, and sibling functions such as
`percentile_approx` accept the same foldable expression, so the
restriction was inconsistent and unnecessary.

Both `checkLegalityBeforeTypeCoercion` and `checkLegalityAfterRewrite`
now share one check that folds the level itself with
`FoldConstantRuleOnFE.evaluateWithoutContext` (as `stack` already does
for its row count), casts the folded literal to DOUBLE and range checks
it. Folding inside the check instead of relying on the rewrite phase
matters because:

- a constant expression such as `0.25 + 0.25` is only a literal after
  folding,
- `INSERT ... VALUES` and load column mappings never run the rewrite
  phase, so an analysis-time check is the only one on those paths,
- `debug_skip_fold_constant` turns off the regular constant folding, and
  a plain literal `0.5` would otherwise stay an unfolded cast.

Foldable constants such as `0.25 + 0.25`, `cast('0.5' as double)` or
`1 - 0.75` are now accepted for the plain aggregate, the window form,
the `_state` combinator and `INSERT ... VALUES`. A constant outside
`[0, 1]`, a non-constant argument and a constant that FE cannot fold
(for example `pow(0.5, 1)`) are still rejected with the same error
messages as before; `cast('NaN' as double)` is now rejected as out of
range instead of as a non-constant.

### Release note

None

### Check List (For Author)

- Test:
    - Unit Test: `PercentileReservoirParameterTest` updated to cover
      literal, foldable, unfoldable, non-constant, string and NULL levels
      through both check phases.
    - Regression test: new `test_percentile_reservoir_constant_level`
      (query_p0/sql_functions/aggregate_functions) including the
      `INSERT ... VALUES` path and `debug_skip_fold_constant = true`;
      existing `test_aggregate_all_functions2`, `agg_distinct_function`,
      `test_agg_state_parameters` and `test_agg_state_nullable_rewrite`
      re-run locally.
- Behavior changed: Yes. Constant expressions that fold to a valid level
  are accepted instead of raising an analysis error. A string literal
  level is cast to DOUBLE before the range check (`'0.5'` is accepted as
  0.5, `'5'` is rejected as out of range, a non-numeric string literal
  is rejected as a cast error) instead of being compared as a
  meaningless hash value. An explicit `cast('abc' as double)` level
  folds to NULL under the default non-strict cast and is then treated
  like a plain NULL level, which was already accepted.
- Does this need documentation: No
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@mrhhsg

mrhhsg commented Sep 24, 2026

Copy link
Copy Markdown
Member Author

/review-light

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review result: request changes (2 inline correctness findings).

Critical checkpoints:

  • Goal and proof: the localized change does accept ordinary FE-foldable DOUBLE/DECIMALV3 levels across direct, window, state, VALUES, and debug-skip shapes, and the added expected results are internally consistent. It does not yet establish correct behavior for all newly accepted constants: M1 changes default non-strict implicit-cast semantics, and M2 can validate/execute different DECIMALV2 values and return a wrong result.
  • Scope and clarity: the production edit is small and reuses the existing context-free folder, but that reuse imports the folder's cast/evaluator semantics into a pre-coercion legality hook; the two inline cases must be resolved before the goal is safely met.
  • Concurrency and lifecycle: no threads, locks, shared mutable state, special lifetime, or static-initialization dependency is introduced.
  • Configuration and parallel paths: no new configuration is added. Direct/window, State/Combine, INSERT VALUES, load mapping, final-rewrite, and debug_skip_fold_constant paths were traced; hook coverage and current-child reconstruction are sound apart from M1/M2.
  • Compatibility: there is no FE/BE field, function symbol, aggregate-state layout, serialization, or storage-format change. M1 is nevertheless a user-visible backward behavior regression, and M2 exposes an FE/BE semantic mismatch for a newly accepted expression.
  • Conditions, errors, and NULLs: the constant/literal/NULL/range structure correctly rejects NaN, infinities, and finite out-of-range DOUBLE values, and BE nullable/state handling is safe. The direct literal cast bypasses session-aware non-strict behavior, as described in M1.
  • Tests and results: the JUnit and regression additions cover the main positive/negative paths, use deterministic ordering, and have matching result blocks. They miss the raw/explicit strict-cast matrix for M1 and DECIMALV2 0 / 2 plus 1 / 0 under both fold modes for M2.
  • Persistence, transactions, data writes, observability, and FE/BE variable propagation: no applicable production change was introduced.
  • Performance: added work is analysis-time constant folding only; no runtime hot path is changed.
  • User focus: -light supplied no additional technical focus; the full PR was reviewed.

Validation was static only, as required by this review runner; no build or test was executed. Author-reported local test runs were not independently verified.

Non-code note: the PR declares Behavior changed: Yes while the release note is None; please align that with the repository release-note rule.

…DECIMALV2 divide fold

### What problem does this PR solve?

Issue Number: None

Problem Summary:

Two follow-ups to the shared `checkLevel()` of `percentile_reservoir`:

1. The level was brought to DOUBLE with a direct `Literal.checkedCastTo`,
   which throws for a string that is not a number regardless of
   `enable_strict_cast`. `percentile_reservoir(number, '')` therefore
   failed with "can't cast to double in strict mode" under the default
   non-strict cast, while the equivalent `cast('' as double)` folded to
   NULL and returned a NULL aggregate. The check now wraps the level in
   the same implicit cast that signature coercion applies
   (`TypeCoercionUtils.castIfNotSameType`) and folds it with
   `FoldConstantRuleOnFE.evaluateWithoutContext`, so both forms agree:
   NULL under non-strict cast, a cast error under strict cast.

2. `NumericArithmetic.divideDecimal` (DECIMALV2) returned NULL when the
   dividend was zero instead of the divisor, so FE folded `0 / 2` to
   NULL while BE computed 0, and `1 / 0` was left unfolded. With the
   level check relying on FE folding this made
   `percentile_reservoir(number, cast(0 as decimalv2(27, 9)) / cast(2 as decimalv2(27, 9)))`
   return NULL normally but 0 with `debug_skip_fold_constant = true`.
   The guard now checks the divisor, matching `divideDecimalV3` and BE.

### Release note

None

### Check List (For Author)

- Test:
    - Unit Test: `PercentileReservoirParameterTest` covers '' / 'abc' /
      `cast('' as double)` levels under both cast modes and DECIMALV2
      `0 / 2`, `3 / 2`, `1 / 0` levels; `FoldConstantTest` covers the
      DECIMALV2 zero-divisor fold.
    - Regression test: `test_percentile_reservoir_constant_level` covers
      both string forms under both cast modes and the DECIMALV2 divisions
      with and without `debug_skip_fold_constant`.
- Behavior changed: Yes. A non-numeric string level follows the session
  cast mode again; DECIMALV2 constant division folds to NULL only for a
  zero divisor. A constant level whose type cannot be cast to DOUBLE at
  all (for example an ARRAY literal, or a DATE literal under strict cast)
  now fails with the generic "can not cast from origin type ... to
  target type=DOUBLE" error instead of a percentile_reservoir-scoped
  message; the statement failed before as well.
- Does this need documentation: No
@mrhhsg
mrhhsg requested a review from yujun777 as a code owner September 24, 2026 14:25
@mrhhsg

mrhhsg commented Sep 24, 2026

Copy link
Copy Markdown
Member Author

/review-light

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review of exact head 4f79258de0f5c946034a88e299177a8b0d9a107e. I found one distinct P2 correctness issue; the two older inline findings are fixed at this head.

Checkpoint conclusions:

  • Goal: foldable percentile levels now cover the intended literal/string/NULL/zero-divisor cases, but valid nonzero decimal quotient constants can still be rejected.
  • Scope: the production and test changes are focused; the shared decimal evaluator is a necessary dependency and contains the remaining mismatch.
  • Concurrency and lifecycle: no threads, locks, initialization-order, ownership, or cleanup behavior changes.
  • Configuration: no new configuration; strict-cast, normal folding, skipped folding, and explicit DECIMALV2 routing were traced.
  • Compatibility: no protocol, persisted-state, journal, or storage-format change.
  • Parallel paths: plain/grouped/window, INSERT/load, partial/final, and state/combine/merge paths were traced; NULL state behavior is safe, while the accepted issue reaches their common legality check.
  • Special conditions and error handling: NULL, invalid strings, strict/non-strict casts, range endpoints, NaN/infinity, and zero dividend/divisor behavior were checked; only nonzero decimal rounding/foldability remains incorrect.
  • Tests: the added Java and regression cases are deterministic and follow the harness conventions, but they omit recurring and excess-scale quotients such as 1/3 and 1/1024.
  • Observability: no new runtime operational surface.
  • Persistence and data writes: no production persistence mutation; INSERT coverage was reviewed as a planning path.
  • FE/BE contracts: no transmitted variable or schema change; the finding is an FE-fold versus BE arithmetic-semantic mismatch.
  • Performance: analysis-time folding is bounded and no material performance regression was found.
  • User focus: -light supplied no additional domain focus; the full changed-file and call-chain review was still completed.

Validation was static only as required; I did not run builds or tests, so author/CI test claims were not independently verified.

…nding BE uses

### What problem does this PR solve?

Issue Number: None

Problem Summary:

`percentile_reservoir(number, cast(1 as decimalv2(27, 9)) / cast(3 as decimalv2(27, 9)))`
and `percentile_reservoir(number, 2.0 / 3)` were still rejected with
"requires second parameter must be a constant" although the level is a
valid constant. The FE fold helpers `NumericArithmetic.divideDecimal`
(DECIMALV2) and `divideDecimalV3` used the exact `BigDecimal.divide`:
a recurring quotient such as `1 / 3` throws, and `1 / 1024` yields a
scale-10 value that the scale-9 literal rejects, so `ExpressionEvaluator`
handed back the unfolded `Divide` and the level check failed. BE computes
these quotients at the result scale: DECIMALV2 keeps scale 9 and rounds
up once the remainder reaches `divisor >> 1` (`DecimalV2Value::operator/`),
DECIMALV3 divides the coercion-scaled integers and truncates toward zero
(`DivideDecimalImpl`).

Both helpers now compute the quotient the way BE does and build the
literal with the type `Divide` analyzes for the operands, so every
DECIMAL constant division folds on FE to the value BE would execute.
The DECIMALV2 result therefore carries DECIMALV2(27, 9) instead of a type
inferred from the digits, which is also why a folded `0 / 2` now prints
`0.000000000` like the BE-computed value does.

### Release note

None

### Check List (For Author)

- Test: Unit Test / Regression test
    - `FoldConstantTest`: DECIMALV2 `1 / 3`, `1 / 1024`, `-2 / 3`,
      `1 / 0.000000003`, `1 / 0.000000001`, DECIMALV3 `2.00000 / 3`,
      `-2.00000 / 3`, `1.00000 / 1024`, `1.00000 / 0`
    - `PercentileReservoirParameterTest`: DECIMALV2 and DECIMALV3 quotient
      levels accepted, `4 / 3` rejected with the rounded value
    - `test_percentile_reservoir_constant_level`: the raw quotients and
      the levels with and without `debug_skip_fold_constant = true`
- Behavior changed: Yes, DECIMAL constant divisions that FE could not fold
  before now fold to the BE value
- Does this need documentation: No
@mrhhsg

mrhhsg commented Sep 24, 2026

Copy link
Copy Markdown
Member Author

/review-light

@github-actions

Copy link
Copy Markdown
Contributor

Codex automated review failed and did not complete.

Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Sep 30th, 2026 2:10 AM.
Workflow run: https://github.com/apache/doris/actions/runs/36028156872

The selected account is excluded until 2026-09-30T02:10:00Z. Please trigger /review again; another configured account may be available.

@mrhhsg

mrhhsg commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

/review-light

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for one P2 correctness and compatibility issue.

The change is focused, and static inspection found that the new DECIMALV2/DECIMALV3 quotient folding matches the analyzed types and BE rounding/truncation rules. The legality hooks and tests cover plain, grouped, window, state/combine, INSERT, strict-cast, NULL, range-error, and normal/skip-fold paths. However, the context-free legality fold can validate a different DOUBLE from the value a retained high-scale DECIMAL expression executes in BE; persisted percentile states produced under the two folding modes can then fail to merge. The inline comment has the exact boundary case and fix direction.

Checkpoint conclusions: the blocker is an FE/BE conversion-parity issue affecting error handling, the skip-fold parallel path, and persisted aggregate-state compatibility. The six-file scope is otherwise minimal; test outputs are deterministic and broad but miss this scale-17 boundary and cross-mode state merge. I found no additional concurrency, lifecycle, transaction, configuration-definition, protocol, storage-format, observability, or material-performance issue. The three existing inline topics are fixed at this head and were duplicate-fenced. Validation was static only as required; no build or tests were run.

@mrhhsg

mrhhsg commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

/review-light

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review completed at exact head 67b27f27d72bf2afe8f58bd8b1d058752873f87d.

Requesting changes for one P2 correctness issue: the temporary string-to-DOUBLE validation can approve NULL while the retained child executes as NaN on skip-fold and DISTINCT paths, violating the level-range and aggregate-state merge contract. One inline comment is attached.

Checkpoint conclusions:

  • Goal and tests: foldable constant support works across the reviewed plain, window, state/combine, and INSERT planning paths, but the parser-parity gap prevents full correctness. The broad unit/regression coverage misses NaN payload and string-grammar parity.
  • Scope and performance: the six-file change is focused; the shared decimal folding fix is necessary and now matches the reviewed BE arithmetic. Added work is analysis-time only.
  • Concurrency, lifecycle, and state: no new threads, locks, shared-state lifecycle, or cleanup behavior. The accepted issue does affect nonempty partial-state merging.
  • Configuration, compatibility, and persistence: no new configuration, protocol, function symbol, storage format, FE/BE variable, transaction, or persistence change. Existing strict-cast, debug-skip, DecimalV2, and DecimalV3 paths were reviewed.
  • Parallel and conditional paths: plain/window/state/combine/DISTINCT/INSERT, strict/non-strict, and FE/BE fold paths were traced. The accepted issue breaks the assumption that the range check examines the same value BE executes.
  • Error handling and observability: NULL, invalid strings, NaN/infinity, endpoints, zero divisors, and decimal rounding were inspected; no separate observability issue was found.
  • Validation: static inspection only; no builds or tests were run in this runner, so author/CI test claims were not independently verified.

Existing review threads were treated as hard duplicate fences and were not reposted. The acknowledged decimal-to-double mismatch remains covered by its existing exact-head thread. Focus -light introduced no additional domain-specific checkpoint beyond the full review.

…parses

### What problem does this PR solve?

Issue Number: None

Related PR: apache#68488

Problem Summary: FE and BE disagreed on which strings are valid FLOAT/DOUBLE values.
BE (`StringParser::string_to_float`, fast_float) skips only ASCII whitespace
(space, \t, \n, \v, \f, \r) and also accepts a NaN payload such as `nan(foo)`.
FE `StringLikeLiteral.castToFloat/castToDouble` trimmed with `String.trim()`,
which also drops other control characters, and its regex rejected the NaN
payload form. So `cast('nan(foo)' as double)` folded to NULL on FE but returned
NaN on BE, and a literal such as '0.25' followed by a \x01 byte folded to 0.25
on FE but was NULL on BE.

For `percentile_reservoir` the level check folds the level through the same
cast, so `percentile_reservoir(v, 'nan(foo)')` was accepted as a NULL level,
while with `debug_skip_fold_constant = true`, or for a DISTINCT aggregate whose
expression is not folded, BE executed the level as NaN, bypassing the [0, 1]
check and breaking the level comparison when merging states.

The FE regex now captures the number without the surrounding ASCII whitespace
and accepts `nan(n-char-seq)`, so the FE fold matches BE and such a level is
rejected as NaN in both folding modes.

### Release note

Constant folding of string to FLOAT/DOUBLE casts now accepts `nan(...)` and no
longer ignores non-whitespace control characters, matching BE execution.

### Check List (For Author)

- Test: Unit Test / Regression test
    - StringLikeLiteralTest, PercentileReservoirParameterTest, FoldConstantTest,
      CheckCastTest, SimplifyComparisonPredicateTest, FloatLiteralTest, DoubleLiteralTest
    - test_percentile_reservoir_constant_level (plain / DISTINCT / _state, with and
      without debug_skip_fold_constant), test_float_special_values,
      test_double_special_values, fold_constant_string_arithmatic,
      fold_constant_numeric_arithmatic
- Behavior changed: Yes. FE folding of string to FLOAT/DOUBLE casts now matches BE for NaN payloads and control characters.
- Does this need documentation: No
@mrhhsg

mrhhsg commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

/review-light

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes: two P2 FE/BE parity defects let the newly accepted constant level pass analysis with one value and execute with the opposite value.

Critical checkpoints:

  • Correctness and parallel paths: traced pre/post-coercion legality, ordinary and DISTINCT aggregation, window/state/combine, skip-fold, INSERT VALUES, BE scalar/batch/window execution, and serialized-state merge. The two inline findings are the surviving issues; existing discussions fence the previously reported zero-divisor, recurring-decimal, non-strict-cast, DECIMAL-to-DOUBLE, and raw NaN-payload cases.
  • Concurrency/lifecycle/configuration/protocol: no new locking, lifecycle, transaction, configuration, FE/BE wire-format, or rolling-upgrade mechanism is introduced. Persisted aggregate state is affected only because the two defects can serialize incompatible level values.
  • Tests and oracles: the changed tests and expected output are otherwise internally consistent and deterministic, but they do not assert signed-NaN raw bits or midpoint-adjacent FLOAT behavior across folded and retained execution.
  • Performance: the added work is bounded to analysis-time folding of one constant expression; no material hot-path regression was found.
  • Focus (-light): no additional focus-specific issue was identified; the full changed-path and call-chain review was still completed.

Validation was static-only as required by the review prompt. I did not run builds or tests, so author/CI test claims were not independently verified.

### What problem does this PR solve?

Issue Number: None

Related PR: apache#68488

Problem Summary: A constant percentile_reservoir level is validated on a
FE-folded copy, while DISTINCT aggregation or debug_skip_fold_constant can
leave the original expression for BE to execute. Several casts folded to
different bits on FE than BE computes, so a level derived from those bits was
validated as one value and executed as the other:

- BE (fast_float) negates the quiet NaN for a leading '-', but FE folded
  '-nan' / '-nan(payload)' to the positive NaN. With
  `cast(signbit(cast('-nan(foo)' as double)) as double)` FE got level 0 and
  BE level 1. Widening a FLOAT NaN to DOUBLE and narrowing a DOUBLE NaN to
  FLOAT on FE also dropped the sign.
- BE parses a FLOAT from a string into a double first and then narrows it,
  while FE parsed directly into a float. For
  '1.00000005960464483090177623170427978038787841796875' the double rounds
  to the midpoint between 1 and the next float and ties to 1.0 on BE, but
  FE got the next float, so `cast(x as float) > cast(1 as float)` was true
  on FE and false on BE.
- FE narrowed a DOUBLE to FLOAT through its shortest decimal string, which
  rounds twice: the double 1 + 2^-24 prints as 1.0000000596046448, above the
  midpoint, so FE got the next float while BE static_cast ties to 1.0.

Each made percentile_reservoir return the max with folding and the min
without it (or vice versa), and states built by the two paths carried
incompatible levels. FE now keeps the sign of a NaN, parses FLOAT through a
double, and narrows DOUBLE to FLOAT with a single rounding, so folded and
BE-executed results agree.

### Release note

None

### Check List (For Author)

- Test: Unit Test (StringLikeLiteralTest, DoubleLiteralTest, PercentileReservoirParameterTest, FoldConstantTest, CastTest, TryCastTest, CheckCastTest, FloatLiteralTest, CompareLiteralTest) / Regression test (test_percentile_reservoir_constant_level, function_p0/cast/to_float)
- Behavior changed: Yes. FE folding of string to FLOAT/DOUBLE and DOUBLE to FLOAT casts now matches BE for signed NaN and midpoint rounding.
- Does this need documentation: No
### What problem does this PR solve?

Issue Number: None

Related PR: apache#68488

Problem Summary: percentile_reservoir validates a FE-folded copy of its
constant level, but BE still received the original constant expression.
Wherever that expression is not folded, BE evaluated it itself: load planning
always skips constant folding, and DISTINCT or debug_skip_fold_constant can
keep it too. Some expressions still fold on FE to another value than BE
computes, so the executed level differed from the validated one:

- `cast(0.1 as float)`: FE widens FLOAT to DOUBLE through the shortest decimal
  and folds 0.1, while BE widens exactly to 0.10000000149011612. A stream load
  mapping `s = percentile_reservoir_state(v, cast(0.1 as float))` therefore
  wrote a state with level 0.10000000149011612, which failed with
  "percentile_reservoir aggregate states have incompatible quantiles" when
  merged with a state written by a query.
- `cast(signbit(if(<unfoldable false condition>, cast('-nan' as double),
  cast('nan' as double))) as double)`: FoldConstantRuleOnFE merges If / CASE
  branches that are equal, and Literal.equals takes NaNs of both signs as
  equal, so FE collapsed the If to its true branch (level 1) while BE takes
  the false branch (level 0).

PercentileReservoir now implements RewriteWhenAnalyze and replaces the level
with the literal checkLevel() validated once the function is analyzed, and
StateCombinator / CombineCombinator forward the rewrite to their nested
function so _state and _combine get the same literal. BE then executes exactly
the validated level on every path. FoldConstantRuleOnFE no longer merges If /
CASE branches that hold a FLOAT/DOUBLE NaN literal, also as an element of an
array, map or struct literal, so the If above stays unfolded and the level is
rejected as not constant instead of taking the wrong branch.

### Release note

None

### Check List (For Author)

- Test: Unit Test (PercentileReservoirParameterTest, FoldConstantTest, StateCombinatorTest, CombineCombinatorTest, SimplifyConditionalFunctionTest, CountTest, WindowFunnelParameterTest, DataSketchesHllUnionAggTest, FillUpMissingSlotsTest and the literal/cast tests) / Regression test (test_percentile_reservoir_constant_level incl. stream load, datatype_p0/agg_state, mv_p0/agg_state, nereids_rules_p0/fold_constant, query_p0/expression/fold_constant, nereids_rules_p0/case_when_rules, test_aggregate_all_functions2, agg_distinct_function)
- Behavior changed: Yes. A constant percentile_reservoir level is executed as the literal FE validated, and FE constant folding keeps If / CASE branches holding a NaN literal apart.
- Does this need documentation: No
@mrhhsg

mrhhsg commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

/review-light

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review of exact head 5495a8602c3a0b028500dfde978dde98e37ff1f7; requesting changes for five P2 correctness/compatibility issues.

Checkpoint conclusions:

  • Goal and scope: the change is focused and correctly makes the validated percentile level execute on the principal direct, DISTINCT, window, state/combine, INSERT, and load paths, but the five inline cases leave observable FE/BE differences or reject a previously valid state-layout conversion.
  • Parallel paths and contracts: analyzer rewriting, combinator reconstruction, expression normalization/optimization, Nereids-to-legacy conversion, binary/compact Thrift fragment transport, and BE state serialization/merge were traced. Signed-NaN equality remains unsafe outside the new local branch helper; FLOAT loses its sign before transport and FLOAT/DOUBLE lose it at the wire boundary.
  • Compatibility and lifecycle: the reservoir state persists the raw level and rejects exact mismatches. Rewriting a merge-base-valid high-precision DECIMAL level changes bytes produced by formerly retained/load writers, so old/new states of the same AggState type can fail to merge. The final legality hook also rejects a safe explicit nullable level layout accepted at the merge base.
  • Conditions and errors: NULL, strict/non-strict casts, range endpoints, NaN/infinity, decimal rounding, nullable wrappers, and dynamic signed-NaN observers were checked. No additional transaction, concurrency, configuration, initialization, cleanup, logging, metrics, or material performance issue was found.
  • Tests and results: the changed tests are broad and their checked outputs are internally consistent, but they do not cover the five dynamic, transport, explicit-layout, or pre-upgrade-state cases below.
  • User focus: -light supplied no additional technical narrowing, so the complete changed-path and call-chain review was performed.

The review reached the required three-round cap; all candidates were independently adjudicated, followed by a final changed-file and live-thread audit. Validation was static only as required: no builds or tests were run, so author/CI test claims were not independently verified.

…state cast

### What problem does this PR solve?

Issue Number: None

Related PR: apache#68488

Problem Summary: An explicit cast of a percentile_reservoir state to another
agg_state layout, such as

```sql
SELECT percentile_reservoir_merge(CAST(percentile_reservoir_state(CAST(number AS DOUBLE), 0.25)
    AS AGG_STATE<percentile_reservoir(DOUBLE NOT NULL, DOUBLE NULL)>))
FROM numbers('number' = '10');
```

was accepted before the level check started folding the level in
checkLegalityAfterRewrite, but now failed with "percentile_reservoir requires
second parameter must be a constant". ConvertAggStateCast keeps the requested
state layout by wrapping the validated level literal in Nullable or
NonNullable. When such casts are chained, the same state is converted again
and the wrappers nest, e.g. NonNullable(Nullable(0.25)) for a nullable and
then a NOT NULL level, or Cast(Nullable(Cast(0.25 AS FLOAT)) AS DOUBLE)
through a nullable FLOAT level. FE constant folding leaves Nullable and
NonNullable in place, so the check saw a non-literal level.

checkLegalityAfterRewrite now validates a copy of the level with every
Nullable / NonNullable replaced by its child, while the executed level keeps
the wrappers, so these casts are accepted again and an out-of-range level
beneath them is still rejected. The analysis-time check and the level rewrite
are unchanged, so a user-written nullable(...) level is still rejected as
before.

### Release note

None

### Check List (For Author)

- Test: Unit Test (PercentileReservoirParameterTest, StateCombinatorTest) / Regression test (test_percentile_reservoir_constant_level, datatype_p0/agg_state)
- Behavior changed: No (explicit and chained agg_state casts of a percentile_reservoir state are accepted again)
- Does this need documentation: No
@mrhhsg

mrhhsg commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

/review-light

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review of exact head 6dd4e45 found one distinct P1 state-layout correctness issue (inline below). The new percentile level validation and FE/BE decimal and floating-point parity work were traced through direct, DISTINCT, window, INSERT/load, state/combine, normalization, translation, and merge paths. Existing NaN equality, signed-NaN transport, decimal-to-double, explicit-wrapper, and cast-mode discussions were treated as hard duplicate fences and were not reposted.

Critical checkpoints:

  • Correctness and parallel paths: the validated level is forwarded to the direct/state/combine paths, but the new forwarding can recompute nullable AGG_STATE metadata and change the serialized layout; this is the surviving issue.
  • Persistence/protocol compatibility: no new wire protocol or lifecycle code is introduced, but AGG_STATE subtype nullability is serialized metadata used by state translation and union/merge, so the inline issue can affect old/new state compatibility and results.
  • Concurrency, lifecycle, configuration, and performance: no new locking, lifecycle, configuration, or material hot-path concern was found; analysis-time work is bounded to one constant level.
  • Tests and observability: changed tests cover the validated literal and explicit wrapper cases, but do not exercise the analyzer hook under keepFunctionSignature(false) while asserting nullable state metadata.
  • User focus (-light): no additional focus-specific issue was found beyond the full changed-path review.

Validation was static-only as required by the review prompt. No builds or tests were run, so author/CI test claims were not independently verified.

…oir level

### What problem does this PR solve?

Issue Number: None

Related PR: apache#68488

Problem Summary: percentile_reservoir replaces its constant level with the
validated literal during analysis. The nullability of the level is part of the
agg_state layout of percentile_reservoir_state / percentile_reservoir_combine,
and the analyzer rebuilds those functions without keeping their signatures, so
a nullable level such as CAST('0.25' AS DOUBLE) turned the layout from
percentile_reservoir(DOUBLE NOT NULL, DOUBLE NULL) into
percentile_reservoir(DOUBLE NOT NULL, DOUBLE NOT NULL). Before the level was
rewritten, the state kept the nullable layout, and it no longer matched a
stored state of that layout:

```sql
SELECT percentile_reservoir_merge(s) FROM (
    SELECT s FROM t  -- s AGG_STATE<percentile_reservoir(DOUBLE NOT NULL, DOUBLE NULL)>
    UNION ALL
    SELECT percentile_reservoir_state(CAST(number AS DOUBLE), CAST('0.25' AS DOUBLE))
    FROM numbers('number' = '5')
) states;
-- Can not find assignment compatible type between
-- agg_state<percentile_reservoir(double not null, double null)> and
-- agg_state<percentile_reservoir(double not null, double not null)> in set operation
```

The executed level now wraps the literal in Nullable when the analyzed level
is nullable and the literal is not, so every later rebuild derives the same
layout (and the direct function keeps its nullability).

The level check now looks through Nullable, which never changes the value, and
through NonNullable when its argument folds to a non-NULL literal, in every
phase. So the rewritten level stays accepted when it is analyzed again, e.g.
INSERT OVERWRITE ... VALUES (1, percentile_reservoir_state(v, if(true, 0.25,
NULL))) into a NOT NULL level column analyzes NonNullable(Nullable(0.25))
after the agg_state cast. A NonNullable over a NULL value is kept, as BE
rejects it, so a level such as non_nullable(CAST('' AS DOUBLE)) is still
rejected as "must be a constant".

### Release note

None

### Check List (For Author)

- Test: Unit Test (PercentileReservoirParameterTest, StateCombinatorTest, CombineCombinatorTest) / Regression test (test_percentile_reservoir_constant_level, datatype_p0/agg_state, test_aggregate_all_functions2, agg_distinct_function)
- Behavior changed: No (a nullable constant level keeps the state layout it had before the level was rewritten)
- Does this need documentation: No
@mrhhsg

mrhhsg commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

/review-light

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review completed at exact head c18a01c. I found no new actionable correctness issue after reconciling the current exact-head inline threads; existing findings were treated as hard duplicate fences.

Checkpoint conclusions:

  • Goal and scope: the changed FE folding, percentile_reservoir validation/rewrite, combinator forwarding, literal parsing, and decimal arithmetic are coherent across the reviewed direct, DISTINCT, window, state/combine, INSERT/VALUES, load, and debug-skip paths.
  • Lifecycle and compatibility: no new concurrency, lifecycle, transaction, persistence, protocol, storage-format, or material runtime-performance issue was found. AggState nullability preservation and chained wrapper handling are covered by the current code and tests.
  • FE/BE parity and conditions: DecimalV2 zero/sign/rounding, DecimalV3 truncation, string/NaN grammar, FLOAT/DOUBLE narrowing, range/NULL handling, and branch NaN guards were traced. Previously reported mismatches are resolved or explicitly fenced as pre-existing/protocol-level.
  • Tests: the changed unit and regression coverage is broad and deterministic, including normal and skipped folding plus state/merge paths. I did not run builds or tests; validation here is static only, and author/CI claims were not independently verified.
  • User focus: review focus was -light; it supplied no additional technical narrowing, so the full authoritative changed-file and call-chain review was completed.

No inline comments are submitted because all candidates were duplicates or unsupported after adjudication.

@mrhhsg

mrhhsg commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 27612 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit c18a01cbf373fbbc643bec837eff1c5c20b8fb52, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17596	3775	3760	3760
q2	2168	367	305	305
q3	10110	1490	777	777
q4	4680	471	346	346
q5	7550	826	538	538
q6	179	174	143	143
q7	717	778	594	594
q8	9357	1615	1530	1530
q9	5394	4190	4126	4126
q10	6817	1307	1006	1006
q11	431	268	243	243
q12	624	408	290	290
q13	18074	2613	1964	1964
q14	263	258	240	240
q15	q16	731	715	673	673
q17	1800	1011	965	965
q18	6509	5594	5520	5520
q19	1158	1230	1065	1065
q20	494	397	281	281
q21	5365	3025	2937	2937
q22	456	367	309	309
Total cold run time: 100473 ms
Total hot run time: 27612 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4438	4403	4315	4315
q2	720	584	528	528
q3	4756	5217	4693	4693
q4	2170	2293	1462	1462
q5	4501	4380	4415	4380
q6	218	167	128	128
q7	1899	1736	1490	1490
q8	2283	2035	1981	1981
q9	7315	6959	6856	6856
q10	3607	3543	3076	3076
q11	502	371	338	338
q12	699	701	499	499
q13	2256	2586	1964	1964
q14	265	274	255	255
q15	q16	661	675	602	602
q17	7213	6697	6628	6628
q18	11855	11055	11667	11055
q19	1105	967	980	967
q20	2203	2213	1922	1922
q21	4914	4056	4265	4056
q22	512	455	382	382
Total cold run time: 64092 ms
Total hot run time: 57577 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 152497 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit c18a01cbf373fbbc643bec837eff1c5c20b8fb52, data reload: false

query5	4333	605	467	467
query6	452	214	199	199
query7	4866	534	304	304
query8	328	176	188	176
query9	8831	4020	4017	4017
query10	451	308	257	257
query11	5834	3528	3234	3234
query12	148	105	87	87
query13	1266	615	439	439
query14	6562	4510	4212	4212
query14_1	3926	3924	3891	3891
query15	201	190	184	184
query16	980	470	457	457
query17	1017	671	542	542
query18	2444	468	332	332
query19	208	184	147	147
query20	103	82	83	82
query21	214	134	114	114
query22	12993	12964	12857	12857
query23	14018	13135	12330	12330
query23_1	12544	12678	12561	12561
query24	7305	1084	645	645
query24_1	723	726	757	726
query25	552	440	370	370
query26	1255	306	165	165
query27	2705	597	342	342
query28	4538	2014	2017	2014
query29	1639	732	516	516
query30	301	216	188	188
query31	880	755	632	632
query32	142	102	97	97
query33	521	308	253	253
query34	1213	1113	640	640
query35	725	754	641	641
query36	805	824	747	747
query37	150	110	100	100
query38	1825	1750	1676	1676
query39	693	664	656	656
query39_1	629	612	673	612
query40	258	119	97	97
query41	66	60	62	60
query42	95	93	96	93
query43	333	344	297	297
query44	1363	730	712	712
query45	184	175	159	159
query46	1155	1183	698	698
query47	1494	1484	1413	1413
query48	418	449	282	282
query49	580	410	279	279
query50	914	345	276	276
query51	10605	10276	10228	10228
query52	87	87	74	74
query53	236	257	177	177
query54	256	229	196	196
query55	78	75	69	69
query56	236	215	200	200
query57	1543	1400	1426	1400
query58	282	255	246	246
query59	1976	2034	1857	1857
query60	291	239	225	225
query61	150	142	150	142
query62	400	320	265	265
query63	211	175	172	172
query64	2798	992	813	813
query65	3476	3408	3405	3405
query66	1776	426	311	311
query67	20091	19816	19785	19785
query68	3458	1493	968	968
query69	411	335	259	259
query70	900	827	801	801
query71	288	221	222	221
query72	2583	2444	2205	2205
query73	861	758	433	433
query74	4649	4509	4329	4329
query75	2349	2330	1976	1976
query76	2345	1103	777	777
query77	352	413	297	297
query78	9032	9089	8434	8434
query79	1380	1209	772	772
query80	587	467	370	370
query81	536	330	287	287
query82	607	179	132	132
query83	315	230	199	199
query84	303	150	111	111
query85	830	470	384	384
query86	321	245	232	232
query87	2045	1999	1853	1853
query88	3752	2776	2737	2737
query89	360	301	252	252
query90	1916	189	189	189
query91	172	156	126	126
query92	101	94	84	84
query93	1578	1479	923	923
query94	550	347	302	302
query95	659	362	334	334
query96	1122	761	357	357
query97	2441	2421	2294	2294
query98	158	153	145	145
query99	720	726	619	619
Total cold run time: 236974 ms
Total hot run time: 152497 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 23.94 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit c18a01cbf373fbbc643bec837eff1c5c20b8fb52, data reload: false

query1	0.00	0.00	0.01
query2	0.10	0.05	0.05
query3	0.26	0.13	0.14
query4	1.61	0.14	0.14
query5	0.25	0.22	0.22
query6	1.15	0.94	0.88
query7	0.04	0.01	0.00
query8	0.05	0.04	0.04
query9	0.38	0.33	0.34
query10	0.55	0.54	0.57
query11	0.19	0.15	0.14
query12	0.19	0.15	0.14
query13	0.47	0.48	0.46
query14	0.96	0.98	0.95
query15	0.61	0.60	0.59
query16	0.30	0.32	0.32
query17	1.06	1.05	1.05
query18	0.21	0.22	0.19
query19	1.93	1.89	1.96
query20	0.02	0.01	0.02
query21	15.48	0.21	0.13
query22	4.89	0.06	0.05
query23	16.12	0.31	0.13
query24	3.19	0.46	0.32
query25	0.12	0.05	0.04
query26	0.74	0.21	0.16
query27	0.05	0.04	0.03
query28	3.51	0.82	0.37
query29	12.50	4.19	3.29
query30	0.28	0.16	0.17
query31	2.77	0.55	0.31
query32	3.22	0.59	0.48
query33	3.06	3.15	3.14
query34	15.51	3.95	3.28
query35	3.23	3.21	3.21
query36	0.56	0.43	0.41
query37	0.09	0.07	0.06
query38	0.05	0.04	0.04
query39	0.05	0.03	0.03
query40	0.19	0.15	0.16
query41	0.10	0.03	0.03
query42	0.03	0.02	0.03
query43	0.05	0.03	0.03
Total cold run time: 96.12 s
Total hot run time: 23.94 s

…vision

### What problem does this PR solve?

Issue Number: None

Problem Summary:

TPC-DS query21 filters with `between 2.0/3.0 and 3.0/2.0`. Before this PR
FE could not fold the recurring DECIMAL quotient `2.000000 / 3.0`, so the
shape plans kept `cast((2.000000 / 3.0) as DOUBLE)` and the `if(...)`
around the ratio. Now that FE folds DECIMAL divisions to the value BE
computes, the bound folds to `0.66666` and the filter simplifies to a
direct comparison. Regenerate the ten query21 shape outputs that record
the plan.

### Release note

None

### Check List (For Author)

- Test: Regression test
    - `shape_check` query21 / query21_constraints suites under tpcds_sf100,
      tpcds_sf1000, tpcds_sf1000_nopkfk, tpcds_sf1000_constraints and
      tpcds_sf10t_orc, regenerated with `-forceGenOut` and re-run
- Behavior changed: No
- Does this need documentation: No
@mrhhsg

mrhhsg commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

/review-light

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static review completed at the exact PR head f903c43 (base 91f1ac4). I found no new actionable PR-introduced findings beyond the existing inline threads: those concerns are fixed in the current changes or are pre-existing/duplicate-fenced. The review covered all 24 authoritative bundle paths, aggregate/rewrite and AggState layout lifecycles, decimal folding parity, floating-point/string/NaN conversion boundaries, and the added tests and shape snapshots. Validation was static only; builds and runtime tests were not run under the review contract. This is intentionally a summary-only review with no inline comments.

@mrhhsg

mrhhsg commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 27610 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit f903c43a546989227617895ff8179edf3a816a4b, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17615	3886	3826	3826
q2	2162	366	312	312
q3	10104	1408	824	824
q4	4677	472	346	346
q5	7488	831	539	539
q6	173	165	136	136
q7	728	782	595	595
q8	9375	1530	1494	1494
q9	5448	4176	4158	4158
q10	6818	1334	1026	1026
q11	429	264	259	259
q12	634	404	289	289
q13	18053	2608	1982	1982
q14	253	259	230	230
q15	q16	732	718	653	653
q17	1656	1064	981	981
q18	6413	5587	5496	5496
q19	1181	1266	958	958
q20	482	382	264	264
q21	5391	2927	3015	2927
q22	471	354	315	315
Total cold run time: 100283 ms
Total hot run time: 27610 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4506	4396	4401	4396
q2	728	580	538	538
q3	4789	5292	4545	4545
q4	2205	2300	1470	1470
q5	4584	4492	4435	4435
q6	225	169	129	129
q7	1849	1712	1515	1515
q8	2437	1944	1981	1944
q9	7409	7140	6895	6895
q10	3615	3527	3067	3067
q11	509	368	337	337
q12	705	699	513	513
q13	2270	2585	1996	1996
q14	266	271	244	244
q15	q16	653	689	592	592
q17	7214	6678	6624	6624
q18	11887	11002	11801	11002
q19	1099	979	985	979
q20	2188	2210	1893	1893
q21	4899	4009	4290	4009
q22	504	451	389	389
Total cold run time: 64541 ms
Total hot run time: 57512 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 151889 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit f903c43a546989227617895ff8179edf3a816a4b, data reload: false

query5	4317	592	474	474
query6	437	222	189	189
query7	4863	568	288	288
query8	321	173	164	164
query9	8804	4016	4026	4016
query10	447	305	251	251
query11	5829	3505	3212	3212
query12	138	90	88	88
query13	1286	595	413	413
query14	6467	4432	4218	4218
query14_1	3957	3949	3911	3911
query15	202	198	177	177
query16	987	481	425	425
query17	929	673	544	544
query18	2441	496	313	313
query19	193	171	130	130
query20	83	77	80	77
query21	210	126	113	113
query22	13054	12932	12806	12806
query23	13876	12975	12395	12395
query23_1	12452	12451	12494	12451
query24	7192	1141	642	642
query24_1	654	684	696	684
query25	531	411	335	335
query26	1275	307	158	158
query27	2729	560	322	322
query28	4552	1963	1991	1963
query29	1576	693	483	483
query30	300	215	178	178
query31	885	754	618	618
query32	145	91	84	84
query33	507	313	229	229
query34	1161	1101	639	639
query35	710	743	650	650
query36	803	787	716	716
query37	141	98	92	92
query38	1832	1742	1673	1673
query39	696	674	665	665
query39_1	663	635	664	635
query40	215	116	95	95
query41	64	63	66	63
query42	98	91	92	91
query43	332	339	294	294
query44	1365	712	729	712
query45	177	176	162	162
query46	1057	1174	712	712
query47	1500	1473	1414	1414
query48	420	396	294	294
query49	590	405	303	303
query50	974	353	266	266
query51	10230	10486	10535	10486
query52	95	97	77	77
query53	240	245	180	180
query54	265	203	198	198
query55	82	82	71	71
query56	255	221	206	206
query57	1485	1425	1413	1413
query58	282	258	280	258
query59	1960	2054	1870	1870
query60	271	230	228	228
query61	143	144	139	139
query62	386	314	270	270
query63	210	168	177	168
query64	2778	978	781	781
query65	3452	3399	3437	3399
query66	1780	422	299	299
query67	19957	20044	19808	19808
query68	3233	1520	911	911
query69	407	298	252	252
query70	873	779	801	779
query71	290	230	205	205
query72	2644	2500	2149	2149
query73	841	794	430	430
query74	4637	4497	4298	4298
query75	2299	2308	1943	1943
query76	2312	1108	734	734
query77	393	397	298	298
query78	8894	8975	8427	8427
query79	1275	1110	712	712
query80	510	435	359	359
query81	524	326	278	278
query82	257	163	126	126
query83	211	221	199	199
query84	295	140	111	111
query85	781	441	410	410
query86	277	251	227	227
query87	1993	1954	1835	1835
query88	3617	2716	2724	2716
query89	322	281	242	242
query90	2157	183	175	175
query91	165	155	124	124
query92	99	88	86	86
query93	1475	1578	880	880
query94	516	353	303	303
query95	680	458	330	330
query96	1075	792	320	320
query97	2387	2422	2340	2340
query98	157	151	148	148
query99	722	731	603	603
Total cold run time: 234181 ms
Total hot run time: 151889 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 23.99 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit f903c43a546989227617895ff8179edf3a816a4b, data reload: false

query1	0.01	0.01	0.00
query2	0.09	0.05	0.05
query3	0.26	0.13	0.14
query4	1.61	0.14	0.14
query5	0.24	0.21	0.21
query6	1.16	0.93	0.93
query7	0.04	0.01	0.01
query8	0.05	0.04	0.04
query9	0.38	0.33	0.33
query10	0.54	0.54	0.57
query11	0.19	0.15	0.14
query12	0.18	0.15	0.15
query13	0.48	0.45	0.46
query14	0.96	0.95	0.95
query15	0.61	0.58	0.58
query16	0.31	0.31	0.31
query17	1.11	1.06	1.07
query18	0.21	0.20	0.20
query19	2.02	1.95	1.96
query20	0.02	0.02	0.01
query21	15.48	0.21	0.15
query22	4.81	0.06	0.05
query23	16.13	0.30	0.13
query24	2.92	0.43	0.32
query25	0.12	0.05	0.04
query26	0.73	0.20	0.14
query27	0.05	0.03	0.04
query28	3.53	0.79	0.35
query29	12.51	4.14	3.30
query30	0.28	0.15	0.15
query31	2.77	0.57	0.31
query32	3.22	0.59	0.48
query33	3.13	3.20	3.16
query34	15.65	3.94	3.24
query35	3.23	3.23	3.20
query36	0.55	0.43	0.40
query37	0.09	0.06	0.06
query38	0.05	0.04	0.04
query39	0.04	0.03	0.03
query40	0.17	0.14	0.14
query41	0.09	0.03	0.03
query42	0.03	0.03	0.03
query43	0.04	0.03	0.04
Total cold run time: 96.09 s
Total hot run time: 23.99 s

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 92.94% (79/85) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 91.76% (78/85) 🎉
Increment coverage report
Complete coverage report

@mrhhsg

mrhhsg commented Sep 25, 2026

Copy link
Copy Markdown
Member Author

run external

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 91.76% (78/85) 🎉
Increment coverage report
Complete coverage report

1 similar comment
@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 91.76% (78/85) 🎉
Increment coverage report
Complete coverage report

@mrhhsg

mrhhsg commented Sep 26, 2026

Copy link
Copy Markdown
Member Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 27525 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit f903c43a546989227617895ff8179edf3a816a4b, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17671	3781	3809	3781
q2	2170	361	309	309
q3	10092	1360	772	772
q4	4676	479	348	348
q5	7469	819	536	536
q6	174	161	140	140
q7	719	777	607	607
q8	9301	1367	1552	1367
q9	5387	4166	4153	4153
q10	6826	1323	1005	1005
q11	422	270	247	247
q12	628	428	299	299
q13	18029	2605	2005	2005
q14	259	251	230	230
q15	q16	747	710	649	649
q17	1658	1148	956	956
q18	6491	5583	5522	5522
q19	1319	1228	1005	1005
q20	457	383	267	267
q21	5789	3063	3017	3017
q22	436	378	310	310
Total cold run time: 100720 ms
Total hot run time: 27525 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4513	4382	4479	4382
q2	759	552	520	520
q3	4757	5275	4584	4584
q4	2224	2293	1463	1463
q5	4592	4526	4402	4402
q6	220	172	129	129
q7	1841	1726	1551	1551
q8	2342	2014	2019	2014
q9	7372	7279	6823	6823
q10	3625	3536	3088	3088
q11	504	382	353	353
q12	705	702	510	510
q13	2262	2599	2005	2005
q14	286	275	251	251
q15	q16	652	690	633	633
q17	7257	6700	6635	6635
q18	11845	11075	11714	11075
q19	1064	994	974	974
q20	2220	2176	1904	1904
q21	4999	4184	4348	4184
q22	519	472	415	415
Total cold run time: 64558 ms
Total hot run time: 57895 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 152290 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit f903c43a546989227617895ff8179edf3a816a4b, data reload: false

query5	4310	601	462	462
query6	425	209	193	193
query7	4849	572	294	294
query8	337	179	166	166
query9	8815	3975	3983	3975
query10	463	318	257	257
query11	5819	3546	3239	3239
query12	141	92	86	86
query13	1292	592	430	430
query14	6515	4529	4239	4239
query14_1	3981	3985	3925	3925
query15	195	197	185	185
query16	1001	465	456	456
query17	913	678	546	546
query18	2462	471	342	342
query19	228	178	141	141
query20	84	81	79	79
query21	225	134	112	112
query22	13120	13062	12832	12832
query23	13977	12917	12316	12316
query23_1	12541	12506	12418	12418
query24	7218	1080	674	674
query24_1	630	727	692	692
query25	532	394	346	346
query26	1257	292	158	158
query27	2687	555	323	323
query28	4514	1989	1998	1989
query29	1625	689	514	514
query30	293	218	183	183
query31	889	757	634	634
query32	149	93	100	93
query33	511	288	245	245
query34	1178	1117	646	646
query35	718	732	630	630
query36	779	788	728	728
query37	144	102	89	89
query38	1847	1786	1707	1707
query39	682	680	676	676
query39_1	645	648	667	648
query40	230	121	97	97
query41	68	63	63	63
query42	97	94	92	92
query43	371	348	300	300
query44	1354	710	729	710
query45	179	173	163	163
query46	1083	1201	775	775
query47	1495	1471	1413	1413
query48	419	418	301	301
query49	599	409	292	292
query50	999	337	260	260
query51	10617	10855	10235	10235
query52	87	95	75	75
query53	234	245	183	183
query54	239	217	191	191
query55	77	75	69	69
query56	225	228	199	199
query57	1410	1447	1290	1290
query58	298	259	251	251
query59	1990	2036	1854	1854
query60	285	236	222	222
query61	144	147	144	144
query62	392	326	267	267
query63	213	173	176	173
query64	2808	960	808	808
query65	3470	3422	3408	3408
query66	1780	446	311	311
query67	20037	19980	19733	19733
query68	3221	1507	911	911
query69	421	323	272	272
query70	938	850	787	787
query71	304	237	222	222
query72	2764	2668	2401	2401
query73	832	836	422	422
query74	4641	4501	4306	4306
query75	2295	2332	1977	1977
query76	2299	1103	770	770
query77	351	388	302	302
query78	8904	9112	8491	8491
query79	1327	1190	780	780
query80	595	441	360	360
query81	529	318	282	282
query82	643	160	128	128
query83	297	219	194	194
query84	315	146	113	113
query85	846	462	379	379
query86	323	236	227	227
query87	1990	1981	1826	1826
query88	3591	2736	2717	2717
query89	354	285	250	250
query90	1939	183	169	169
query91	168	172	125	125
query92	106	107	86	86
query93	1481	1525	876	876
query94	531	323	301	301
query95	651	362	423	362
query96	1064	790	318	318
query97	2437	2460	2330	2330
query98	161	149	153	149
query99	721	731	618	618
Total cold run time: 235848 ms
Total hot run time: 152290 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 23.91 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit f903c43a546989227617895ff8179edf3a816a4b, data reload: false

query1	0.00	0.00	0.00
query2	0.09	0.04	0.05
query3	0.25	0.13	0.14
query4	1.61	0.14	0.14
query5	0.24	0.21	0.21
query6	1.16	0.94	0.93
query7	0.04	0.01	0.00
query8	0.05	0.04	0.04
query9	0.38	0.35	0.33
query10	0.55	0.59	0.53
query11	0.19	0.14	0.14
query12	0.18	0.15	0.14
query13	0.46	0.47	0.48
query14	0.95	0.96	0.95
query15	0.60	0.58	0.58
query16	0.32	0.32	0.31
query17	1.05	1.12	1.07
query18	0.21	0.20	0.20
query19	1.99	1.89	1.94
query20	0.02	0.02	0.01
query21	15.48	0.22	0.15
query22	4.70	0.05	0.06
query23	16.14	0.30	0.11
query24	3.00	0.41	0.29
query25	0.10	0.04	0.04
query26	0.73	0.21	0.16
query27	0.04	0.03	0.04
query28	3.55	0.79	0.34
query29	12.51	4.20	3.30
query30	0.28	0.15	0.15
query31	2.77	0.55	0.31
query32	3.23	0.58	0.49
query33	3.25	3.26	3.10
query34	15.74	3.92	3.28
query35	3.24	3.23	3.23
query36	0.55	0.42	0.42
query37	0.09	0.07	0.06
query38	0.06	0.03	0.03
query39	0.04	0.03	0.03
query40	0.17	0.14	0.14
query41	0.09	0.04	0.03
query42	0.04	0.04	0.03
query43	0.05	0.04	0.03
Total cold run time: 96.19 s
Total hot run time: 23.91 s

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 91.76% (78/85) 🎉
Increment coverage report
Complete coverage report

3 similar comments
@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 91.76% (78/85) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 91.76% (78/85) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 91.76% (78/85) 🎉
Increment coverage report
Complete coverage report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants