Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22358 )
Change subject: IMPALA-13672: Migrate query_test/test_kudu.py to use hs2 protocol ...................................................................... Patch Set 8: (2 comments) I will file another patch set to address the second comment. http://gerrit.cloudera.org:8080/#/c/22358/8/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/22358/8/tests/query_test/test_kudu.py@169 PS8, Line 169: execute_query_using_vector > it is not clear to me why do we need execute_query_using_vector - would sim This test accept 'vector' fixture. That means, it is possible that TestKuduOperations.add_test_dimensions() someday extend this matrix to also test other protocol that is 'hs2'-compatible (ie., hs2-http). Thus, free-style SQL execution need to use execute_query_using_vector() so it can use the right cient for protocol-under-test instead of using self.hs2_client directly. http://gerrit.cloudera.org:8080/#/c/22358/8/tests/query_test/test_kudu.py@1438 PS8, Line 1438: assert result.data == [ : "a\tint\t\ttrue\ttrue\tfalse\t\tAUTO_ENCODING\tDEFAULT_COMPRESSION\t0"] > Can we assume that this will be always run with hs2 protocol? In that case Al tests under this class does not have 'vector' fixture. So it is totally up to test class on which client to use. We can use self.hs2_client directly here, but it will not save use from this conversion: https://github.com/apache/impala/blob/8d74bfd18cbdd63f2497fc007ffaa4ee0fadc270/tests/common/impala_connection.py#L573 Personally, I dislike accessing self.hs2_client directly because it prevent us from extending the protocol-under-test in the future. But for this single test class, I suppose it is fine. -- To view, visit http://gerrit.cloudera.org:8080/22358 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5f38baf5a0bbde1a1ad0bb4666c300f4f3cabd33 Gerrit-Change-Number: 22358 Gerrit-PatchSet: 8 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: Joe McDonnell <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Mon, 03 Feb 2025 15:51:58 +0000 Gerrit-HasComments: Yes
