Skip to content

Conversation

@keypointt
Copy link
Contributor

What changes were proposed in this pull request?

Clean up unused variables and unused import statements, unnecessary return and toArray, and some more style improvement, when I walk through the code examples.

How was this patch tested?

Testet manually on local laptop.


/**
* Create an [[org.apache.spark.Accumulator]] variable of a given type, which tasks can "add"
* Create an [[org.apache.spark.Accumulator]] variable of a given type, to which tasks can "add"
Copy link
Member

Choose a reason for hiding this comment

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

This isn't grammatical now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh right...reverting it...


val d = sc.parallelize(1 to 20)
an [Exception] should be thrownBy {d.foreach{x => acc.value = x}}
an [SparkException] should be thrownBy {d.foreach{x => acc.value = x}}
Copy link
Contributor

Choose a reason for hiding this comment

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

can you change this to

intercept[SparkException] {
  d.foreach(x => acc.value = x)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, doing it now

@SparkQA
Copy link

SparkQA commented Aug 26, 2016

Test build #64501 has finished for PR 14836 at commit ec656ec.

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

@keypointt
Copy link
Contributor Author

keypointt commented Aug 26, 2016

I just noticed that there are many other places where 'PropSpec' style is used with Matchers, while I guess spark project is mainly using FunSuite style

@SparkQA
Copy link

SparkQA commented Aug 27, 2016

Test build #64506 has finished for PR 14836 at commit a5e1db3.

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

@SparkQA
Copy link

SparkQA commented Aug 27, 2016

Test build #64504 has finished for PR 14836 at commit 70a751c.

  • This patch fails from timeout after a configured wait of 250m.
  • This patch merges cleanly.
  • This patch adds no public classes.

@keypointt
Copy link
Contributor Author

Jenkins retest this please

@SparkQA
Copy link

SparkQA commented Aug 27, 2016

Test build #64513 has finished for PR 14836 at commit a5e1db3.

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

@SparkQA
Copy link

SparkQA commented Aug 27, 2016

Test build #64522 has finished for PR 14836 at commit 217ff7a.

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

withSQLConf("spark.sql.autoBroadcastJoinThreshold" -> "2") {
val df = spark.range(100).toDF()
val join = df.join(df, "id")
val plan = join.queryExecution.executedPlan
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is without side effects, even though clearly the variable isn't used. I don't think you can remove the line, necessarily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I look carefully about it and you are right. I'll undo it.

@srowen
Copy link
Member

srowen commented Aug 29, 2016

I think the other changes are trivial but not wrong. I'd generally not bother with these bitty changes. It's not that they're wrong but that it takes me some time to go think through whether they're valid, and it's probably not worth our time collectively.

@keypointt
Copy link
Contributor Author

Thanks Sean, I got what you mean. I'll avoid such PRs in the future, thanks a lot for the tips.

@SparkQA
Copy link

SparkQA commented Aug 29, 2016

Test build #64584 has finished for PR 14836 at commit 95a6e1f.

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

@srowen
Copy link
Member

srowen commented Aug 30, 2016

Merged to master

@asfgit asfgit closed this in 2720925 Aug 30, 2016
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.

4 participants