Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-15322][SQL][FOLLOWUP] Use the new long accumulator for old int accumulators. #13434

Closed
wants to merge 3 commits into from
Closed

[SPARK-15322][SQL][FOLLOWUP] Use the new long accumulator for old int accumulators. #13434

wants to merge 3 commits into from

Conversation

HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Jun 1, 2016 •
edited
Loading

What changes were proposed in this pull request?

This PR corrects the remaining cases for using old accumulators.

This does not change some old accumulator usages below:

  • ImplicitSuite.scala - Tests dedicated to old accumulator, for implicits with AccumulatorParam
  • AccumulatorSuite.scala - Tests dedicated to old accumulator
  • JavaSparkContext.scala - For supporting old accumulators for Java API.
  • debug.package.scala - Usage with HashSet[String]. Currently, it seems no implementation for this. I might be able to write an anonymous class for this but I didn't because I think it is not worth writing a lot of codes only for this.
  • SQLMetricsSuite.scala - This uses the old accumulator for checking type boxing. It seems new accumulator does not require type boxing for this case whereas the old one requires (due to the use of generic).

How was this patch tested?

Existing tests cover this.

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Jun 1, 2016 •
edited
Loading

@srowen @andrewor14 I am sorry that I happened to open the same subject's PR twice. Could you please take a look?

@SparkQA
Copy link

SparkQA commented Jun 1, 2016

Test build #59722 has finished for PR 13434 at commit d9cf1cf.

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

srowen
srowen reviewed Jun 1, 2016
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/columnar/InMemoryTableScanExec.scala
@@ -289,8 +288,8 @@ private[sql] case class InMemoryTableScanExec(
sqlContext.getConf("spark.sql.inMemoryTableScanStatistics.enable", "false").toBoolean

// Accumulators used for testing purposes
lazy val readPartitions: Accumulator[Int] = sparkContext.accumulator(0)
lazy val readBatches: Accumulator[Int] = sparkContext.accumulator(0)
lazy val readPartitions: LongAccumulator = sparkContext.longAccumulator
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 even sure we need to declare the type of these, unless I'm really missing a reason for it

@srowen
Copy link
Member

srowen commented Jun 1, 2016

Are these all the other instances of making an Accumulator[Int]?

@srowen
Copy link
Member

srowen commented Jun 1, 2016

But LGTM, I don't mind even merging as-is.

@andrewor14
Copy link
Contributor

LGTM2

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Jun 2, 2016 •
edited
Loading

@srowen @andrewor14 Please let me double check if there are more. Could you a bit wait please?

@HyukjinKwon
Copy link
Member Author

There were some tests in ReplSuite.scala which I could not catch because they are strings. It seems there are some cases in programming-guide.md and streaming-programming-guide.md as well but I left them. I hope they are changed by someone who has a better writing skill than me.

@SparkQA
Copy link

SparkQA commented Jun 2, 2016

Test build #59799 has finished for PR 13434 at commit d7caeb7.

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

@srowen
Copy link
Member

srowen commented Jun 2, 2016

Merged to master/2.0

asfgit pushed a commit that referenced this pull request Jun 2, 2016
@HyukjinKwon @srowen
[SPARK-15322][SQL][FOLLOWUP] Use the new long accumulator for old int…
fe639ad 
… accumulators.

## What changes were proposed in this pull request?

This PR corrects the remaining cases for using old accumulators.

This does not change some old accumulator usages below:

- `ImplicitSuite.scala` - Tests dedicated to old accumulator, for implicits with `AccumulatorParam`

- `AccumulatorSuite.scala` -  Tests dedicated to old accumulator

- `JavaSparkContext.scala` - For supporting old accumulators for Java API.

- `debug.package.scala` - Usage with `HashSet[String]`. Currently, it seems no implementation for this. I might be able to write an anonymous class for this but I didn't because I think it is not worth writing a lot of codes only for this.

- `SQLMetricsSuite.scala` - This uses the old accumulator for checking type boxing. It seems new accumulator does not require type boxing for this case whereas the old one requires (due to the use of generic).

## How was this patch tested?

Existing tests cover this.

Author: hyukjinkwon <[email protected]>

Closes #13434 from HyukjinKwon/accum.

(cherry picked from commit 252417f)
Signed-off-by: Sean Owen <[email protected]>
@asfgit asfgit closed this in 252417f Jun 2, 2016
@HyukjinKwon HyukjinKwon deleted the accum branch January 2, 2018 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HyukjinKwon @SparkQA @srowen @andrewor14