morrySnow commented on code in PR #14380:
URL: https://github.com/apache/doris/pull/14380#discussion_r1026157027
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/NereidsPlanner.java:
##########
@@ -64,18 +66,30 @@ public class NereidsPlanner extends Planner {
private List<ScanNode> scanNodeList = null;
private DescriptorTable descTable;
+ private Plan parsedPlan;
+ private Plan analyzedPlan;
+ private Plan rewrittenPlan;
+ private Plan optimizedPlan;
+
public NereidsPlanner(StatementContext statementContext) {
this.statementContext = statementContext;
}
@Override
- public void plan(StatementBase queryStmt,
org.apache.doris.thrift.TQueryOptions queryOptions) throws UserException {
+ public void plan(StatementBase queryStmt,
org.apache.doris.thrift.TQueryOptions queryOptions) {
if (!(queryStmt instanceof LogicalPlanAdapter)) {
throw new RuntimeException("Wrong type of queryStmt, expected: <?
extends LogicalPlanAdapter>");
}
LogicalPlanAdapter logicalPlanAdapter = (LogicalPlanAdapter) queryStmt;
- PhysicalPlan physicalPlan = plan(logicalPlanAdapter.getLogicalPlan(),
PhysicalProperties.ANY);
+ ExplainLevel explainLevel =
getExplainLevel(queryStmt.getExplainOptions());
+ Plan resultPlan = plan(logicalPlanAdapter.getLogicalPlan(),
PhysicalProperties.ANY, explainLevel);
+ if (explainLevel == ExplainLevel.PARSED_PLAN || explainLevel ==
ExplainLevel.ANALYZED_PLAN
+ || explainLevel == ExplainLevel.REWRITTEN_PLAN || explainLevel
== ExplainLevel.OPTIMIZED_PLAN) {
Review Comment:
nit: add a function to ExplainLevel `isPlanLevel`
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/ScalarFunction.java:
##########
@@ -74,6 +74,13 @@ public ScalarFunction(FunctionName fnName, List<Type>
argTypes, Type retType, bo
NullableMode.DEPEND_ON_ARGUMENT);
}
+ /** nerieds custom scalar function */
+ public ScalarFunction(FunctionName fnName, List<Type> argTypes, Type
retType, boolean hasVarArgs, String symbolName,
+ TFunctionBinaryType binaryType, boolean userVisible, boolean
isVec, NullableMode nullableMode) {
Review Comment:
isVec is always true, right?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindFunction.java:
##########
@@ -171,7 +171,9 @@ public BoundFunction visitUnboundFunction(UnboundFunction
unboundFunction, Env e
}
if (arguments.size() == 1) {
boolean isGlobalAgg = true;
- AggregateParam aggregateParam = new
AggregateParam(unboundFunction.isDistinct(), isGlobalAgg);
+ boolean isDisassembled = false;
+ AggregateParam aggregateParam = new AggregateParam(
+ unboundFunction.isDistinct(), isGlobalAgg,
isDisassembled);
Review Comment:
remove local var: isGlobalAgg and isDisassembled ?
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/AggregateFunction.java:
##########
@@ -183,6 +183,30 @@ public AggregateFunction(FunctionName fnName, List<Type>
argTypes,
returnsNonNullOnEmpty = false;
}
+ public AggregateFunction(FunctionName fnName, List<Type> argTypes,
+ Type retType, Type intermediateType, boolean hasVarArgs,
+ URI location, String updateFnSymbol, String initFnSymbol,
+ String serializeFnSymbol, String mergeFnSymbol, String
getValueFnSymbol,
+ String removeFnSymbol, String finalizeFnSymbol, boolean
ignoresDistinct,
+ boolean isAnalyticFn, boolean returnsNonNullOnEmpty,
TFunctionBinaryType binaryType,
+ boolean userVisible, boolean vectorized, NullableMode
nullableMode) {
+ // only `count` is always not nullable, other aggregate function is
always nullable
+ super(0, fnName, argTypes, retType, hasVarArgs, binaryType,
userVisible, vectorized, nullableMode);
+ setLocation(location);
+ this.intermediateType = (intermediateType.equals(retType)) ? null :
intermediateType;
+ this.updateFnSymbol = updateFnSymbol;
+ this.initFnSymbol = initFnSymbol;
+ this.serializeFnSymbol = serializeFnSymbol;
+ this.mergeFnSymbol = mergeFnSymbol;
+ this.getValueFnSymbol = getValueFnSymbol;
+ this.removeFnSymbol = removeFnSymbol;
+ this.finalizeFnSymbol = finalizeFnSymbol;
Review Comment:
do we still need this? use for UDAF?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java:
##########
@@ -217,11 +217,7 @@ public PlanFragment visitPhysicalAggregate(
outputTupleDesc = localAggNode.getAggInfo().getOutputTupleDesc();
}
- if (aggregate.getAggPhase() == AggPhase.GLOBAL) {
- for (FunctionCallExpr execAggregateFunction :
execAggregateFunctions) {
- execAggregateFunction.setMergeForNereids(true);
- }
- }
Review Comment:
why remove this?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/HllUnion.java:
##########
@@ -32,7 +32,11 @@
/** HllUnion */
public class HllUnion extends AggregateFunction
- implements UnaryExpression, PropagateNullable, ImplicitCastInputTypes {
+ implements UnaryExpression, AlwaysNotNullable,
ExplicitlyCastableSignature {
+ public static final List<FunctionSignature> SIGNATURES = ImmutableList.of(
+ FunctionSignature.ret(HllType.INSTANCE).args(HllType.INSTANCE)
+ );
+
public HllUnion(Expression arg0) {
// TODO: change to hll_union in the future
Review Comment:
add a reason to comment?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/NereidsParser.java:
##########
@@ -53,19 +53,20 @@ public class NereidsParser {
public List<StatementBase> parseSQL(String originStr) {
List<Pair<LogicalPlan, StatementContext>> logicalPlans =
parseMultiple(originStr);
List<StatementBase> statementBases = Lists.newArrayList();
- for (Pair<LogicalPlan, StatementContext> logicalPlan : logicalPlans) {
+ for (Pair<LogicalPlan, StatementContext> parsedPlan2Context :
logicalPlans) {
Review Comment:
nit: suggest to use To instead of 2
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/Substring.java:
##########
@@ -66,8 +66,9 @@ public Substring(Expression arg0, Expression arg1, Expression
arg2) {
}
@Override
- protected FunctionSignature computeSignature(FunctionSignature signature) {
- Optional<Expression> length = getLength();
+ protected FunctionSignature computeSignature(FunctionSignature signature,
List<Expression> arguments) {
+ Optional<Expression> length = arguments.size() == 2
Review Comment:
why == 2 not 3?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/typecoercion/ExpectsInputTypes.java:
##########
@@ -35,4 +35,8 @@
public interface ExpectsInputTypes {
List<AbstractDataType> expectedInputTypes();
+
+ default void checkInputTypes() {
+
+ }
Review Comment:
why we need a blank check?
--
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]
