SPARK-22830 Scala Coding style has been improved in Spark Examples #20016

chetkhatri · 2017-12-19T07:21:20Z

What changes were proposed in this pull request?

Under Spark Scala Examples: Some of the syntax were written like Java way, It has been re-written as per scala style guide.
Most of all changes are followed to println() statement.

How was this patch tested?

Since, All changes proposed are re-writing println statements in scala way, manual run used to test println.

HyukjinKwon

Not sure if it's worth. String interpolation might be closer to Scala syntax vs I wonder if it makes users to understand it more easily, in particular, who are not used to Scala.

I think I don't have a strong preference between them and I'd just leave them out.

chetkhatri · 2017-12-19T07:41:32Z

@HyukjinKwon I agree with you ! But since Spark is scala project - A lot's of developers refer examples available here and if those are Java Developers they might don't understand that this is right way to do in Scala ! I had a same discussion in Scala Days with developers and I think it does make sense here too.

chetkhatri · 2017-12-19T07:41:45Z

@holdenk @sameeragarwal Please review this.

srowen · 2017-12-19T13:40:39Z

examples/src/main/scala/org/apache/spark/examples/DFSReadWriteTest.scala

    parseArgs(args)

-    println("Performing local word count")
+    println(s"Performing local word count")


There is no interpolation here; revert changes like this.

@srowen Thanks for review, I did addressed changes. Please review

srowen · 2017-12-19T13:41:11Z

examples/src/main/scala/org/apache/spark/examples/DFSReadWriteTest.scala

