[FLINK-6658][cep] Use scala Collections in scala CEP API #3963
Conversation
dawidwys
changed the title
| } | ||
|
|
||
| private[flink] def mapToScala[T](map: JMap[String, JList[T]]): Map[String, Iterable[T]] = { | ||
| map.asScala.mapValues(_.asScala.toIterable).toMap |
There was a problem hiding this comment.
Why not getting rid of the last .toMap and return a collection.Map? This will avoid an iteration over the elements of the JMap which is important given that this method will be invoked at every incoming element. From the rest, I think we are ok as the asScalas are just wrappers and the .mapValues are lazily evaluated.
| pattern: JMap[String, JList[T]], | ||
| timeoutTimestamp: Long, out: Collector[L]) | ||
| : Unit = { | ||
| cleanTimeoutFun(mapToScala(pattern), timeoutTimestamp, out) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are the following two methods added?
There was a problem hiding this comment.
This is the scala way of applying flatFunction. We also provide both of those alternatives for e.g. DataStream#flatMap.
There was a problem hiding this comment.
I was referring to the 2 methods that follow :) . The ones that for example use the foreach (lines 332 and 366).
There was a problem hiding this comment.
Yes, I am also referring to those methods. :) In fact I should write all three of those alternatives.
The version that results with a TraversableOnce is important for composable style of programming in scala. Scala supports constructs like transform1 andThen transform2 which can be applied only in the case with a meaningful return value (not in case of Unit).
| * @param name The name of the pattern. | ||
| */ | ||
| def apply(name: String): Iterable[T] | ||
| } |
There was a problem hiding this comment.
Why having different method names between Java and Scala? In Java this method is called getEventsForPattern(). I would suggest that same methods should have the same name and also, specifically for this, writing context(stateName) does not really seem nice. If you agree I can change it and then merge.
There was a problem hiding this comment.
Right I was also not really decided on that one. Of course you can change it.
|
@dawidwys merged this. Could you close the PR and the related JIRA? |
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the How To Contribute guide.
In addition to going through the list, please provide a meaningful description of your changes.
General
Documentation
Tests & Build
mvn clean verifyhas been executed successfully locally or a Travis build has passed