Conversation
### 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
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review-light |
There was a problem hiding this comment.
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_constantpaths 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 / 2plus1 / 0under 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:
-lightsupplied 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
|
/review-light |
There was a problem hiding this comment.
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/3and1/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:
-lightsupplied 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
|
/review-light |
|
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. The selected account is excluded until 2026-09-30T02:10:00Z. Please trigger /review again; another configured account may be available. |
|
/review-light |
There was a problem hiding this comment.
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.
|
/review-light |
There was a problem hiding this comment.
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
|
/review-light |
There was a problem hiding this comment.
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
|
/review-light |
There was a problem hiding this comment.
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:
-lightsupplied 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
|
/review-light |
There was a problem hiding this comment.
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
|
/review-light |
There was a problem hiding this comment.
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.
|
run buildall |
TPC-H: Total hot run time: 27612 ms |
TPC-DS: Total hot run time: 152497 ms |
ClickBench: 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
|
/review-light |
There was a problem hiding this comment.
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.
|
run buildall |
TPC-H: Total hot run time: 27610 ms |
TPC-DS: Total hot run time: 151889 ms |
ClickBench: Total hot run time: 23.99 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
|
run external |
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
TPC-H: Total hot run time: 27525 ms |
TPC-DS: Total hot run time: 152290 ms |
ClickBench: Total hot run time: 23.91 s |
FE Regression Coverage ReportIncrement line coverage |
3 similar comments
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
Issue Number: None
Problem Summary:
percentile_reservoirvalidated its level argument only incheckLegalityBeforeTypeCoercion, which runs during analysis beforeconstant folding. Besides requiring the argument to be constant, it also
required it to already be a
Literal, so any constant expression thatonly becomes a literal after folding was rejected:
The equivalent literal
0.5works, and sibling functions such aspercentile_approxaccept the same foldable expression, so therestriction was inconsistent and unnecessary.
Both
checkLegalityBeforeTypeCoercionandcheckLegalityAfterRewritenow share one check that folds the level itself with
FoldConstantRuleOnFE.evaluateWithoutContext(asstackalready doesfor 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:
0.25 + 0.25is only a literal afterfolding,
INSERT ... VALUESand load column mappings never run the rewritephase, so an analysis-time check is the only one on those paths,
debug_skip_fold_constantturns off the regular constant folding, anda plain literal
0.5would otherwise stay an unfolded cast.Foldable constants such as
0.25 + 0.25,cast('0.5' as double)or1 - 0.75are now accepted for the plain aggregate, the window form,the
_statecombinator andINSERT ... 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 errormessages as before;
cast('NaN' as double)is now rejected as out ofrange 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_constantcan keep the expression too. The validated andthe 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))wrotea state that failed with "incompatible quantiles" when merged with a state
built by a query.
percentile_reservoir(and its_state/_combinecombinators) therefore replaces the level with the validated literal once
it is analyzed (
RewriteWhenAnalyze), so BE always executes the checkedvalue. 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 negativeNaN 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.equalstakes NaNs of both signs asequal, but
signbit()tells them apart).An explicit cast of a
percentile_reservoirstate to another agg_statelayout, e.g.
CAST(percentile_reservoir_state(v, 0.25) AS AGG_STATE<percentile_reservoir(DOUBLE NOT NULL, DOUBLE NULL)>), includingchained casts, makes
ConvertAggStateCastwrap the level inNullable/NonNullable; the post-rewrite check validates the level beneath thosewrappers, 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 byFE (
BigDecimal.doubleValue()), as queries andINSERT ... VALUESalreadydid 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_reservoirnow accepts any constant expression that folds to a level in[0, 1](for example0.25 + 0.25orcast('0.5' as double)), including inINSERT ... VALUESand withdebug_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 underenable_strict_cast = true), the same ascast('' 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 ascast(1 as decimalv2(27, 9)) / cast(3 as decimalv2(27, 9))or2.0 / 3fold 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 anddebug_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)
PercentileReservoirParameterTestupdated to coverliteral, foldable, unfoldable, non-constant, string, NULL and
DECIMALV2/DECIMALV3 quotient levels through both check phases;
FoldConstantTestpins DECIMALV2/DECIMALV3 division folds againstBE-computed values.
test_percentile_reservoir_constant_level(query_p0/sql_functions/aggregate_functions) including the
INSERT ... VALUESpath anddebug_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_arithmaticandtest_int128_unaligned_accessre-run locally. Later follow-ups addfold-on / skip-fold / DISTINCT / window / state-merge / stream load
cases,
StringLikeLiteralTest,DoubleLiteralTestandFoldConstantTestraw-bit and NaN-branch cases, and re-runfunction_p0/cast/to_float,datatype_p0/agg_state,mv_p0/agg_state,query_p0/expression/fold_constant,nereids_rules_p0/fold_constantandnereids_rules_p0/case_when_rules;nullable / chained agg_state cast cases in
PercentileReservoirParameterTestand the regression test.
are accepted instead of raising an analysis error. A string literal
level is cast to DOUBLE before the range check (
'0.5'is accepted as0.5,
'5'is rejected as out of range) instead of being compared as ameaningless 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, acast error under
enable_strict_cast = true. A constant level whosetype 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) nowreturns 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 ason BE. FE folding of a nonzero DECIMAL quotient now uses BE's scale and
rounding instead of an exact
BigDecimal.divide, which threw for arecurring quotient (
1 / 3) and produced a value too precise for theliteral (
1 / 1024), leaving the division unfolded: DECIMALV2 keepsscale 9 with the analyzed
DECIMALV2(27, 9)type and rounds up once theremainder reaches
divisor >> 1(DecimalV2Value::operator/), DECIMALV3truncates 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 of0.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 isparsed through a double), a DOUBLE is narrowed to FLOAT with a single
rounding, and If / CASE branches holding a NaN literal are not merged.