Skip to content

[Native] Add decimal arithmetic overflow tests#19432

Merged
majetideepak merged 1 commit intoprestodb:masterfrom
karteekmurthys:decimal-overflow-tests
May 2, 2023
Merged

[Native] Add decimal arithmetic overflow tests#19432
majetideepak merged 1 commit intoprestodb:masterfrom
karteekmurthys:decimal-overflow-tests

Conversation

@karteekmurthys
Copy link
Contributor

@karteekmurthys karteekmurthys commented Apr 19, 2023 •
edited
Loading

The decimal overflow tests had to be removed due to this issue: facebookincubator/velox#3009
which was fixed here: #18776 and facebookincubator/velox#3593.

This PR re-introduces the overflow tests in decimal arithmetic.

@karteekmurthys karteekmurthys requested a review from a team as a code owner April 19, 2023 05:22
@karteekmurthys karteekmurthys marked this pull request as draft April 19, 2023 05:22
@karteekmurthys karteekmurthys self-assigned this Apr 19, 2023
@karteekmurthys karteekmurthys marked this pull request as ready for review April 19, 2023 16:21
@majetideepak majetideepak changed the title Decimal arithmetic e2e overflow tests Add decimal arithmetic e2e overflow tests Apr 20, 2023
@majetideepak
Copy link
Collaborator

majetideepak commented Apr 20, 2023 •
edited
Loading

@karteekmurthys can you change the commit title to [Native] Add decimal arithmetic overflow tests?

@majetideepak majetideepak changed the title Add decimal arithmetic e2e overflow tests Add decimal arithmetic overflow tests Apr 20, 2023
@majetideepak majetideepak changed the title Add decimal arithmetic overflow tests [Native] Add decimal arithmetic overflow tests Apr 20, 2023
aditi-pandit
aditi-pandit approved these changes Apr 24, 2023
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks Karteek !

@karteekmurthys karteekmurthys force-pushed the decimal-overflow-tests branch from a81988d to 4f175ca Compare April 24, 2023 21:48
majetideepak
majetideepak requested changes Apr 25, 2023
View reviewed changes
Copy link
Collaborator

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

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

Please change the commit title to
[Native] Add decimal arithmetic overflow tests

@karteekmurthys karteekmurthys force-pushed the decimal-overflow-tests branch from 4f175ca to 9d3afdd Compare April 25, 2023 16:18
majetideepak
majetideepak approved these changes Apr 25, 2023
View reviewed changes
Copy link
Collaborator

@majetideepak majetideepak left a comment •
edited
Loading

Choose a reason for hiding this comment

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

native CI test is failing.

@karteekmurthys
Copy link
Contributor Author

karteekmurthys commented Apr 25, 2023

native CI test is failing.

I retriggered another build. The presto_cpp unit tests failed. But I don't have any native code changes in this PR.

@karteekmurthys karteekmurthys force-pushed the decimal-overflow-tests branch from 9d3afdd to 461ab82 Compare April 25, 2023 18:49
@karteekmurthys
Copy link
Contributor Author

karteekmurthys commented Apr 26, 2023

There are 5 failures and they are not related to decimal overflow tests. These are TPCH tests that failed:

Error:    TestPrestoNativeTpchQueriesThrift>AbstractTestNativeTpchQueries.testTpchQ18:172->AbstractTestQueryFramework.assertQuery:144->AbstractTestQueryFramework.assertQuery:149 Execution of 'actual' query failed: -- TPC-H/TPC-R Large Volume Customer Query (Q18)
-- Function Query Definition
-- Approved February 1998

Error:    TestPrestoNativeTpchQueriesThrift>AbstractTestNativeTpchQueries.testTpchQ19:179->AbstractTestQueryFramework.assertQuery:144->AbstractTestQueryFramework.assertQuery:149 Execution of 'actual' query failed: -- TPC-H/TPC-R Discounted Revenue Query (Q19)
-- Functional Query Definition
-- Approved February 1998

