This is an automated email from the ASF dual-hosted git repository.
morrysnow pushed a commit to branch branch-2.1
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/branch-2.1 by this push:
new 17c33665e56 [fix](agg_state) adjust nullable should apply on agg_state
inner type too (#37489) (#38281)
17c33665e56 is described below
commit 17c33665e56a9bfbcc5298da2584c34897ec1a4a
Author: morrySnow <101034200+morrys...@users.noreply.github.com>
AuthorDate: Wed Jul 24 13:55:09 2024 +0800
[fix](agg_state) adjust nullable should apply on agg_state inner type too
(#37489) (#38281)
pick from master #37489
after adjust nullable, some children nullable has changed. so, we need
to update agg_state type inner type nullable too.
---
.../apache/doris/nereids/analyzer/MappingSlot.java | 7 +++-
.../nereids/rules/rewrite/AdjustNullable.java | 16 ++++-----
.../trees/expressions/ArrayItemReference.java | 7 +++-
.../doris/nereids/trees/expressions/Slot.java | 7 +++-
.../nereids/trees/expressions/SlotReference.java | 14 ++++++--
.../trees/expressions/VirtualSlotReference.java | 15 +++++++--
.../trees/plans/commands/CreateTableCommand.java | 2 ++
.../trees/plans/commands/info/CreateMTMVInfo.java | 6 ++--
.../java/org/apache/doris/qe/SessionVariable.java | 9 +++++
.../data/nereids_p0/create_table/test_ctas.out | 3 ++
.../nereids_p0/create_table/test_ctas.groovy | 5 +++
.../adjust_nullable/agg_state_type.groovy | 39 ++++++++++++++++++++++
12 files changed, 111 insertions(+), 19 deletions(-)
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/analyzer/MappingSlot.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/analyzer/MappingSlot.java
index 8acfb03762d..c7a020fd2ab 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/analyzer/MappingSlot.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/analyzer/MappingSlot.java
@@ -102,7 +102,12 @@ public class MappingSlot extends Slot {
}
@Override
- public Slot withNullable(boolean newNullable) {
+ public Slot withNullable(boolean nullable) {
+ return this;
+ }
+
+ @Override
+ public Slot withNullableAndDataType(boolean nullable, DataType dataType) {
return this;
}
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AdjustNullable.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AdjustNullable.java
index 8249202dbd5..1f47f8bfc21 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AdjustNullable.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AdjustNullable.java
@@ -293,20 +293,20 @@ public class AdjustNullable extends
DefaultPlanRewriter<Map<ExprId, Slot>> imple
return result.build();
}
- private Map<ExprId, Slot> collectChildrenOutputMap(LogicalPlan plan) {
- return plan.children().stream()
- .map(Plan::getOutputSet)
- .flatMap(Set::stream)
- .collect(Collectors.toMap(NamedExpression::getExprId, s -> s));
- }
-
private static class SlotReferenceReplacer extends
DefaultExpressionRewriter<Map<ExprId, Slot>> {
public static SlotReferenceReplacer INSTANCE = new
SlotReferenceReplacer();
@Override
public Expression visitSlotReference(SlotReference slotReference,
Map<ExprId, Slot> context) {
if (context.containsKey(slotReference.getExprId())) {
- return
slotReference.withNullable(context.get(slotReference.getExprId()).nullable());
+ Slot slot = context.get(slotReference.getExprId());
+ if (slot.getDataType().isAggStateType()) {
+ // we must replace data type, because nested type and agg
state contains nullable of their children.
+ // TODO: remove if statement after we ensure be constant
folding do not change expr type at all.
+ return
slotReference.withNullableAndDataType(slot.nullable(), slot.getDataType());
+ } else {
+ return slotReference.withNullable(slot.nullable());
+ }
} else {
return slotReference;
}
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/ArrayItemReference.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/ArrayItemReference.java
index 1a853207cc6..c54ad358561 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/ArrayItemReference.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/ArrayItemReference.java
@@ -158,7 +158,12 @@ public class ArrayItemReference extends NamedExpression
implements ExpectsInputT
}
@Override
- public SlotReference withNullable(boolean newNullable) {
+ public SlotReference withNullable(boolean nullable) {
+ return new ArrayItemSlot(exprId, name.get(), dataType,
this.nullable);
+ }
+
+ @Override
+ public Slot withNullableAndDataType(boolean nullable, DataType
dataType) {
return new ArrayItemSlot(exprId, name.get(), dataType, nullable);
}
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Slot.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Slot.java
index 2460c964463..40cf360ab46 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Slot.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Slot.java
@@ -19,6 +19,7 @@ package org.apache.doris.nereids.trees.expressions;
import org.apache.doris.common.Pair;
import org.apache.doris.nereids.trees.expressions.shape.LeafExpression;
+import org.apache.doris.nereids.types.DataType;
import com.google.common.collect.ImmutableList;
@@ -43,7 +44,11 @@ public abstract class Slot extends NamedExpression
implements LeafExpression {
return this;
}
- public Slot withNullable(boolean newNullable) {
+ public Slot withNullable(boolean nullable) {
+ throw new RuntimeException("Do not implement");
+ }
+
+ public Slot withNullableAndDataType(boolean nullable, DataType dataType) {
throw new RuntimeException("Do not implement");
}
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/SlotReference.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/SlotReference.java
index 920bb791b5b..145d8dd29f7 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/SlotReference.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/SlotReference.java
@@ -249,12 +249,20 @@ public class SlotReference extends Slot {
}
@Override
- public SlotReference withNullable(boolean newNullable) {
- if (this.nullable == newNullable) {
+ public SlotReference withNullable(boolean nullable) {
+ if (this.nullable == nullable) {
return this;
}
+ return new SlotReference(exprId, name, dataType, nullable,
+ qualifier, table, column, internalName, subPath,
indexInSqlString);
+ }
- return new SlotReference(exprId, name, dataType, newNullable,
+ @Override
+ public Slot withNullableAndDataType(boolean nullable, DataType dataType) {
+ if (this.nullable == nullable && this.dataType.equals(dataType)) {
+ return this;
+ }
+ return new SlotReference(exprId, name, dataType, nullable,
qualifier, table, column, internalName, subPath,
indexInSqlString);
}
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/VirtualSlotReference.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/VirtualSlotReference.java
index 326bd202cd5..43f48537581 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/VirtualSlotReference.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/VirtualSlotReference.java
@@ -121,11 +121,20 @@ public class VirtualSlotReference extends SlotReference
implements SlotNotFromCh
return false;
}
- public VirtualSlotReference withNullable(boolean newNullable) {
- if (this.nullable == newNullable) {
+ public VirtualSlotReference withNullable(boolean nullable) {
+ if (this.nullable == nullable) {
return this;
}
- return new VirtualSlotReference(exprId, name.get(), dataType,
newNullable, qualifier,
+ return new VirtualSlotReference(exprId, name.get(), dataType,
nullable, qualifier,
+ originExpression, computeLongValueMethod);
+ }
+
+ @Override
+ public Slot withNullableAndDataType(boolean nullable, DataType dataType) {
+ if (this.nullable == nullable && this.dataType.equals(dataType)) {
+ return this;
+ }
+ return new VirtualSlotReference(exprId, name.get(), dataType,
nullable, qualifier,
originExpression, computeLongValueMethod);
}
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateTableCommand.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateTableCommand.java
index f2dd92fe328..38c92631d41 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateTableCommand.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateTableCommand.java
@@ -107,6 +107,8 @@ public class CreateTableCommand extends Command implements
ForwardWithSync {
LogicalPlan query = ctasQuery.get();
List<String> ctasCols = createTableInfo.getCtasColumns();
NereidsPlanner planner = new NereidsPlanner(ctx.getStatementContext());
+ // must disable constant folding by be, because be constant folding
may return wrong type
+ ctx.getSessionVariable().disableConstantFoldingByBEOnce();
Plan plan = planner.plan(new UnboundResultSink<>(query),
PhysicalProperties.ANY, ExplainLevel.NONE);
if (ctasCols == null) {
// we should analyze the plan firstly to get the columns' name.
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateMTMVInfo.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateMTMVInfo.java
index 55b2e379f83..8bd90673f24 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateMTMVInfo.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateMTMVInfo.java
@@ -141,7 +141,7 @@ public class CreateMTMVInfo {
/**
* analyze create table info
*/
- public void analyze(ConnectContext ctx) {
+ public void analyze(ConnectContext ctx) throws Exception {
// analyze table name
mvName.analyze(ctx);
try {
@@ -201,12 +201,14 @@ public class CreateMTMVInfo {
/**
* analyzeQuery
*/
- public void analyzeQuery(ConnectContext ctx, Map<String, String>
mvProperties) {
+ public void analyzeQuery(ConnectContext ctx, Map<String, String>
mvProperties) throws Exception {
// create table as select
StatementContext statementContext = ctx.getStatementContext();
NereidsPlanner planner = new NereidsPlanner(statementContext);
// this is for expression column name infer when not use alias
LogicalSink<Plan> logicalSink = new UnboundResultSink<>(logicalQuery);
+ // must disable constant folding by be, because be constant folding
may return wrong type
+ ctx.getSessionVariable().disableConstantFoldingByBEOnce();
Plan plan = planner.plan(logicalSink, PhysicalProperties.ANY,
ExplainLevel.ALL_PLAN);
if (plan.anyMatch(node -> node instanceof OneRowRelation)) {
throw new AnalysisException("at least contain one table");
diff --git a/fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java
b/fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java
index fba4324a621..7c35abd3451 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java
@@ -3702,6 +3702,15 @@ public class SessionVariable implements Serializable,
Writable {
new
SetVar(SessionVariable.ENABLE_FALLBACK_TO_ORIGINAL_PLANNER, new
StringLiteral("true")));
}
+ public void disableConstantFoldingByBEOnce() throws DdlException {
+ if (!enableFoldConstantByBe) {
+ return;
+ }
+ setIsSingleSetVar(true);
+ VariableMgr.setVar(this,
+ new SetVar(SessionVariable.ENABLE_FOLD_CONSTANT_BY_BE, new
StringLiteral("false")));
+ }
+
public void disableNereidsPlannerOnce() throws DdlException {
if (!enableNereidsPlanner) {
return;
diff --git a/regression-test/data/nereids_p0/create_table/test_ctas.out
b/regression-test/data/nereids_p0/create_table/test_ctas.out
index 976a2ead90b..7d3765477d9 100644
--- a/regression-test/data/nereids_p0/create_table/test_ctas.out
+++ b/regression-test/data/nereids_p0/create_table/test_ctas.out
@@ -24,3 +24,6 @@ r2 {"title":"Amount","value":2.1}
-- !desc --
__substring_0 VARCHAR(30) Yes true \N
+-- !desc --
+__substring_0 VARCHAR(30) Yes true \N
+
diff --git a/regression-test/suites/nereids_p0/create_table/test_ctas.groovy
b/regression-test/suites/nereids_p0/create_table/test_ctas.groovy
index d415291d1ef..bb5b4f886a0 100644
--- a/regression-test/suites/nereids_p0/create_table/test_ctas.groovy
+++ b/regression-test/suites/nereids_p0/create_table/test_ctas.groovy
@@ -270,6 +270,11 @@ suite("nereids_test_ctas") {
sql """DROP TABLE IF EXISTS test_varchar_length"""
sql """set use_max_length_of_varchar_in_ctas = false"""
+ sql """set enable_fold_constant_by_be = false"""
+ sql """CREATE TABLE test_varchar_length properties ("replication_num"="1")
AS SELECT CAST("1" AS VARCHAR(30))"""
+ qt_desc """desc test_varchar_length"""
+ sql """DROP TABLE IF EXISTS test_varchar_length"""
+ sql """set enable_fold_constant_by_be = true"""
sql """CREATE TABLE test_varchar_length properties ("replication_num"="1")
AS SELECT CAST("1" AS VARCHAR(30))"""
qt_desc """desc test_varchar_length"""
sql """DROP TABLE IF EXISTS test_varchar_length"""
diff --git
a/regression-test/suites/nereids_rules_p0/adjust_nullable/agg_state_type.groovy
b/regression-test/suites/nereids_rules_p0/adjust_nullable/agg_state_type.groovy
new file mode 100644
index 00000000000..782306918af
--- /dev/null
+++
b/regression-test/suites/nereids_rules_p0/adjust_nullable/agg_state_type.groovy
@@ -0,0 +1,39 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_agg_state_type_adjust_nullable") {
+
+ sql """
+ DROP TABLE IF EXISTS test_agg_state_type
+ """
+
+ sql """
+ CREATE TABLE test_agg_state_type(`id` INT NOT NULL, `c1` INT NOT NULL)
DISTRIBUTED BY hash(id) PROPERTIES ("replication_num" = "1")
+ """
+
+ sql """
+ insert into test_agg_state_type values (1, 1)
+ """
+
+ sql """
+ select * from (select sum_state(c1) as c2, 1 as c1 from (select
avg(id) as c1 from test_agg_state_type) v) v join test_agg_state_type on v.c1 =
test_agg_state_type.id
+ """
+
+ sql """
+ DROP TABLE IF EXISTS test_agg_state_type
+ """
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
