You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by mo...@apache.org on 2024/07/10 09:13:42 UTC
(doris) branch master updated: [fix](agg_state) adjust nullable should apply on agg_state inner type too (#37489)
This is an automated email from the ASF dual-hosted git repository.
morrysnow pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 2ec7903e8e1 [fix](agg_state) adjust nullable should apply on agg_state inner type too (#37489)
2ec7903e8e1 is described below
commit 2ec7903e8e1bb642c5d9019d88c27f7fc09dacf8
Author: morrySnow <10...@users.noreply.github.com>
AuthorDate: Wed Jul 10 17:13:35 2024 +0800
[fix](agg_state) adjust nullable should apply on agg_state inner type too (#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 | 15 +++++++--
.../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, 112 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 7e5b92e5a27..d4f1e18111f 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
@@ -99,7 +99,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 62137a4d30c..26870bb92be 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
@@ -157,7 +157,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 b587a26864e..678753d46d7 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
@@ -18,6 +18,7 @@
package org.apache.doris.nereids.trees.expressions;
import org.apache.doris.nereids.trees.expressions.shape.LeafExpression;
+import org.apache.doris.nereids.types.DataType;
import com.google.common.collect.ImmutableList;
@@ -37,7 +38,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 b5fb57abf3c..992d1ea6623 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
@@ -246,11 +246,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, newNullable,
+ return new SlotReference(exprId, name, dataType, nullable,
+ qualifier, table, column, internalName, subPath);
+ }
+
+ @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);
}
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 b9b9eeb9b2f..77e9ae39c41 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
@@ -120,11 +120,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 b5bfefd0678..53d6fa1353e 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 4aa310cc56f..476c7a36533 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
@@ -3857,6 +3857,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-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org