[SPARK-23167][SQL] Add TPCDS queries v2.7 in TPCDSQuerySuite #20343

maropu · 2018-01-21T15:55:48Z

What changes were proposed in this pull request?

This pr added TPCDS v2.7 (latest) queries in TPCDSQuerySuite because the current TPCDSQuerySuite tests older one (v1.4) and some queries are different from v1.4 and v2.7. Since the original v2.7 queries have the syntaxes that Spark cannot parse, I changed these queries in a following way:

[date] + 14 days -> date + INTERVAL 14 days
[column name] as "30 days" -> [column name] as `30 days`
Fix some syntax errors, e.g., missing brackets

How was this patch tested?

Added tests in TPCDSQuerySuite.

SparkQA · 2018-01-21T15:59:43Z

Test build #86444 has finished for PR 20343 at commit 71e0e1a.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
i_class in ('personal','portable','reference','self-help') and
i_class in ('accessories','classical','fragrances','pants') and
and i_class in ('personal','portable','reference','self-help')
and i_class in ('accessories','classical','fragrances','pants')
( select i_category ,i_class ,i_brand ,i_product_name ,d_year ,d_qoy ,d_moy ,s_store_id
( select sum(ws_net_paid) as total_sum, i_category, i_class, 0 as g_category, 0 as g_class
i_class in ('wallpaper','parenting','musical')
i_class in ('womens','birdal','pants')

gatorsmile · 2018-01-21T16:02:38Z

sql/core/src/test/scala/org/apache/spark/sql/TPCDSQuerySuite.scala

Could we just add the queries that are different from the v1.4 version?

@maropu . It's great to have v2.7.
Could you check the schema too?
For example, we had better update the following in the schema?

- |`web_country` STRING, `web_gmt_offset` STRING, `web_tax_percentage` DECIMAL(5,2)) + |`web_country` STRING, `web_gmt_offset` DECIMAL(5,2), `web_tax_percentage` DECIMAL(5,2))

oh, good catch. Why we use a string for web_gmt_offset? It's just a bug? Anyway, I'll check all the schema again. Should we include this fix in this pr, or follow-up? @gatorsmile

Thanks. @maropu .
While reviewing this, I found that we missed that bug at the original PR of @gatorsmile .
If the fix is able to be included in Apache Spark 2.3, I think the followup PR also sounds good to me.

ok, thanks.

This is only related to test cases. Thus, it is fine Spark 2.3 release does not have it. You can do it in this PR.

Actually, this PR can be merged as long as we can fix all the issues.

SparkQA · 2018-01-21T21:05:08Z

Test build #86448 has finished for PR 20343 at commit 9ac04ed.

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

maropu · 2018-01-22T01:45:21Z

retest this please.

SparkQA · 2018-01-22T04:12:25Z

Test build #86452 has finished for PR 20343 at commit 9ac04ed.

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

SparkQA · 2018-01-22T06:18:16Z

Test build #86457 has finished for PR 20343 at commit 12f687c.

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

SparkQA · 2018-01-22T08:05:01Z

Test build #86462 has finished for PR 20343 at commit 5d6092c.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2018-01-22T08:09:33Z

retest this please.

SparkQA · 2018-01-22T11:25:40Z

Test build #86465 has finished for PR 20343 at commit 5d6092c.

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

maropu · 2018-01-22T12:41:23Z

We need to update TPCDSQueryBenchmark, too? I think we could replace the update queries there.

gatorsmile · 2018-01-22T17:56:36Z

sql/core/src/test/scala/org/apache/spark/sql/TPCDSQuerySuite.scala

    }
  }

+  val tpcdsQueriesV2_7_0 = Seq(


Please add a comment to explain what the list contains?

gatorsmile · 2018-01-22T18:05:37Z

sql/core/src/test/scala/org/apache/spark/sql/TPCDSQuerySuite.scala

        |`web_street_name` STRING, `web_street_type` STRING, `web_suite_number` STRING,
        |`web_city` STRING, `web_county` STRING, `web_state` STRING, `web_zip` STRING,
-        |`web_country` STRING, `web_gmt_offset` STRING, `web_tax_percentage` DECIMAL(5,2))
+        |`web_country` STRING, `web_gmt_offset` DECIMAL(5,2), `web_tax_percentage` DECIMAL(5,2))


I double checked the schema changes made in this PR is consistent with the TPC-DS doc http://www.tpc.org/tpc_documents_current_versions/pdf/tpc-ds_v2.1.0.pdf

Thank you a lot, @maropu and @gatorsmile !

gatorsmile · 2018-01-22T18:11:31Z

sql/core/src/test/resources/tpcds-v2.7.0/q11.sql

+                  t_s_secyear.customer_id
+                 ,t_s_secyear.customer_first_name
+                 ,t_s_secyear.customer_last_name
+                 ,t_s_secyear.customer_email_address


Could we highlight the changes we made in version 2.7 compared with the original version by adding the comments like -- ?

If possible, could you also use the original SQL query files with the same styles/indents?

