Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/189#discussion_r43714802
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/expr/EvaluationVisitor.java
---
@@ -106,19 +177,30 @@ public HoldingContainer
visitFunctionCall(FunctionCall call, ClassGenerator<?> g
@Override
public HoldingContainer visitBooleanOperator(BooleanOperator op,
ClassGenerator<?> generator) throws RuntimeException {
+ HoldingContainer hc = getPrevious(op, generator.getMappingSet());
--- End diff --
You put the check whether it's common subexpression in EvalVisitor, when
handling with different expression.
I feel the code would be much cleaner, if we add a CSEFilter in a similar
to ConstantFilter. The logic will go through ConstantFilter -> CSEFilter ->
EvalVisitor. The check of common subexpression and adding to the Hashmap would
be done solely in CSEFilter:visitiUnknow(). In AbstractExrVisitor, other
methods will by default come to visitUnknow(). In this way, there is no need
to repeat the checking of common subexpression for different expression in
EvalVisitor; actually no change has to be done to EvalVisitor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
