Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatability checks based on pyparsing ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG@15 PS3, Line 15: gramma nit: grammar http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG@20 PS3, Line 20: The thrift_parser is used to nit: Import thrift_parser to 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.2.0" Do we need to reconcile this version with another one defined and used here? infra/python/deps/requirements.txt:41:pyparsing == 2.0.3 tests/comparison/db_connection.py:37:from pyparsing import ( http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py@379 PS3, Line 379: curr_structs, curr_enums = extract_thrift_defs(revision, curr_file) : old_structs, old_enums = extract_thrift_defs(base_revision, curr_file) : compare_thrift_structs(curr_file, old_structs, curr_structs, comments) : compare_thrift_enums(curr_file, old_enums, curr_enums, comments) What do you think of moving function definitions of these into thrift_parser.py? http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py File bin/jenkins/thrift_parser.py: http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@19 PS3, Line 19: Developed based on nit: Developed based on example at: http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@20 PS3, Line 20: https://github.com/pyparsing/pyparsing/blob/master/examples/protobuf_parser.py Consider using permalink: https://github.com/pyparsing/pyparsing/blob/c4cf4a5/examples/protobuf_parser.py http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@54 PS3, Line 54: https://github.com/apache/thrift/blob/master/compiler/cpp/src/thrift/thriftl.ll Consider using permalink: https://github.com/apache/thrift/blob/154d154/compiler/cpp/src/thrift/thriftl.ll http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@186 PS3, Line 186: # thrift_dir = "/home/quanlong/workspace/Impala/common/thrift" nit: # thrift_dir = os.environ["IMPALA_HOME"] + "/common/thrift" http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@195 PS3, Line 195: res.pprint() Can you write a simple test to check output of this script? Maybe put it under tests/infra/? -- 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: 3 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: Mon, 30 Dec 2024 16:28:05 +0000 Gerrit-HasComments: Yes
