[SPARK-21973][SQL] Add an new option to filter queries in TPC-DS #19188

maropu · 2017-09-11T07:42:09Z

What changes were proposed in this pull request?

This pr added a new option to filter TPC-DS queries to run in TPCDSQueryBenchmark.
By default, TPCDSQueryBenchmark runs all the TPC-DS queries.
This change could enable developers to run some of the TPC-DS queries by this option,
e.g., to run q2, q4, and q6 only:

spark-submit --class <this class> --conf spark.sql.tpcds.queryFilter="q2,q4,q6" --jars <spark sql test jar>

How was this patch tested?

Manually checked.

maropu · 2017-09-11T07:45:41Z

sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/TPCDSQueryBenchmark.scala

This change is not directly related to this pr though, I added this log here cuz this change is much trivial and I think this log helps to easily check which query fails.

SparkQA · 2017-09-11T07:49:13Z

Test build #81633 has finished for PR 19188 at commit 4665e85.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class TpcdsQueries(spark: SparkSession, queries: Seq[String], dataLocation: String)

kiszk · 2017-09-11T08:35:55Z

sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/TPCDSQueryBenchmark.scala

nit: class -> variable?

kiszk · 2017-09-11T08:40:20Z

sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/TPCDSQueryBenchmark.scala

nit: Do we need queries =?

SparkQA · 2017-09-11T15:01:14Z

Test build #81637 has finished for PR 19188 at commit 322c335.

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

gatorsmile · 2017-09-11T23:19:58Z

sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/TPCDSQueryBenchmark.scala

Could we add an argument, instead of using the SQLConf? See #18592 (comment)

gatorsmile · 2017-09-12T17:50:50Z

This #18592 has been merged. @maropu Could you update this PR? Thanks!

maropu · 2017-09-12T23:40:52Z

ok, I'll update in a day! Thanks!

maropu · 2017-09-13T02:25:41Z

sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/TPCDSQueryBenchmark.scala

      benchmark.addCase(name) { i =>
        spark.sql(queryString).collect()
      }
+      logInfo(s"\n\n===== TPCDS QUERY BENCHMARK OUTPUT FOR $name =====\n")


See #19188 (comment)

viirya · 2017-09-13T02:28:31Z

sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/TPCDSQueryBenchmark.scala

