Skip to content

Conversation

@grantatspothero
Copy link
Contributor

Description

This test does not run in CI and became stale after changing the default of testing behaviors to true: 6781fa3

I ran tests locally but I'm not sure how to test this given this test does not run in CI.

Additional context and related issues

See commit here: 6781fa3

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label May 5, 2023
@grantatspothero grantatspothero requested a review from findepi May 5, 2023 16:29
@findepi findepi requested review from findinpath and hashhar and removed request for findepi May 5, 2023 18:17
case SUPPORTS_AGGREGATION_PUSHDOWN_VARIANCE:
case SUPPORTS_AGGREGATION_PUSHDOWN_COUNT_DISTINCT:
return true;
case SUPPORTS_AGGREGATION_PUSHDOWN_COVARIANCE:
Copy link
Contributor

Choose a reason for hiding this comment

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

affected tests in BCT:

  • testCovarianceAggregationPushdown
  • testCorrAggregationPushdown
  • testRegrAggregationPushdown

@findinpath
Copy link
Contributor

@grantatspothero I did run the tests also locally and can confirm as well that we need to set the support for the behavior to false.

            case SUPPORTS_AGGREGATION_PUSHDOWN_COVARIANCE:
            case SUPPORTS_AGGREGATION_PUSHDOWN_CORRELATION:
            case SUPPORTS_AGGREGATION_PUSHDOWN_REGRESSION:
                return false;

I ran tests locally but I'm not sure how to test this given this test does not run in CI.

We can temporarily add a !fixup commit (to be dropped before merging) to include TestRedshiftConnectorTest to be tested as well - make sure to increase timeout-minutes to a higher value (e.g. : 180) to make sure that the build doesn't time out. Afterwards we can run the PR with secrets to make sure there are no other hidden regressions.

@hashhar
Copy link
Member

hashhar commented May 8, 2023

why not add cloud-tests profile in ci.yml? @ebyhr I see you authored #16484

@ebyhr
Copy link
Member

ebyhr commented May 8, 2023

ci.yml already contains cloud-tests profile, but the profile doesn't run TestRedshiftConnectorTest.

                            <includes>
                                <!-- Run only the smoke tests of the connector on the CI environment due to unpredictable -->
                                <!-- locations of GitHub runners which can lead to increased client latency on the -->
                                <!-- JDBC operations performed on the ephemeral AWS Redshift cluster.  -->
                                <include>**/TestRedshiftConnectorSmokeTest.java</include>
                            </includes>

https://github.com/trinodb/trino/blob/4f787a676d1c03d41c615cfc7eba22c09cb3f049/plugin/trino-redshift/pom.xml#L238C42-L241

@ebyhr
Copy link
Member

ebyhr commented May 8, 2023

I sent #17381 within the repository. No need to push the fixup commit.

@ebyhr
Copy link
Member

ebyhr commented May 8, 2023

@grantatspothero @findinpath Did you run all tests of TestRedshiftConnectorTest locally? testAddNotNullColumnToEmptyTable & testDecimalAvgPushdownForMaximumDecimalScale failed in https://github.com/trinodb/trino/actions/runs/4912391199/jobs/8772192021?pr=17381

@grantatspothero
Copy link
Contributor Author

grantatspothero commented May 8, 2023

Will rerun all tests on latest HEAD and confirm. I was running on a slightly old trino version.

@ebyhr
Copy link
Member

ebyhr commented May 11, 2023

Superseded by #17381. Thanks for preparing the base change.

@ebyhr ebyhr closed this May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants