mrhhsg commented on code in PR #68488:
URL: https://github.com/apache/doris/pull/68488#discussion_r4105747372
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinator.java:
##########
@@ -165,4 +167,12 @@ public void checkLegalityBeforeTypeCoercion() {
public void checkLegalityAfterRewrite() {
nested.withChildren(children()).checkLegalityAfterRewrite();
}
+
+ @Override
+ public Expression rewriteWhenAnalyze() {
+ AggregateFunction coercedNested = nested.withChildren(children());
Review Comment:
Confirmed and fixed in c18a01cbf37. On 6dd4e45,
`percentile_reservoir_state(v, CAST('0.25' AS DOUBLE))` got the layout `(DOUBLE
NOT NULL, DOUBLE NOT NULL)`. A `UNION ALL` with a stored
`AGG_STATE<percentile_reservoir(DOUBLE NOT NULL, DOUBLE NULL)>` column then
failed with "Can not find assignment compatible type".
I didn't keep the origin signature in the combinator. The Analyzer runs
under `keepFunctionSignature(false)`, and later analysis rules
(NormalizeAggregate, AdjustNullable, `ExpressionUtils.rebuildSignature`)
rebuild `_state`/`_combine` again, which would drop a kept signature. Instead,
the executed level keeps the analyzed nullability:
`PercentileReservoir.rewriteWhenAnalyze` wraps the validated literal in
`Nullable` when the original level is nullable and the literal is not. Any
rebuild then derives the same AggState layout, and the direct function's
nullability stays the same. `StateCombinator` and `CombineCombinator` just
forward the rewritten children, so both are covered.
`checkLevel` now looks through the wrappers by value in every phase, so the
rewritten level is accepted when it is analyzed again (e.g. by non-fast INSERT
VALUES):
- `Nullable` is always looked through.
- `NonNullable` is looked through only when its argument folds to a non-NULL
literal.
- A user-written `non_nullable(CAST('' AS DOUBLE))` is still rejected.
Tests:
- UT `testAnalyzedLevelKeepsStateLayout` covers direct, `_state` and
`_combine` under `keepFunctionSignature(false)`, including a second rebuild. It
asserts the same `getDataType()`/`nullable()`.
- UT `testUserNonNullableLevelIsNotUnwrappedInAnalysis` and the extended
`testLevelWrappedByAggStateCastIsAccepted` cover the wrapper handling.
- New regression cases: `qt_nullable_level_state_union_merge`,
`qt_overwrite_nullable_level_state`, and two `non_nullable` rejection cases.
- datatype_p0/agg_state still passes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
