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 4: (10 comments) Thanks for the quick review! http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG@7 PS3, Line 7: compatibility > nit: compatibility Done http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG@15 PS3, Line 15: gramma > nit: grammar Done http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG@20 PS3, Line 20: Import thrift_parser to pars > nit: Import thrift_parser to Done 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 Oh, I just realize we already use pyparsing in other place! I think it's ok since this script uses the virtualenv set up at L127. They are independent. I simply choose the latest version of pyparsing. http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py@379 PS3, Line 379: : # All clean. Run script and comment any changes. : check_call(['./testdata/bin/restore-stats-on-planner-tests.py'], stdout=DEVNULL) : diff = check_output( > What do you think of moving function definitions of these into thrift_parse We can move extract_thrift_defs(). But for compare_thrift_structs() and compare_thrift_enums(), they use get_comment_input() defined here. So might be better to keep them here. 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: Done http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@20 PS3, Line 20: https://github.com/pyparsing/pyparsing/blob/c4cf4a5/examples/protobuf_parser.p > Consider using permalink: Done http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@54 PS3, Line 54: Word(alphas + "_", bodyChars=alphanums + "_.").setName("identifier") > Consider using permalink: Done http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@186 PS3, Line 186: thrift_parser.ignore(pythonStyleComment) > nit: Done http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@195 PS3, Line 195: ct a dict of > Can you write a simple test to check output of this script? Maybe put it un Done -- 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: 4 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: Tue, 31 Dec 2024 02:50:41 +0000 Gerrit-HasComments: Yes
