From ee0b4c60b1ae084bba6fe5106f7d123ac5e1c9dd Mon Sep 17 00:00:00 2001 From: Xujian Duan <50550370+DarvenDuan@users.noreply.github.com> Date: Wed, 24 Jul 2024 19:02:00 +0800 Subject: [PATCH] [bug](delete) fix delete random distributed tbl (#37985) ## Proposed changes Bug description: In PR https://github.com/apache/doris/pull/33630, Doris supports auto aggregation for random distributed table, but it not only effects query statements, so if we delete from a random distributed table, will get an error because of unexpectedly rewriting. ``` CREATE TABLE `test_tbl` ( `k` INT NULL, `v` BIGINT SUM NULL ) ENGINE=OLAP AGGREGATE KEY(`k`) DISTRIBUTED BY RANDOM BUCKETS AUTO; mysql > delete from test_tbl where k=1; ERROR 1105 (HY000): errCode = 2, detailMessage = Where clause only supports compound predicate, binary predicate, is_null predicate or in predicate. ``` fix: Check whether it is a query statement before rewriting. --- .../BuildAggForRandomDistributedTable.java | 32 +++++++++++----- ...est_delete_from_random_distributed_tbl.out | 8 ++++ ..._delete_from_random_distributed_tbl.groovy | 38 +++++++++++++++++++ 3 files changed, 69 insertions(+), 9 deletions(-) create mode 100644 regression-test/data/delete_p0/test_delete_from_random_distributed_tbl.out create mode 100644 regression-test/suites/delete_p0/test_delete_from_random_distributed_tbl.groovy diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BuildAggForRandomDistributedTable.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BuildAggForRandomDistributedTable.java index 86c89e49d3d6a5..e547a55f9e39fe 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BuildAggForRandomDistributedTable.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BuildAggForRandomDistributedTable.java @@ -51,6 +51,7 @@ import org.apache.doris.nereids.trees.plans.logical.LogicalAggregate; import org.apache.doris.nereids.trees.plans.logical.LogicalOlapScan; import org.apache.doris.nereids.trees.plans.logical.LogicalPlan; +import org.apache.doris.qe.ConnectContext; import com.google.common.collect.ImmutableList; @@ -67,22 +68,29 @@ public class BuildAggForRandomDistributedTable implements AnalysisRuleFactory { public List buildRules() { return ImmutableList.of( // Project(Scan) -> project(agg(scan)) - logicalProject(logicalOlapScan()).when(project -> isRandomDistributedTbl(project.child())) + logicalProject(logicalOlapScan()) + .when(this::isQuery) + .when(project -> isRandomDistributedTbl(project.child())) .then(project -> preAggForRandomDistribution(project, project.child())) .toRule(RuleType.BUILD_AGG_FOR_RANDOM_DISTRIBUTED_TABLE_PROJECT_SCAN), // agg(scan) -> agg(agg(scan)), agg(agg) may optimized by MergeAggregate - logicalAggregate(logicalOlapScan()).when(agg -> isRandomDistributedTbl(agg.child())).whenNot(agg -> { - Set functions = agg.getAggregateFunctions(); - List groupByExprs = agg.getGroupByExpressions(); - // check if need generate an inner agg plan or not - // should not rewrite twice if we had rewritten olapScan to aggregate(olapScan) - return functions.stream().allMatch(this::aggTypeMatch) && groupByExprs.stream() + logicalAggregate(logicalOlapScan()) + .when(this::isQuery) + .when(agg -> isRandomDistributedTbl(agg.child())) + .whenNot(agg -> { + Set functions = agg.getAggregateFunctions(); + List groupByExprs = agg.getGroupByExpressions(); + // check if need generate an inner agg plan or not + // should not rewrite twice if we had rewritten olapScan to aggregate(olapScan) + return functions.stream().allMatch(this::aggTypeMatch) && groupByExprs.stream() .allMatch(this::isKeyOrConstantExpr); - }) + }) .then(agg -> preAggForRandomDistribution(agg, agg.child())) .toRule(RuleType.BUILD_AGG_FOR_RANDOM_DISTRIBUTED_TABLE_AGG_SCAN), // filter(scan) -> filter(agg(scan)) - logicalFilter(logicalOlapScan()).when(filter -> isRandomDistributedTbl(filter.child())) + logicalFilter(logicalOlapScan()) + .when(this::isQuery) + .when(filter -> isRandomDistributedTbl(filter.child())) .then(filter -> preAggForRandomDistribution(filter, filter.child())) .toRule(RuleType.BUILD_AGG_FOR_RANDOM_DISTRIBUTED_TABLE_FILTER_SCAN)); @@ -101,6 +109,12 @@ private boolean isRandomDistributedTbl(LogicalOlapScan olapScan) { return keysType == KeysType.AGG_KEYS && distributionInfo.getType() == DistributionInfoType.RANDOM; } + private boolean isQuery(LogicalPlan plan) { + return ConnectContext.get() != null + && ConnectContext.get().getState() != null + && ConnectContext.get().getState().isQuery(); + } + /** * add LogicalAggregate above olapScan for preAgg * diff --git a/regression-test/data/delete_p0/test_delete_from_random_distributed_tbl.out b/regression-test/data/delete_p0/test_delete_from_random_distributed_tbl.out new file mode 100644 index 00000000000000..5d73997faa32cc --- /dev/null +++ b/regression-test/data/delete_p0/test_delete_from_random_distributed_tbl.out @@ -0,0 +1,8 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !sql -- +1 10 +2 30 + +-- !sql -- +2 30 + diff --git a/regression-test/suites/delete_p0/test_delete_from_random_distributed_tbl.groovy b/regression-test/suites/delete_p0/test_delete_from_random_distributed_tbl.groovy new file mode 100644 index 00000000000000..17f81df776de9c --- /dev/null +++ b/regression-test/suites/delete_p0/test_delete_from_random_distributed_tbl.groovy @@ -0,0 +1,38 @@ +// 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_delete_from_random_distributed_tbl") { + def tableName = "test_delete_from_random_distributed_tbl" + + sql """ DROP TABLE IF EXISTS ${tableName}; """ + sql """ CREATE TABLE ${tableName} ( + `k` INT NULL, + `v` BIGINT SUM NULL + ) ENGINE=OLAP + AGGREGATE KEY(`k`) + DISTRIBUTED BY RANDOM BUCKETS 4 + PROPERTIES ("replication_num"="1") + """ + + sql """ insert into ${tableName} values(1, 10),(2,10),(2,20)""" + qt_sql """ select * from ${tableName} order by k """ + + sql """ delete from ${tableName} where k=1 """ + qt_sql """ select * from ${tableName} order by k """ + + sql """ DROP TABLE IF EXISTS ${tableName}; """ +}