SPARK-22896 Improvement in String interpolation by chetkhatri · Pull Request #20070 · apache/spark

chetkhatri · 2017-12-24T08:35:41Z

What changes were proposed in this pull request?

String interpolation in ml pipeline example has been corrected as per scala standard.

How was this patch tested?

manually tested.

…hetan-contrib

srowen · 2017-12-26T14:42:37Z

OK, we can do this for consistency. In general we don't spend a ton of time on changes like this, because there is already way too much else to review. Please make one change only (this one) that updates all examples in one go.

chetkhatri · 2017-12-26T16:38:32Z

@srowen Absolutely correct, this all in one shot. I did changes in all.

chetkhatri · 2017-12-26T17:13:33Z

@srowen also i did merge another similiar PR with graphx to here. so Just FYI - we are good.

srowen · 2017-12-26T17:53:37Z

I think there are many more examples of this; a quick search suggests there are about 40 example files that have some string concatenation.

chetkhatri · 2017-12-26T17:56:07Z

In scala ? I don't think so. I am re-iterating and doing double check.

chetkhatri · 2017-12-26T17:57:16Z

You're correct - I missed other packages. I will re-confirm soon. Thanks.

… - 2

chetkhatri · 2017-12-26T19:20:04Z

@srowen I rechecked all scala examples and this is commulative PR for the same.

srowen

A few small comments here but looking fine as an improvement. Also have a look for more old .format() calls if you like; we could update those.

srowen · 2017-12-26T22:22:43Z

 .partitionBy(partitionStrategy.getOrElse(RandomVertexCut)).cache()
 val triangles = TriangleCount.run(graph)
- println("Triangles: " + triangles.vertices.map {
+ println(s"Triangles: ${triangles.vertices.map {


This is probably more readable if the expression is stored in a local val first. Anything nontrivial like this gets hard to parse in an interpolated string. While we're here, worth fixing up.

srowen · 2017-12-26T22:23:09Z

- println("pValues = " + chi.getAs[Vector](0))
- println("degreesOfFreedom = " + chi.getSeq[Int](1).mkString("[", ",", "]"))
- println("statistics = " + chi.getAs[Vector](2))
+ println(s"pValues = ${chi.getAs[Vector](0)}")


I think this is OK; anything more complex I might suggest breaking out the expression into a val.

srowen · 2017-12-26T22:23:28Z

 val df = data.map(Tuple1.apply).toDF("features")
 val Row(coeff1: Matrix) = Correlation.corr(df, "features").head
- println("Pearson correlation matrix:\n" + coeff1.toString)
+ println(s"Pearson correlation matrix:\n ${coeff1.toString}")


Another thing we could improve: .toString is redundant here I believe

srowen · 2017-12-26T22:24:03Z

 }

- /** Number of classes the label can take. 2 indicates binary classification. */
+ // Number of classes the label can take. 2 indicates binary classification.


Yeah, good to make this a standard comment, not scaladoc style

srowen · 2017-12-26T22:25:07Z

 // $example on$
 val data = Array((0, 18.0), (1, 19.0), (2, 8.0), (3, 5.0), (4, 2.2))
- val df = spark.createDataFrame(data).toDF("id", "hour")
+ val df = spark.createDataFrame(data).toDF("id", "hour").repartition(1)


Although it looks weird, I think the author intended the repartition(1) to not appear in the body of the example that's copied into the docs. I wouldn't change this.

srowen · 2017-12-26T22:26:20Z

 var userInput: String = null
 try {
- logInfo("Connecting to " + host + ":" + port)
+ logInfo(s"Connecting to $host $port")


Nit: we could make the string consistent with the one two lines below by adding a colon

Done, addressed

SparkQA · 2017-12-28T13:34:57Z

Test build #4026 has finished for PR 20070 at commit 5507cad.

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

srowen · 2017-12-28T13:51:05Z


 }
-// scalastyle:on println
+// scalastyle:on println


It didn't like the lack of newline at the end of this file

Done, addressed.

ghost

@srowen apologize for delay :) I have addressed suggestion. thanks

ghost · 2017-12-28T17:57:02Z


 }
-// scalastyle:on println
+// scalastyle:on println


Done, addressed.