-      println(s"Success! Local Word Count ($localWordCount) " +
-        s"and DFS Word Count ($dfsWordCount) agree.")
+      println(s"Success! Local Word Count ($localWordCount) 
+                and DFS Word Count ($dfsWordCount) agree.")


This should be reverted; you've added a bunch of space to the string.

@srowen Thanks for review, I did addressed changes. Please review.

srowen · 2017-12-19T13:41:47Z

examples/src/main/scala/org/apache/spark/examples/LocalFileLR.scala

    // Initialize w to a random value
    val w = DenseVector.fill(D) {2 * rand.nextDouble - 1}
-    println("Initial w: " + w)
+    println(s"Initial w: ${w}")


You can just write $w in cases like this

@srowen Thanks for review, I did addressed changes. Please review

markhamstra · 2017-12-19T15:44:02Z

examples/src/main/scala/org/apache/spark/examples/BroadcastTest.scala


    for (i <- 0 until 3) {
-      println("Iteration " + i)
+      println(s"Iteration ${i}")


Beyond the unnecessary { } that @srowen has already mentioned, this isn't really a style improvement. "a string " + anotherString is arguably at least as good stylistically as using string interpolation for such simple concatenations of a string reference to the end of a string literal. It's only when there are multiple concatenations and/or multiple string references that interpolation is clearly the better way.

@markhamstra Thank you for valueable suggestion, I am addressed and did new commit.

mgaido91 · 2017-12-19T16:44:10Z

examples/src/main/scala/org/apache/spark/examples/DFSReadWriteTest.scala

-    "\n" +
-    "localFile - (string) local file to use in test\n" +
-    "dfsDir - (string) DFS directory for read/write tests\n"
+    val usage = s"""DFS Read-Write Test 


here you should use

""" |... """.stripMargin

otherwise you introduce a lot of spaces

@mgaido91 Thank you for feedback, changed addressed.

mgaido91 · 2017-12-19T16:45:45Z

examples/src/main/scala/org/apache/spark/examples/LocalALS.scala

  def showWarning() {
    System.err.println(
-      """WARN: This is a naive implementation of ALS and is given as an example!
+      s"""WARN: This is a naive implementation of ALS and is given as an example!


please here revert the change and do the same in all similar places, since there is no variable to interpolate

@mgaido91 Thank you for feedback, changed addressed.

HyukjinKwon · 2017-12-20T02:30:28Z

examples/src/main/scala/org/apache/spark/examples/HdfsTest.scala

      for (x <- mapped) { x + 2 }
      val end = System.currentTimeMillis()
-      println("Iteration " + iter + " took " + (end-start) + " ms")
+      println(s"Iteration ${iter} took ${(end-start)} ms")


Let's just write as $iter and $end-start

@HyukjinKwon $end-start won't work, both are different variables see. I made changes.

HyukjinKwon · 2017-12-20T02:30:59Z

examples/src/main/scala/org/apache/spark/examples/SparkALS.scala

  def showWarning() {
    System.err.println(
-      """WARN: This is a naive implementation of ALS and is given as an example!
+      s"""WARN: This is a naive implementation of ALS and is given as an example!


Seems we don't need s.

@HyukjinKwon Addressed ! Kindly do review

HyukjinKwon · 2017-12-20T02:31:12Z

examples/src/main/scala/org/apache/spark/examples/SparkALS.scala

        slices = slices_.getOrElse("2").toInt
      case _ =>
-        System.err.println("Usage: SparkALS [M] [U] [F] [iters] [partitions]")
+        System.err.println(s"Usage: SparkALS [M] [U] [F] [iters] [partitions]")


ditto for s.

@HyukjinKwon Addressed ! Kindly do review

chetkhatri · 2017-12-20T17:32:39Z

@HyukjinKwon @mgaido91 @srowen All the changes are addressed and committed, please do review and needful.

SparkQA · 2017-12-20T17:53:15Z

Test build #4018 has finished for PR 20016 at commit a14eb3e.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

chetkhatri · 2017-12-20T18:28:29Z

@srowen why only recent commit is going to merge, can't we get "squash merge" ?
Please re-run test build. and let me know if still seems wrong.

srowen · 2017-12-20T18:29:42Z

@chetkhatri not sure what you mean. The whole change here is considered, and doesn't pass style checks. Look at the test results and fix the errors.

chetkhatri · 2017-12-20T18:33:40Z

@srowen Thanks for response, correct - i went through error of jenkins and found error Online[https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4018/console] which fixed accordingly to me and committed so please take a look, if not correct please suggest the same.
Thank you

SparkQA · 2017-12-20T18:59:31Z

Test build #4019 has finished for PR 20016 at commit 319e282.

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

chetkhatri · 2017-12-20T19:03:15Z

@srowen Thank you for re-run, now it passes all.

chetkhatri · 2017-12-20T19:27:35Z

@srowen I think, we can merge this now.

srowen · 2017-12-20T20:48:09Z

Merged to master

SPARK-22830 Scala Coding style has been improved in Spark Examples

4ac1cb1

HyukjinKwon reviewed Dec 19, 2017

View reviewed changes

srowen requested changes Dec 19, 2017

View reviewed changes

markhamstra reviewed Dec 19, 2017

View reviewed changes

[SPARK-22830] [Improvements] Requested changes addressed

8e0e4ee

mgaido91 reviewed Dec 19, 2017

View reviewed changes

[SPARK-22830] Requested changes are being addressed

9da7e36

HyukjinKwon reviewed Dec 20, 2017

View reviewed changes

chetkhatri added 2 commits December 20, 2017 11:20

SPARK-22830 Scala Coding style - Changes made

5d8d0c9

SPARK-22830 Scala Coding style - Changes made

a14eb3e

srowen approved these changes Dec 20, 2017

View reviewed changes

SPARK-22830 Scala Coding style - scala style failed fixed

319e282

asfgit closed this in 792915c Dec 20, 2017

SPARK-22830 Scala Coding style has been improved in Spark Examples #20016

SPARK-22830 Scala Coding style has been improved in Spark Examples #20016

Uh oh!

Conversation

chetkhatri commented Dec 19, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

chetkhatri commented Dec 19, 2017

Uh oh!

chetkhatri commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chetkhatri commented Dec 20, 2017

Uh oh!

SparkQA commented Dec 20, 2017

Uh oh!

chetkhatri commented Dec 20, 2017

Uh oh!

srowen commented Dec 20, 2017

Uh oh!

chetkhatri commented Dec 20, 2017

Uh oh!

SparkQA commented Dec 20, 2017

Uh oh!

chetkhatri commented Dec 20, 2017

Uh oh!

chetkhatri commented Dec 20, 2017

Uh oh!

srowen commented Dec 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chetkhatri commented Dec 19, 2017 •

edited

Loading