Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22626 )
Change subject: IMPALA-13864: Implement ImpylaHS2ResultSet.exec_summary ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/22626/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22626/3//COMMIT_MSG@15 PS3, Line 15: be Nit: by http://gerrit.cloudera.org:8080/#/c/22626/3/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/22626/3/shell/impala_client.py@141 PS3, Line 141: if not summary.nodes: Would it make sense to also return if len(summary.nodes) <= idx? Not having this check has not caused issues to date, so the check probably doesn't need to be added. http://gerrit.cloudera.org:8080/#/c/22626/3/tests/beeswax/impala_beeswax.py File tests/beeswax/impala_beeswax.py: http://gerrit.cloudera.org:8080/#/c/22626/3/tests/beeswax/impala_beeswax.py@398 PS3, Line 398: return None Can you please add a note about this change to the commit message? http://gerrit.cloudera.org:8080/#/c/22626/3/tests/common/impala_connection.py File tests/common/impala_connection.py: http://gerrit.cloudera.org:8080/#/c/22626/3/tests/common/impala_connection.py@113 PS3, Line 113: from shell.impala_client import build_exec_summary_table Even though this import is only used here, please consider moving it to the top of the file since there is where imports are expected to be. http://gerrit.cloudera.org:8080/#/c/22626/3/tests/common/impala_connection.py@305 PS3, Line 305: pass Nit: passed http://gerrit.cloudera.org:8080/#/c/22626/3/tests/common/impala_connection.py@487 PS3, Line 487: support Nit: supports http://gerrit.cloudera.org:8080/#/c/22626/3/tests/custom_cluster/test_executor_groups.py File tests/custom_cluster/test_executor_groups.py: http://gerrit.cloudera.org:8080/#/c/22626/3/tests/custom_cluster/test_executor_groups.py@a1053 PS3, Line 1053: Does self.hs2_client get re-initialized before this test begins executing? If not, configuration from other tests will leak into this test. http://gerrit.cloudera.org:8080/#/c/22626/3/tests/custom_cluster/test_executor_groups.py@992 PS3, Line 992: self.hs2_client.set_configuration_option(k, v) Please note this change in the commit message. -- To view, visit http://gerrit.cloudera.org:8080/22626 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7d88f78e58eeda29ce21e7828884c7a129d7efe6 Gerrit-Change-Number: 22626 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Wed, 19 Mar 2025 20:34:15 +0000 Gerrit-HasComments: Yes
