Decimal arithmetic and logical functions e2e tests

[karteekmurthys]

Test plan -
Run and compare SQL queries with Decimal Arithmetic operations on PrestoDB and PrestoCPP.

Removed "Decimal overflow" related tests as they trigger this issue in Prestissimo. Will re-add them once that issue is fixed.

[majetideepak]

The coverage looks good. Will be nice to add a few with bounds.

[majetideepak]

I feel reviewers might be confused as to why the type is DECIMAL(35,20) when none of the inputs carry that. Should we simply say decimal '999999999999999.999' like below and clarify that the inferred column type is DECIMAL(35,20)

[majetideepak]

Do we need cast? can we simply say decimal '99999999999999999999999999999999999999'?

[majetideepak]

Should we add a test between decimal limits? between [min, max] and [max, min].

[majetideepak]

Test limits here and other functions below?

[karteekmurthys]

@mbasmanova @kagamiori Would you please review this. I had to remove the "Decimal overflow" related tests and I have explained in the description.
test native / build-and-test which executes e2e tests is passing.

[kagamiori]

Hi Karteek, Queries that process only literal values like this may be constant folded by Prestissimo coordinator without invoking Velox evaluation. Could you try selecting from columns instead of literal values only? (cc @amitkdutta)

[karteekmurthys]

I understand just executing literals may be constant folded. But, here we are testing arithmetic operation and Presto Query plan has SUBTRACT operator here:

[kagamiori]

My concern is that the constant folding may use Presto's code instead of Velox code (that you would like to test IIUC). Let me try if I can verify this locally.

[karteekmurthys]

The concern is valid. From what I understand if the plan says there is a SUBTRACT operator, then that is what gets executed in the backend. Or else, we would just see a project node. CMIW.

[karteekmurthys]

@kagamiori did you get a chance to verify this?

[kagamiori]

Hi @karteekmurthys, I'm having trouble running the e2e query on my laptop (due to my setup of Prestissimo). I'm asking @amitkdutta and @mbasmanova to help confirm.

[mbasmanova]

@karteekmurthys Karteek, to speed up the review, would you share the output of EXPLAIN (TYPE DISTRIBUTED) command for one of these queries?

[karteekmurthys]

presto:default> explain (type distributed) SELECT n - m from (values (CAST('999999999999999.999' as decimal(18,3)), CAST('1' as decimal(1,0)))) t(n, m);
                                         Query Plan                                         
--------------------------------------------------------------------------------------------
 Fragment 0 [SINGLE]                                                                       +
     Output layout: [expr_6]                                                               +
     Output partitioning: SINGLE []                                                        +
     Stage Execution Strategy: UNGROUPED_EXECUTION                                         +
     - Output[_col0] => [expr_6:decimal(19,3)]                                             +
             Estimates: {rows: 1 (17B), cpu: 35.00, memory: 0.00, network: 0.00}           +
             _col0 := expr_6 (1:37)                                                        +
         - Project[projectLocality = LOCAL] => [expr_6:decimal(19,3)]                      +
                 Estimates: {rows: 1 (17B), cpu: 35.00, memory: 0.00, network: 0.00}       +
                 expr_6 := (field) - (field_0) (1:48)                                      +
             - LocalExchange[ROUND_ROBIN] () => [field:decimal(18,3), field_0:decimal(1,0)]+
                     Estimates: {rows: 1 (18B), cpu: 18.00, memory: 0.00, network: 0.00}   +
                 - Values => [field:decimal(18,3), field_0:decimal(1,0)]                   +
                         Estimates: {rows: 1 (18B), cpu: 0.00, memory: 0.00, network: 0.00}+
                         (DECIMAL'999999999999999.999', DECIMAL'1')                        +
                                                                                           +
                                                                                            

[mbasmanova]

@karteekmurthys Thanks. This plan shows that the computation (a - b) will happen on the worker.

[kagamiori]

Thanks @mbasmanova for confirming!

[kagamiori]

LGTM. Thank you for adding the test!

[kagamiori]

Thanks @mbasmanova for confirming!

[kagamiori]

There is a failure of "product test (basic)". Please resolve it before landing the code.

[karteekmurthys]

@kagamiori The e2e tests are executed in test native / build-and-test (pull_request) which is green and the failures seem unrelated to this change. A different test fails each time, not necessarily related to this change. I will try to rerun again.

[karteekmurthys]

@kagamiori @mbasmanova there is one failure, which is unrelated:

Error:  Failures: 
Error:    TestHiveDistributedJoinQueriesWithDynamicFiltering>AbstractTestJoinQueries.testShuffledStatsWithInnerJoin:88 expected [75175] but found [74541]
[INFO] 
Error:  Tests run: 2531, Failures: 1, Errors: 0, Skipped: 90
[INFO] 

Let me know if this is a blocker to merge this PR.

[mbasmanova]

@tdcmeehan @highker Tim, James, is it expected to have intermittent test failures? What should we do in these cases?

[karteekmurthys]

@mbasmanova @tdcmeehan just following up on this.

[ethanyzhang]

@mbasmanova Can you help merge this? Thanks a lot!

[mbasmanova]

@yzhang1991 I see that Aditi already merged this PR. Thank you, Aditi.