ghost · 2017-12-28T18:01:26Z

 var userInput: String = null
 try {
- logInfo("Connecting to " + host + ":" + port)
+ logInfo(s"Connecting to $host $port")


Done, addressed

ghost · 2017-12-29T00:13:52Z

Test build #4028 has finished for PR 20070 at commit 79e6789.

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

ghost · 2017-12-29T06:10:46Z

@srowen please do re-run the build.

ghost · 2017-12-29T13:41:48Z

Test build #4029 has finished for PR 20070 at commit 70ce734.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

ghost · 2017-12-29T13:43:53Z

- println(s"Chose ${categoricalFeatures.size} categorical features: " +
- categoricalFeatures.mkString(", "))
+ println(s"Chose ${categoricalFeatures.size} " +
+s"categorical features: {$categoricalFeatures.mkString(", ")}")


Typo here. Please make sure build/test/style passes locally before pushing again, or else this takes a lot of work to review.

I did fixed this. Can you please give me steps as a check list before commit for test.

./dev/run-tests

ghost · 2017-12-30T16:57:22Z

Test build #4031 has finished for PR 20070 at commit 0321faf.

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

ghost · 2017-12-30T17:36:25Z

@srowen Okey. current status looks good

ghost · 2017-12-30T21:29:27Z


 val result = discretizer.fit(df).transform(df)
- result.show()
+ result.show(false)


One more question - is it necessary to make this not truncate?

We're following same style in other examples so it is good to do.

Which other examples? most do not set this, and the Java equivalent doesn't either. If there's a good reason that the output needs to be untruncated, that's fine, just also change the Java example.

@srowen correct either way it works for ex. examples/ml/LDAExamples.scala

ghost · 2017-12-30T21:30:25Z

 val featureTestResults: Array[ChiSqTestResult] = Statistics.chiSqTest(obs)
 featureTestResults.zipWithIndex.foreach { case (k, v) =>
- println("Column " + (v + 1).toString + ":")
+ println(s"Column ${(v + 1).toString} :")


.toString is redundant here and elsewhere with interpolation. I think that should be simplified.

@srowen Thanks , Changes addressed

ghost · 2017-12-30T21:30:58Z

- print("Topic " + topic + ":")
- for (word <- Range(0, ldaModel.vocabSize)) { print(" " + topics(word, topic)); }
+ print(s"Topic $topic :")
+ for (word <- Range(0, ldaModel.vocabSize)) { print(s" ${topics(word, topic)}") }


Go ahead and put the print on a new line (I know it wasn't before)

@srowen Thanks for suggestion, it has been addressed.

ghost

Still more instances of .toString. Just search on the changes page, even, to see them.

ghost · 2017-12-31T16:51:10Z


 val result = discretizer.fit(df).transform(df)
- result.show()
+ result.show(false)


Which other examples? most do not set this, and the Java equivalent doesn't either. If there's a good reason that the output needs to be untruncated, that's fine, just also change the Java example.

ghost · 2017-12-31T16:52:08Z

 print(s"Topic $topic :")
- for (word <- Range(0, ldaModel.vocabSize)) { print(s" ${topics(word, topic)}") }
+ for (word <- Range(0, ldaModel.vocabSize))
+ {


No, we put the open brace on the preceding line with for, and don't triple indent. See any other for loop in the code.

@srowen sure done.

ghost · 2018-01-03T15:26:04Z

@srowen Request for review when you get on this.

ghost · 2018-01-03T16:26:51Z

Test build #4033 has finished for PR 20070 at commit e891f53.

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

ghost · 2018-01-03T17:31:46Z

Merged to master/2.3

chetkhatri added 3 commits December 24, 2017 14:03

SPARK-22896 Improvement in String interpolation
9916fd1

SPARK-22896 Improvement in String interpolation - fixed typo
162ac27

Merge branch 'master' ofhttps://github.com/apache/sparkinto mllib-c…
aa2de00
…hetan-contrib

srowen mentioned this pull request Dec 26, 2017
SPARK-22896 Improvement in String interpolation | Graphx #20071
Closed

graphx changed got merged to mllib example
74d41d7

chetkhatri added 2 commits December 27, 2017 00:31

All examples under spark scala - String interpolation has implemented
8d729fa

All examples under spark scala - String interpolation has implemented…
5507cad
… - 2

srowen requested changes Dec 26, 2017
View reviewed changes

srowen reviewed Dec 28, 2017
View reviewed changes

Spark scala code change for request addressed
79e6789

chetkhatri commented Dec 28, 2017
View reviewed changes

new line at the end of file added
70ce734

srowen reviewed Dec 29, 2017
View reviewed changes

typo fixed
0321faf

srowen requested changes Dec 30, 2017
View reviewed changes

Review changes addressed
c33e90c

srowen requested changes Dec 31, 2017
View reviewed changes

chetkhatri added 2 commits January 1, 2018 15:30

Changes - II addressed
18d047f

Changes - III addressed
e891f53

asfgit closed this in 9a2b65aJan 3, 2018

Uh oh!

Conversation

chetkhatri commented Dec 24, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen commented Dec 26, 2017

Uh oh!

chetkhatri commented Dec 26, 2017

Uh oh!

chetkhatri commented Dec 26, 2017

Uh oh!

srowen commented Dec 26, 2017

Uh oh!

chetkhatri commented Dec 26, 2017

Uh oh!

chetkhatri commented Dec 26, 2017

Uh oh!

chetkhatri commented Dec 26, 2017

Uh oh!

srowen left a comment

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!

SparkQA commented Dec 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghostDec 28, 2017

Choose a reason for hiding this comment

Uh oh!

ghostDec 28, 2017

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 29, 2017

Uh oh!

ghost commented Dec 29, 2017

Uh oh!

ghost commented Dec 29, 2017

Uh oh!

ghostDec 29, 2017

Choose a reason for hiding this comment

Uh oh!

ghostDec 29, 2017

Choose a reason for hiding this comment

Uh oh!

ghostDec 30, 2017

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 30, 2017

Uh oh!

ghost commented Dec 30, 2017

ghost commented Jan 3, 2018•
edited by srowen
Loading