Quanlong Huang 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 9: (3 comments) 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: structs = defaultdict() : enums = defaultdict() : for top_level_item in parse_results: : if top_level_item.getName() == 'struct': : # A dict from field id to thrift_parser.ThriftField : struct_fields = defaultdict() : for field in to > I think this part is better to stay in critique-gerrit-review.py, so this s Yeah, it looks cleaner. For testing extract_thrift_defs(), it's hard to invoke the method directly in the test code which runs in Python2. So I modified the main method to use this method. 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: niversal > Can this simply be 'impala-python3'? Good point! http://gerrit.cloudera.org:8080/#/c/22264/8/tests/infra/test_thrift_parser.py@34 PS8, Line 34: assert "TErrorCode" in out > Is the full output too big to show? I have not run the script in my local m The full output has 4k lines. Uploaded it in the JIRA: https://issues.apache.org/jira/secure/attachment/13073760/thrift_parser_output.txt I think it'd be better to verify the content of it since whenever there is a thrift change, the full output changes as well. So I choose TStatus and TErrorCode here which are less likely to be changed. -- 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: 9 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: Fri, 03 Jan 2025 02:40:15 +0000 Gerrit-HasComments: Yes
