-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
public final class DisaggregationPass implements Pass { | ||
public DAG process(final DAG dag) throws Exception { | ||
dag.doTopological(operator -> { | ||
final Optional<List<Edge>> inEdges = dag.getInEdgesOf(operator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing out!
* It runs a list of passes sequentially to optimize the DAG. | ||
*/ | ||
private static final class Policy { | ||
private final Iterator<Pass> passes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using a List instead? Then maybe we can use lambdas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can lambdas throw exceptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. (http://stackoverflow.com/questions/37726874/why-cant-i-throw-an-exception-in-a-java-8-lambda-expression)
If exception handling is needed, using a simple foreach loop might be a good option.
} | ||
}); | ||
private DAG process(final DAG dag) throws Exception { | ||
DAG optimizedDAG = passes.next().process(dag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentionally placed outside the while loop to throw a NoSuchElementException
, when passes
is empty?
If so, how about instead we proactively check for that and also check whether passes
is actually null in L35?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll fix this point
LGTM. Left some minor comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Can one transform the DAG (e.g., change the graph structure) in a pass? If not, what API should the optimizer provide? |
@bgchun Yes, we can. A
which returns an optimized DAG, after receiving and processing an input DAG. |
Good. Do we have examples? |
- Makes the optimizer configurable - Receiving the policy as a parameter is left as an issue #52 - Introduce instantiation `policy`. - Composed of a list of `pass`es. - Each pass traverses the DAG to tag each operators/edges with attributes. - Two policies available for now: `Pado` and `Disaggregation` - Introduce `Pair` class as a util.
This PR:
policy
.pass
es.Pado
andDisaggregation
Pair
class as a util.Resolves #29