Skip to content

Commit 7e1a126

Browse files
committed
[fix](fe) Preserve aggregate state layouts across rewrites
### What problem does this PR solve? Issue Number: N/A Related PR: #67805 Problem Summary: Constant folding can replace a nullable string cast with a non-null literal. AggState producers then recomputed their input layout while UNION consumers retained the analyzed nullable layout, returning NULL instead of aggregate values or incompatible-parameter errors. Preserve the analyzed state signature for _state and _combine and align combine raw inputs with it. Explicit state casts still rebind layouts, and expression identity includes the declared state type. Cover these paths together with the rule that empty reservoir samples do not constrain merge parameters. ### Release note AggState expressions preserve nullable layouts across constant folding, preventing incorrect NULL results and skipped parameter mismatch errors. ### Check List (For Author) - Test: FE build and Checkstyle passed; 43 FE unit tests and all 13 AggState regression suites passed. Regression expectations generated and verified with the repository runner; git diff whitespace check passed. - Behavior changed: Yes, folded AggState expressions return correct values or parameter errors, while non-contributing empty states ignore parameters. - Does this need documentation: No
1 parent 822f212 commit 7e1a126

7 files changed

Lines changed: 479 additions & 14 deletions

File tree

‎fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/ExpressionTranslator.java‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@
101101
import org.apache.doris.nereids.trees.expressions.functions.scalar.DictGetMany;
102102
import org.apache.doris.nereids.trees.expressions.functions.scalar.ElementAt;
103103
import org.apache.doris.nereids.trees.expressions.functions.scalar.Lambda;
104+
import org.apache.doris.nereids.trees.expressions.functions.scalar.Nullable;
104105
import org.apache.doris.nereids.trees.expressions.functions.scalar.ScalarFunction;
105106
import org.apache.doris.nereids.trees.expressions.functions.udf.JavaUdaf;
106107
import org.apache.doris.nereids.trees.expressions.functions.udf.JavaUdf;
@@ -111,6 +112,7 @@
111112
import org.apache.doris.nereids.trees.expressions.functions.window.WindowFunction;
112113
import org.apache.doris.nereids.trees.expressions.literal.Literal;
113114
import org.apache.doris.nereids.trees.expressions.visitor.DefaultExpressionVisitor;
115+
import org.apache.doris.nereids.types.AggStateType;
114116
import org.apache.doris.nereids.types.DataType;
115117
import org.apache.doris.thrift.TDictFunction;
116118

@@ -838,20 +840,15 @@ public Expr visitStateCombinator(StateCombinator combinator, PlanTranslatorConte
838840
visitAggregateFunction(combinator.getNestedFunction(), context).getFn(),
839841
new FunctionParams(false, arguments), isReturnNullable);
840842
return convertToStateCombinator(combinator.getName(), functionCallExpr,
841-
arguments.stream().map(Expr::getType).collect(Collectors.toList()),
842-
combinator.getArguments().stream().map(Expression::nullable).collect(Collectors.toList()),
843-
isReturnNullable);
843+
combinator.getDataType().toCatalogDataType());
844844
}
845845

846-
private FunctionCallExpr convertToStateCombinator(String name, FunctionCallExpr fnCall,
847-
List<Type> argTypes, List<Boolean> argNullables,
848-
boolean returnNullable) {
846+
private FunctionCallExpr convertToStateCombinator(String name, FunctionCallExpr fnCall, Type returnType) {
849847
Function aggFunction = fnCall.getFn();
850848
List<Type> arguments = Arrays.asList(aggFunction.getArgs());
851849
org.apache.doris.catalog.ScalarFunction fn = new org.apache.doris.catalog.ScalarFunction(
852850
new FunctionName(name), arguments,
853-
Expr.createAggStateType(aggFunction.getFunctionName().getFunction(),
854-
argTypes, argNullables, returnNullable),
851+
returnType,
855852
aggFunction.hasVarArgs(), aggFunction.isUserVisible());
856853
fn.setNullableMode(NullableMode.ALWAYS_NOT_NULLABLE);
857854
fn.setBinaryType(Function.BinaryType.AGG_STATE);
@@ -1015,6 +1012,25 @@ public Expr visitPythonUdtf(PythonUdtf udtf, PlanTranslatorContext context) {
10151012
private Expr translateAggregateFunction(AggregateFunction function,
10161013
List<Expression> currentPhaseArguments, List<Expr> aggFnArguments,
10171014
AggregateParam aggregateParam, PlanTranslatorContext context) {
1015+
if (function instanceof CombineCombinator) {
1016+
AggStateType stateType = (AggStateType) function.getDataType();
1017+
List<Boolean> nullables = stateType.getSubTypeNullables();
1018+
// The state layout survives rewrites even when a cast folds to a non-null literal.
1019+
// _combine consumes columns directly, so align both its signature and raw inputs here.
1020+
for (int i = 0; i < aggFnArguments.size(); i++) {
1021+
aggFnArguments.set(i, new SlotRef(
1022+
stateType.getSubTypes().get(i).toCatalogDataType(), nullables.get(i)));
1023+
}
1024+
if (!aggregateParam.aggMode.consumeAggregateBuffer) {
1025+
ImmutableList.Builder<Expression> arguments =
1026+
ImmutableList.builderWithExpectedSize(currentPhaseArguments.size());
1027+
for (int i = 0; i < currentPhaseArguments.size(); i++) {
1028+
Expression argument = currentPhaseArguments.get(i);
1029+
arguments.add(nullables.get(i) && !argument.nullable() ? new Nullable(argument) : argument);
1030+
}
1031+
currentPhaseArguments = arguments.build();
1032+
}
1033+
}
10181034
List<Expr> currentPhaseCatalogArguments = Lists.newArrayListWithCapacity(currentPhaseArguments.size());
10191035
for (Expression arg : currentPhaseArguments) {
10201036
if (arg instanceof OrderExpression) {

‎fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/ConvertAggStateCast.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.apache.doris.nereids.trees.expressions.functions.scalar.Nullable;
2929
import org.apache.doris.nereids.types.AggStateType;
3030
import org.apache.doris.nereids.types.DataType;
31+
import org.apache.doris.nereids.util.MoreFieldsThread;
3132
import org.apache.doris.nereids.util.TypeCoercionUtils;
3233

3334
import com.google.common.collect.ImmutableList;
@@ -74,7 +75,10 @@ public static Expression convert(Cast cast) {
7475
}
7576
newChildren.add(newChild);
7677
}
77-
child = child.withChildren(newChildren.build());
78+
// An explicit state cast changes the serialized layout, unlike ordinary rewrites.
79+
StateCombinator state = (StateCombinator) child;
80+
child = MoreFieldsThread.keepFunctionSignature(false,
81+
() -> state.withChildren(newChildren.build()));
7882
return cast.withChildren(ImmutableList.of(child));
7983
}
8084
}

‎fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/CombineCombinator.java‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,15 +96,15 @@ private static AggStateType createReturnType(List<Expression> arguments, Aggrega
9696

9797
@Override
9898
public CombineCombinator withChildren(List<Expression> children) {
99-
return new CombineCombinator(getFunctionParams(children), nested);
99+
return new CombineCombinator(getFunctionParams(children), nested.withChildren(children));
100100
}
101101

102102
@Override
103103
public AggregateFunction withDistinctAndChildren(boolean distinct, List<Expression> children) {
104104
if (distinct) {
105105
throw new AnalysisException(getName() + " doesn't support DISTINCT");
106106
}
107-
return new CombineCombinator(getFunctionParams(false, children), nested);
107+
return new CombineCombinator(getFunctionParams(false, children), nested.withChildren(children));
108108
}
109109

110110
@Override
@@ -121,7 +121,17 @@ public <R, C> R accept(ExpressionVisitor<R, C> visitor, C context) {
121121

122122
@Override
123123
public DataType getDataType() {
124-
return returnType;
124+
return getSignature().returnType;
125+
}
126+
127+
@Override
128+
public boolean equals(Object other) {
129+
return super.equals(other) && getDataType().equals(((CombineCombinator) other).getDataType());
130+
}
131+
132+
@Override
133+
public int computeHashCode() {
134+
return Objects.hash(super.computeHashCode(), getDataType());
125135
}
126136

127137
@Override

‎fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinator.java‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ public static StateCombinator create(AggregateFunction nested) {
102102

103103
@Override
104104
public StateCombinator withChildren(List<Expression> children) {
105-
return new StateCombinator(getFunctionParams(children), nested);
105+
return new StateCombinator(getFunctionParams(children), nested.withChildren(children));
106106
}
107107

108108
@Override
@@ -119,7 +119,19 @@ public <R, C> R accept(ExpressionVisitor<R, C> visitor, C context) {
119119

120120
@Override
121121
public DataType getDataType() {
122-
return returnType;
122+
// Input nullability is part of the serialized state layout. Keep the analyzed
123+
// signature when rewrites replace nullable expressions with non-null literals.
124+
return getSignature().returnType;
125+
}
126+
127+
@Override
128+
protected boolean extraEquals(Expression that) {
129+
return super.extraEquals(that) && getDataType().equals(that.getDataType());
130+
}
131+
132+
@Override
133+
public int computeHashCode() {
134+
return Objects.hash(super.computeHashCode(), getDataType());
123135
}
124136

125137
@Override
Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
18+
package org.apache.doris.nereids.trees.expressions.functions.combinator;
19+
20+
import org.apache.doris.analysis.FunctionCallExpr;
21+
import org.apache.doris.nereids.glue.translator.ExpressionTranslator;
22+
import org.apache.doris.nereids.glue.translator.PlanTranslatorContext;
23+
import org.apache.doris.nereids.rules.expression.rules.ConvertAggStateCast;
24+
import org.apache.doris.nereids.rules.expression.rules.FoldConstantRuleOnFE;
25+
import org.apache.doris.nereids.trees.expressions.AggregateExpression;
26+
import org.apache.doris.nereids.trees.expressions.Cast;
27+
import org.apache.doris.nereids.trees.expressions.Expression;
28+
import org.apache.doris.nereids.trees.expressions.functions.agg.AggregateParam;
29+
import org.apache.doris.nereids.trees.expressions.functions.agg.PercentileReservoir;
30+
import org.apache.doris.nereids.trees.expressions.literal.DoubleLiteral;
31+
import org.apache.doris.nereids.trees.expressions.literal.VarcharLiteral;
32+
import org.apache.doris.nereids.types.AggStateType;
33+
import org.apache.doris.nereids.types.DoubleType;
34+
import org.apache.doris.nereids.util.MemoTestUtils;
35+
import org.apache.doris.nereids.util.MoreFieldsThread;
36+
import org.apache.doris.nereids.util.PlanChecker;
37+
38+
import com.google.common.collect.ImmutableList;
39+
import com.google.common.collect.ImmutableSet;
40+
import org.junit.jupiter.api.Assertions;
41+
import org.junit.jupiter.api.BeforeEach;
42+
import org.junit.jupiter.api.Test;
43+
44+
class StateCombinatorTest {
45+
@BeforeEach
46+
void setUp() {
47+
MemoTestUtils.createConnectContext();
48+
}
49+
50+
@Test
51+
void testConstantFoldingPreservesStateLayout() {
52+
StateCombinator state = StateCombinator.create(new PercentileReservoir(
53+
new Cast(new VarcharLiteral("7"), DoubleType.INSTANCE), new DoubleLiteral(0.25)));
54+
AggStateType analyzedType = (AggStateType) state.getDataType();
55+
Assertions.assertEquals(ImmutableList.of(true, false), analyzedType.getSubTypeNullables());
56+
57+
StateCombinator folded = (StateCombinator) MoreFieldsThread.keepFunctionSignature(
58+
() -> FoldConstantRuleOnFE.evaluateWithoutContext(state));
59+
Assertions.assertEquals(new DoubleLiteral(7), folded.child(0));
60+
Assertions.assertFalse(folded.child(0).nullable());
61+
Assertions.assertEquals(analyzedType, folded.getDataType());
62+
63+
FunctionCallExpr translated = (FunctionCallExpr) ExpressionTranslator.translate(
64+
folded, new PlanTranslatorContext());
65+
Assertions.assertEquals(analyzedType.toCatalogDataType(), translated.getFn().getReturnType());
66+
}
67+
68+
@Test
69+
void testCombineConstantFoldingPreservesInputLayout() {
70+
PercentileReservoir nested = new PercentileReservoir(
71+
new Cast(new VarcharLiteral("7"), DoubleType.INSTANCE), new DoubleLiteral(0.25));
72+
CombineCombinator combine = new CombineCombinator(nested.children(), nested);
73+
AggStateType analyzedType = (AggStateType) combine.getDataType();
74+
CombineCombinator folded = (CombineCombinator) MoreFieldsThread.keepFunctionSignature(
75+
() -> FoldConstantRuleOnFE.evaluateWithoutContext(combine));
76+
Assertions.assertFalse(folded.child(0).nullable());
77+
Assertions.assertEquals(new DoubleLiteral(7), folded.child(0));
78+
Assertions.assertEquals(analyzedType, folded.getDataType());
79+
Assertions.assertEquals(folded, MoreFieldsThread.keepFunctionSignature(
80+
() -> folded.withDistinctAndChildren(false, folded.children())));
81+
FunctionCallExpr translated = (FunctionCallExpr) ExpressionTranslator.translate(
82+
new AggregateExpression(folded, AggregateParam.LOCAL_RESULT), new PlanTranslatorContext());
83+
Assertions.assertEquals(analyzedType.toCatalogDataType(), translated.getFn().getReturnType());
84+
Assertions.assertTrue(translated.getChild(0).isNullable());
85+
}
86+
87+
@Test
88+
void testAnalysisCanRecomputeStateType() {
89+
StateCombinator state = StateCombinator.create(new PercentileReservoir(
90+
new Cast(new VarcharLiteral("7"), DoubleType.INSTANCE), new DoubleLiteral(0.25)));
91+
state.getSignature();
92+
StateCombinator rebound = MoreFieldsThread.keepFunctionSignature(false,
93+
() -> state.withChildren(ImmutableList.of(new DoubleLiteral(7), new DoubleLiteral(0.25))));
94+
Assertions.assertEquals(ImmutableList.of(false, false),
95+
((AggStateType) rebound.getDataType()).getSubTypeNullables());
96+
}
97+
98+
@Test
99+
void testStateLayoutsHaveDistinctExpressionIdentity() {
100+
PercentileReservoir nullable = new PercentileReservoir(
101+
new Cast(new VarcharLiteral("7"), DoubleType.INSTANCE), new DoubleLiteral(0.25));
102+
PercentileReservoir nonnullable = new PercentileReservoir(new DoubleLiteral(7), new DoubleLiteral(0.25));
103+
for (boolean combine : ImmutableList.of(false, true)) {
104+
Expression beforeFold = combine ? new CombineCombinator(nullable.children(), nullable)
105+
: StateCombinator.create(nullable);
106+
Expression direct = combine ? new CombineCombinator(nonnullable.children(), nonnullable)
107+
: StateCombinator.create(nonnullable);
108+
Expression folded = MoreFieldsThread.keepFunctionSignature(
109+
() -> FoldConstantRuleOnFE.evaluateWithoutContext(beforeFold));
110+
Assertions.assertEquals(direct.children(), folded.children());
111+
Assertions.assertNotEquals(direct.getDataType(), folded.getDataType());
112+
Assertions.assertNotEquals(direct, folded);
113+
Assertions.assertEquals(2, ImmutableSet.of(direct, folded).size());
114+
Assertions.assertEquals(folded, MoreFieldsThread.keepFunctionSignature(
115+
() -> folded.withChildren(folded.children())));
116+
}
117+
}
118+
119+
@Test
120+
void testExplicitStateCastAfterConstantFolding() {
121+
PlanChecker.from(MemoTestUtils.createConnectContext())
122+
.analyze("select cast(percentile_reservoir_state(cast('7' as double), 0.25) "
123+
+ "as agg_state<percentile_reservoir(double not null, double not null)>)")
124+
.rewrite();
125+
}
126+
127+
@Test
128+
void testExplicitStateCastRebindsCustomSignature() {
129+
for (String nullable : ImmutableList.of("null", "not null")) {
130+
PlanChecker.from(MemoTestUtils.createConnectContext())
131+
.analyze("select cast(max_state(null) as agg_state<max(int " + nullable + ")>)")
132+
.rewrite();
133+
}
134+
}
135+
136+
@Test
137+
void testExplicitStateCastUpdatesStateLayout() {
138+
for (boolean nullable : ImmutableList.of(false, true)) {
139+
Expression argument = nullable ? new DoubleLiteral(7)
140+
: new Cast(new VarcharLiteral("7"), DoubleType.INSTANCE);
141+
StateCombinator state = StateCombinator.create(
142+
new PercentileReservoir(argument, new DoubleLiteral(0.25)));
143+
AggStateType target = new AggStateType("percentile_reservoir",
144+
ImmutableList.of(DoubleType.INSTANCE, DoubleType.INSTANCE),
145+
ImmutableList.of(nullable, false), true);
146+
Assertions.assertNotEquals(target, state.getDataType());
147+
148+
Cast converted = (Cast) MoreFieldsThread.keepFunctionSignature(
149+
() -> ConvertAggStateCast.convert(new Cast(state, target)));
150+
Assertions.assertEquals(nullable, converted.child().child(0).nullable());
151+
Assertions.assertEquals(target, converted.child().getDataType());
152+
FunctionCallExpr translated = (FunctionCallExpr) ExpressionTranslator.translate(
153+
converted.child(), new PlanTranslatorContext());
154+
Assertions.assertEquals(target.toCatalogDataType(), translated.getFn().getReturnType());
155+
}
156+
}
157+
}

0 commit comments

Comments
 (0)