morrySnow commented on code in PR #68320: URL: https://github.com/apache/doris/pull/68320#discussion_r4129136915
########## fe/fe-catalog/src/main/java/org/apache/doris/analysis/ShortCircuitFunctionCallExpr.java: ########## @@ -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. + +package org.apache.doris.analysis; + +import org.apache.doris.catalog.Function; + +/** Function call whose unselected arguments must not be evaluated. */ +public final class ShortCircuitFunctionCallExpr extends FunctionCallExpr { Review Comment: 这里，在fe侧，直接实现成expr上的一个成员变量 ########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/CaseWhenToCompoundPredicate.java: ########## @@ -70,7 +71,12 @@ public List<ExpressionPatternMatcher<? extends Expression>> buildRules() { } private boolean checkBooleanType(Expression expression) { - return expression.getDataType().isBooleanType(); + return expression.getDataType().isBooleanType() + && !requiresShortCircuitEvaluation(expression); + } Review Comment: 这个 requiresShortCircuitEvaluation 不应当加到 checkBooleanType 中，而是另外写一个单独的check检查 ########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/FoldConstantRuleOnFE.java: ########## @@ -665,7 +666,9 @@ public Expression visitCaseWhen(CaseWhen caseWhen, ExpressionRewriteContext cont @Override public Expression visitIf(If ifExpr, ExpressionRewriteContext context) { If originIf = ifExpr; - ifExpr = rewriteChildren(ifExpr, context); + if (!(ifExpr instanceof RequiresShortCircuitEvaluation)) { + ifExpr = rewriteChildren(ifExpr, context); + } Review Comment: 应当在 visitor 中，增加一个 visitShortCircuitIf，然后这里实现这个visit， 而不是在这里增加if。 ########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/ExpressionBottomUpRewriter.java: ########## @@ -97,7 +103,10 @@ private static Expression rewriteBottomUp( if (changed) { afterRewrite = applied.get(); // ensure children are rewritten - afterRewrite = rewriteChildren(afterRewrite, context, currentBatch, rules, listeners); + if (!(afterRewrite instanceof RequiresShortCircuitEvaluation)) { + afterRewrite = rewriteChildren( + afterRewrite, context, currentBatch, rules, listeners); + } Review Comment: 不允许rewrite children说不通呢？应该是不允许rewrite 当前函数就可以？ ########## fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/CommonSubExpressionCollector.java: ########## @@ -44,6 +45,9 @@ public int collect(Expression expr) { @Override public Integer visit(Expression expr, Boolean inLambda) { + if (expr instanceof RequiresShortCircuitEvaluation) { + return 0; + } Review Comment: 每个孩子的内部还可以做cse的优化，所以不应当直接返回 ########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/CaseWhenToCompoundPredicate.java: ########## @@ -70,7 +71,12 @@ public List<ExpressionPatternMatcher<? extends Expression>> buildRules() { } private boolean checkBooleanType(Expression expression) { - return expression.getDataType().isBooleanType(); + return expression.getDataType().isBooleanType() + && !requiresShortCircuitEvaluation(expression); + } + + private static boolean requiresShortCircuitEvaluation(Expression expression) { + return expression.anyMatch(node -> node instanceof RequiresShortCircuitEvaluation); Review Comment: 这个规则改动的有些粗暴，可以更细粒度的处理 ########## fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java: ########## @@ -2188,7 +2188,8 @@ public LogicalPlan visitDelete(DeleteContext ctx) { cte = Optional.ofNullable(withCte(query, ctx.cteContext)); } deleteCommand = new DeleteFromUsingCommand(tableName, tableAlias, - partitionSpec.first, partitionSpec.second, query, cte, hasQueryOrganization); + partitionSpec.first, partitionSpec.second, query, cte, + hasQueryOrganization); Review Comment: 没有必要的修改 -- 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]
