Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing ...................................................................... Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py File bin/jenkins/critique-gerrit-review.py: http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py@53 PS3, Line 53: PYPARSING_VERSION = "3.1.4" > In order to run the new test added in tests/infra, I changed the version in Ack http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py@379 PS3, Line 379: for diff_line in diff.splitlines(): : _, _, path = diff_line.split() : comments[path].append(get_comment_input(UNCOMMITTED_CHANGE)) : if len(comments) > 0: > We can move extract_thrift_defs(). But for compare_thrift_structs() and com Ack http://gerrit.cloudera.org:8080/#/c/22264/8/bin/jenkins/thrift_parser.py File bin/jenkins/thrift_parser.py: http://gerrit.cloudera.org:8080/#/c/22264/8/bin/jenkins/thrift_parser.py@196 PS8, Line 196: try: : contents = check_output(["git", "show", f"{revision}:{file_name}"], : universal_newlines=True) : except Exception as e: : # Usually it's due to file doesn't exist in that revision : print(f"Failed to read {file_name} of revision {revision}: {e}") : return None, None I think this part is better to stay in critique-gerrit-review.py, so this script itself does not call git command at all. Thus, the function signature can be: def extract_thrift_defs(thrift_file_content): Consider testing extract_thrift_defs in test_thrift_parser.py as well. http://gerrit.cloudera.org:8080/#/c/22264/8/tests/infra/test_thrift_parser.py File tests/infra/test_thrift_parser.py: http://gerrit.cloudera.org:8080/#/c/22264/8/tests/infra/test_thrift_parser.py@30 PS8, Line 30: py3_path Can this simply be 'impala-python3'? http://gerrit.cloudera.org:8080/#/c/22264/8/tests/infra/test_thrift_parser.py@34 PS8, Line 34: assert "TStatus" in output Is the full output too big to show? I have not run the script in my local machine, but want to see the full output. assert expected_output == output -- To view, visit http://gerrit.cloudera.org:8080/22264 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d Gerrit-Change-Number: 22264 Gerrit-PatchSet: 8 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Thu, 02 Jan 2025 20:32:41 +0000 Gerrit-HasComments: Yes
