Michael Brown has posted comments on this change. Change subject: Use unique_database fixture in test_partition_metadata.py ......................................................................
Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/2962/1/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: Line 492: def run_stmt_in_hive(self, stmt): Since you promoted this, there's an opportunity to kill some redundant code in tests.custom_cluster.test_permanent_udfs.TestUdfPersistence.run_stmt_in_hive which also defines this method. It seems like it would be easy to include that deletion in this patch, providing self-testing doesn't find unexpected problems whose resolutions are out of the scope of this patch. http://gerrit.cloudera.org:8080/#/c/2962/1/tests/metadata/test_partition_metadata.py File tests/metadata/test_partition_metadata.py: Line 51: table_name = "same_loc_test" I don't have a strong opinion either way about this, but I think some prefer local constants to be all-caps. Note this is done below in test_partition_metadata_compatibility (L93-94) Line 52: self.client.execute("use %s" % unique_database) "use" has a side-effect across tests because of persistent connections. Ideally we avoid "use" and use absolute `db`.`table` names when referring to tables. However, in some cases "use" may be required. Do L58-59 require a "use", or is it possible to do "create table `unique_database`.`table_name` [etc] location etc." ? If not, maybe we should consider removing L52 and updating the references to the table to include the db. It's a bigger change but it removes a side-effect. Thoughts? -- To view, visit http://gerrit.cloudera.org:8080/2962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I70d87941fbfc30f41e1c6fcfee8d8c7f16b88831 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
