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-22036][SQL][FOLLOWUP] Fix decimalArithmeticOperations.sql #20498

Closed
wants to merge 4 commits into from
Closed

[SPARK-22036][SQL][FOLLOWUP] Fix decimalArithmeticOperations.sql #20498

wants to merge 4 commits into from

Conversation

wangyum
Copy link
Member

@wangyum wangyum commented Feb 3, 2018 •
edited
Loading

What changes were proposed in this pull request?

Fix decimalArithmeticOperations.sql test

How was this patch tested?

N/A

@SparkQA
Copy link

SparkQA commented Feb 3, 2018

Test build #87029 has finished for PR 20498 at commit 2f532ea.

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

@wangyum
Copy link
Member Author

wangyum commented Feb 3, 2018

retest this please.

dongjoon-hyun
dongjoon-hyun reviewed Feb 3, 2018
View reviewed changes
...core/src/test/resources/sql-tests/inputs/typeCoercion/native/decimalArithmeticOperations.sql Outdated
@@ -49,7 +49,7 @@ select 1e35 / 0.1;

-- arithmetic operations causing a precision loss are truncated
select 123456789123456789.1234567890 * 1.123456789123456789;
select 0.001 / 9876543210987654321098765432109876543.2
select 0.001 / 9876543210987654321098765432109876543.2;
Copy link
Member

Choose a reason for hiding this comment

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

Hi, @wangyum .
If this is only for a specific test file, can we have more proper title? Fix decimalArithmeticOperations.sql?

@wangyum wangyum changed the title [SPARK-22036][SQL][FOLLOWUP] Fix imperfect test [SPARK-22036][SQL][FOLLOWUP] Fix decimalArithmeticOperations.sql Feb 3, 2018
@SparkQA
Copy link

SparkQA commented Feb 3, 2018

Test build #87034 has finished for PR 20498 at commit 2f532ea.

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

@SparkQA
Copy link

SparkQA commented Feb 3, 2018

Test build #87035 has finished for PR 20498 at commit 2f532ea.

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

gatorsmile
gatorsmile reviewed Feb 3, 2018
View reviewed changes
...core/src/test/resources/sql-tests/inputs/typeCoercion/native/decimalArithmeticOperations.sql Outdated
@@ -49,7 +49,7 @@ select 1e35 / 0.1;

-- arithmetic operations causing a precision loss are truncated
select 123456789123456789.1234567890 * 1.123456789123456789;
select 0.001 / 9876543210987654321098765432109876543.2
select 0.001 / 9876543210987654321098765432109876543.2;
Copy link
Member

Choose a reason for hiding this comment

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

... A good catch! We need to review the PR more carefully.

Copy link
Member

Choose a reason for hiding this comment

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

This is not a good test case. The results and schemas are the same no matter whether we set spark.sql.decimalOperations.allowPrecisionLoss to true or false.

@mgaido91
Copy link
Contributor

mgaido91 commented Feb 3, 2018

@wangyum yes, you're right. I don't know how I missed it, I am very sorry for my mistake. Anyway, this is fixed also in this PR #20350, which is the "second part" of making Spark SQL-2011 compliant for decimal operations.

@gatorsmile
Copy link
Member

@mgaido91 We just remove this test case? I think we can still keep it, right?

@mgaido91
Copy link
Contributor

mgaido91 commented Feb 3, 2018

@gatorsmile because this is an example of overflow, ie. what is covered in the new PR: in the new PR I added many tests for this case, so I felt this unnecessary.

@gatorsmile
Copy link
Member

@mgaido91 If we remove it, we still need test cases for verifying the effects of spark.sql.decimalOperations.allowPrecisionLoss.

@mgaido91
Copy link
Contributor

mgaido91 commented Feb 3, 2018

@gatorsmile yes, I see what you mean, we should keep it to check that changing spark.sql.decimalOperations.allowPrecisionLoss doesn't affect this case, you are right. I will comment on my PR and update it accordingly if you agree, thanks.

@gatorsmile
Copy link
Member

If possible, I prefer to doing it in this PR. We should merge this test-only PR to 2.3 and master for verifying the behavior of the changes made in 2.3.

@mgaido91
Copy link
Contributor

mgaido91 commented Feb 3, 2018

Yes sure,I'll rebase my PR after this is merged.

@SparkQA
Copy link

SparkQA commented Feb 4, 2018

Test build #87041 has finished for PR 20498 at commit dac2dc8.

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

wangyum
wangyum commented Feb 4, 2018
View reviewed changes
...core/src/test/resources/sql-tests/inputs/typeCoercion/native/decimalArithmeticOperations.sql
@@ -48,8 +48,9 @@ select 12345678901234567890.0 * 12345678901234567890.0;
select 1e35 / 0.1;

-- arithmetic operations causing a precision loss are truncated
select 12345678912345678912345678912.1234567 + 9999999999999999999999999999999.12345;
Copy link
Member Author

Choose a reason for hiding this comment

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

The result is:

-- !query 17
select 12345678912345678912345678912.1234567 + 9999999999999999999999999999999.12345
-- !query 17 schema
struct<(CAST(12345678912345678912345678912.1234567 AS DECIMAL(38,6)) + CAST(9999999999999999999999999999999.12345 AS DECIMAL(38,6))):decimal(38,6)>
-- !query 17 output
10012345678912345678912345678911.246907


-- !query 18
select 123456789123456789.1234567890 * 1.123456789123456789
-- !query 18 schema
struct<(CAST(123456789123456789.1234567890 AS DECIMAL(36,18)) * CAST(1.123456789123456789 AS DECIMAL(36,18))):decimal(38,18)>
-- !query 18 output
138698367904130467.654320988515622621


-- !query 19
select 12345678912345.123456789123 / 0.000000012345678
-- !query 19 schema
struct<(CAST(12345678912345.123456789123 AS DECIMAL(29,15)) / CAST(1.2345678E-8 AS DECIMAL(29,15))):decimal(38,9)>
-- !query 19 output
1000000073899961059796.725866332

wangyum
wangyum commented Feb 4, 2018
View reviewed changes
...core/src/test/resources/sql-tests/inputs/typeCoercion/native/decimalArithmeticOperations.sql
@@ -74,7 +75,8 @@ select 12345678901234567890.0 * 12345678901234567890.0;
select 1e35 / 0.1;

-- arithmetic operations causing a precision loss return NULL
select 12345678912345678912345678912.1234567 + 9999999999999999999999999999999.12345;
Copy link
Member Author

Choose a reason for hiding this comment

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

The result is:

-- !query 32
select 12345678912345678912345678912.1234567 + 9999999999999999999999999999999.12345
-- !query 32 schema
struct<(CAST(12345678912345678912345678912.1234567 AS DECIMAL(38,7)) + CAST(9999999999999999999999999999999.12345 AS DECIMAL(38,7))):decimal(38,7)>
-- !query 32 output
NULL


-- !query 33
select 123456789123456789.1234567890 * 1.123456789123456789
-- !query 33 schema
struct<(CAST(123456789123456789.1234567890 AS DECIMAL(36,18)) * CAST(1.123456789123456789 AS DECIMAL(36,18))):decimal(38,28)>
-- !query 33 output
NULL


-- !query 34
select 12345678912345.123456789123 / 0.000000012345678
-- !query 34 schema
struct<(CAST(12345678912345.123456789123 AS DECIMAL(29,15)) / CAST(1.2345678E-8 AS DECIMAL(29,15))):decimal(38,18)>
-- !query 34 output
NULL

@mgaido91
Copy link
Contributor

mgaido91 commented Feb 4, 2018

LGTM

@SparkQA
Copy link

SparkQA commented Feb 4, 2018

Test build #87050 has finished for PR 20498 at commit aee5a55.

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

@gatorsmile
Copy link
Member

LGTM

Thanks! Merged to master/2.3!

asfgit pushed a commit that referenced this pull request Feb 4, 2018
@wangyum @gatorsmile
[SPARK-22036][SQL][FOLLOWUP] Fix decimalArithmeticOperations.sql
430025c 
## What changes were proposed in this pull request?

Fix decimalArithmeticOperations.sql test

## How was this patch tested?

N/A

Author: Yuming Wang <[email protected]>
Author: wangyum <[email protected]>
Author: Yuming Wang <[email protected]>

Closes #20498 from wangyum/SPARK-22036.

(cherry picked from commit 6fb3fd1)
Signed-off-by: gatorsmile <[email protected]>
@asfgit asfgit closed this in 6fb3fd1 Feb 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dongjoon-hyun dongjoon-hyun dongjoon-hyun left review comments

@gatorsmile gatorsmile gatorsmile left review comments

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
@wangyum @SparkQA @mgaido91 @gatorsmile @dongjoon-hyun