amrishlal commented on a change in pull request #7568:
URL: https://github.com/apache/pinot/pull/7568#discussion_r746092243
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/DocIdSetOperator.java
##########
@@ -34,6 +37,7 @@
*/
public class DocIdSetOperator extends BaseOperator<DocIdSetBlock> {
private static final String OPERATOR_NAME = "DocIdSetOperator";
+ private static final String EXPLAIN_NAME = null;
Review comment:
Yes, for an operator to be included in EXPLAIN PLAN output, it must
define an EXPLAIN PLAN name.
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/BaseOperator.java
##########
@@ -52,4 +54,25 @@ public final T nextBlock() {
// Make it protected because we should always call nextBlock()
protected abstract T getNextBlock();
+
+ // Enforcing sub-class to implement the getOperatorName(), as they can just
return a static final,
+ // as opposed to this super class calling getClass().getSimpleName().
+ public abstract String getOperatorName();
Review comment:
That method was being only used twice (`DocIdSetOperator` and
`TransformOperator`), so adding the function at the interface level didn't make
sense. The way it is set up now is that if an operator doesn't define a
non-null value for getExplainPlanName(), it won't be included in EXPLAIN PLAN
output.
```
/**
* @return EXPLAIN PLAN name if one is defined; otherwise, null. null
EXPLAIN PLAN name means that this operator
* won't be included in EXPLAIN PLAN output.
*/
String getExplainPlanName();
```
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/BaseOperator.java
##########
@@ -52,4 +54,25 @@ public final T nextBlock() {
// Make it protected because we should always call nextBlock()
protected abstract T getNextBlock();
+
+ // Enforcing sub-class to implement the getOperatorName(), as they can just
return a static final,
+ // as opposed to this super class calling getClass().getSimpleName().
+ public abstract String getOperatorName();
+
+ public abstract String getExplainPlanName();
+
+ @Override
+ public List<Operator> getChildOperators() {
+ return new ArrayList<>();
+ }
+
+ @Override
+ public String toExplainString() {
+ return getExplainPlanName();
+ }
+
+ @Override
Review comment:
By default, the explain plan description for an operator is its name,
unless the operator overrides `toExplainString` function. If I remember
correctly this is same as what was in the original code except that I
simplified things a bit.
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/BaseOperator.java
##########
@@ -52,4 +54,25 @@ public final T nextBlock() {
// Make it protected because we should always call nextBlock()
protected abstract T getNextBlock();
+
+ // Enforcing sub-class to implement the getOperatorName(), as they can just
return a static final,
+ // as opposed to this super class calling getClass().getSimpleName().
+ public abstract String getOperatorName();
+
+ public abstract String getExplainPlanName();
+
+ @Override
+ public List<Operator> getChildOperators() {
+ return new ArrayList<>();
Review comment:
Fixed. `getChildOperators()` is abstract now.
--
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...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
