morrySnow commented on code in PR #8910:
URL: https://github.com/apache/incubator-doris/pull/8910#discussion_r851054253
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java:
##########
@@ -613,6 +617,102 @@ private void whereClauseRewrite() {
}
}
+ private void convertOutJoinToInnerJoin(Analyzer analyzer) {
+ // Use HashSet to ensure tableRefs are unique, such as avoid two table
A in it;
+ HashSet<String> notNullTables = Sets.newHashSet();
+
+ // TODO[jackwener]: normalize the expr and consider more case
+ // Now, just check all is AND
+ if (PredicateUtils.existOr(whereClause))
Review Comment:
always add brace for if statement
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java:
##########
@@ -613,6 +617,102 @@ private void whereClauseRewrite() {
}
}
+ private void convertOutJoinToInnerJoin(Analyzer analyzer) {
+ // Use HashSet to ensure tableRefs are unique, such as avoid two table
A in it;
+ HashSet<String> notNullTables = Sets.newHashSet();
+
+ // TODO[jackwener]: normalize the expr and consider more case
+ // Now, just check all is AND
+ if (PredicateUtils.existOr(whereClause))
+ return;
+ List<Expr> flatExprs = PredicateUtils.flatAnd(whereClause);
+ if (flatExprs.isEmpty()) {
+ return;
+ }
+
+ // Find tables which is in not-null predicate
+ for (Expr expr : flatExprs) {
+ if (expr instanceof Predicate && ((Predicate)
expr).isNotNullPred()) {
+ for (Expr child : expr.getChildren()) {
+ if (child instanceof SlotRef) {
+ SlotRef slotRef = (SlotRef) child;
+ if (slotRef.getTableName() != null) {
+ notNullTables.add(slotRef.getTableName().getTbl());
+ }
+ }
+ }
+ }
+ }
+
+ // For each not null predicate, find the table transfer outer join:
+ // self table:
+ // full outer join -> right outer join
+ // left outer join -> inner join
+ // next table:
+ // full outer join -> left outer join
+ // right outer join -> inner join
+ UnaryOperator<JoinOperator> convertSelf = joinOp -> {
+ if (joinOp.isFullOuterJoin()) {
+ return JoinOperator.RIGHT_OUTER_JOIN;
+ } else if (joinOp.isLeftOuterJoin()) {
+ return JoinOperator.INNER_JOIN;
+ }
+ return joinOp;
+ };
+ UnaryOperator<JoinOperator> convertNext = joinOp -> {
+ if (joinOp.isFullOuterJoin()) {
+ return JoinOperator.LEFT_OUTER_JOIN;
+ } else if (joinOp.isRightOuterJoin()) {
+ return JoinOperator.INNER_JOIN;
+ }
+ return joinOp;
+ };
+
+ for (int i = 0; i < fromClause.size(); i++) {
+ if (fromClause == null)
+ return;
+
+ TableRef tableRef = fromClause.get(i);
+ if (tableRef == null || tableRef.getName() == null)
+ continue;
+ if (notNullTables.contains(tableRef.getName().getTbl())) {
+ tableRef.setJoinOp(convertSelf.apply(tableRef.getJoinOp()));
+
+ if (i < fromClause.size() - 1) {
+ TableRef nextTableRef = fromClause.get(i + 1);
+
nextTableRef.setJoinOp(convertNext.apply(nextTableRef.getJoinOp()));
+ }
+ }
+ }
+
+ // Clean
outerJoinedTupleIds/fullOuterJoinedTupleIds/fullOuterJoinedConjuncts in
+ // globalState.
+ // Make predicate can be pushed down after conversion.
+ ArrayList<ExprId> conjunctIds = Lists.newArrayList();
+ analyzer.getFullOuterJoinedConjuncts().forEach((id, tableRef) -> {
+ if (tableRef != null && tableRef.joinOp.isRightOuterJoin() ||
tableRef.joinOp.isInnerJoin()
+ || (tableRef.leftTblRef != null
+ && (tableRef.leftTblRef.joinOp.isLeftOuterJoin()
+ || tableRef.joinOp.isInnerJoin())))
+ conjunctIds.add(id);
+ });
+ conjunctIds.forEach(analyzer.getFullOuterJoinedConjuncts()::remove);
+
+ Consumer<Map<TupleId, TableRef>> remove = (tupleIds) -> {
+ ArrayList<TupleId> ids = Lists.newArrayList();
+ tupleIds.forEach((id, tableRef) -> {
+ if (tableRef != null && tableRef.joinOp.isRightOuterJoin() ||
tableRef.joinOp.isInnerJoin()
+ || (tableRef.leftTblRef != null
+ &&
(tableRef.leftTblRef.joinOp.isLeftOuterJoin()
+ || tableRef.joinOp.isInnerJoin())))
+ ids.add(id);
+ });
+ ids.forEach(tupleIds::remove);
+ };
+ remove.accept(analyzer.getFullOuterJoinedTupleIds());
+ remove.accept(analyzer.getOuterJoinedTupleIds());
Review Comment:
1. Do we need to remove conjuncts from
`org.apache.doris.analysis.Analyzer.GlobalState#fullOuterJoinedConjuncts`?
2. Do we need to remove TupleId from
`org.apache.doris.analysis.Analyzer.GlobalState#outerJoinedMaterializedTupleIds`?
@EmmyMiao87 could you help to check out whether it is correct?
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/Analyzer.java:
##########
@@ -1534,6 +1534,18 @@ public boolean isSjConjunct(Expr e) {
return globalState.sjClauseByConjunct.containsKey(e.getId());
}
+ public Map<ExprId, TableRef> getFullOuterJoinedConjuncts(){
Review Comment:
nit: add space after right parenthesis
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/PredicateUtils.java:
##########
@@ -52,4 +52,46 @@ private static void splitDisjunctivePredicates(Expr expr,
List<Expr> result) {
result.add(expr);
}
}
+
+ /**
+ * Split predicates in conjunctive form recursively, i.e., split the input
+ * expression, if the root node of the expression tree is `and` predicate.
+ *
+ * Some examples:
+ * a and b -> a, b
+ * a and b and c -> a, b, c
+ * (a or b) and (c or d) -> (a or b), (c or d)
+ * (a and b) or c -> (a and b) or c
+ * a -> a
+ */
+ public static List<Expr> flatAnd(Expr expr) {
+ ArrayList<Expr> result = Lists.newArrayList();
+ if (expr == null) {
+ return result;
+ }
+
+ flatAnd(expr, result);
+ return result;
+ }
+
+ private static void flatAnd(Expr expr, List<Expr> result) {
+ if (expr instanceof CompoundPredicate && ((CompoundPredicate)
expr).getOp() == CompoundPredicate.Operator.AND) {
+ flatAnd(expr.getChild(0), result);
+ flatAnd(expr.getChild(1), result);
+ } else {
+ result.add(expr);
+ }
+ }
+
+ public static boolean existOr(Expr expr) {
+ if (expr == null) {
+ return false;
+ }
+
+ if (expr instanceof CompoundPredicate && ((CompoundPredicate)
expr).getOp() == CompoundPredicate.Operator.OR) {
+ return true;
+ } else {
+ return existOr(expr.getChild(0)) || existOr(expr.getChild(1));
Review Comment:
expr always has two children?
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/Predicate.java:
##########
@@ -41,6 +41,10 @@ public boolean isEqJoinConjunct() {
return isEqJoinConjunct;
}
+ // It's used for judging predicate output not-null value (and column is in
a
+ // tableRef)
+ public abstract boolean isNotNullPred();
Review Comment:
nit: use java doc comment style
```java
/**
*
*/
```
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java:
##########
@@ -613,6 +617,102 @@ private void whereClauseRewrite() {
}
}
+ private void convertOutJoinToInnerJoin(Analyzer analyzer) {
+ // Use HashSet to ensure tableRefs are unique, such as avoid two table
A in it;
+ HashSet<String> notNullTables = Sets.newHashSet();
+
+ // TODO[jackwener]: normalize the expr and consider more case
+ // Now, just check all is AND
+ if (PredicateUtils.existOr(whereClause))
+ return;
+ List<Expr> flatExprs = PredicateUtils.flatAnd(whereClause);
+ if (flatExprs.isEmpty()) {
+ return;
+ }
+
+ // Find tables which is in not-null predicate
+ for (Expr expr : flatExprs) {
+ if (expr instanceof Predicate && ((Predicate)
expr).isNotNullPred()) {
+ for (Expr child : expr.getChildren()) {
+ if (child instanceof SlotRef) {
+ SlotRef slotRef = (SlotRef) child;
+ if (slotRef.getTableName() != null) {
+ notNullTables.add(slotRef.getTableName().getTbl());
Review Comment:
is it better to use TupleId instead of table name string ?
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java:
##########
@@ -613,6 +617,102 @@ private void whereClauseRewrite() {
}
}
+ private void convertOutJoinToInnerJoin(Analyzer analyzer) {
+ // Use HashSet to ensure tableRefs are unique, such as avoid two table
A in it;
+ HashSet<String> notNullTables = Sets.newHashSet();
+
+ // TODO[jackwener]: normalize the expr and consider more case
+ // Now, just check all is AND
+ if (PredicateUtils.existOr(whereClause))
+ return;
+ List<Expr> flatExprs = PredicateUtils.flatAnd(whereClause);
+ if (flatExprs.isEmpty()) {
+ return;
+ }
+
+ // Find tables which is in not-null predicate
+ for (Expr expr : flatExprs) {
+ if (expr instanceof Predicate && ((Predicate)
expr).isNotNullPred()) {
+ for (Expr child : expr.getChildren()) {
+ if (child instanceof SlotRef) {
+ SlotRef slotRef = (SlotRef) child;
+ if (slotRef.getTableName() != null) {
+ notNullTables.add(slotRef.getTableName().getTbl());
+ }
+ }
+ }
+ }
+ }
+
+ // For each not null predicate, find the table transfer outer join:
+ // self table:
+ // full outer join -> right outer join
+ // left outer join -> inner join
+ // next table:
+ // full outer join -> left outer join
+ // right outer join -> inner join
+ UnaryOperator<JoinOperator> convertSelf = joinOp -> {
+ if (joinOp.isFullOuterJoin()) {
+ return JoinOperator.RIGHT_OUTER_JOIN;
+ } else if (joinOp.isLeftOuterJoin()) {
+ return JoinOperator.INNER_JOIN;
+ }
+ return joinOp;
+ };
+ UnaryOperator<JoinOperator> convertNext = joinOp -> {
+ if (joinOp.isFullOuterJoin()) {
+ return JoinOperator.LEFT_OUTER_JOIN;
+ } else if (joinOp.isRightOuterJoin()) {
+ return JoinOperator.INNER_JOIN;
+ }
+ return joinOp;
+ };
+
+ for (int i = 0; i < fromClause.size(); i++) {
+ if (fromClause == null)
+ return;
+
+ TableRef tableRef = fromClause.get(i);
+ if (tableRef == null || tableRef.getName() == null)
+ continue;
+ if (notNullTables.contains(tableRef.getName().getTbl())) {
+ tableRef.setJoinOp(convertSelf.apply(tableRef.getJoinOp()));
+
+ if (i < fromClause.size() - 1) {
+ TableRef nextTableRef = fromClause.get(i + 1);
+
nextTableRef.setJoinOp(convertNext.apply(nextTableRef.getJoinOp()));
+ }
+ }
+ }
+
+ // Clean
outerJoinedTupleIds/fullOuterJoinedTupleIds/fullOuterJoinedConjuncts in
+ // globalState.
+ // Make predicate can be pushed down after conversion.
+ ArrayList<ExprId> conjunctIds = Lists.newArrayList();
+ analyzer.getFullOuterJoinedConjuncts().forEach((id, tableRef) -> {
+ if (tableRef != null && tableRef.joinOp.isRightOuterJoin() ||
tableRef.joinOp.isInnerJoin()
Review Comment:
should add parentheses after the first AND ？
--
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: commits-unsubscr...@doris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
