Remove identity casts from optimizer #329
Conversation
|
from optimizer is not necessary in commit msg. |
| public RemoveIdentityCastContext(Map<Symbol, Expression> expressionAssignments, | ||
| Map<Symbol, Type> typeAssignments) | ||
| { | ||
| this.expressionAssignments = expressionAssignments; |
|
Back to you, @zachyee. |
zachyee
force-pushed
the
remove_identity_casts
branch
from
17a5081
to
e5507b9
Compare
|
I addressed the easy-to-fix comments. I responded with some info on the harder-to-fix comments to figure out what would be best before trying to address them. @maciejgrzybek |
| @@ -36,4 +36,15 @@ public static void assertPlan(Session session, Metadata metadata, Plan actual, P | |||
| assertTrue(matches, format("Plan does not match:\n %s\n, to pattern:\n%s", logicalPlan, pattern)); | |||
| } | |||
| } | |||
|
|
|||
| public static void assertNotPlan(Session session, Metadata metadata, Plan actual, PlanMatchPattern pattern) | |||
There was a problem hiding this comment.
I don't like this name. As it is a plan. Maybe assertPlanDoesNotMatch
| } | ||
|
|
||
| for (Map.Entry<Symbol, Expression> assignment : ((ProjectNode) node).getAssignments().entrySet()) { | ||
| Expression expression = assignment.getValue(); |
There was a problem hiding this comment.
It is a expression tree and you check only the root of it.
There was a problem hiding this comment.
I think you should use ExpressionVerifier here and compare expected expression vs actual one.
zachyee
force-pushed
the
remove_identity_casts
branch
from
e5507b9
to
970cf51
Compare
| import org.intellij.lang.annotations.Language; | ||
|
|
||
| public interface PlanTester | ||
| { |
There was a problem hiding this comment.
IMO interface here is an over engineering. I would prefer abstract class with below methods implemented.
zachyee
force-pushed
the
remove_identity_casts
branch
from
970cf51
to
ed8b85d
Compare
prestodb#4259
@maciejgrzybek