ok, I'll do as much as possible. one question; I think the sql format is not consistent in all the files, e.g., SQLQueryTestSuite. Probably, I think uppercase letters for SQL reserved words and 2 indents seem to be de-facto, but we don't have any format rule for that anywhere, right? we'd better to write the rule somewhere? We don't need to re-format existing code though, IMHO we'd better to make them consistent in future prs.

Regarding a keywords capitalization rule, this is just for readability. We do not enforce it, but it is preferred.

gatorsmile · 2018-01-22T18:14:08Z

Regarding the updates of TPCDSQueryBenchmark, we can do it in a separate PR.

gatorsmile · 2018-01-22T18:16:32Z

Regarding the change you made in [date] + 14 days -> date + INTERVAL 14 days, could we first support it before we merge this PR? It sounds like this is trivial to support, right?

maropu · 2018-01-23T00:25:17Z

ok, I'll try and check. Just s sec.

SparkQA · 2018-01-23T04:33:29Z

Test build #86501 has finished for PR 20343 at commit fff88d2.

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

maropu · 2018-01-30T06:26:20Z

I opened a new pr to support [date] + 14 days: #20433

maropu · 2018-01-30T06:49:08Z

I checked all the queries again and I found that some queries (q6, q11, q20, q22, q24, q34, q35, q47, q49, q57, q64, q72, q74, q75, q78, q98) only have minor changes (See the comments to point out the changes). So, how about directly applying these changes in sql/core/src/test/resources/tpcds?

SparkQA · 2018-01-30T08:05:01Z

Test build #86800 has finished for PR 20343 at commit d04b087.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2018-01-30T08:33:04Z

retest this please.

SparkQA · 2018-01-30T11:58:53Z

Test build #86812 has finished for PR 20343 at commit d04b087.

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

dongjoon-hyun

+1, LGTM.

gatorsmile · 2018-01-30T18:24:51Z

@maropu Yeah. As long as the queries are different, we should keep both versions. This is to help the others understand we fully support TPC-DS queries without the changes. Thanks!

gatorsmile · 2018-01-30T18:25:48Z

Thanks for submitting the PR #20433. It sounds like there are still some test failure. Will review it after 2.3 release. Thanks!

maropu · 2018-01-30T23:03:02Z

ok, I'll fix soon. Many Thanks!

kiszk · 2018-02-28T16:03:23Z

ping @gatorsmile

maropu · 2018-03-02T10:06:37Z

We need to review #20433 first

SparkQA · 2018-03-16T21:07:38Z

Test build #88321 has finished for PR 20343 at commit d04b087.

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

maropu · 2018-03-22T03:26:59Z

A support of the optional intervals will be planed in 3.x (#20433), so is it okay to restart this again? @gatorsmile

gatorsmile · 2018-03-25T07:19:17Z

retest this please

SparkQA · 2018-03-25T10:41:33Z

Test build #88569 has finished for PR 20343 at commit d04b087.

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

gatorsmile · 2018-03-25T16:17:05Z

LGTM

Thanks! Merged to master

gatorsmile reviewed Jan 21, 2018

View reviewed changes

maropu added 2 commits January 22, 2018 01:57

Update TPCD queries to v2.7.0

5dc7e55

Drop the same queries

9ac04ed

maropu force-pushed the TPCDSV2_7 branch from 71e0e1a to 9ac04ed Compare January 21, 2018 18:39

maropu added 2 commits January 22, 2018 16:20

Fix q72 query

d1d5996

Update TPCDS schema

5d6092c

maropu force-pushed the TPCDSV2_7 branch from 12f687c to 5d6092c Compare January 22, 2018 07:21

gatorsmile reviewed Jan 22, 2018

View reviewed changes

Add doc

fff88d2

Fix

d04b087

dongjoon-hyun approved these changes Jan 30, 2018

View reviewed changes

asfgit closed this in 5f653d4 Mar 25, 2018

[SPARK-23167][SQL] Add TPCDS queries v2.7 in TPCDSQuerySuite #20343

[SPARK-23167][SQL] Add TPCDS queries v2.7 in TPCDSQuerySuite #20343

Uh oh!

Conversation

maropu commented Jan 21, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jan 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jan 21, 2018 • 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!

maropu Jan 22, 2018 • 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

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 Jan 21, 2018

Uh oh!

maropu commented Jan 22, 2018

Uh oh!

SparkQA commented Jan 22, 2018

Uh oh!

SparkQA commented Jan 22, 2018

Uh oh!

SparkQA commented Jan 22, 2018

Uh oh!

maropu commented Jan 22, 2018

Uh oh!

SparkQA commented Jan 22, 2018

Uh oh!

maropu commented Jan 22, 2018

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!

gatorsmile commented Jan 22, 2018

Uh oh!

gatorsmile commented Jan 22, 2018

Uh oh!

maropu commented Jan 23, 2018

Uh oh!

SparkQA commented Jan 23, 2018

Uh oh!

maropu commented Jan 30, 2018

Uh oh!

maropu commented Jan 30, 2018

Uh oh!

SparkQA commented Jan 30, 2018

Uh oh!

maropu commented Jan 30, 2018

Uh oh!

dongjoon-hyun Jan 21, 2018 •

edited

Loading

maropu Jan 22, 2018 •

edited

Loading

gatorsmile commented Mar 25, 2018 •

edited

Loading