+    // If `--query-filter` defined, filters the queries that this option selects
+    val queriesToRun = if (benchmarkArgs.queryFilter.nonEmpty) {
+      val queries = tpcdsQueries.filter { case queryName =>
+        benchmarkArgs.queryFilter.contains(queryName)


Add case insensitive?

yea, I like the idea.

viirya · 2017-09-13T02:31:11Z

sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/TPCDSQueryBenchmark.scala

+        benchmarkArgs.queryFilter.contains(queryName)
+      }
+      if (queries.isEmpty) {
+        throw new RuntimeException("Bad query name filter: " + benchmarkArgs.queryFilter)


"Empty queries to run. Bad query name filter: " + benchmarkArgs.queryFilter.

viirya · 2017-09-13T02:34:31Z

sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/TPCDSQueryBenchmark.scala

 /**
 * Benchmark to measure TPCDS query performance.
 * To run this:
 *  spark-submit --class <this class> <spark sql test jar> <TPCDS data location>


Update this usage text too?

maropu · 2017-09-13T02:52:41Z

Also, I manually checked if it worked.

viirya · 2017-09-13T02:57:47Z

LGTM with few minor comments.

maropu · 2017-09-13T03:05:47Z

better to add tests for TPCDSQueryBenchmarkArguments? we have tests for SparkSubmitArguments in SparkSubmitSuite though.

viirya · 2017-09-13T03:07:51Z

Yeah, it's good if you can add one.

viirya · 2017-09-13T03:10:22Z

But looks like current TPCDSQueryBenchmarkArguments is not good to test individually...

maropu · 2017-09-13T03:18:56Z

hmmm. ok, currently, we have two options only, so I feel okay to keep it now.

SparkQA · 2017-09-13T04:51:06Z

Test build #81701 has finished for PR 19188 at commit 12767bc.

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

SparkQA · 2017-09-13T05:03:26Z

Test build #81702 has finished for PR 19188 at commit be1a199.

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

SparkQA · 2017-09-13T05:59:23Z

Test build #81703 has finished for PR 19188 at commit cc11163.

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

maropu · 2017-09-13T06:01:37Z

@gatorsmile could u check? Thanks~

viirya · 2017-09-13T07:41:04Z

sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/TPCDSQueryBenchmark.scala

 * Benchmark to measure TPCDS query performance.
 * To run this:
- *  spark-submit --class <this class> <spark sql test jar> <TPCDS data location>
+ *  spark-submit --class <this class> <spark sql test jar> --data-location <TPCDS data location>


This looks incorrect?

sorry, but I missed your point. what's correct?

--data-location <TPCDS data location> [--query-filter Queries to filter]?

aha, thanks. better to add optional parameters here? I like a simple example here.

gatorsmile · 2017-09-13T17:09:36Z

...e/src/test/scala/org/apache/spark/sql/execution/benchmark/TPCDSQueryBenchmarkArguments.scala

          args = tail

+        case ("--query-filter") :: value :: tail =>
+          queryFilter = value.toLowerCase(Locale.ROOT).split(",").map(_.trim).toSet


Could you also make "--data-location" case insensitive?

gatorsmile · 2017-09-13T23:23:13Z

...e/src/test/scala/org/apache/spark/sql/execution/benchmark/TPCDSQueryBenchmarkArguments.scala

Sorry, I mean the option name --data-location need to be case insensitive?

oh! missed...

SparkSubmitArguments handles options as case-sensitive, so better to make them case-insensitive, too?

I am not sure about that one.

SparkQA · 2017-09-14T00:49:01Z

Test build #81740 has finished for PR 19188 at commit c957e47.

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

SparkQA · 2017-09-14T03:45:01Z

Test build #81745 has finished for PR 19188 at commit b543e71.

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

gatorsmile · 2017-09-14T04:53:43Z

LGTM

gatorsmile · 2017-09-14T04:53:49Z

Merging to master.

maropu commented Sep 11, 2017

View reviewed changes

maropu mentioned this pull request Sep 11, 2017

[SPARK-21368][SQL] TPCDSQueryBenchmark can't refer query files. #18592

Closed

kiszk reviewed Sep 11, 2017

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/TPCDSQueryBenchmark.scala Outdated

Copy link

Member

kiszk Sep 11, 2017

Choose a reason for hiding this comment

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

nit: class -> variable?

kiszk reviewed Sep 11, 2017

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/TPCDSQueryBenchmark.scala Outdated

Copy link

Member

kiszk Sep 11, 2017

Choose a reason for hiding this comment

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

nit: Do we need queries =?

maropu force-pushed the RunPartialQueriesInTPCDS branch from 9d9eff2 to 322c335 Compare September 11, 2017 09:19

gatorsmile reviewed Sep 11, 2017

View reviewed changes

maropu force-pushed the RunPartialQueriesInTPCDS branch 2 times, most recently from 9eedcd5 to 12767bc Compare September 13, 2017 02:14

Add a new option to filter queries

be1a199

maropu force-pushed the RunPartialQueriesInTPCDS branch from 12767bc to be1a199 Compare September 13, 2017 02:24

maropu commented Sep 13, 2017

View reviewed changes

viirya reviewed Sep 13, 2017

View reviewed changes

Fix

cc11163

viirya reviewed Sep 13, 2017

View reviewed changes

gatorsmile reviewed Sep 13, 2017

View reviewed changes

case-insensitive option matches

b543e71

maropu force-pushed the RunPartialQueriesInTPCDS branch from c957e47 to b543e71 Compare September 14, 2017 01:15

asfgit closed this in 8be7e6b Sep 14, 2017

[SPARK-21973][SQL] Add an new option to filter queries in TPC-DS #19188

[SPARK-21973][SQL] Add an new option to filter queries in TPC-DS #19188

Uh oh!

Conversation

maropu commented Sep 11, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

maropu Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maropu commented Sep 12, 2017

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!

maropu commented Sep 13, 2017

Uh oh!

viirya commented Sep 13, 2017

Uh oh!

maropu commented Sep 13, 2017

Uh oh!

viirya commented Sep 13, 2017

Uh oh!

viirya commented Sep 13, 2017

Uh oh!

maropu commented Sep 13, 2017

Uh oh!

SparkQA commented Sep 13, 2017

Uh oh!

SparkQA commented Sep 13, 2017

Uh oh!

SparkQA commented Sep 13, 2017

Uh oh!

maropu commented Sep 13, 2017

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!

maropu Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

maropu Sep 11, 2017 •

edited

Loading

gatorsmile commented Sep 12, 2017 •

edited

Loading

maropu Sep 13, 2017 •

edited

Loading