[GitHub] flink pull request: [FLINK-1745] Add exact k-nearest-neighbours al...

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

Github user chiwanpark commented on the pull request:

    https://github.com/apache/flink/pull/1220#issuecomment-170875879
  
    Hi @danielblazevski, I'm sorry for late reply. If you turn off IntelliJ IDEA align option (Turn off Preferences -> Editor -> Code Style -> Scala -> Wrapping and Braces -> Method declaration parameters -> Align when multiline), you can get style that is suggested by @tillrohrmann.
    
    Could you apply this option?
    
    I think that your PR is almost ready to merge. But I have to check few problems that still exist.
    
    First, about a meaning of `UseQuadTree` parameter, you said that it means force-use quadtree. I think this would be a problem because `DistanceMeasure` parameter can be conflict with quadtree. I would like to raise an error earlier if the parameter setting has a problem. Could you add this into top of fit operation?
    
    Second, how about avoiding `cross` operation? As @tillrohrmann said, `cross` operation is a very heavy operation. Is there any nicer solution to this?
    
    Other problems such as some difference styles, unnecessary spaces can be addressed by me before merge this. :)


---
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.
---