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-17011][SQL] Support testing exceptions in SQLQueryTestSuite #14592

Closed
wants to merge 2 commits into from

Conversation

petermaxlee
Copy link
Contributor

What changes were proposed in this pull request?

This patch adds exception testing to SQLQueryTestSuite. When there is an exception in query execution, the query result contains the the exception class along with the exception message.

As part of this, I moved some additional test cases for limit from SQLQuerySuite over to SQLQueryTestSuite.

How was this patch tested?

This is a test harness change.

-- !query 2 schema
struct<9223372036854775808:decimal(19,0),(-9223372036854775809):decimal(19,0)>
struct<9223372036854775807:bigint,(-9223372036854775808):decimal(19,0)>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"-9223372036854775808" is a valid long value (Long.MinValue) but Spark treats it as a decimal(19, 0) because "9223372036854775808" is out of range. Is this expected?

cc @cloud-fan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also cc @sarutak who wrote the original test case.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a parser bug? cc @hvanhovell

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd call this a bug. I tried in Spark 1.6 and it was returning double (which was worse).

Here's postgres:

rxin=# select pg_typeof(-9223372036854775808);
 pg_typeof 
-----------
 bigint
(1 row)

rxin=# select pg_typeof(-9223372036854775807);
 pg_typeof 
-----------
 bigint
(1 row)

rxin=# select pg_typeof(-9223372036854775806);
 pg_typeof 
-----------
 bigint
(1 row)

Copy link
Contributor

Choose a reason for hiding this comment

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

@petermaxlee can you file a jira ticket for this bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@petermaxlee
Copy link
Contributor Author

@cloud-fan after adding enough features to the test harness, do you think I should port all tests over in a single pull request, or more incremental?

@cloud-fan
Copy link
Contributor

I'd like to do it incrementally, and ideally one SQL testing file(xxx.sql) one PR, but we can have many PRs at the same time, they are not likely to get conflicted.

@@ -19,16 +19,24 @@ struct<2147483648:bigint,(-2147483649):bigint>


-- !query 2
select 9223372036854775808, -9223372036854775809
select 9223372036854775807, -9223372036854775808
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also add the boundary conditions for int as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I add it in a separate pull request? I want to add all literal parsing here, but don't want to distract this pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

sgtm

@rxin
Copy link
Contributor

rxin commented Aug 11, 2016

LGTM pending Jenkins.

@SparkQA
Copy link

SparkQA commented Aug 11, 2016

Test build #63580 has finished for PR 14592 at commit 1a7cdc0.

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

@rxin
Copy link
Contributor

rxin commented Aug 11, 2016

Merging in master.

@asfgit asfgit closed this in 0db373a Aug 11, 2016
@SparkQA
Copy link

SparkQA commented Aug 11, 2016

Test build #63582 has finished for PR 14592 at commit 76defce.

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

asfgit pushed a commit that referenced this pull request Aug 11, 2016
## What changes were proposed in this pull request?
This patch adds exception testing to SQLQueryTestSuite. When there is an exception in query execution, the query result contains the the exception class along with the exception message.

As part of this, I moved some additional test cases for limit from SQLQuerySuite over to SQLQueryTestSuite.

## How was this patch tested?
This is a test harness change.

Author: petermaxlee <petermaxlee@gmail.com>

Closes #14592 from petermaxlee/SPARK-17011.

(cherry picked from commit 0db373a)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@cloud-fan
Copy link
Contributor

backport to 2.0!

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.

4 participants