Skip to content

Conversation

@HyukjinKwon
Copy link
Member

What changes were proposed in this pull request?

This PR removes extra anonymous closure within functional transformations.

For example,

.map(item => {
  ...
})

which can be just simply as below:

.map { item => 
  ...
}

How was this patch tested?

Related unit tests and sbt scalastyle.

@HyukjinKwon
Copy link
Member Author

cc @rxin

@rxin
Copy link
Contributor

rxin commented Apr 14, 2016

LGTM pending tests.

@SparkQA
Copy link

SparkQA commented Apr 14, 2016

Test build #55793 has finished for PR 12382 at commit a62b333.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member Author

retest this please

val words = lines.flatMap(_.split(" "))
val wordCounts = words.map(x => (x, 1)).reduceByKey(_ + _)
wordCounts.foreachRDD((rdd: RDD[(String, Int)], time: Time) => {
wordCounts.foreachRDD { (rdd: RDD[(String, Int)], time: Time) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're here, can this just be (rdd, time) =>? and likewise in the following file, maybe others

@SparkQA
Copy link

SparkQA commented Apr 14, 2016

Test build #55802 has finished for PR 12382 at commit a62b333.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Apr 14, 2016

Merged to master

@asfgit asfgit closed this in 6fc3dc8 Apr 14, 2016
@andrewor14
Copy link
Contributor

Maybe we should add a scalastyle rule for this

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Apr 15, 2016

@andrewor14 Hm.. I tried to add a rule but I realised (or I think) it cannot find perfectly by a static way. One of the problems I met was below:

For example, there is a class GraphOps and at line 74,

graph.aggregateMessages(ctx => { ctx.sendToSrc(1); ctx.sendToDst(1) }, _ + _,
    TripletFields.None)
}

In this case, this should not be the error case since ( and ) exists for other arguments _ + _ and TripletFields.None.

In order to check this, it should be able to check , after ctx => { ctx.sendToSrc(1); ctx.sendToDst(1) }. In order to skip ctx => { ctx.sendToSrc(1); ctx.sendToDst(1) }, we need to skip the range between { and } which might have nested {, } and ,. AFAIK, it is impossible for regex in theory but it looks possible in practical by recursive regular expression but it looks scalastyle does not support this.

Maybe I am not smart enough but this was just my conclusion.

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Apr 15, 2016

@andrewor14 @srowen Do you mind If I open another (minor) PR for some more corrections?

While trying to test regular expressions, I found some more things to correct about this and about @srowen comment, which are pretty many.

+If you think it is able to add a rule, then I won't.

@srowen
Copy link
Member

srowen commented Apr 15, 2016

If there's a built-in scalastyle rule for this, let's enable it, but otherwise not sure it's worth building custom code to enforce it. If you've found ways to look for more instances of this and found more classes of this problem, OK open a new PR.

@HyukjinKwon HyukjinKwon changed the title [MINOR][SQL] Remove extra anonymous closure within functional transformations [MINOR] Remove extra anonymous closure within functional transformations Apr 15, 2016
zzcclp added a commit to zzcclp/spark that referenced this pull request Jul 27, 2016
…riting apache#14323

[MINOR] Remove extra anonymous closure within functional transformations apache#12382  just JdbcUtils.scala
@HyukjinKwon HyukjinKwon deleted the minor-extra-closers branch January 2, 2018 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants