himanshug commented on a change in pull request #7588: multi-value string
column support for expressions
URL: https://github.com/apache/incubator-druid/pull/7588#discussion_r289611329
##########
File path: core/src/main/java/org/apache/druid/math/expr/Expr.java
##########
@@ -252,12 +528,24 @@ public void visit(Visitor visitor)
this.expr = expr;
}
+ abstract UnaryExpr copy(Expr expr);
+
@Override
public void visit(Visitor visitor)
{
expr.visit(visitor);
visitor.visit(this);
}
+
+ @Override
+ public Expr visit(Shuttle shuttle)
+ {
+ Expr newExpr = expr.visit(shuttle);
+ if (newExpr != expr) {
+ return shuttle.visit(copy(newExpr));
Review comment:
I think I understand now why call to copy is necessary . it will probably be
more readable if instead of having a copy(..) method , each impls just
implemented `visit(Shuttle visit)` method and the common code was refactored
into a static utility method in the top level class ... e.g. `UnaryExpr` would
contain the utility method and `UnaryMinusExpr` and `UnaryPlusExpr` would just
implement `visit(shuttle)` themselves .
That said, these kind of nuances are heavily biased so I leave them to the
author.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
