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