From 8409f24062c868280f5fc4e9be4a73b5a24ff48a Mon Sep 17 00:00:00 2001 From: LiBinfeng <46676950+LiBinfeng-01@users.noreply.github.com> Date: Fri, 18 Oct 2024 20:34:03 +0800 Subject: [PATCH] [fix](Nereids) fix fold constant by be return type mismatched (#39723)(#41164)(#41331)(#41546) (#41838) cherry-pick: #39723 #41164 #41331 #41546 because later problem is intro by prev one, so put them together when using fold constant by be, the return type of substring('123456',1, 3) would changed to be text, which we want it to be 3 remove windowframe in window expression to avoid folding constant on be --- .../nereids/parser/LogicalPlanBuilder.java | 4 +- .../nereids/properties/SelectHintSetVar.java | 41 +++++++++ .../analysis/EliminateLogicalSelectHint.java | 40 +-------- .../rules/FoldConstantRuleOnBE.java | 18 ++-- .../trees/expressions/WindowExpression.java | 4 +- .../translator/ExpressionTranslatorTest.java | 3 +- .../nereids/preprocess/SelectHintTest.java | 88 ------------------- .../apache/doris/qe/SessionVariablesTest.java | 8 ++ .../fold_constant/fold_constant_by_be.groovy | 8 +- .../nereids_syntax_p0/system_var.groovy | 2 +- 10 files changed, 76 insertions(+), 140 deletions(-) delete mode 100644 fe/fe-core/src/test/java/org/apache/doris/nereids/preprocess/SelectHintTest.java diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java index 05e8feabd7..3a11d5307e 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java @@ -3140,7 +3140,9 @@ public class LogicalPlanBuilder extends DorisParserBaseVisitor { parameters.put(parameterName, value); } } - hints.add(new SelectHintSetVar(hintName, parameters)); + SelectHintSetVar setVar = new SelectHintSetVar(hintName, parameters); + setVar.setVarOnceInSql(ConnectContext.get().getStatementContext()); + hints.add(setVar); break; case "leading": List leadingParameters = new ArrayList(); diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/properties/SelectHintSetVar.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/properties/SelectHintSetVar.java index 8c08dcf378..231e2fdbb9 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/properties/SelectHintSetVar.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/properties/SelectHintSetVar.java @@ -17,6 +17,13 @@ package org.apache.doris.nereids.properties; +import org.apache.doris.analysis.SetVar; +import org.apache.doris.analysis.StringLiteral; +import org.apache.doris.nereids.StatementContext; +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.qe.SessionVariable; +import org.apache.doris.qe.VariableMgr; + import java.util.Map; import java.util.Optional; import java.util.stream.Collectors; @@ -38,6 +45,40 @@ public class SelectHintSetVar extends SelectHint { return parameters; } + /** + * set session variable in sql level + * @param context statement context + */ + public void setVarOnceInSql(StatementContext context) { + SessionVariable sessionVariable = context.getConnectContext().getSessionVariable(); + // set temporary session value, and then revert value in the 'finally block' of StmtExecutor#execute + sessionVariable.setIsSingleSetVar(true); + for (Map.Entry> kv : getParameters().entrySet()) { + String key = kv.getKey(); + Optional value = kv.getValue(); + if (value.isPresent()) { + try { + VariableMgr.setVar(sessionVariable, new SetVar(key, new StringLiteral(value.get()))); + context.invalidCache(key); + } catch (Throwable t) { + throw new AnalysisException("Can not set session variable '" + + key + "' = '" + value.get() + "'", t); + } + } + } + // if sv set enable_nereids_planner=true and hint set enable_nereids_planner=false, we should set + // enable_fallback_to_original_planner=true and revert it after executing. + // throw exception to fall back to original planner + if (!sessionVariable.isEnableNereidsPlanner()) { + try { + sessionVariable.enableFallbackToOriginalPlannerOnce(); + } catch (Throwable t) { + throw new AnalysisException("failed to set fallback to original planner to true", t); + } + throw new AnalysisException("The nereids is disabled in this sql, fallback to original planner"); + } + } + @Override public String toString() { String kvString = parameters diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/EliminateLogicalSelectHint.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/EliminateLogicalSelectHint.java index 13eee57637..35081a9b61 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/EliminateLogicalSelectHint.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/EliminateLogicalSelectHint.java @@ -17,12 +17,9 @@ package org.apache.doris.nereids.rules.analysis; -import org.apache.doris.analysis.SetVar; -import org.apache.doris.analysis.StringLiteral; import org.apache.doris.common.DdlException; import org.apache.doris.nereids.CascadesContext; import org.apache.doris.nereids.StatementContext; -import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.hint.Hint; import org.apache.doris.nereids.hint.LeadingHint; import org.apache.doris.nereids.hint.OrderedHint; @@ -39,15 +36,10 @@ import org.apache.doris.nereids.rules.rewrite.OneRewriteRuleFactory; import org.apache.doris.nereids.trees.plans.Plan; import org.apache.doris.nereids.trees.plans.logical.LogicalSelectHint; import org.apache.doris.qe.ConnectContext; -import org.apache.doris.qe.SessionVariable; -import org.apache.doris.qe.VariableMgr; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.util.Map.Entry; -import java.util.Optional; - /** * eliminate logical select hint and set them to cascade context */ @@ -61,7 +53,7 @@ public class EliminateLogicalSelectHint extends OneRewriteRuleFactory { for (SelectHint hint : selectHintPlan.getHints()) { String hintName = hint.getHintName(); if (hintName.equalsIgnoreCase("SET_VAR")) { - setVar((SelectHintSetVar) hint, ctx.statementContext); + ((SelectHintSetVar) hint).setVarOnceInSql(ctx.statementContext); } else if (hintName.equalsIgnoreCase("ORDERED")) { try { ctx.cascadesContext.getConnectContext().getSessionVariable() @@ -90,36 +82,6 @@ public class EliminateLogicalSelectHint extends OneRewriteRuleFactory { }).toRule(RuleType.ELIMINATE_LOGICAL_SELECT_HINT); } - private void setVar(SelectHintSetVar selectHint, StatementContext context) { - SessionVariable sessionVariable = context.getConnectContext().getSessionVariable(); - // set temporary session value, and then revert value in the 'finally block' of StmtExecutor#execute - sessionVariable.setIsSingleSetVar(true); - for (Entry> kv : selectHint.getParameters().entrySet()) { - String key = kv.getKey(); - Optional value = kv.getValue(); - if (value.isPresent()) { - try { - VariableMgr.setVar(sessionVariable, new SetVar(key, new StringLiteral(value.get()))); - context.invalidCache(key); - } catch (Throwable t) { - throw new AnalysisException("Can not set session variable '" - + key + "' = '" + value.get() + "'", t); - } - } - } - // if sv set enable_nereids_planner=true and hint set enable_nereids_planner=false, we should set - // enable_fallback_to_original_planner=true and revert it after executing. - // throw exception to fall back to original planner - if (!sessionVariable.isEnableNereidsPlanner()) { - try { - sessionVariable.enableFallbackToOriginalPlannerOnce(); - } catch (Throwable t) { - throw new AnalysisException("failed to set fallback to original planner to true", t); - } - throw new AnalysisException("The nereids is disabled in this sql, fallback to original planner"); - } - } - private void extractLeading(SelectHintLeading selectHint, CascadesContext context, StatementContext statementContext, LogicalSelectHint selectHintPlan) { LeadingHint hint = new LeadingHint("Leading", selectHint.getParameters(), selectHint.toString()); diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/FoldConstantRuleOnBE.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/FoldConstantRuleOnBE.java index 2852c1c441..c614b50c80 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/FoldConstantRuleOnBE.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/FoldConstantRuleOnBE.java @@ -175,14 +175,23 @@ public class FoldConstantRuleOnBE implements ExpressionPatternRuleFactory { if (newChild != child) { hasNewChildren = true; } - newChildren.add(newChild); + if (!newChild.getDataType().equals(child.getDataType())) { + try { + newChildren.add(newChild.castTo(child.getDataType())); + } catch (Exception e) { + LOG.warn("expression of type {} cast to {} failed. ", newChild.getDataType(), child.getDataType()); + newChildren.add(newChild); + } + } else { + newChildren.add(newChild); + } } return hasNewChildren ? root.withChildren(newChildren) : root; } private static void collectConst(Expression expr, Map constMap, Map tExprMap, IdGenerator idGenerator) { - if (expr.isConstant() && !shouldSkipFold(expr)) { + if (expr.isConstant() && !expr.isLiteral() && !expr.anyMatch(e -> shouldSkipFold((Expression) e))) { String id = idGenerator.getNextId().toString(); constMap.put(id, expr); Expr staleExpr; @@ -208,11 +217,6 @@ public class FoldConstantRuleOnBE implements ExpressionPatternRuleFactory { // Some expressions should not do constant folding private static boolean shouldSkipFold(Expression expr) { - // Skip literal expr - if (expr.isLiteral()) { - return true; - } - // Frontend can not represent those types if (expr.getDataType().isAggStateType() || expr.getDataType().isObjectType() || expr.getDataType().isVariantType() || expr.getDataType().isTimeLikeType() diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/WindowExpression.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/WindowExpression.java index 457c6d3a64..5bea07fff0 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/WindowExpression.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/WindowExpression.java @@ -66,7 +66,6 @@ public class WindowExpression extends Expression { .add(function) .addAll(partitionKeys) .addAll(orderKeys) - .add(windowFrame) .build()); this.function = function; this.partitionKeys = ImmutableList.copyOf(partitionKeys); @@ -153,6 +152,9 @@ public class WindowExpression extends Expression { if (index < children.size()) { return new WindowExpression(func, partitionKeys, orderKeys, (WindowFrame) children.get(index)); } + if (windowFrame.isPresent()) { + return new WindowExpression(func, partitionKeys, orderKeys, windowFrame.get()); + } return new WindowExpression(func, partitionKeys, orderKeys); } diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/ExpressionTranslatorTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/ExpressionTranslatorTest.java index cdea9eea5d..22add37d8e 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/ExpressionTranslatorTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/ExpressionTranslatorTest.java @@ -23,7 +23,6 @@ import org.apache.doris.analysis.Expr; import org.apache.doris.analysis.IntLiteral; import org.apache.doris.catalog.Function.NullableMode; import org.apache.doris.catalog.Type; -import org.apache.doris.common.AnalysisException; import org.apache.doris.nereids.trees.expressions.BitNot; import org.apache.doris.nereids.trees.expressions.literal.IntegerLiteral; @@ -33,7 +32,7 @@ import org.junit.jupiter.api.Test; public class ExpressionTranslatorTest { @Test - public void testUnaryArithmetic() throws AnalysisException { + public void testUnaryArithmetic() throws Exception { BitNot bitNot = new BitNot(new IntegerLiteral(1)); ExpressionTranslator translator = ExpressionTranslator.INSTANCE; Expr actual = translator.visitUnaryArithmetic(bitNot, null); diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/preprocess/SelectHintTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/preprocess/SelectHintTest.java deleted file mode 100644 index 62965ac27b..0000000000 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/preprocess/SelectHintTest.java +++ /dev/null @@ -1,88 +0,0 @@ -// 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.nereids.preprocess; - -import org.apache.doris.catalog.Env; -import org.apache.doris.nereids.NereidsPlanner; -import org.apache.doris.nereids.StatementContext; -import org.apache.doris.nereids.exceptions.AnalysisException; -import org.apache.doris.nereids.parser.NereidsParser; -import org.apache.doris.nereids.properties.PhysicalProperties; -import org.apache.doris.qe.ConnectContext; -import org.apache.doris.qe.OriginStatement; -import org.apache.doris.qe.SessionVariable; -import org.apache.doris.qe.StmtExecutor; -import org.apache.doris.thrift.TUniqueId; - -import mockit.Expectations; -import mockit.Mock; -import mockit.MockUp; -import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.BeforeAll; -import org.junit.jupiter.api.Test; - -public class SelectHintTest { - - @BeforeAll - public static void init() { - ConnectContext ctx = new ConnectContext(); - new MockUp() { - @Mock - public ConnectContext get() { - return ctx; - } - }; - new MockUp() { - @Mock - public boolean isMaster() { - return true; - } - }; - } - - @Test - public void testFallbackToOriginalPlanner() throws Exception { - String sql = " SELECT /*+ SET_VAR(enable_nereids_planner=\"false\") */ 1"; - - ConnectContext ctx = new ConnectContext(); - ctx.setEnv(Env.getCurrentEnv()); - StatementContext statementContext = new StatementContext(ctx, new OriginStatement(sql, 0)); - SessionVariable sv = ctx.getSessionVariable(); - Assertions.assertNotNull(sv); - sv.setEnableNereidsPlanner(true); - sv.enableFallbackToOriginalPlanner = false; - Assertions.assertThrows(AnalysisException.class, () -> new NereidsPlanner(statementContext) - .planWithLock(new NereidsParser().parseSingle(sql), PhysicalProperties.ANY)); - - // manually recover sv - sv.setEnableNereidsPlanner(true); - sv.enableFallbackToOriginalPlanner = false; - StmtExecutor stmtExecutor = new StmtExecutor(ctx, sql); - - new Expectations(stmtExecutor) { - { - stmtExecutor.executeByLegacy((TUniqueId) any); - } - }; - - stmtExecutor.execute(); - - Assertions.assertTrue(sv.isEnableNereidsPlanner()); - Assertions.assertFalse(sv.enableFallbackToOriginalPlanner); - } -} diff --git a/fe/fe-core/src/test/java/org/apache/doris/qe/SessionVariablesTest.java b/fe/fe-core/src/test/java/org/apache/doris/qe/SessionVariablesTest.java index bad7842e01..541b474c74 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/qe/SessionVariablesTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/qe/SessionVariablesTest.java @@ -27,6 +27,7 @@ import org.apache.doris.common.PatternMatcher; import org.apache.doris.common.PatternMatcherWrapper; import org.apache.doris.common.VariableAnnotation; import org.apache.doris.common.util.ProfileManager; +import org.apache.doris.nereids.parser.NereidsParser; import org.apache.doris.thrift.TQueryOptions; import org.apache.doris.utframe.TestWithFeService; @@ -168,4 +169,11 @@ public class SessionVariablesTest extends TestWithFeService { Assertions.assertEquals("test", sessionVariableClone.getSessionOriginValue().get(txIsolationSessionVariableField)); } + + @Test + public void testSetVarInHint() { + String sql = "insert into test_t1 select /*+ set_var(enable_nereids_dml_with_pipeline=false)*/ * from test_t1 where enable_nereids_dml_with_pipeline=true"; + new NereidsParser().parseSQL(sql); + Assertions.assertEquals(false, connectContext.getSessionVariable().enableNereidsDmlWithPipeline); + } } diff --git a/regression-test/suites/nereids_p0/expression/fold_constant/fold_constant_by_be.groovy b/regression-test/suites/nereids_p0/expression/fold_constant/fold_constant_by_be.groovy index 9f306860df..882b84b776 100644 --- a/regression-test/suites/nereids_p0/expression/fold_constant/fold_constant_by_be.groovy +++ b/regression-test/suites/nereids_p0/expression/fold_constant/fold_constant_by_be.groovy @@ -48,5 +48,11 @@ suite("fold_constant_by_be") { qt_sql "explain select sleep(sign(1)*100);" sql 'set query_timeout=12;' - qt_sql "select sleep(sign(1)*5);" + qt_sql "select sleep(sign(1)*10);" + + explain { + sql("verbose select substring('123456', 1, 3)") + contains "varchar(3)" + } + } diff --git a/regression-test/suites/nereids_syntax_p0/system_var.groovy b/regression-test/suites/nereids_syntax_p0/system_var.groovy index 242df6ec94..8f1270702b 100644 --- a/regression-test/suites/nereids_syntax_p0/system_var.groovy +++ b/regression-test/suites/nereids_syntax_p0/system_var.groovy @@ -38,7 +38,7 @@ suite("nereids_sys_var") { // set an invalid parameter, and throw an exception test { sql "select /*+SET_VAR(runtime_filter_type=10000)*/ * from supplier limit 10" - exception "Can not set session variable" + exception "errCode" } sql "select @@session.time_zone"