Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove identity casts from optimizer #329

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

zachyee
Copy link

@zachyee zachyee commented Aug 30, 2016

@maciejgrzybek
Copy link

from optimizer is not necessary in commit msg.
What about Add remove identity casts rewriter?
Commit msgs guidelines: http://chris.beams.io/posts/git-commit/

@maciejgrzybek maciejgrzybek self-assigned this Aug 30, 2016
public RemoveIdentityCastContext(Map<Symbol, Expression> expressionAssignments,
Map<Symbol, Type> typeAssignments)
{
this.expressionAssignments = expressionAssignments;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requireNonNull

@maciejgrzybek
Copy link

Back to you, @zachyee.

@zachyee
Copy link
Author

zachyee commented Aug 30, 2016

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a expression tree and you check only the root of it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should use ExpressionVerifier here and compare expected expression vs actual one.

import org.intellij.lang.annotations.Language;

public interface PlanTester
{
Copy link

@kokosing kokosing Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO interface here is an over engineering. I would prefer abstract class with below methods implemented.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this iface is needed at all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants