morrySnow commented on code in PR #13659:
URL: https://github.com/apache/doris/pull/13659#discussion_r1033613800
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/StatementContext.java:
##########
@@ -29,6 +30,8 @@
*/
public class StatementContext {
+ private static final EventChannel channel =
EventChannel.getDefaultChannel().start();
Review Comment:
why not put in EventChannel class?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/StatementContext.java:
##########
@@ -79,4 +82,8 @@ public RelationId getNextRelationId() {
public void setParsedStatement(StatementBase parsedStatement) {
this.parsedStatement = parsedStatement;
}
+
+ public static EventChannel getChannel() {
+ return channel;
+ }
Review Comment:
if channle is global, we should not put it in statementContext
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/StatementContext.java:
##########
@@ -40,12 +43,12 @@ public class StatementContext {
private StatementBase parsedStatement;
public StatementContext() {
- this.connectContext = ConnectContext.get();
+ setConnectContext(ConnectContext.get());
}
public StatementContext(ConnectContext connectContext, OriginStatement
originStatement) {
- this.connectContext = connectContext;
- this.originStatement = originStatement;
+ setConnectContext(connectContext);
+ setOriginStatement(originStatement);
Review Comment:
why change to set?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/Job.java:
##########
@@ -88,48 +94,33 @@ public List<Rule> getValidRules(GroupExpression
groupExpression, List<Rule> cand
public abstract void execute() throws AnalysisException;
- protected Optional<CopyInResult> invokeRewriteRuleWithTrace(Rule rule,
Plan before, Group targetGroup) {
+ protected Optional<CopyInResult> invokeRewriteRuleWithTrace(Rule rule,
Plan before, Group targetGroup,
+ EventProducer transformTracer) {
context.onInvokeRule(rule.getRuleType());
-
- String traceBefore = enableTrace ? getPlanTraceLog() : null;
+ COUNTER_TRACER.log(CounterEvent.of(Memo.getStateId(),
+ CounterType.EXPRESSION_TRANSFORM, targetGroup,
targetGroup.getLogicalExpression(), before));
List<Plan> afters = rule.transform(before,
context.getCascadesContext());
Preconditions.checkArgument(afters.size() == 1);
Plan after = afters.get(0);
-
- if (after != before) {
- CopyInResult result = context.getCascadesContext()
- .getMemo()
- .copyIn(after, targetGroup, rule.isRewrite());
-
- if ((result.generateNewExpression ||
result.correspondingExpression.getOwnerGroup() != targetGroup)
- && enableTrace) {
- String traceAfter = getPlanTraceLog();
- printTraceLog(rule, traceBefore, traceAfter);
- }
-
- return Optional.of(result);
+ if (after == before) {
+ return Optional.empty();
}
- return Optional.empty();
- }
-
- protected String getPlanTraceLog() {
- return context.getCascadesContext()
+ CopyInResult result = context.getCascadesContext()
.getMemo()
- .copyOut(false)
- .treeString();
- }
+ .copyIn(after, targetGroup, rule.isRewrite());
- protected String getMemoTraceLog() {
- return context.getCascadesContext()
- .getMemo()
- .getRoot()
- .treeString();
+ if (result.generateNewExpression ||
result.correspondingExpression.getOwnerGroup() != targetGroup) {
+
transformTracer.log(TransformEvent.of(targetGroup.getLogicalExpression(),
before, afters,
+ rule.getRuleType()), rule::isRewrite);
+ }
+
+ return Optional.of(result);
}
- protected void printTraceLog(Rule rule, String traceBefore, String
traceAfter) {
- logger.info("========== {} {} ==========\nbefore:\n{}\n\nafter:\n{}\n",
- getClass().getSimpleName(), rule.getRuleType(), traceBefore,
traceAfter);
+ protected void trace(GroupExpression groupExpression) {
Review Comment:
this function name is too generalize. we should know trace what from
function name. and need to add some comment to explain this function
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/Job.java:
##########
@@ -39,12 +46,15 @@
* Abstract class for all job using for analyze and optimize query plan in
Nereids.
*/
public abstract class Job {
+ protected static final EventProducer COUNTER_TRACER = new
EventProducer(CounterEvent.class,
Review Comment:
add comment to each tracer in all job class to explain what it is trace for
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/rewrite/RewriteBottomUpJob.java:
##########
@@ -72,12 +79,14 @@ public void execute() throws AnalysisException {
return;
}
+ trace(logicalExpression);
List<Rule> validRules = getValidRules(logicalExpression, rules);
for (Rule rule : validRules) {
GroupExpressionMatching groupExpressionMatching
= new GroupExpressionMatching(rule.getPattern(),
logicalExpression);
for (Plan before : groupExpressionMatching) {
- Optional<CopyInResult> copyInResult =
invokeRewriteRuleWithTrace(rule, before, group);
+ Optional<CopyInResult> copyInResult =
invokeRewriteRuleWithTrace(rule, before, group,
+ REWRITE_BOTTOM_UP_JOB_TRACER);
Review Comment:
you could use a mixin interface to get the tracer from a method from this
interface and to avoid this parameter.
```java
RewriteBottomUpJob extends Job implements TracerSupplier {
...
}
interface TracerSupplier {
EventProducer getTracer();
}
```
--
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]
