From d87ab69eadb01f6db5688abb43c85ac4888a2c48 Mon Sep 17 00:00:00 2001 From: HappenLee Date: Wed, 24 Aug 2022 16:19:43 +0800 Subject: [PATCH] [bug](vectorized) fix bug of tuple is null null side do not set (#12012) --- .../org/apache/doris/analysis/Analyzer.java | 23 +++++++ .../apache/doris/analysis/InlineViewRef.java | 7 ++- .../org/apache/doris/analysis/TableRef.java | 2 + .../doris/analysis/TupleIsNullPredicate.java | 62 +++---------------- .../apache/doris/planner/HashJoinNode.java | 6 +- .../doris/planner/SingleNodePlanner.java | 13 +++- .../analysis/TupleIsNullPredicateTest.java | 2 +- .../data/query/keyword/test_keyword.out | 7 +++ .../suites/query/keyword/test_keyword.groovy | 4 +- 9 files changed, 63 insertions(+), 63 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/Analyzer.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/Analyzer.java index 7c72c937cf..624a108ff6 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/Analyzer.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/Analyzer.java @@ -289,6 +289,11 @@ public class Analyzer { // to the last Join clause (represented by its rhs table ref) that outer-joined it private final Map outerJoinedTupleIds = Maps.newHashMap(); + // set of left side and right side of tuple id to mark null side in vec + // exec engine + private final Set outerLeftSideJoinTupleIds = Sets.newHashSet(); + private final Set outerRightSideJoinTupleIds = Sets.newHashSet(); + // Map of registered conjunct to the last full outer join (represented by its // rhs table ref) that outer joined it. public final Map fullOuterJoinedConjuncts = Maps.newHashMap(); @@ -1001,6 +1006,16 @@ public class Analyzer { } } + public void registerOuterJoinedRightSideTids(List tids) { + globalState.outerRightSideJoinTupleIds.addAll(tids); + } + + public void registerOuterJoinedLeftSideTids(List tids) { + globalState.outerLeftSideJoinTupleIds.addAll(tids); + } + + + /** * Register the given tuple id as being the invisible side of a semi-join. */ @@ -2233,6 +2248,14 @@ public class Analyzer { return globalState.outerJoinedTupleIds.containsKey(tid); } + public boolean isOuterJoinedLeftSide(TupleId tid) { + return globalState.outerLeftSideJoinTupleIds.contains(tid); + } + + public boolean isOuterJoinedRightSide(TupleId tid) { + return globalState.outerRightSideJoinTupleIds.contains(tid); + } + public boolean isInlineView(TupleId tid) { return globalState.inlineViewTupleIds.contains(tid); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/InlineViewRef.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/InlineViewRef.java index 032fe0b260..2fcf3681f8 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/InlineViewRef.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/InlineViewRef.java @@ -28,6 +28,7 @@ import org.apache.doris.common.ErrorCode; import org.apache.doris.common.ErrorReport; import org.apache.doris.common.UserException; import org.apache.doris.rewrite.ExprRewriter; +import org.apache.doris.thrift.TNullSide; import com.google.common.base.Preconditions; import com.google.common.collect.Lists; @@ -364,7 +365,11 @@ public class InlineViewRef extends TableRef { if (!requiresNullWrapping(analyzer, smap.getRhs().get(i), nullSMap)) { continue; } - params.add(new TupleIsNullPredicate(materializedTupleIds)); + if (analyzer.isOuterJoinedLeftSide(materializedTupleIds.get(0))) { + params.add(new TupleIsNullPredicate(materializedTupleIds, TNullSide.LEFT)); + } else { + params.add(new TupleIsNullPredicate(materializedTupleIds, TNullSide.RIGHT)); + } params.add(NullLiteral.create(smap.getRhs().get(i).getType())); params.add(smap.getRhs().get(i)); Expr ifExpr = new FunctionCallExpr("if", params); diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/TableRef.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/TableRef.java index 98a252c27e..ee7cdfb86c 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/TableRef.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/TableRef.java @@ -490,10 +490,12 @@ public class TableRef implements ParseNode, Writable { if (joinOp == JoinOperator.LEFT_OUTER_JOIN || joinOp == JoinOperator.FULL_OUTER_JOIN) { analyzer.registerOuterJoinedTids(getId().asList(), this); + analyzer.registerOuterJoinedRightSideTids(getId().asList()); } if (joinOp == JoinOperator.RIGHT_OUTER_JOIN || joinOp == JoinOperator.FULL_OUTER_JOIN) { analyzer.registerOuterJoinedTids(leftTblRef.getAllTableRefIds(), this); + analyzer.registerOuterJoinedLeftSideTids(leftTblRef.getAllTableRefIds()); } // register the tuple ids of a full outer join if (joinOp == JoinOperator.FULL_OUTER_JOIN) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/TupleIsNullPredicate.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/TupleIsNullPredicate.java index fd92081afd..2f5de7d4fc 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/TupleIsNullPredicate.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/TupleIsNullPredicate.java @@ -43,14 +43,13 @@ import java.util.Objects; */ public class TupleIsNullPredicate extends Predicate { private List tupleIds = Lists.newArrayList(); + // Only effective in vectorized exec engine to mark null side, + // can set null in origin exec engine private TNullSide nullSide = null; - public TupleIsNullPredicate(List tupleIds) { - Preconditions.checkState(tupleIds != null && !tupleIds.isEmpty()); + public TupleIsNullPredicate(List tupleIds, TNullSide nullSide) { + Preconditions.checkState(tupleIds != null && (!tupleIds.isEmpty() || nullSide != null)); this.tupleIds.addAll(tupleIds); - } - - public TupleIsNullPredicate(TNullSide nullSide) { this.nullSide = nullSide; } @@ -131,7 +130,7 @@ public class TupleIsNullPredicate extends Predicate { * Returns a new list with the nullable exprs. */ public static List wrapExprs(List inputExprs, - List tids, Analyzer analyzer) throws UserException { + List tids, TNullSide nullSide, Analyzer analyzer) throws UserException { // Assert that all tids are materialized. for (TupleId tid : tids) { TupleDescriptor tupleDesc = analyzer.getTupleDesc(tid); @@ -140,42 +139,23 @@ public class TupleIsNullPredicate extends Predicate { // Perform the wrapping. List result = Lists.newArrayListWithCapacity(inputExprs.size()); for (Expr e : inputExprs) { - result.add(wrapExpr(e, tids, analyzer)); + result.add(wrapExpr(e, tids, nullSide, analyzer)); } return result; } - /** - * Makes each input expr nullable, if necessary, by wrapping it as follows: - * IF(TupleIsNull(nullSide), NULL, expr) - *

- * The given inputExprs are expected to be bound - * by null side tuple id once fully substituted against base tables. However, inputExprs may not yet - * be fully substituted at this point. - *

- * Returns a new list with the nullable exprs. only use in vectorized exec engine - */ - public static List wrapExprs(List inputExprs, - TNullSide nullSide, Analyzer analyzer) throws UserException { - // Perform the wrapping. - List result = Lists.newArrayListWithCapacity(inputExprs.size()); - for (Expr e : inputExprs) { - result.add(wrapExpr(e, nullSide, analyzer)); - } - return result; - } /** * Returns a new analyzed conditional expr 'IF(TupleIsNull(tids), NULL, expr)', * if required to make expr nullable. Otherwise, returns expr. */ - public static Expr wrapExpr(Expr expr, List tids, Analyzer analyzer) + public static Expr wrapExpr(Expr expr, List tids, TNullSide nullSide, Analyzer analyzer) throws UserException { if (!requiresNullWrapping(expr, analyzer)) { return expr; } List params = Lists.newArrayList(); - params.add(new TupleIsNullPredicate(tids)); + params.add(new TupleIsNullPredicate(tids, nullSide)); params.add(new NullLiteral()); params.add(expr); Expr ifExpr = new FunctionCallExpr("if", params); @@ -192,32 +172,6 @@ public class TupleIsNullPredicate extends Predicate { return ifExpr; } - /** - * Returns a new analyzed conditional expr 'IF(TupleIsNull(nullSide), NULL, expr)', - * if required to make expr nullable. Otherwise, returns expr. only use in vectorized exec engine - */ - public static Expr wrapExpr(Expr expr, TNullSide nullSide, Analyzer analyzer) - throws UserException { - if (!requiresNullWrapping(expr, analyzer)) { - return expr; - } - List params = Lists.newArrayList(); - params.add(new TupleIsNullPredicate(nullSide)); - params.add(new NullLiteral()); - params.add(expr); - Expr ifExpr = new FunctionCallExpr("if", params); - ifExpr.analyzeNoThrow(analyzer); - // The type of function which is different from the type of expr will return the incorrect result in query. - // Example: - // the type of expr is date - // the type of function is int - // So, the upper fragment will receive a int value instead of date while the result expr is date. - // If there is no cast function, the result of query will be incorrect. - if (expr.getType().getPrimitiveType() != ifExpr.getType().getPrimitiveType()) { - ifExpr = ifExpr.uncheckedCastTo(expr.getType()); - } - return ifExpr; - } /** * Returns true if the given expr evaluates to a non-NULL value if all its contained diff --git a/fe/fe-core/src/main/java/org/apache/doris/planner/HashJoinNode.java b/fe/fe-core/src/main/java/org/apache/doris/planner/HashJoinNode.java index 3221b542e8..b856eec20a 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/planner/HashJoinNode.java +++ b/fe/fe-core/src/main/java/org/apache/doris/planner/HashJoinNode.java @@ -505,8 +505,8 @@ public class HashJoinNode extends PlanNode { // Then: add tuple is null in left child columns if (leftNullable && getChild(0).tblRefIds.size() == 1 && analyzer.isInlineView(getChild(0).tblRefIds.get(0))) { List tupleIsNullLhs = TupleIsNullPredicate - .wrapExprs(vSrcToOutputSMap.getLhs().subList(0, leftNullableNumber), TNullSide.LEFT, - analyzer); + .wrapExprs(vSrcToOutputSMap.getLhs().subList(0, leftNullableNumber), new ArrayList<>(), + TNullSide.LEFT, analyzer); tupleIsNullLhs .addAll(vSrcToOutputSMap.getLhs().subList(leftNullableNumber, vSrcToOutputSMap.getLhs().size())); vSrcToOutputSMap.updateLhsExprs(tupleIsNullLhs); @@ -519,7 +519,7 @@ public class HashJoinNode extends PlanNode { int rightBeginIndex = vSrcToOutputSMap.size() - rightNullableNumber; List tupleIsNullLhs = TupleIsNullPredicate .wrapExprs(vSrcToOutputSMap.getLhs().subList(rightBeginIndex, vSrcToOutputSMap.size()), - TNullSide.RIGHT, analyzer); + new ArrayList<>(), TNullSide.RIGHT, analyzer); List newLhsList = Lists.newArrayList(); if (rightBeginIndex > 0) { newLhsList.addAll(vSrcToOutputSMap.getLhs().subList(0, rightBeginIndex)); diff --git a/fe/fe-core/src/main/java/org/apache/doris/planner/SingleNodePlanner.java b/fe/fe-core/src/main/java/org/apache/doris/planner/SingleNodePlanner.java index 1ebb5ad1a7..b93f7f15c0 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/planner/SingleNodePlanner.java +++ b/fe/fe-core/src/main/java/org/apache/doris/planner/SingleNodePlanner.java @@ -65,6 +65,7 @@ import org.apache.doris.common.Reference; import org.apache.doris.common.UserException; import org.apache.doris.common.util.VectorizedUtil; import org.apache.doris.planner.external.ExternalFileScanNode; +import org.apache.doris.thrift.TNullSide; import com.google.common.base.Preconditions; import com.google.common.base.Predicate; @@ -1393,8 +1394,14 @@ public class SingleNodePlanner { //set outputSmap to substitute literal in outputExpr unionNode.setWithoutTupleIsNullOutputSmap(inlineViewRef.getSmap()); if (analyzer.isOuterJoined(inlineViewRef.getId())) { - List nullableRhs = TupleIsNullPredicate.wrapExprs( - inlineViewRef.getSmap().getRhs(), unionNode.getTupleIds(), analyzer); + List nullableRhs; + if (analyzer.isOuterJoinedLeftSide(inlineViewRef.getId())) { + nullableRhs = TupleIsNullPredicate.wrapExprs(inlineViewRef.getSmap().getRhs(), + unionNode.getTupleIds(), TNullSide.LEFT, analyzer); + } else { + nullableRhs = TupleIsNullPredicate.wrapExprs(inlineViewRef.getSmap().getRhs(), + unionNode.getTupleIds(), TNullSide.RIGHT, analyzer); + } unionNode.setOutputSmap(new ExprSubstitutionMap(inlineViewRef.getSmap().getLhs(), nullableRhs)); } return unionNode; @@ -1422,7 +1429,7 @@ public class SingleNodePlanner { // because the rhs exprs must first be resolved against the physical output of // 'planRoot' to correctly determine whether wrapping is necessary. List nullableRhs = TupleIsNullPredicate.wrapExprs( - outputSmap.getRhs(), rootNode.getTupleIds(), analyzer); + outputSmap.getRhs(), rootNode.getTupleIds(), null, analyzer); outputSmap = new ExprSubstitutionMap(outputSmap.getLhs(), nullableRhs); } // Set output smap of rootNode *before* creating a SelectNode for proper resolution. diff --git a/fe/fe-core/src/test/java/org/apache/doris/analysis/TupleIsNullPredicateTest.java b/fe/fe-core/src/test/java/org/apache/doris/analysis/TupleIsNullPredicateTest.java index 4fc39ebf8a..b517f773e7 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/analysis/TupleIsNullPredicateTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/analysis/TupleIsNullPredicateTest.java @@ -30,7 +30,7 @@ public class TupleIsNullPredicateTest { List tupleIds = Lists.newArrayList(); tupleIds.add(new TupleId(20)); tupleIds.add(new TupleId(21)); - TupleIsNullPredicate tupleIsNullPredicate = new TupleIsNullPredicate(tupleIds); + TupleIsNullPredicate tupleIsNullPredicate = new TupleIsNullPredicate(tupleIds, null); Assert.assertFalse(tupleIsNullPredicate.isBoundByTupleIds(Lists.newArrayList(new TupleId(1)))); } } diff --git a/regression-test/data/query/keyword/test_keyword.out b/regression-test/data/query/keyword/test_keyword.out index 07f4cf8f64..f369ce6a90 100644 --- a/regression-test/data/query/keyword/test_keyword.out +++ b/regression-test/data/query/keyword/test_keyword.out @@ -635,3 +635,10 @@ true 15 1992 3021 11011920 0 true 9999-12-12 2015-04-02T00:00 3.141592653 20.45 14 255 103 15 1992 3021 +-- !alias20 -- +\N 2 + +-- !alias21 -- +1 \N +\N 2 + diff --git a/regression-test/suites/query/keyword/test_keyword.groovy b/regression-test/suites/query/keyword/test_keyword.groovy index 930529b0d3..857e3d57a4 100644 --- a/regression-test/suites/query/keyword/test_keyword.groovy +++ b/regression-test/suites/query/keyword/test_keyword.groovy @@ -97,13 +97,15 @@ suite("test_keyword", "query,p0") { qt_alias13 "select * from (select * from baseall union all \ (select * from baseall order by k1 limit 2)) a order by k1" qt_alias14 "SELECT * FROM (SELECT k1 FROM test) as b ORDER BY k1 ASC LIMIT 0,20;" - // qt_alias15 "select * from (select 1 as a) b left join (select 2 as a) c using(a);" + qt_alias15 "select * from (select 1 as a) b left join (select 2 as a) c using(a);" try_sql "select 1 from (select 2) a order by 0;" qt_alias16 "select * from (select k1 from test group by k1) bar order by k1;" qt_alias17 "SELECT a.x FROM (SELECT 1 AS x) AS a HAVING a.x = 1;" try_sql "select k1 as a, k2 as b, k3 as c from baseall t where a > 0;" qt_alias18 "select k1 as a, k2 as b, k3 as c from baseall t group by a, b, c order by a, b, c;" qt_alias19 "select k1 as a, k2 as b, k3 as c from baseall t group by a, b, c having a > 5 order by a, b, c;" + qt_alias20 "select * from (select 1 as a) b right join (select 2 as a) c using(a);" + qt_alias21 "select * from (select 1 as a) b full join (select 2 as a) c using(a);" sql "select k1 as k7, k2 as k8, k3 as k9 from baseall t group by k7, k8, k9 having k7 > 5 \ order by k7;" sql "select k1 as k7, k2 as k8, k3 as k9 from baseall t where k8 > 0 group by k7, k8, k9 having k7 > 5 order by k7;"