morrySnow commented on code in PR #24600:
URL: https://github.com/apache/doris/pull/24600#discussion_r1330934558
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/AbstractPlan.java:
##########
@@ -69,10 +67,9 @@ protected AbstractPlan(PlanType type, List<Plan> children) {
protected AbstractPlan(PlanType type, Optional<GroupExpression>
groupExpression,
Optional<LogicalProperties> optLogicalProperties, @Nullable
Statistics statistics,
List<Plan> children) {
- super(groupExpression, children);
- this.type = Objects.requireNonNull(type, "type can not be null");
- this.groupExpression = Objects.requireNonNull(groupExpression,
"groupExpression can not be null");
- Objects.requireNonNull(optLogicalProperties, "logicalProperties can
not be null");
+ super(children);
+ this.type = type;
+ this.groupExpression = groupExpression;
Review Comment:
why remove null check?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/AbstractTreeNode.java:
##########
@@ -39,12 +37,12 @@ public abstract class AbstractTreeNode<NODE_TYPE extends
TreeNode<NODE_TYPE>>
// TODO: Maybe we should use a GroupPlan to avoid TreeNode hold the
GroupExpression.
// https://github.com/apache/doris/pull/9807#discussion_r884829067
- public AbstractTreeNode(NODE_TYPE... children) {
- this(Optional.empty(), ImmutableList.copyOf(children));
+ protected AbstractTreeNode(NODE_TYPE... children) {
+ this(ImmutableList.copyOf(children));
}
- public AbstractTreeNode(Optional<GroupExpression> groupExpression,
List<NODE_TYPE> children) {
- this.children = ImmutableList.copyOf(children);
+ protected AbstractTreeNode(List<NODE_TYPE> children) {
+ this.children = children;
Review Comment:
why remove copyOf?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalJoin.java:
##########
@@ -123,18 +123,16 @@ public LogicalJoin(
Optional.empty(), Optional.empty(), children);
}
- /**
- * Just use in withXXX method.
- */
private LogicalJoin(JoinType joinType, List<Expression> hashJoinConjuncts,
List<Expression> otherJoinConjuncts,
JoinHint hint, Optional<MarkJoinSlotReference>
markJoinSlotReference,
Optional<GroupExpression> groupExpression,
Optional<LogicalProperties> logicalProperties,
List<Plan> children, JoinReorderContext joinReorderContext) {
+ // Just use in withXXX method. Don't need check/copyOf()
super(PlanType.LOGICAL_JOIN, groupExpression, logicalProperties,
children);
- this.joinType = Objects.requireNonNull(joinType, "joinType can not be
null");
- this.hashJoinConjuncts = ImmutableList.copyOf(hashJoinConjuncts);
- this.otherJoinConjuncts = ImmutableList.copyOf(otherJoinConjuncts);
- this.hint = Objects.requireNonNull(hint, "hint can not be null");
+ this.joinType = joinType;
+ this.hashJoinConjuncts = hashJoinConjuncts;
+ this.otherJoinConjuncts = otherJoinConjuncts;
+ this.hint = hint;
Review Comment:
ditto
--
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]
