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-25581][SQL] Rename method benchmark as runBenchmarkSuite in BenchmarkBase #22599

Closed
wants to merge 3 commits into from
Closed

[SPARK-25581][SQL] Rename method benchmark as runBenchmarkSuite in BenchmarkBase #22599

wants to merge 3 commits into from

Conversation

gengliangwang
Copy link
Member

@gengliangwang gengliangwang commented Oct 1, 2018 •
edited
Loading

What changes were proposed in this pull request?

Rename method benchmark in BenchmarkBase as runBenchmarkSuite . Also add comments.
Currently the method name benchmark is a bit confusing. Also the name is the same as instances of Benchmark:

spark/sql/hive/src/test/scala/org/apache/spark/sql/hive/orc/OrcReadBenchmark.scala

Lines 330 to 339 in f246813

benchmark.addCase("Hive built-in ORC") { _ =>
spark.sql(s"SELECT sum(c$middle) FROM hiveOrcTable").collect()
}
benchmark.run()
}
}
}
override def benchmark(): Unit = {

How was this patch tested?

Unit test.

@gengliangwang
Copy link
Member Author

@wangyum @dongjoon-hyun @cloud-fan

dongjoon-hyun
dongjoon-hyun reviewed Oct 1, 2018
View reviewed changes
core/src/test/scala/org/apache/spark/benchmark/BenchmarkBase.scala Outdated
/**
* Main process of the whole benchmark.
*/
def benchmarkSuite(): Unit
Copy link
Member

Choose a reason for hiding this comment

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

I got the idea of this PR. But, for me, benchmarkSuite looks like a test suite again. Isn't it confusing in another way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well it means a collection of benchmarks. Just like TestSuite is a collection of test cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the word suite, it doesn't have to be about testing.

jiangxb1987
jiangxb1987 approved these changes Oct 1, 2018
View reviewed changes
Copy link
Contributor

@jiangxb1987 jiangxb1987 left a comment

Choose a reason for hiding this comment

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

LGTM

@SparkQA
Copy link

SparkQA commented Oct 1, 2018

Test build #96816 has finished for PR 22599 at commit 289ee3e.

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

@SparkQA
Copy link

SparkQA commented Oct 1, 2018

Test build #96818 has finished for PR 22599 at commit 78e48ea.

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

@gengliangwang gengliangwang changed the title [SPARK-25581][SQL] Rename method benchmark as benchmarkSuite in BenchmarkBase [SPARK-25581][SQL] Rename method benchmark as runBenchmarkSuite in BenchmarkBase Oct 2, 2018
@gengliangwang
Copy link
Member Author

Discuss with @cloud-fan offline. Rename method benchmark as runBenchmarkSuite. Also add comment to guide developers to use runBenchmark for each scenario in implementations.

@dongjoon-hyun @wangyum Is this OK to you?

@wangyum
Copy link
Member

wangyum commented Oct 2, 2018

Thanks @gengliangwang I agree with you. It’s a good change.

@SparkQA
Copy link

SparkQA commented Oct 2, 2018

Test build #96855 has finished for PR 22599 at commit 1c15c25.

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

dongjoon-hyun
dongjoon-hyun approved these changes Oct 2, 2018
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Merged to master.

@asfgit asfgit closed this in 7b4e94f Oct 2, 2018
@dongjoon-hyun dongjoon-hyun mentioned this pull request Oct 2, 2018
Closed
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@gengliangwang @jackylee-ch
[SPARK-25581][SQL] Rename method benchmark as runBenchmarkSuite i…
22f3194 
…n `BenchmarkBase`

## What changes were proposed in this pull request?

Rename method `benchmark` in `BenchmarkBase` as `runBenchmarkSuite `. Also add comments.
Currently the method name `benchmark` is a bit confusing. Also the name is the same as instances of `Benchmark`:

https://github.com/apache/spark/blob/f246813afba16fee4d703f09e6302011b11806f3/sql/hive/src/test/scala/org/apache/spark/sql/hive/orc/OrcReadBenchmark.scala#L330-L339

## How was this patch tested?

Unit test.

Closes apache#22599 from gengliangwang/renameBenchmarkSuite.

Authored-by: Gengliang Wang <gengliang.wang@databricks.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jiangxb1987 jiangxb1987 jiangxb1987 approved these changes

@wangyum wangyum wangyum approved these changes

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gengliangwang @SparkQA @wangyum @jiangxb1987 @dongjoon-hyun