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.

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

[SPARK-22140] Add TPCDSQuerySuite #19361

Closed
wants to merge 1 commit into from

Conversation

gatorsmile
Copy link
Member

@gatorsmile gatorsmile commented Sep 27, 2017

What changes were proposed in this pull request?

Now, we are not running TPC-DS queries as regular test cases. Thus, we need to add a test suite using empty tables for ensuring the new code changes will not break them. For example, optimizer/analyzer batches should not exceed the max iteration.

How was this patch tested?

N/A

@gatorsmile
Copy link
Member Author

@SparkQA
Copy link

SparkQA commented Sep 27, 2017

Test build #82227 has finished for PR 19361 at commit 0e623aa.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class TPCDSQuerySuite extends QueryTest with SharedSQLContext with BeforeAndAfterAll

@gatorsmile
Copy link
Member Author

cc @liancheng

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. It's great to have this.

Copy link
Member

@sameeragarwal sameeragarwal left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Verified that the new test suite takes an additional 49s which seems like a reasonable trade-off to catch any compile time regressions in TPC-DS.

asfgit pushed a commit that referenced this pull request Sep 28, 2017
## What changes were proposed in this pull request?
Now, we are not running TPC-DS queries as regular test cases. Thus, we need to add a test suite using empty tables for ensuring the new code changes will not break them. For example, optimizer/analyzer batches should not exceed the max iteration.

## How was this patch tested?
N/A

Author: gatorsmile <gatorsmile@gmail.com>

Closes #19361 from gatorsmile/tpcdsQuerySuite.

(cherry picked from commit 9244957)
Signed-off-by: gatorsmile <gatorsmile@gmail.com>
@gatorsmile
Copy link
Member Author

Thanks! Merged to master/2.2

@asfgit asfgit closed this in 9244957 Sep 28, 2017
classLoader = Thread.currentThread().getContextClassLoader)
test(name) {
withSQLConf(SQLConf.CROSS_JOINS_ENABLED.key -> "true") {
sql(queryString).collect()
Copy link
Contributor

Choose a reason for hiding this comment

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

(Sorry for the late review.)

If the purpose of this test suite were testing the query planner, we can avoid running an end-to-end SQL job by replacing this line with:

sql(queryString).queryExecution.executedPlan

Also, please add comments to describe:

  1. The purpose of this test suite.
  2. How and why this line may fail (the analyzer and the optimizer may throw an exception if any rule batch reaches the max iteration limit).

We might also want to assert that the spark.testing Java property is indeed set here. Otherwise, this test suite catches nothing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do.

MatthewRBruce pushed a commit to Shopify/spark that referenced this pull request Jul 31, 2018
## What changes were proposed in this pull request?
Now, we are not running TPC-DS queries as regular test cases. Thus, we need to add a test suite using empty tables for ensuring the new code changes will not break them. For example, optimizer/analyzer batches should not exceed the max iteration.

## How was this patch tested?
N/A

Author: gatorsmile <gatorsmile@gmail.com>

Closes apache#19361 from gatorsmile/tpcdsQuerySuite.

(cherry picked from commit 9244957)
Signed-off-by: gatorsmile <gatorsmile@gmail.com>
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.

5 participants