Github user StephanEwen commented on the pull request:
https://github.com/apache/incubator-flink/pull/2#issuecomment-47893090
This is a good feature addition. I think we should address a few points
before merging it:
- The old code had the `isFieldConstant()` method on the optimizer nodes.
You are adding more methods for forwarded fields. I think better design is to
remove all of those methods and directly supply the `SemanticProperties` to the
(Requested)Global/LocalProperties. The Semantic properties would need a method
like `getForwardedFields(int input, int field)`.
- The methods in the (Requested)Global/LocalProperties can then be
renamed to `filterBySemanticProperties()`, rather than
`filterByNodesConstantSet()`.
- The `LocalProperties` have recently been changed to be an immutable
type. That is much less error-prone in applications like the optimizer. The
getters (for the set of unique fields) should also return immutable types (like
`Collections.unmodifiableSet(set)`). Similar things would be good for the
`GlobalProperties`, `RequestedGlobalProperties`, and `RequestedLocalProperties`.
- Given that this is really logic is sensitive and crucial to the
correctness of the optimizer, I think we need to have a few dedicated tests for
the `filterBySemanticProperties()` method in the classes `GlobalProperties`,
`LocalProperties`, `RequestedGlobalProperties`, and `RequestedLocalProperties`.
There is currently only one test that test end-to-end some properties, but
leaves out many cases.
---
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.
---
