morrySnow commented on code in PR #59591:
URL: https://github.com/apache/doris/pull/59591#discussion_r2703130754
##########
fe/fe-core/src/main/antlr4/org/apache/doris/nereids/DorisLexer.g4:
##########
@@ -83,6 +83,7 @@ APPEND: 'APPEND';
ARRAY: 'ARRAY';
AS: 'AS';
ASC: 'ASC';
+ASOF: 'ASOF';
Review Comment:
add new keywords into non-reserved keywords list
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/stats/JoinEstimation.java:
##########
@@ -359,6 +359,83 @@ private static Statistics estimateSemiOrAnti(Statistics
leftStats, Statistics ri
}
}
+ private static Statistics estimateAsofInnerJoin(Statistics leftStats,
Statistics rightStats,
Review Comment:
add ut for new estimation
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -4360,21 +4382,48 @@ private LogicalPlan withJoinRelations(LogicalPlan
input, RelationContext ctx) {
.collect(ImmutableList.toImmutableList());
}
} else {
+ if (isAsofJoin) {
+ throw new ParseException("ASOF JOIN must have on or using
clause", join);
+ }
// keep same with original planner, allow cross/inner join
if (!joinType.isInnerOrCrossJoin()) {
throw new ParseException("on mustn't be empty except for
cross/inner join", join);
}
}
+
if (ids == null) {
- last = new LogicalJoin<>(joinType,
ExpressionUtils.EMPTY_CONDITION,
- condition.map(ExpressionUtils::extractConjunction)
- .orElse(ExpressionUtils.EMPTY_CONDITION),
- distributeHint,
- Optional.empty(),
- last,
- plan(join.relationPrimary()), null);
+ if (isAsofJoin) {
+ if (!condition.isPresent()) {
+ throw new ParseException("ASOF JOIN can't be used
without ON clause", join);
+ }
+ List<Expression> conjuncts =
ExpressionUtils.extractConjunction(condition.get());
+ for (Expression expression : conjuncts) {
+ if (!(expression instanceof EqualTo)) {
+ throw new ParseException("ASOF JOIN's ON clause
must be one or more EQUAL(=) conjuncts",
+ join);
+ }
+ if (expression.child(0).isConstant() ||
expression.child(1).isConstant()) {
+ throw new ParseException("ASOF JOIN's EQUAL
conjunct's children must not be constant");
+ }
Review Comment:
why check it here? could we reject all `constant` expression here?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/join/OuterJoinLAsscomProject.java:
##########
@@ -47,7 +47,18 @@ public class OuterJoinLAsscomProject extends
OneExplorationRuleFactory {
// newBottomJoin Type = topJoin Type, newTopJoin Type = bottomJoin Type
public static Set<Pair<JoinType, JoinType>> VALID_TYPE_PAIR_SET =
ImmutableSet.of(
Pair.of(JoinType.LEFT_OUTER_JOIN, JoinType.INNER_JOIN),
+ Pair.of(JoinType.LEFT_OUTER_JOIN, JoinType.ASOF_LEFT_INNER_JOIN),
+ Pair.of(JoinType.LEFT_OUTER_JOIN, JoinType.ASOF_RIGHT_INNER_JOIN),
+ Pair.of(JoinType.ASOF_LEFT_OUTER_JOIN, JoinType.INNER_JOIN),
+ Pair.of(JoinType.ASOF_LEFT_OUTER_JOIN,
JoinType.ASOF_LEFT_INNER_JOIN),
+ Pair.of(JoinType.ASOF_LEFT_OUTER_JOIN,
JoinType.ASOF_RIGHT_INNER_JOIN),
Pair.of(JoinType.INNER_JOIN, JoinType.LEFT_OUTER_JOIN),
+ Pair.of(JoinType.INNER_JOIN, JoinType.ASOF_LEFT_OUTER_JOIN),
+ Pair.of(JoinType.ASOF_LEFT_INNER_JOIN, JoinType.LEFT_OUTER_JOIN),
+ Pair.of(JoinType.ASOF_LEFT_INNER_JOIN,
JoinType.ASOF_LEFT_OUTER_JOIN),
+ Pair.of(JoinType.ASOF_RIGHT_INNER_JOIN, JoinType.LEFT_OUTER_JOIN),
+ Pair.of(JoinType.ASOF_RIGHT_INNER_JOIN,
JoinType.ASOF_LEFT_OUTER_JOIN),
+ Pair.of(JoinType.ASOF_LEFT_OUTER_JOIN,
JoinType.ASOF_LEFT_OUTER_JOIN),
Pair.of(JoinType.LEFT_OUTER_JOIN, JoinType.LEFT_OUTER_JOIN));
Review Comment:
why not have LEFT_OUTER_JOIN -> ASOF_LEFT_OUTER_JOIN and
ASOF_LEFT_OUTER_JOIN -> LEFT_OUTER_JOIN
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/HyperGraphComparator.java:
##########
@@ -67,12 +67,24 @@ public class HyperGraphComparator {
static Map<Pair<JoinType, JoinType>, Pair<Boolean, Boolean>>
canInferredJoinTypeMap = ImmutableMap
.<Pair<JoinType, JoinType>, Pair<Boolean, Boolean>>builder()
.put(Pair.of(JoinType.LEFT_SEMI_JOIN, JoinType.INNER_JOIN),
Pair.of(false, false))
+ .put(Pair.of(JoinType.LEFT_SEMI_JOIN,
JoinType.ASOF_LEFT_INNER_JOIN), Pair.of(false, false))
+ .put(Pair.of(JoinType.LEFT_SEMI_JOIN,
JoinType.ASOF_RIGHT_INNER_JOIN), Pair.of(false, false))
.put(Pair.of(JoinType.RIGHT_SEMI_JOIN, JoinType.INNER_JOIN),
Pair.of(false, false))
+ .put(Pair.of(JoinType.RIGHT_SEMI_JOIN,
JoinType.ASOF_LEFT_INNER_JOIN), Pair.of(false, false))
+ .put(Pair.of(JoinType.RIGHT_SEMI_JOIN,
JoinType.ASOF_RIGHT_INNER_JOIN), Pair.of(false, false))
.put(Pair.of(JoinType.INNER_JOIN, JoinType.LEFT_OUTER_JOIN),
Pair.of(false, true))
Review Comment:
why not include INNER_JOIN -> ASOF_LEFT_OUTER_JOIN? and INNER_JOIN ->
ASOF_RIGHT_OUTER_JOIN?
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/JoinOperator.java:
##########
@@ -39,7 +39,11 @@ public enum JoinOperator {
NULL_AWARE_LEFT_ANTI_JOIN("NULL AWARE LEFT ANTI JOIN",
TJoinOp.NULL_AWARE_LEFT_ANTI_JOIN),
NULL_AWARE_LEFT_SEMI_JOIN("NULL AWARE LEFT SEMI JOIN",
- TJoinOp.NULL_AWARE_LEFT_SEMI_JOIN);
+ TJoinOp.NULL_AWARE_LEFT_SEMI_JOIN),
+ ASOF_LEFT_INNER_JOIN("ASOF_LEFT_INNER_JOIN", TJoinOp.ASOF_LEFT_INNER_JOIN),
+ ASOF_RIGHT_INNER_JOIN("ASOF_RIGHT_INNER_JOIN",
TJoinOp.ASOF_RIGHT_INNER_JOIN),
+ ASOF_LEFT_OUTER_JOIN("ASOF_LEFT_OUTER_JOIN", TJoinOp.ASOF_LEFT_OUTER_JOIN),
+ ASOF_RIGHT_OUTER_JOIN("ASOF_LEFT_INNER_JOIN",
TJoinOp.ASOF_RIGHT_OUTER_JOIN);
Review Comment:
```suggestion
ASOF_RIGHT_OUTER_JOIN("ASOF_LEFT_OUTER_JOIN",
TJoinOp.ASOF_RIGHT_OUTER_JOIN);
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -4340,6 +4345,23 @@ private LogicalPlan withJoinRelations(LogicalPlan input,
RelationContext ctx) {
} else {
joinType = JoinType.CROSS_JOIN;
}
+ Expression matchCondition = null;
+ if (join.matchCondition() != null) {
+ if (!isAsofJoin) {
+ throw new ParseException("only ASOF JOIN support
MATCH_CONDITION", join);
+ }
+ matchCondition =
typedVisit(join.matchCondition().valueExpression());
+ if (!(matchCondition instanceof LessThan
+ || matchCondition instanceof LessThanEqual
+ || matchCondition instanceof GreaterThan
+ || matchCondition instanceof GreaterThanEqual)) {
+ throw new ParseException("ASOF JOIN's MATCH_CONDITION must
be <, <=, >, >=", join);
+ }
Review Comment:
It's better to enforce the restriction directly in the parser, as it can
speed up parsing.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java:
##########
@@ -762,11 +791,19 @@ private LogicalPlan
bindUsingJoin(MatchingContext<LogicalUsingJoin<Plan, Plan>>
hashEqExprs.add(new EqualTo(usingLeftSlot, usingRightSlot));
}
- return new LogicalJoin<>(
- using.getJoinType() == JoinType.CROSS_JOIN ?
JoinType.INNER_JOIN : using.getJoinType(),
- hashEqExprs.build(), ImmutableList.of(),
- using.getDistributeHint(), Optional.empty(),
rightConjunctsSlots,
- using.children(), null);
+ if (using.getJoinType().isAsofJoin()) {
+ return new LogicalJoin<>(
+ using.getJoinType(),
+ hashEqExprs.build(),
ImmutableList.of(using.getMatchCondition().get()),
+ using.getDistributeHint(), Optional.empty(),
rightConjunctsSlots,
+ using.children(), null);
+ } else {
+ return new LogicalJoin<>(
+ using.getJoinType() == JoinType.CROSS_JOIN ?
JoinType.INNER_JOIN : using.getJoinType(),
+ hashEqExprs.build(), ImmutableList.of(),
+ using.getDistributeHint(), Optional.empty(),
rightConjunctsSlots,
+ using.children(), null);
Review Comment:
```suggestion
return new LogicalJoin<>(
using.getJoinType() == JoinType.CROSS_JOIN ?
JoinType.INNER_JOIN : using.getJoinType(),
hashEqExprs.build(),
using.getMatchCondition().map(ImmutableList::of).orElse(ImmutableList.of()),
using.getDistributeHint(), Optional.empty(),
rightConjunctsSlots,
using.children(), null);
```
--
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]
