Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/22487 )
Change subject: IMPALA-13758: Use context manager in ImpalaTestSuite.change_database ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/22487/1/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/22487/1/tests/common/impala_test_suite.py@1027 PS1, Line 1027: impala_client.clear_configuration() Not new in this patch, but shouldn't we also restore the configuration after the "USE db;" statement? http://gerrit.cloudera.org:8080/#/c/22487/1/tests/common/impala_test_suite.py@1049 PS1, Line 1049: default What if we originally weren't using the 'default' database? Instead, shouldn't we store the current database before changing it and restore that? http://gerrit.cloudera.org:8080/#/c/22487/1/tests/common/impala_test_suite.py@1059 PS1, Line 1059: TODO Why would this be good? If every test is responsible for setting the db, test writers may forget to do it. Would you like to do this in another patch? Then we should open a Jira to track it. http://gerrit.cloudera.org:8080/#/c/22487/1/tests/common/impala_test_suite.py@1132 PS1, Line 1132: def execute_query_using_client(self, client, query, vector): Removing 'change_database' here modifies the behaviour of this function and execute_query_async_using_client(). It should be noted in the commit message. http://gerrit.cloudera.org:8080/#/c/22487/1/tests/query_test/test_cancellation.py File tests/query_test/test_cancellation.py: http://gerrit.cloudera.org:8080/#/c/22487/1/tests/query_test/test_cancellation.py@115 PS1, Line 115: n Nit: iterations? http://gerrit.cloudera.org:8080/#/c/22487/1/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: http://gerrit.cloudera.org:8080/#/c/22487/1/tests/query_test/test_scanners.py@322 PS1, Line 322: with self.change_database(self.client, vector.get_table_format()): Do we need to change the database here? We're explicitly specifying the 'unique_database' in the query text. Also on L327. http://gerrit.cloudera.org:8080/#/c/22487/1/tests/query_test/test_scanners.py@1506 PS1, Line 1506: t We could rename it to something more descriptive, e.g. "fq_table_name". -- To view, visit http://gerrit.cloudera.org:8080/22487 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75bec7403cc302728a630efe3f95e852a84594e2 Gerrit-Change-Number: 22487 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Tue, 18 Feb 2025 09:49:11 +0000 Gerrit-HasComments: Yes