Error:    TestPrestoNativeTpchQueriesThrift>AbstractTestNativeTpchQueries.testTpchQ2:49->AbstractTestQueryFramework.assertQuery:144->AbstractTestQueryFramework.assertQuery:149 Execution of 'actual' query failed: -- TPC-H/TPC-R Minimum Cost Supplier Query (Q2)
-- Functional Query Definition
-- Approved February 1998

Error:    TestPrestoNativeTpchQueriesThrift>AbstractTestNativeTpchQueries.testTpchQ20:186->AbstractTestQueryFramework.assertQuery:144->AbstractTestQueryFramework.assertQuery:149 Execution of 'actual' query failed: -- TPC-H/TPC-R Potential Part Promotion Query (Q20)
-- Function Query Definition

Error:    TestPrestoNativeTpchQueriesThrift>AbstractTestNativeTpchQueries.testTpchQ20:186->AbstractTestQueryFramework.assertQuery:144->AbstractTestQueryFramework.assertQuery:149 Execution of 'actual' query failed: -- TPC-H/TPC-R Potential Part Promotion Query (Q20)

Error:    TestPrestoNativeTpchQueriesThrift>AbstractTestNativeTpchQueries.testTpchQ21:193->AbstractTestQueryFramework.assertQuery:144->AbstractTestQueryFramework.assertQuery:149 Execution of 'actual' query failed: -- TPC-H/TPC-R Suppliers Who Kept Orders Waiting Query (Q21)
-- Functional Query Definition

@karteekmurthys karteekmurthys force-pushed the decimal-overflow-tests branch from 461ab82 to 0802389 Compare April 26, 2023 16:53
@mbasmanova
Copy link
Contributor

mbasmanova commented Apr 26, 2023

@karteekmurthys Are these tests flaky? What are the failures? If these are flaky, then let's disable them (in a separate PR) and file GitHub issue to have them fixed.

@karteekmurthys
Copy link
Contributor Author

karteekmurthys commented Apr 26, 2023

@karteekmurthys Are these tests flaky? What are the failures? If these are flaky, then let's disable them (in a separate PR) and file GitHub issue to have them fixed.

Caused by: java.lang.RuntimeException: Failed to fetched data from 127.0.0.1:1235 /v1/task/20230425_211033_00012_x52vw.2.0.1/results/3/0 - Exhausted retries: AsyncSocketException: connect failed, type = Socket not open, errno = 111 (Connection refused)
	at com.facebook.presto.tests.AbstractTestingPrestoClient.execute(AbstractTestingPrestoClient.java:124)
	at com.facebook.presto.tests.DistributedQueryRunner.execute(DistributedQueryRunner.java:726)
	at com.facebook.presto.tests.DistributedQueryRunner.execute(DistributedQueryRunner.java:694)
	at com.facebook.presto.tests.QueryAssertions.assertQuery(QueryAssertions.java:175)
	... 19 more

They seem to be flaky. Opened an issue here: #19480

@karteekmurthys
Copy link
Contributor Author

karteekmurthys commented Apr 27, 2023

@aditi-pandit @mbasmanova the native tests are successful. Would you please help merge this PR?

@majetideepak
Copy link
Collaborator

majetideepak commented Apr 28, 2023

@karteekmurthys the maven checks job failed. I restarted it

@karteekmurthys karteekmurthys force-pushed the decimal-overflow-tests branch from 7561418 to a42f2f3 Compare April 28, 2023 04:28
@karteekmurthys karteekmurthys force-pushed the decimal-overflow-tests branch from a42f2f3 to bfe84fa Compare April 28, 2023 04:32
@majetideepak majetideepak merged commit 1eb6559 into prestodb:master May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@majetideepak majetideepak majetideepak approved these changes

@aditi-pandit aditi-pandit aditi-pandit approved these changes

@mbasmanova mbasmanova Awaiting requested review from mbasmanova

Assignees

@karteekmurthys karteekmurthys

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@karteekmurthys @majetideepak @mbasmanova @aditi-pandit