Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22404 )
Change subject: IMPALA-13694: Add ImpalaTestSuite.__reset_impala_clients method ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/22404/2/tests/common/custom_cluster_test_suite.py File tests/common/custom_cluster_test_suite.py: http://gerrit.cloudera.org:8080/#/c/22404/2/tests/common/custom_cluster_test_suite.py@368 PS2, Line 368: pytest.fail("Cannot specify with_args on both class and methods") > Why not always call the super setup_method? Reverted this. Explanation is in commit message. http://gerrit.cloudera.org:8080/#/c/22404/2/tests/common/custom_cluster_test_suite.py@402 PS2, Line 402: def wait_for_wm_init_complete(self, timeout_s=120): > Why not always call the super teardown_method? Reverted this. Explanation is in commit message. http://gerrit.cloudera.org:8080/#/c/22404/2/tests/common/test_vector.py File tests/common/test_vector.py: http://gerrit.cloudera.org:8080/#/c/22404/2/tests/common/test_vector.py@102 PS2, Line 102: value = self.get_value(EXEC_OPTION_KEY).get(option_name.lower()) > Nit: unnecessary variable declaration. Removed. The lower() is still needed as defensive measure. We'll need to comb our test files and normalize everything before changing it to assert option_name.islower(). http://gerrit.cloudera.org:8080/#/c/22404/2/tests/common/test_vector.py@108 PS2, Line 108: > Nit: unnecessary variable declaration. Done http://gerrit.cloudera.org:8080/#/c/22404/2/tests/common/test_vector.py@113 PS2, Line 113: return ' | '.join(['%s' % vector_value for vector_value in self.vector_values]) > Nit: unnecessary variable declaration. Done http://gerrit.cloudera.org:8080/#/c/22404/2/tests/hs2/hs2_test_suite.py File tests/hs2/hs2_test_suite.py: http://gerrit.cloudera.org:8080/#/c/22404/2/tests/hs2/hs2_test_suite.py@123 PS2, Line 123: # TODO: Rename this method to connect() and provide proper py.test setup_method(). > Will these todos be addressed in this change? No. I think we are move away from using HS2TestSuite as a cursor and use ImpylaHS2Connection instead. I'm punting this until I can fully understand how it is being used, and my other overlapping patches merged. https://gerrit.cloudera.org/c/22358/ https://gerrit.cloudera.org/c/22362/ http://gerrit.cloudera.org:8080/#/c/22404/2/tests/query_test/test_exprs.py File tests/query_test/test_exprs.py: http://gerrit.cloudera.org:8080/#/c/22404/2/tests/query_test/test_exprs.py@227 PS2, Line 227: g_vector(sql_str, vector) > There are a lot of instances of this code sprinkled throughout this patch. Added execute_query_using_vector(). http://gerrit.cloudera.org:8080/#/c/22404/2/tests/query_test/test_hdfs_caching.py File tests/query_test/test_hdfs_caching.py: http://gerrit.cloudera.org:8080/#/c/22404/2/tests/query_test/test_hdfs_caching.py@110 PS2, Line 110: > Please consider adding an execute_query_using_default_client function which Added execute_query_using_vector(). -- To view, visit http://gerrit.cloudera.org:8080/22404 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieb47fec9f384cb58b19fdbd10ff7aa0850ad6277 Gerrit-Change-Number: 22404 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: Jason Fehr <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Thu, 30 Jan 2025 03:51:10 +0000 Gerrit-HasComments: Yes
