[GitHub] incubator-flink pull request: ForwardFields Optimizer integration

[StephanEwen <gi...@git.apache.org>]

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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---