Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/311#issuecomment-70421463
Look good. The test coverage is massive - very good for a delicate concept
like this :-)
One remaining issue is the naming of the methods/concepts and how they are
exposed in the APIs.
- I personally liked it better the way it was before. For example, binary
operators returned `DualInputSematicProperties` and the methods were
`getForwardedFieldsFirst(int field)`. That looked more intuitive for API users
than operating on the interface `SemanticProperties` and using
`getTargetFields(input, position)`.
- The backwards access (get source field for target field) is relevant
only in the optimizer, correct?
Is there some way to expose that to th eoptimizer only? It makes the public
API more confusing.
- Is it necessary to have the extra methods
`hasFieldForwardInformation()`? This again makes it more complicated to use.
Can we not simply have a null entry for fields that have no forward information?
---
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.
---
