morrySnow commented on code in PR #15714:
URL: https://github.com/apache/doris/pull/15714#discussion_r1064366410


##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java:
##########
@@ -1102,10 +1102,13 @@ public PlanFragment 
visitPhysicalProject(PhysicalProject<? extends Plan> project
         }
 
         TupleDescriptor tupleDescriptor = generateTupleDesc(slotList, null, 
context);
-        inputPlanNode.setProjectList(execExprList);
+        if (!(inputPlanNode instanceof EmptySetNode)) {
+            inputPlanNode.setProjectList(execExprList);
+        }

Review Comment:
   add a todo, we need to merge project(emptyset) to a new emptyset in rewrite 
step of Nereids



##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/ResolveOrdinalInOrderByAndGroupBy.java:
##########
@@ -74,6 +76,11 @@ public List<Rule> buildRules() {
                                     int ord = i.getIntValue();
                                     checkOrd(ord, aggOutput.size());
                                     Expression aggExpr = aggOutput.get(ord - 
1);
+                                    if 
(aggExpr.containsType(AggregateFunction.class)) {
+                                        throw new AnalysisException(
+                                                "GROUP BY expression must not 
contain aggregate functions: "
+                                                        + aggExpr.toSql());
+                                    }

Review Comment:
   we should not add check here, instead we need to add a check in CheckAnalysis



##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/logical/NormalizeToSlot.java:
##########
@@ -133,7 +136,10 @@ public static NormalizeToSlotTriplet toTriplet(Expression 
expression, @Nullable
             }
 
             Alias alias = new Alias(expression, expression.toSql());
-            return new NormalizeToSlotTriplet(expression, alias.toSlot(), 
alias);
+            SlotReference slot = (SlotReference) alias.toSlot();
+            // BE will create a nullable uint8 column to expand NullLiteral 
when necessary
+            return new NormalizeToSlotTriplet(expression, expression 
instanceof NullLiteral ? slot.withDataType(
+                    TinyIntType.INSTANCE) : slot, alias);

Review Comment:
   if we just want to process `group by null`, maybe a better way to do it is 
generating NullLiteral with TinyIntType when we do rewrite.



##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/JoinUtils.java:
##########
@@ -151,11 +152,14 @@ public static Pair<List<ExprId>, List<ExprId>> 
getOnClauseUsedSlots(
 
         for (Expression expr : join.getHashJoinConjuncts()) {
             EqualTo equalTo = (EqualTo) expr;
-            if (!(equalTo.left() instanceof Slot) || !(equalTo.right() 
instanceof Slot)) {
+            Optional<Slot> leftSlot = 
ExpressionUtils.extractSlotOrCastOnSlot(equalTo.left());
+            Optional<Slot> rightSlot = 
ExpressionUtils.extractSlotOrCastOnSlot(equalTo.right());
+            if (!leftSlot.isPresent() || !rightSlot.isPresent()) {

Review Comment:
   add a TODO on it, to explain why and we will fix normalize join hash equals 
future



##########
fe/fe-core/src/main/java/org/apache/doris/planner/SortNode.java:
##########
@@ -320,7 +320,12 @@ public Set<SlotId> computeInputSlotIds(Analyzer analyzer) 
throws NotImplementedE
      */
     public void finalizeForNereids(TupleDescriptor tupleDescriptor,
             List<Expr> outputList, List<Expr> orderingExpr) {
-        resolvedTupleExprs = Lists.newArrayList(orderingExpr);
+        resolvedTupleExprs = Lists.newArrayList();
+        for (Expr order : orderingExpr) {
+            if (!resolvedTupleExprs.contains(order)) {
+                resolvedTupleExprs.add(order);
+            }
+        }

Review Comment:
   add a TODO, we need to fix it in Nereids' code later



##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -215,6 +215,9 @@ public static Optional<DataType> 
findTightestCommonType(BinaryOperator binaryOpe
             }
         } else if (left instanceof CharacterType && right instanceof 
CharacterType) {
             tightestCommonType = 
CharacterType.widerCharacterType((CharacterType) left, (CharacterType) right);
+        } else if (left instanceof CharacterType && right instanceof 
DateLikeType) {
+            // TODO: need check implicitCastMap to keep the behavior 
consistent with old optimizer
+            tightestCommonType = right;

Review Comment:
   should we also add `right instanceof CharacterType && left instanceof 
DateLikeType`



-- 
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

Reply via email to