Riza Suminto 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 3: (6 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: temporarily change database and reverting back to default database. > Not new in this patch, but shouldn't we also restore the configuration afte For run_test_case(), restoring options is handled by __restore_query_options. For other cases, it is best to be consistent by always reverting back to default configuration. http://gerrit.cloudera.org:8080/#/c/22487/1/tests/common/impala_test_suite.py@1049 PS1, Line 1049: > What if we originally weren't using the 'default' database? Instead, should It is indeed problematic in that case. Instead of trying to perfectly cover all cases, I rather to have this method to have clear behavior on exit. Test writer that does not read the documentation of this method will likely have its test failed and unable to pass GVO. http://gerrit.cloudera.org:8080/#/c/22487/1/tests/common/impala_test_suite.py@1059 PS1, Line 1059: > Why would this be good? If every test is responsible for setting the db, te Filed https://issues.apache.org/jira/browse/IMPALA-13766. See the argument for removal there. http://gerrit.cloudera.org:8080/#/c/22487/1/tests/common/impala_test_suite.py@1132 PS1, Line 1132: return self.__execute_query(self.client, query, query_options) > Removing 'change_database' here modifies the behaviour of this function and Done 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: self.execute_query_using_client(self.client, > Do we need to change the database here? We're explicitly specifying the 'un Reverted it. Done. http://gerrit.cloudera.org:8080/#/c/22487/1/tests/query_test/test_scanners.py@1506 PS1, Line 1506: e > We could rename it to something more descriptive, e.g. "fq_table_name". Done -- 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: 3 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 23:18:00 +0000 Gerrit-HasComments: Yes
